Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove state machine gem from Spree::Payment #2664

Closed
wants to merge 2 commits into from
Closed

Remove state machine gem from Spree::Payment #2664

wants to merge 2 commits into from

Conversation

jgayfer
Copy link

@jgayfer jgayfer commented Mar 29, 2018

This PR removes the state machine gem from Spree::Payment while keeping the external API intact. The logic that was previously hidden within the state machine is now contained within the Spree::Payment model.

Please see #2656 for the rationale behind these changes.

@jgayfer jgayfer changed the title Remove payment state machine Remove state machine gem from Spree::Payment Mar 29, 2018
@jgayfer jgayfer force-pushed the remove_payment_state_machine branch 3 times, most recently from 005982d to 0b85aba Compare March 29, 2018 23:42
tvdeyen
tvdeyen requested changes Apr 4, 2018
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

The same change requests from #2656 apply here as well, though.

@jgayfer jgayfer force-pushed the remove_payment_state_machine branch from 0b85aba to 924e439 Compare April 4, 2018 22:09
BenMorganIO
BenMorganIO reviewed Apr 4, 2018
View reviewed changes
core/app/models/spree/payment.rb
COMPLETED = 'completed'
INVALID = 'invalid'
VOID = 'void'
FAILED = 'failed'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should freeze these strings since they shouldn't be mutatable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, just realised the frozen_string_literal at the top of the file takes care of this already.

Copy link
Contributor

@BenMorganIO BenMorganIO Apr 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. We should delete all of the other freeze calls then :p.

BenMorganIO
BenMorganIO reviewed Apr 4, 2018
View reviewed changes
core/app/models/spree/payment.rb Outdated
@@ -43,6 +53,7 @@ class Payment < Spree::Base
validates :amount, numericality: true
validates :source, presence: true, if: :source_required?
validates :payment_method, presence: true
validates :state, inclusion: { in: [PENDING, PROCESSING, CHECKOUT, COMPLETED, INVALID, VOID, FAILED] }
Copy link
Contributor

@BenMorganIO BenMorganIO Apr 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a method should be used here. So that way people can easily extend payment states if they need to. Perhaps add a config to Solidus for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point Ben. @jgayfer would you mind to address this?

@jgayfer jgayfer force-pushed the remove_payment_state_machine branch from 7140c8b to 924e439 Compare April 5, 2018 16:48
@jgayfer
Copy link
Author

jgayfer commented Apr 6, 2018

@tvdeyen Requested changes have been made. See my comments on #2656

tvdeyen
tvdeyen approved these changes Apr 20, 2018
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same two nits mentioned in #2656 apply here as well, but still not mandatory

core/app/models/spree/payment.rb

def change_state!(new_state)
previous_state = state
return if new_state == previous_state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try to make use of ActiveModel::Dirty here?

http://api.rubyonrails.org/classes/ActiveModel/Dirty.html

core/db/migrate/20180404215857_add_default_state_to_payment.rb Outdated

class AddDefaultStateToPayment < ActiveRecord::Migration[5.1]
def change
change_column_default(:spree_payments, :state, Spree::Payment::CHECKOUT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the value of the constant here so we do not rely on the code may change in the future.

@jgayfer jgayfer force-pushed the remove_payment_state_machine branch from 924e439 to 49fc12d Compare May 22, 2018 16:18
@jgayfer
Copy link
Author

jgayfer commented May 22, 2018

@tvdeyen I've changed the database migration to use the actual value. Same question applies as in #2656 about ActiveModel::Dirty though.

tvdeyen
tvdeyen requested changes Jun 7, 2018
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenMorganIO made a good point. @jgayfer could you please extract this state validation into a method? Thanks

core/app/models/spree/payment.rb Outdated
@@ -43,6 +53,7 @@ class Payment < Spree::Base
validates :amount, numericality: true
validates :source, presence: true, if: :source_required?
validates :payment_method, presence: true
validates :state, inclusion: { in: [PENDING, PROCESSING, CHECKOUT, COMPLETED, INVALID, VOID, FAILED] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point Ben. @jgayfer would you mind to address this?

@jgayfer jgayfer force-pushed the remove_payment_state_machine branch from 49fc12d to 0f2a44c Compare June 7, 2018 18:30
Fix invalid Payment in Spree::Payment spec
f9f398d 
While this spec will  pass, any additional attempts to save the payment
in question would fail as the Payment was not able to save on create
due to it being invalid (missing a source).
@jgayfer jgayfer force-pushed the remove_payment_state_machine branch from 0f2a44c to d59ee42 Compare June 7, 2018 20:28
@jgayfer
Copy link
Author

jgayfer commented Jun 7, 2018

@tvdeyen Made some changes. See my comments on #2656.

tvdeyen
tvdeyen requested changes Jun 8, 2018
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change requests as in #2656

Remove state machine gem from Spree::Payment
7be4165 
The state machine is a problem for a few reasons: transitions are hard
to understand and debug, transition order is tough to control, and
database transactions during a transition aren't well understood.
Removing it brings increased code clarity, as well as the ability to
more easily extend the functionality that was previously hidden by the
state machine.
@jgayfer jgayfer force-pushed the remove_payment_state_machine branch from d59ee42 to 7be4165 Compare June 8, 2018 16:26
@jgayfer
Copy link
Author

jgayfer commented Jun 8, 2018

@tvdeyen Changes have been made 👍

tvdeyen
tvdeyen approved these changes Jun 8, 2018
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👌

@tvdeyen tvdeyen added this to the 3.0 milestone Jun 13, 2018
@BenMorganIO
Copy link
Contributor

Are we able to place state machine specific methods in their own class? I feel like that would make managing the model so much easier.

@tvdeyen
Copy link
Member

tvdeyen commented Jul 2, 2018

Are we able to place state machine specific methods in their own class? I feel like that would make managing the model so much easier.

@BenMorganIO a module would be fine 👍 But we agreed to add this after we merge these PRs

@cedum cedum mentioned this pull request Jan 16, 2019
Closed
@kennyadsl
Copy link
Member

reopened in #3039

@kennyadsl kennyadsl closed this Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenMorganIO BenMorganIO BenMorganIO left review comments

@tvdeyen tvdeyen tvdeyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jgayfer @BenMorganIO @tvdeyen @kennyadsl