solidusio / solidus Goto Github PK

https://github.com/solidusio/solidus/blob/master/core/db/migrate/20140713140455_create_spree_return_authorization_reasons.rb#L13-L23

Afaict if defined?(Spree::ReturnAuthorizationReason) will never be true, as it was renamed Spree::ReturnReason. What is the intention here? Should we be adding these to the table directly, or should we not add them at all?

Some stores need to customize how prices are detmined when buying an item.

The price books extension provided some of this functionality, but hasn't been updated, and wasn't an extensible solution.

Some use cases:

• bulk discounts
• different pricing by country
• discounts for certain users
• pricing by custom attributes

Our current (and default) behaviour is to find a Spree::Price on the variant with the right currency and use that.

The main concern in designing this extension point is deciding at what point an items price needs to be (re)calculated.

• Calculate an item's price when adding it to the cart - Simplest and my preference
• Recalculate all items when adding another item to the cart - Would allow pricing models which depended on other items (different variants count for buying in bulk). It would add a fair bit of complexity to support this and such features might be achievable with promotions.
• Recalculated on every order update - I don't think this is viable. It would be a performance and maintenance nightmere.

Solidus currently provides a working upgrade path from Spree 2.2, 2.3, and 2.4 (and likely earlier) but not from 3.0. This may be tricky since our fork point was 2.4.

Rather than using state machine and ActiveRecord callbacks for order calculation and state progression, it would be nice to have an explicit interface for making changes to an order, both for performance reasons of not doing the same calculations multiple times, and for ease of development / testing.

In #219 we added a data-hook to help hide a so-slow-its-broken search field. We should instead do it properly.

RE: https://github.com/solidusio/solidus/blob/master/core/app/models/spree/promotion.rb#L158-L164

Basically, when dealing with line item adjustments, the usage count is wrong.

Per conversation on #274

There are a few different implementations so far, and it could be nice to have a single recommended way of bundling products together.

The order state machine is heavily tied to the solidus_frontend checkout states. It would be nice to remove that tie, and allow the order to be handled without those specific states, or maybe without the state_machine gem at all.

It's due time we have a simple dashboard for the frontpage of the admin. We added a placeholder page in #136.

Since there is a store model already in Solidus, a lot of the multi_domain overrides probably make sense to live in Solidus itself, rather than relying on lots of overrides in that extension.

When adding #310 it was apparent that the default email templates we provide aren't amazing. Most larger stores will override these templates, but it would be nice to provide better templates for stores which are starting out.

Current confirmation email:

I made this quick example improvement by hardcoding some better styles and alignment

The text emails could also be made a lot cleaner

While doing a transition from Spree 3 --> Solidus I received this error in specs:

     Failure/Error: visit cart_path
     ActionView::Template::Error:
       PG::UndefinedColumn: ERROR:  column spree_trackers.environment does not exist
       LINE 1: ...rackers" WHERE "spree_trackers"."active" = $1 AND "spree_tra...
                                                                    ^
       : SELECT  "spree_trackers".* FROM "spree_trackers" WHERE "spree_trackers"."active" = $1 AND "spree_trackers"."environment" = 'test'  ORDER BY "spree_trackers"."id" ASC LIMIT 1
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/postgresql_adapter.rb:641:in `prepare'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/postgresql_adapter.rb:641:in `prepare_statement'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/postgresql_adapter.rb:600:in `exec_cache'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/postgresql_adapter.rb:589:in `execute_and_clear'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/postgresql/database_statements.rb:160:in `exec_query'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:351:in `select'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/abstract/database_statements.rb:32:in `select_all'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/abstract/query_cache.rb:68:in `block in select_all'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/abstract/query_cache.rb:83:in `cache_sql'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/connection_adapters/abstract/query_cache.rb:68:in `select_all'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/querying.rb:39:in `find_by_sql'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/relation.rb:638:in `exec_queries'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/relation.rb:514:in `load'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/relation.rb:243:in `to_a'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/relation/finder_methods.rb:500:in `find_nth_with_limit'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/relation/finder_methods.rb:484:in `find_nth'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/gems/activerecord-4.2.3/lib/active_record/relation/finder_methods.rb:127:in `first'
     # /Users/BenAMorgan/.rvm/gems/ruby-2.2.2/bundler/gems/solidus-f26f31b83c2e/core/app/models/spree/tracker.rb:4:in `current'
     # ./app/helpers/application_helper.rb:19:in `current_tracker'

I'll get a PR in very soon to address this issue.

The fix in #237 doesn't cause any feature specs to fail, so presumably no feature spec ever hits that page.

At minimum we should test creating an order in this "supposed to be confirmed but not" state, getting here from the order edit page, and completing the order. Ideally we'd find a way to remove the page or make the situation not happen.

We currently have a separate token auth solution at Bonobos that is considerably better than the default in Spree now:

• not susceptible to timing attacks
• stores hashed tokens, rather than cleartext tokens in the database
• supports multiple tokens per user
• supports token expiration

I've started work on breaking this out into a devise module, but there's more work to do there. I think we should make this a part of solidus_auth_devise by default and remove the current api_key from spree_users.

It will be easier to review pull requests with the auto-creation of a review app

Originally written by @cbrunsdon on #171

This rule changed its preferences from (amount, operator) to (amount_min, operator_min, amount_max, operator_max) without any migration to update it. I also think the new version is silly.

Suggesting that both of these be separated from the core to be optionally remerged later when they are both ready for real life usage (probably involving a rewrite). I have never had a case where both actually fit all needs of a store, and there are many stores that don't need promotions at all.

We'd be happy to send a pull request for this if it'd be merged and push to the new repos (might take us a week or two to get to).

I just want to know whats a distinguishing difference between solidusio and spree v.3?

Make Solidus use only immutable data structures, perhaps using hamster? This would in the long run help to eliminate a whole class of bugs, even if it creates some short term pain.

There is no a reasonable way that I can see to add a before_transition to: :complete validation for Orders within your app or within an extension.

For example if you want to add something of your own along the lines of [these validations](

before_transition to: :complete, do: :validate_line_item_availability, unless: :unreturned_exchange?
before_transition to: :complete, do: :ensure_available_shipping_rates
before_transition to: :complete, do: :ensure_promotions_eligible
before_transition to: :complete, do: :ensure_line_item_variants_are_not_deleted
before_transition to: :complete, do: :ensure_inventory_units, unless: :unreturned_exchange?

there is no good way to do that. You can't call Spree::Order.before_transition from StateMachine directly because you will want it to fire before this existing before_transition:

before_transition to: :complete, do: :process_payments_before_complete

but you won't be able to insert your own before_transition before that one.

A couple solutions I can think of:

1) Move payment processing outside of state machine

And call it explicitly instead. e.g. to complete an order and process payments you'd call:

order.complete!
order.process_payments!

instead of just order.complete!.
I like how this is more explicit and is probably more adaptable to different ways that various stores handle payments, but I don't know how pragmatic it would be.

2) Provide a way to add validations

E.g.:

before_transition to: :complete, do: :run_before_complete_validations
before_transition to: :complete, do: :process_payments_before_complete

def self.add_before_complete_validation(proc)
  @@before_complete_validations << proc
end

def run_before_complete_validations
  @@before_complete_validations.each { ... }
end

Thoughts? Other ideas?

See discussion here: #321 (comment)

As of #279 we're deprecating "open" and "close" on Adjustment in general. Shall we also change the /open_adjustments and /close_adjustments routes to /finalize_adjustments and /unfinalize_adjustments?

As far as I know, if you have to perform an action when a orders' state, shipment_state, and payment_state is "complete", "shipped", and "paid" respectively at the time that the order satisfies those conditions, you're pretty much out of luck.

I would consider an order in those three states to be a "fulfilled" order where all obligations of the merchant to the customer are complete.

I've tried the below before I tried to implement some other custom solution on top of this, or inject behaviour into every state machine.

1. Using an after save hook on the order
2. Registering an update hook with Spree::Order.register_update_hook

The first doesn't work because the order updater uses update_columns

The second doesn't work because the several modules, like OrderShipping, uses the updater to calculate the new state, and then persists the state itself. run_hooks only ever being called in OrderUpdater#update!

Does anyone have any suggestions on making this a more fluid and robust process?

The spree repo is massive and likely has some big binary objects from its deep dark past we can kill. This is going to be the best time we could potentially filter-branch them out.

this migration is missing, causing this template to raise an exception

See the comment from @gmacdougall here: #134 (diff)

If auto_capture=true and you call Payment#process! on a payment that has already been authorized then I believe we will end up doing a new auth+capture (at least in the Braintree gateway code I looked at), leaving the previous auth stranded and over-authorizing on the user's card.

Going with a null object pattern approach will help us to have more reliable extensions with expected extension points, rather than encouraging overriding of behavior in ways that may break from updates.

We were discussing disabling code reloading of the solidus engines as it really slows down development mode. @gmacdougall suggested we should have the ability to try this out by setting an ENV variable

Ideas regarding payments from a brainstorm with @allisonlarson @cbrunsdon @Dkendal

The way payments are handled needs some work. The current implementation has a lot of drawbacks:

• Poor error handling
• auto_capture is dangerous
• Difficult to implement PaymentMethods that don't use CreditCard
• Payments are processed as part of the Order state machine using transient attributes on the CreditCard

Our current payments have three ways they may be used:

• Vaulted - A CreditCard with a gateway_payment_profile_id, usually created in one Payment and then allowed to be reused in future Payments
• Auth/Capture - When filling in CC details, an authorization is made in the gateway
• Sale - When auto_capture is turned on, a sale transaction is made immediately when completing the order. A sale transaction combines and auth and a capture in a single call.

Proposals

Remove sale transactions

I want to remove "sale" transactions entirely. They can't be used safely since they always charge immediately. Instead we should always authorize out of the payment step. We can keep most of the intention of auto_capture by instead performing a capture once the order is complete. This will break compatibility with any gateways which only implement purchase.

Remove Payment state machine

There's currently a state_machine on payment which switches between the processing, checkout, pending, completed, ... states. This is of basically no use as all states are allowed to move to pending, and pending can move to any other state. This has been done intentionally to bypass the state machine since we really do not want to rollback a transaction instead of saving the result of the transaction. We can replicate the current behaviour just by setting the state directly, and could improve on it by manually checking the current state when performing an action before moving to processing.

Move source construction to PaymentMethod

The PaymentMethod should be responsible for building sources. This will allow PaymentMethods like Braintree v.zero to create their CC payment source from the payment nonce with out having to override any Payment or CreditCard methods.

This may eventually help us remove the transient attributes on CreditCard

So that we can become less reliant on the frontend specs for confidence around major refactorings

Calling create_proposed_shipments on an order when shipments already exist, does not preserve the selected shipping methods.

Example scenario:
A store with two shipping methods, 2-day and overnight.
2-day is less expensive than overnight.
A user creates an order and chooses upgraded shipping to overnight.
Calling create_proposed_shipments on the order would assign 2-day shipping to the newly generated shipments.

Maintaining the selected shipping method becomes even more challenging if a store wanted to allow different shipping methods for different shipments within the same order.

Split from #240

This controller has two "lightweight" versions of existing api index actions. We should switch to using the API, but it's currently significantly slower.

We'll have to either optimize the endpoint, or allow specifying which fields we want (ex. ?fields=name,id or ?include=)

Rabl is difficult to extend for stores looking to add more information (essentially have to bypass it by adding an attribute to a helper array, or replace the template entirely), is currently used with a caching strategy whereby we still load all of the records from the database on a warm cache, and can swallow errors. Other options that exist can make these issues less difficult, and we can maintain the same api contract as a first pass if we would like so that a major upgrade is not required.

I got this failure:

  1) New Order start by customer address completes order fine
     Failure/Error: expect(page).to have_css('.stock-item')
       expected to find css ".stock-item" but there were no matches
     # ./spec/features/admin/orders/new_order_spec.rb:141:in `block (3 levels) in <top (required)>'

in this CircleCI run:
https://circleci.com/gh/solidusio/solidus/433
for this pull request:
#248
The same spec passed when I re-triggered the specs with an empty commit:
https://circleci.com/gh/solidusio/solidus/436

Updating a file causes Solidus's classes to be reloaded in development. This was made worse by #333, as permission sets are now used by default.

A copy of Spree::PermissionSets::DefaultCustomer has been removed from the module tree but is still active!

This is a common need for any store with multiple stock locations. Shipping methods should be able to restrict which stock locations they can be used from.

This will likely require improvements or changes to Spree::Stock::Package

Should add something at spree/admin/payments/source_views/_storecredit otherwise store credit payments will result in server errors when viewing them in the admin.

Taxons are great for tagging products, but many stores have implemented a categorization approach in addition to taxons to allow for a more hierarchical merchandising. Let's combine our solutions and have it supported inside of solidus.

Empty is a confusing method to have on an object because it affects the behaviour of blank? and present?

> Spree::Address.new.present?
false

We should remove empty to avoid this confusion

It's currently defined as

def empty?
  attributes.except('id', 'created_at', 'updated_at', 'country_id').all? { |_, v| v.nil? }
end

Which is basically the same as new_record? && country_id.nil?

Rather than rescuing all exceptions in the api, maybe we can leverage a few pieces of Rails' built-in functionality.

1. ExceptionWrapper (e.g. varvet/pundit#111)
2. Custom exceptions app (e.g. https://coderwall.com/p/w3ghqq/rails-3-2-error-handling-with-exceptions_app)

I couldn't find why in the difference from stable -> bleeding edge but running ( $ bundle exec rails g spree:install ) with just gem 'solidus' gives following error.

solidus_auth_devise.rb:1:in `require': cannot load such file -- spree_core (LoadError)
line 1 -> require "spree_core"

Figured out that initial generator works when using gem 'solidus', github: 'solidusio/solidus'

1. Make a better representation of a realistic data set (e.g. shipped shipments include cartons)
2. Make an optional 'large' data set to test potential performance impacts for stores with many products / variants / stores / promotions, etc.

Since it is used by a subset of solidus users, if we remove it, then core features can be built more quickly, and those using solidus_frontend can keep that up to date as the need arises.

Will also want to prevent other extensions from requiring solidus_frontend directly so that people can exclude it if they desire.

As I've promised for the last two weekly meetings, here's a place to discuss where we're going post 1.0.

As this is our first serious roadmap discussion we're really experimenting with the format here, so please take any roughness with a grain of salt.

I see there being split into four main areas for work to take place after a 1.0

1. Refactoring / Improving existing code
   • #142 Cleaning out the "front end" states
   • #143 Cleaning up the Order updating
   • Cleaning up the admin views/javascript/css/etc
2. Updating Extensions / Extension points
   • #112 Pulling multi_domain code into solidus
   • Removing class_eval's from "supported" extensions
   • Adding clearer points of extension into core
3. Adding new features
   • #146 New non-rabl API
   • #145 Adding Categories
4. Big Infrastructure
   • Renamespace Spree:: to Solidus::
   • #141 phasing out solidus_frontend

What we need to do is figure out what things we definitely want to do, and what things we don't want to slot in right now. After that, we need to prioritize and order, and figure out what should be going into releases.

Bonobos has some specific code we want to invoke when a new credit card is created and added to an order. And the Solidus community has talked about trying to provide specific, focused configuration hooks for customization, so this seems like a good opportunity for that.

I'm envisioning Solidus invoking something like:

Spree::Config.credit_card_created.try!(:call, credit_card, payment) (where payment will be present if the card was created in the context of a payment).

Does that seem reasonable? Does anyone have preferences on any particular details of the above?

We want to avoid using after_create because:

1. We're trying to avoid after_create &etc in general
2. In this particular situation a CreditCard after_create doesn't have access to any associated payments, due to the way our polymorphic relationship is currently working. And time spent fixing that seems like time spent in the wrong direction
3. We need this hook to fire after everything else in the associated update_cart call finishes executing.

More context:
What we actually want to do is mark the payment.order's ship address as a 'verified address' for that credit card whenever a credit card is newly created and added to an order.

Triggered by comments here

Taxons interface in the admin uses jstree to display and edit the taxons. It does this using a "jstree" route in the API which returns taxons in the jstree-specific format. We should instead be using the normal show route and transform it in JS.

I started refactoring the taxons page in #157 but didn't get around to this.

Guessing not, but wanted to check.

A lot of spree stores I've worked on in the past always get complaints from the client about how awful the backend UI is in Spree pre 3.0.

Wondering if Solidus has any plans here?

Thanks for all the hard work all!