New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seperate checkout process from Order #1418

Closed
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@radar
Member

radar commented Jun 28, 2012

One of main problems I have had with customising spree is the checkout process. In order to change the entire process for sites I have to override the state machine contained in Order which really doesn't feel like a clean way of doing it.

The way I have currently overridden the process (which I think is the way defined in the documentation) is like this

Spree::Order.state_machine[:state] = StateMachine::Machine.new(Spree::Order, :initial => "cart") do
  event :next do
    transition :from => 'cart', :to => 'summary'
    transition :from => 'summary', :to => 'complete'
  end
end

This causes serval methods to be redefined within the class which state machine doesn't always seem happy about. It is possible to use a different event name but this causes me to completely overide the checkout controller and creates more work rather than less.

A better solution in my opinion would be to set the checkout process in the same why as the ProductSearcher is set within spree and pass the Order into this to manage the checkout process. This could also allow to use multiple checkout processes for a site (something I am currently trying to deal with) in a far more separate maner than having a large state machine with a guard at the start to push it down the correct path.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Apr 19, 2012

Member

This is an interesting issue and I've bumped into it more than once, in fact in Spree core the admin checkout process has to manhandle the state machine to get an order completed correctly. As does PayPal Express extension (and probably numerous other sites and extensions).

My thinking is that the state machine definitely needs to remain, but that it should be "loadable" by each order instance.

So by default orders would load the "DefaultCheckout" and in the admin they would load "AdminCheckout", etc, etc.

I know that the state_machine gem supports "dynamic definitions" so loading a state machine from an class/module or even the database is very doable.

Ultimately I think we could build a visual checkout editor that would let you reconfigure a state machine flow visually, but that's a different problem.

Member

BDQ commented Apr 19, 2012

This is an interesting issue and I've bumped into it more than once, in fact in Spree core the admin checkout process has to manhandle the state machine to get an order completed correctly. As does PayPal Express extension (and probably numerous other sites and extensions).

My thinking is that the state machine definitely needs to remain, but that it should be "loadable" by each order instance.

So by default orders would load the "DefaultCheckout" and in the admin they would load "AdminCheckout", etc, etc.

I know that the state_machine gem supports "dynamic definitions" so loading a state machine from an class/module or even the database is very doable.

Ultimately I think we could build a visual checkout editor that would let you reconfigure a state machine flow visually, but that's a different problem.

@wandtasie

This comment has been minimized.

Show comment
Hide comment
@wandtasie

wandtasie Jun 22, 2012

bump:

I found this nice comment modifying the order:
https://github.com/spree/spree/blob/master/core/app/controllers/spree/checkout_controller.rb#L2

I got a lot of trouble with the state machine implementing directebanking in a proper way. This payment integration needs to modify the state machine cause it needs a custom step after the confirmation step.

So I can't build an engine for directebanking because I have to modify the state machine in my local app as well?

wandtasie commented Jun 22, 2012

bump:

I found this nice comment modifying the order:
https://github.com/spree/spree/blob/master/core/app/controllers/spree/checkout_controller.rb#L2

I got a lot of trouble with the state machine implementing directebanking in a proper way. This payment integration needs to modify the state machine cause it needs a custom step after the confirmation step.

So I can't build an engine for directebanking because I have to modify the state machine in my local app as well?

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 27, 2012

Member

What if we were to extract the order model and checkout flow from Spree completely, like we did with auth? Super super super crazy idea. We can provide a barebones checkout process and then have gems that come in with their own versions of it then.

What we have now is a highly complicated state machine that's successfully countering any efforts that try to customize it. Having the Order model without all that state logic in it would be a great first move.

I'll have a look into this now that you've tickled my brain.

Member

radar commented Jun 27, 2012

What if we were to extract the order model and checkout flow from Spree completely, like we did with auth? Super super super crazy idea. We can provide a barebones checkout process and then have gems that come in with their own versions of it then.

What we have now is a highly complicated state machine that's successfully countering any efforts that try to customize it. Having the Order model without all that state logic in it would be a great first move.

I'll have a look into this now that you've tickled my brain.

@pythonandchips

This comment has been minimized.

Show comment
Hide comment
@pythonandchips

pythonandchips Jun 27, 2012

To me this isn't a crazy idea at all and would make complete sense. I've thought about extracting different parts of spree in the past when I see our apps replacing and overwriting mass amounts of the logic to get it to work the way we need.

pythonandchips commented Jun 27, 2012

To me this isn't a crazy idea at all and would make complete sense. I've thought about extracting different parts of spree in the past when I see our apps replacing and overwriting mass amounts of the logic to get it to work the way we need.

@wandtasie

This comment has been minimized.

Show comment
Hide comment
@wandtasie

wandtasie Jun 27, 2012

Sounds good, this would save me many hours in future:-)

wandtasie commented Jun 27, 2012

Sounds good, this would save me many hours in future:-)

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Jun 27, 2012

Member

I don't see how extracting the order model or the checkout flow from Spree to a separate gem is going to make it any easier to customize the checkout process.

I think the idea of an "Order" is super core to Spree and should not be removable, it strikes me as modularity for modularities sake.

Making the checkout flow easy to customize, doesn't mean extracting it from Spree. We need to make it easy to customize within Spree before we talk about extracting it, IMO.

Member

BDQ commented Jun 27, 2012

I don't see how extracting the order model or the checkout flow from Spree to a separate gem is going to make it any easier to customize the checkout process.

I think the idea of an "Order" is super core to Spree and should not be removable, it strikes me as modularity for modularities sake.

Making the checkout flow easy to customize, doesn't mean extracting it from Spree. We need to make it easy to customize within Spree before we talk about extracting it, IMO.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 27, 2012

Member

Ok, so removing it completely probably isn't a good idea.

There's a lot of things that are very particular to the checkout flow in that model (i.e. the state machine), and we could provide a very light-weight cart in core and have another part of spree extend that with the address, shipping and payment steps.

This way, whoever wants to customize it can take out spree_cart and put in their own solution.

Member

radar commented Jun 27, 2012

Ok, so removing it completely probably isn't a good idea.

There's a lot of things that are very particular to the checkout flow in that model (i.e. the state machine), and we could provide a very light-weight cart in core and have another part of spree extend that with the address, shipping and payment steps.

This way, whoever wants to customize it can take out spree_cart and put in their own solution.

radar added some commits Jun 28, 2012

Move state machine out into a separate, overrideable file
This allows for people to easily customize the state machine without getting a bazillion warnings about methods being redefined

Also bundled in this commit is a small refactoring of process_payments,
moving the rescue for the gateway error into the method so that it
raises it there, rather than in the state transition. This would mean
that if you wanted to call process_payments in a custom state machine,
you could just do that without the other four lines of stuff.
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 28, 2012

Member

ALRIGHTY. I've spent all day bashing my head against this problem and finally come up with the five commits above.

What this allows you to do is define an app/models/spree/order/state_machine.rb file inside your own app which will stop the default state machine being defined. From here, you define your own like this one without a delivery step:

Spree::Order.class_eval do
  state_machine :initial => 'cart', :use_transactions => false do
    event :next do
      transition :from => 'cart', :to => 'address'
      transition :from => 'address', :to => 'payment'
      transition :from => 'payment', :to => 'complete'
    end

    event :cancel do
      transition :to => 'canceled', :if => :allow_cancel?
    end

    event :return do
      transition :to => 'returned', :from => 'awaiting_return'
    end

    event :resume do
      transition :to => 'resumed', :from => 'canceled', :if => :allow_resume?
    end

    event :authorize_return do
      transition :to => 'awaiting_return'
    end

    before_transition :to => 'complete' do |order|
      order.process_payments!
    end

    after_transition :to => 'complete', :do => :finalize!
    after_transition :to => 'resumed',  :do => :after_resume
    after_transition :to => 'canceled', :do => :after_cancel
  end
end

Of course, this would require you to also override the app/helpers/spree/checkout_helper.rb file too, which you can do in a decorator called app/helpers/spree/checkout_helper_decorator.rb:

module Spree
  module CheckoutHelper
    def checkout_states
      if @order.payment and @order.payment.payment_method.payment_profiles_supported?
        %w(address payment confirm complete)
      else
        %w(address payment complete)
      end
    end
  end
end

Finally, I've added a hook to the Order model called deliverable? which determines if things like shipping address and so on should be displayed for an order. Override this in app/models/spree/order_decorator.rb to turn off those things:

Spree::Order.class_eval do
  def deliverable?
    false
  end
end

Anyway, what I'm trying to say is that I think we have some actual progress on this issue, which is great.

It would be extremely useful to us to find out how exactly people are trying to customize the checkout process and if this is the way that's actually going to be useful to them.

So is this a step in the right direction or not?

Member

radar commented Jun 28, 2012

ALRIGHTY. I've spent all day bashing my head against this problem and finally come up with the five commits above.

What this allows you to do is define an app/models/spree/order/state_machine.rb file inside your own app which will stop the default state machine being defined. From here, you define your own like this one without a delivery step:

Spree::Order.class_eval do
  state_machine :initial => 'cart', :use_transactions => false do
    event :next do
      transition :from => 'cart', :to => 'address'
      transition :from => 'address', :to => 'payment'
      transition :from => 'payment', :to => 'complete'
    end

    event :cancel do
      transition :to => 'canceled', :if => :allow_cancel?
    end

    event :return do
      transition :to => 'returned', :from => 'awaiting_return'
    end

    event :resume do
      transition :to => 'resumed', :from => 'canceled', :if => :allow_resume?
    end

    event :authorize_return do
      transition :to => 'awaiting_return'
    end

    before_transition :to => 'complete' do |order|
      order.process_payments!
    end

    after_transition :to => 'complete', :do => :finalize!
    after_transition :to => 'resumed',  :do => :after_resume
    after_transition :to => 'canceled', :do => :after_cancel
  end
end

Of course, this would require you to also override the app/helpers/spree/checkout_helper.rb file too, which you can do in a decorator called app/helpers/spree/checkout_helper_decorator.rb:

module Spree
  module CheckoutHelper
    def checkout_states
      if @order.payment and @order.payment.payment_method.payment_profiles_supported?
        %w(address payment confirm complete)
      else
        %w(address payment complete)
      end
    end
  end
end

Finally, I've added a hook to the Order model called deliverable? which determines if things like shipping address and so on should be displayed for an order. Override this in app/models/spree/order_decorator.rb to turn off those things:

Spree::Order.class_eval do
  def deliverable?
    false
  end
end

Anyway, what I'm trying to say is that I think we have some actual progress on this issue, which is great.

It would be extremely useful to us to find out how exactly people are trying to customize the checkout process and if this is the way that's actually going to be useful to them.

So is this a step in the right direction or not?

@steveh

This comment has been minimized.

Show comment
Hide comment
@steveh

steveh Jun 28, 2012

Contributor

I like the idea of being able to customize this, as I had to do exactly that, but the problem I have with overriding the state machine is that I need to remember to update it along with Spree, merging in any additional code.

It might simply be too complex to do it any other way but overriding it, but it also means that having multiple extensions that both modify the state machine will not be possible.

https://github.com/steveh/spree_optional_steps

Here's my (not production ready, extracted directly out of an existing app) that shows the customizations that I use, if you're looking for a real life example. I got what I wanted by adding conditions on the states, e.g. address_required? or delivery_required? This does basically what your deliverable? method does, but does it dynamically based on the shipping category, and also makes the payment step optional.

Contributor

steveh commented Jun 28, 2012

I like the idea of being able to customize this, as I had to do exactly that, but the problem I have with overriding the state machine is that I need to remember to update it along with Spree, merging in any additional code.

It might simply be too complex to do it any other way but overriding it, but it also means that having multiple extensions that both modify the state machine will not be possible.

https://github.com/steveh/spree_optional_steps

Here's my (not production ready, extracted directly out of an existing app) that shows the customizations that I use, if you're looking for a real life example. I got what I wanted by adding conditions on the states, e.g. address_required? or delivery_required? This does basically what your deliverable? method does, but does it dynamically based on the shipping category, and also makes the payment step optional.

@steveh

This comment has been minimized.

Show comment
Hide comment
@steveh

steveh Jun 28, 2012

Contributor

Another point I came across, as you touched on - the business logic about available states is duplicated between Order and CheckoutHelper. It would be nice to remove this.

Contributor

steveh commented Jun 28, 2012

Another point I came across, as you touched on - the business logic about available states is duplicated between Order and CheckoutHelper. It would be nice to remove this.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 28, 2012

Member

I looked into changing it so that the logic wasn't duped. I couldn't find a clear way to do that, so I think it will stay for now...

Ryan Bigg

On Thursday, 28 June 2012 at 18:29, Steve Hoeksema wrote:

Another point I came across, as you touched on - the business logic about available state machines is duplicated between Order and CheckoutHelper. It would be nice to remove this.


Reply to this email directly or view it on GitHub:
#1418 (comment)

Member

radar commented Jun 28, 2012

I looked into changing it so that the logic wasn't duped. I couldn't find a clear way to do that, so I think it will stay for now...

Ryan Bigg

On Thursday, 28 June 2012 at 18:29, Steve Hoeksema wrote:

Another point I came across, as you touched on - the business logic about available state machines is duplicated between Order and CheckoutHelper. It would be nice to remove this.


Reply to this email directly or view it on GitHub:
#1418 (comment)

@wandtasie

This comment has been minimized.

Show comment
Hide comment
@wandtasie

wandtasie Jun 28, 2012

For example: I need the optional confirmation step for check as well cause german law needs an explicit confirmation of an order. This is important especially related to the (new) order merge functionality.

If I'm logged in (as a customer) an look around in the shop and put some products to basket without purchasing them and then after some hours I get back to the shop and forget to login, add some product and wants to purchase them, I'm forced to login, but I dont see the now merged order at any time. Just the price increases at the minimalistic order overview.

Changing this behavior needs to extend the order statemachine cause the confirmation step is bound to the option source_required. But check doesn't need a source and other logic is bound to the "option".

Same problem with the payment method directebanking, which in Germany quite popular (Sofortüberweisung).

Directebanking can't be captured or voided cause the customer invokes the payment. Because it's a sort of prepayment, I need the confirm step and an additional step for the customer to invoke the payment after the confirm step.

So i think it would be a good idea to to include an option to payment method like payment_requires_confirmation to keep the logic apart from the credit cart approach.

And maybe the order merge process should be transparent to the customer. This is important for german law and at my opinion it makes the shops more trustworthy.... but this is another issue.

PS: sorry for my bad english

wandtasie commented Jun 28, 2012

For example: I need the optional confirmation step for check as well cause german law needs an explicit confirmation of an order. This is important especially related to the (new) order merge functionality.

If I'm logged in (as a customer) an look around in the shop and put some products to basket without purchasing them and then after some hours I get back to the shop and forget to login, add some product and wants to purchase them, I'm forced to login, but I dont see the now merged order at any time. Just the price increases at the minimalistic order overview.

Changing this behavior needs to extend the order statemachine cause the confirmation step is bound to the option source_required. But check doesn't need a source and other logic is bound to the "option".

Same problem with the payment method directebanking, which in Germany quite popular (Sofortüberweisung).

Directebanking can't be captured or voided cause the customer invokes the payment. Because it's a sort of prepayment, I need the confirm step and an additional step for the customer to invoke the payment after the confirm step.

So i think it would be a good idea to to include an option to payment method like payment_requires_confirmation to keep the logic apart from the credit cart approach.

And maybe the order merge process should be transparent to the customer. This is important for german law and at my opinion it makes the shops more trustworthy.... but this is another issue.

PS: sorry for my bad english

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 28, 2012

Member

Where? English seems fine to me

On 28/06/2012, at 10:30 PM, Matthias Wagnerreply@reply.github.com wrote:

PS: sorry for my bad english

Member

radar commented Jun 28, 2012

Where? English seems fine to me

On 28/06/2012, at 10:30 PM, Matthias Wagnerreply@reply.github.com wrote:

PS: sorry for my bad english

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 28, 2012

Member

I like the idea of having more hooks in the Order model that determine if things like address or payment are needed. This could be an easier way to remove states from the state machine.

Regarding the confirmation step: I'm going to add a needs_confirmation? method to Order which will have this code from Spree::CheckoutHelper in it by default:

@order.payment and @order.payment.payment_method.payment_profiles_supported?

We can then use that method inside the state transition as well, so instead of:

transition :from => 'payment',  :to => 'confirm',
  :if => Proc.new { |order| order.payment_method && order.payment_method.payment_profiles_supported? }

We would then instead have:

transition :from => 'payment',  :to => 'confirm', :if => :needs_confirmation?

Then you can override that method in your order_decorator.rb as you wish.

Member

radar commented Jun 28, 2012

I like the idea of having more hooks in the Order model that determine if things like address or payment are needed. This could be an easier way to remove states from the state machine.

Regarding the confirmation step: I'm going to add a needs_confirmation? method to Order which will have this code from Spree::CheckoutHelper in it by default:

@order.payment and @order.payment.payment_method.payment_profiles_supported?

We can then use that method inside the state transition as well, so instead of:

transition :from => 'payment',  :to => 'confirm',
  :if => Proc.new { |order| order.payment_method && order.payment_method.payment_profiles_supported? }

We would then instead have:

transition :from => 'payment',  :to => 'confirm', :if => :needs_confirmation?

Then you can override that method in your order_decorator.rb as you wish.

@steveh

This comment has been minimized.

Show comment
Hide comment
@steveh

steveh Jun 28, 2012

Contributor

Any particular reason to mix 3 different terminologies with Order#deliverable? and Order#needs_confirmation?

I used Order#address_required? and Order#delivery_required? because Spree already defines Order#payment_required?

Contributor

steveh commented Jun 28, 2012

Any particular reason to mix 3 different terminologies with Order#deliverable? and Order#needs_confirmation?

I used Order#address_required? and Order#delivery_required? because Spree already defines Order#payment_required?

radar added some commits Jun 28, 2012

Move order step defintion into order/state_machine.rb
This allows for an easier overriding of both the state machine and steps inside one file
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 29, 2012

Member

The methods will be called <blah>_required? now, given there's already a method called payment_required?

Member

radar commented Jun 29, 2012

The methods will be called <blah>_required? now, given there's already a method called payment_required?

radar added some commits Jun 29, 2012

Make state methods consistent
Also checks them now on order_details page
Don't run has_available_shipment or has_available_payment validators …
…if delivery_required? and payment_required? are false-y

Also, don't create a shipment if delivery_required? is false
Rename Order#require_email to Order#require_email?
The methods returns either true/nil, so it should be a query method

radar added some commits Jun 29, 2012

Rename Order#require_email? to email_required?
Fits in better with the other <blah>_required? methods
Apparently you can't use an :unless option on the end of a validate c…
…all, but can on the end of a validate*s* call.

Thanks Rails
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 29, 2012

Member

@wandtasie: I think that having the confirmation step should be enough to tell the user "Hey, this is your order. Make sure everything is ok." The current implementation of the state machine in my branch here will allow you to enable the confirmation step by overriding the confirmation_required? method in your Order decorator.

Member

radar commented Jun 29, 2012

@wandtasie: I think that having the confirmation step should be enough to tell the user "Hey, this is your order. Make sure everything is ok." The current implementation of the state machine in my branch here will allow you to enable the confirmation step by overriding the confirmation_required? method in your Order decorator.

radar added some commits Jun 29, 2012

Make email_required? method public
This is now used in checkout/_address, and so needs to be accessible outside the scope of the class
Move payment processing back into before_transition block for order c…
…ompletion

It appears that you can't call methods that rescue exceptions within state transitions
Add spree:state_machine generator
This will copy over the state machine from Spree's models into the application's models so that it can be customizeD
Define before_complete hook which just processes payments
Removed old process_payments method which was only being used in this
one case

radar added a commit to spree/spree-guides that referenced this pull request Jun 29, 2012

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 29, 2012

Member

Aaand I think I'm done. Please follow the instructions in the guide, read through the commits, and kick the tyres on the diff.

I really feel good about this now!

Member

radar commented Jun 29, 2012

Aaand I think I'm done. Please follow the instructions in the guide, read through the commits, and kick the tyres on the diff.

I really feel good about this now!

@BDQ BDQ closed this Jun 29, 2012

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Jun 29, 2012

Member

@radar - I like some aspects of this design, but overall I think it's the wrong approach for a coupe of reasons:

  1. Your proposed new order state machine is very complicated, and while it does provide for some basic (and common) customizations, it makes the state machine very hard to understand when compared to the current version.

  2. Encouraging people to duplicate the entire state machine when the built in customizations points above are insufficient is bad idea, because it puts them at a major disadvantage at upgrade time. (They'd need to compare the state machines and manually merge any changes etc). The fact the propose state machine is even bigger than the current version componds this problem.

I think we can be a bit more aggressive about this feature and build a new DSL that allows people to easily redefine the core state machine. The state_machine gem already allows you to add / remove / redefine states, events, transitions etc - but the provided api to do is (horrible)[https://gist.github.com/3016726].

I'd like to be able to define named state machines, that can inherit from each other, and be swapped in on a per order basis.

Here's an pseudo example of defining a state machine / checkout:

CellPhoneCheckout << Spree::DefaultCheckout
  remove_state :delivery

  add_state :calling_plain

  redefine_event :next do
    transition :from => 'cart', :to => 'delivery'
    transition :from => 'delivery',  :to => 'calling_plan', :if => :cell_phone_included?
    transition :from => 'delivery', :to => 'confirm'
    transition :from => 'calling_plan',  :to => 'confirm'
    transition :from => 'confirm',  :to => 'complete'
  end
end

And it could be loaded in a number of ways:

# globally
Spree::Order.checkout = CellPhoneCheckout


# per instance
Spree::Order.class_eval do
  def checkout
    self.contains_cell_phone? ? CellPhoneCheckout : Spree::DefaultCheckout
  end
end
Member

BDQ commented Jun 29, 2012

@radar - I like some aspects of this design, but overall I think it's the wrong approach for a coupe of reasons:

  1. Your proposed new order state machine is very complicated, and while it does provide for some basic (and common) customizations, it makes the state machine very hard to understand when compared to the current version.

  2. Encouraging people to duplicate the entire state machine when the built in customizations points above are insufficient is bad idea, because it puts them at a major disadvantage at upgrade time. (They'd need to compare the state machines and manually merge any changes etc). The fact the propose state machine is even bigger than the current version componds this problem.

I think we can be a bit more aggressive about this feature and build a new DSL that allows people to easily redefine the core state machine. The state_machine gem already allows you to add / remove / redefine states, events, transitions etc - but the provided api to do is (horrible)[https://gist.github.com/3016726].

I'd like to be able to define named state machines, that can inherit from each other, and be swapped in on a per order basis.

Here's an pseudo example of defining a state machine / checkout:

CellPhoneCheckout << Spree::DefaultCheckout
  remove_state :delivery

  add_state :calling_plain

  redefine_event :next do
    transition :from => 'cart', :to => 'delivery'
    transition :from => 'delivery',  :to => 'calling_plan', :if => :cell_phone_included?
    transition :from => 'delivery', :to => 'confirm'
    transition :from => 'calling_plan',  :to => 'confirm'
    transition :from => 'confirm',  :to => 'complete'
  end
end

And it could be loaded in a number of ways:

# globally
Spree::Order.checkout = CellPhoneCheckout


# per instance
Spree::Order.class_eval do
  def checkout
    self.contains_cell_phone? ? CellPhoneCheckout : Spree::DefaultCheckout
  end
end
@cmar

This comment has been minimized.

Show comment
Hide comment
@cmar

cmar Jun 29, 2012

Member

I recently hooked into the state machine with a decorator. Maybe we should encourage modifying the state_machine rather then redefining it?

Spree::Order.class_eval do

  self.state_machine.after_transition :to => 'payment', :do => :lookup_tax_cloud

  self.state_machine.after_transition :to => 'complete', :do => :capture_tax_cloud

  def lookup_tax_cloud
  end

  def capture_tax_cloud
  end

end
Member

cmar commented Jun 29, 2012

I recently hooked into the state machine with a decorator. Maybe we should encourage modifying the state_machine rather then redefining it?

Spree::Order.class_eval do

  self.state_machine.after_transition :to => 'payment', :do => :lookup_tax_cloud

  self.state_machine.after_transition :to => 'complete', :do => :capture_tax_cloud

  def lookup_tax_cloud
  end

  def capture_tax_cloud
  end

end
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 29, 2012

Member

How do you go about removing steps from the checkout though?

Ryan Bigg

On Friday, 29 June 2012 at 21:32, cmar wrote:

I recently hooked into the state machine with a decorator. Maybe we should encourage modifying the state_machine rather then redefining it?

Spree::Order.class_eval do

self.state_machine.after_transition :to => 'payment', :do => :lookup_tax_cloud

self.state_machine.after_transition :to => 'complete', :do => :capture_tax_cloud

def lookup_tax_cloud
end

def capture_tax_cloud
end

end

Reply to this email directly or view it on GitHub:
#1418 (comment)

Member

radar commented Jun 29, 2012

How do you go about removing steps from the checkout though?

Ryan Bigg

On Friday, 29 June 2012 at 21:32, cmar wrote:

I recently hooked into the state machine with a decorator. Maybe we should encourage modifying the state_machine rather then redefining it?

Spree::Order.class_eval do

self.state_machine.after_transition :to => 'payment', :do => :lookup_tax_cloud

self.state_machine.after_transition :to => 'complete', :do => :capture_tax_cloud

def lookup_tax_cloud
end

def capture_tax_cloud
end

end

Reply to this email directly or view it on GitHub:
#1418 (comment)

@cmar

This comment has been minimized.

Show comment
Hide comment
@cmar

cmar Jun 29, 2012

Member

I think it can mostly be done by manipulating the Spree::Order.state_machine.states collection. We could probably monkey patch the state_machine to add a remove_state method to clean up all transitions and helper methods.

Member

cmar commented Jun 29, 2012

I think it can mostly be done by manipulating the Spree::Order.state_machine.states collection. We could probably monkey patch the state_machine to add a remove_state method to clean up all transitions and helper methods.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 30, 2012

Member
  1. Your proposed new order state machine is very complicated, and while it does provide for some basic (and common) customizations, it makes the state machine very hard to understand when compared to the current version.

If you want to understand the checkout process then you should reading the guide, not the code that defines the transitions. State machines that implement some sort of workflow and need to be customizable like this are inherently complex. If you make direct eye contact with them, then expect trouble. We can put a comment in the code to this effect.

  1. Encouraging people to duplicate the entire state machine when the built in customizations points above are insufficient is bad idea, because it puts them at a major disadvantage at upgrade time. (They'd need to compare the state machines and manually merge any changes etc). The fact the propose state machine is even bigger than the current version componds this problem.

If people are going to want to customize the state machine, then why would it matter what changes we make to the state machine? The user has made their choice.

We've not changed the state machine since at least 0.60 (as far as I know, anyway), so I don't see this being a common thing. Unless there's plans to add more steps to the order process that I'm not familiar with?


I spent some time attempting to implement it in an alternative way using the aasm gem (see #1733) but found it too difficult.

Member

radar commented Jun 30, 2012

  1. Your proposed new order state machine is very complicated, and while it does provide for some basic (and common) customizations, it makes the state machine very hard to understand when compared to the current version.

If you want to understand the checkout process then you should reading the guide, not the code that defines the transitions. State machines that implement some sort of workflow and need to be customizable like this are inherently complex. If you make direct eye contact with them, then expect trouble. We can put a comment in the code to this effect.

  1. Encouraging people to duplicate the entire state machine when the built in customizations points above are insufficient is bad idea, because it puts them at a major disadvantage at upgrade time. (They'd need to compare the state machines and manually merge any changes etc). The fact the propose state machine is even bigger than the current version componds this problem.

If people are going to want to customize the state machine, then why would it matter what changes we make to the state machine? The user has made their choice.

We've not changed the state machine since at least 0.60 (as far as I know, anyway), so I don't see this being a common thing. Unless there's plans to add more steps to the order process that I'm not familiar with?


I spent some time attempting to implement it in an alternative way using the aasm gem (see #1733) but found it too difficult.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Jul 2, 2012

Member

Ultimately I think the best approach for checkout customization is a single approach for all customizations, so if you just want to remove one state or completely redefine the whole thing then the guides / directions should be the same (or the former should be a subset of the later).

Building the flexibility for people to easily remove a step (using the existing state_machine approach) IMO increases the complexity and reduces the chances that someone who needs something slightly more custom will be able to comprehend the existing state machine. Their reaction is always going to be: "this is too complex for my needs".

I'd much prefer Spree to have a simple default checkout state_machine, with no "built-in" flexibility and a nicer DSL / approach to achieving any customization.

I think we could have a few checkout state_machine extensions, that could show alternative state_machines and people could just install and use (like downloadable products, artwork upload, etc).

On occasion we do change the state machine, granted it's usually a small change but generally it's a bug fix that solves an issue. Allowing people's state machine customizations to decorator the core ensures that come upgrade time these users get the benefits of our improvements.

For big state machine redefinitions where the process it completely changed then I agree the user has made their choice, and it's their problem if they chose to upgrade.

Member

BDQ commented Jul 2, 2012

Ultimately I think the best approach for checkout customization is a single approach for all customizations, so if you just want to remove one state or completely redefine the whole thing then the guides / directions should be the same (or the former should be a subset of the later).

Building the flexibility for people to easily remove a step (using the existing state_machine approach) IMO increases the complexity and reduces the chances that someone who needs something slightly more custom will be able to comprehend the existing state machine. Their reaction is always going to be: "this is too complex for my needs".

I'd much prefer Spree to have a simple default checkout state_machine, with no "built-in" flexibility and a nicer DSL / approach to achieving any customization.

I think we could have a few checkout state_machine extensions, that could show alternative state_machines and people could just install and use (like downloadable products, artwork upload, etc).

On occasion we do change the state machine, granted it's usually a small change but generally it's a bug fix that solves an issue. Allowing people's state machine customizations to decorator the core ensures that come upgrade time these users get the benefits of our improvements.

For big state machine redefinitions where the process it completely changed then I agree the user has made their choice, and it's their problem if they chose to upgrade.

cbrunsdon pushed a commit to StemboltHQ/spree that referenced this pull request Mar 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment