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

Ensure transition to payment processing state happens outside transaction #4542

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@shioyama
Contributor

shioyama commented Apr 4, 2014

The current method for preventing double payment submissions is simply not working, because the payment state is set in a transaction (see discusion in #4499). This PR turns off transactions on the order state machine and in addition aliases save_state to save to make this work (see these inline notes for more details on why this is necessary).

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Apr 4, 2014

Contributor

Sorry, there appear to be a bunch of other errors on master with this commit that didn't come up in #4499. Will investigate.

Contributor

shioyama commented Apr 4, 2014

Sorry, there appear to be a bunch of other errors on master with this commit that didn't come up in #4499. Will investigate.

shioyama added some commits Mar 27, 2014

Mark test of subclassed order as pending.
This test is now failing but it is unclear if it was ever really
testing what it claims to be testing. An order with no email
is not valid and should not save, so the test should not pass.
@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Apr 4, 2014

Contributor

Ok fixed, problem was with the test which was not cleaning up properly in the after block. Tests now pass except for the one I marked as pending.

Contributor

shioyama commented Apr 4, 2014

Ok fixed, problem was with the test which was not cleaning up properly in the after block. Tests now pass except for the one I marked as pending.

@@ -38,7 +38,7 @@ def self.define_state_machine!
# To avoid multiple occurrences of the same transition being defined
# On first definition, state_machines will not be defined
state_machines.clear if respond_to?(:state_machines)
state_machine :state, :initial => :cart do
state_machine :state, :initial => :cart, :use_transactions => false, :action => :save_state do

This comment has been minimized.

@radar

radar Apr 4, 2014

Member

I'm curious about the use of save_state here. Why are you calling that method which is just an alias to save?

@radar

radar Apr 4, 2014

Member

I'm curious about the use of save_state here. Why are you calling that method which is just an alias to save?

This comment has been minimized.

@shioyama

shioyama Apr 4, 2014

Contributor

Yes this is an issue with state_machine. Using save as the action name triggers methods which run the state callbacks as ActiveRecord callbacks on the model, which means that they are inside the save transaction. To get around that, you rename save to save_state and alias the method itself. It seems to no longer be mentioned in the inline comments on state machine 1.2.0 but it was there in 1.1.2.

@shioyama

shioyama Apr 4, 2014

Contributor

Yes this is an issue with state_machine. Using save as the action name triggers methods which run the state callbacks as ActiveRecord callbacks on the model, which means that they are inside the save transaction. To get around that, you rename save to save_state and alias the method itself. It seems to no longer be mentioned in the inline comments on state machine 1.2.0 but it was there in 1.1.2.

@huoxito

This comment has been minimized.

Show comment
Hide comment
@huoxito

huoxito Apr 4, 2014

Member

Looks good 👍 thanks! I'm ok with marking that spec as pending for now.

Member

huoxito commented Apr 4, 2014

Looks good 👍 thanks! I'm ok with marking that spec as pending for now.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Apr 4, 2014

Contributor

Ah forgot changelog. Will add now.

Contributor

shioyama commented Apr 4, 2014

Ah forgot changelog. Will add now.

@huoxito huoxito closed this in a4b31ae Apr 4, 2014

huoxito added a commit to huoxito/spree that referenced this pull request Apr 4, 2014

Turn off transactions around Spree::Order state transitions.
Add failing test showing payment processing is set in a transaction.

Mark test of subclassed order as pending.

This test is now failing but it is unclear if it was ever really
testing what it claims to be testing. An order with no email
is not valid and should not save, so the test should not pass.

Add changelog entry.

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