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.

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

Add flexibility to Spree::Order#restart_checkout_flow #4369

Merged
merged 2 commits into from
May 19, 2022

Conversation

spaghetticode
Copy link
Member

Description

When the checkout flow is customized, the current version of the method Spree::Order#restart_checkout_flow may raise StateMachines::InvalidTransition which are not really necessary and meaningful. The error is caused by the line

next! if !line_items.empty?

that force-advances the order state from cart to the next state. This doesn't seem to be necessary, and forces stores that experience this situation to override the method definition or find workarounds in order to handle these scenarios. The bang version next! was introduced with c9eb45d apparently only for fixing specs.

Please refer to this PR commit messages for further details.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change (if needed)

This new spec fails and shows that with a very simple customization
the method `Spree::Order#restart_checkout_flow` can misbehave when
the order cannot transition forward from the `cart` state by raising
a `StateMachines::InvalidTransition` error.

This happens because of this line from `#restart_checkout_flow`:

    next! if !line_items.empty?

This line assumes that the order can always transition from `cart` to
the next state, if it has line items. The `if` guard exists exactly
for that reason: trying to `next!` an order without line items raises
the above mentioned `StateMachines::InvalidTransition` error.

This line was modified with c9eb45d for no other reason than fixing
some specs, according to the commit message.
Like exposed with the previous commit, the bang version of `next`
has the drawback that it raises an error when it cannot advance the
order state machine, which may happen especially when the order
state machine has been customized.

In general, restarting the checkout flow for an order should be able
to advance the order to the previous state, but this may not be the
case if some attributes of the order changed before calling this
method and now the order cannot advance anymore. In that situation,
the previous code was going to raise an error but that seemed to be
a bug, since there doesn't seem to be anything wrong with keeping the
order in cart state, if it cannot advance.
@spaghetticode spaghetticode added the changelog:solidus_core Changes to the solidus_core gem label May 10, 2022
@spaghetticode spaghetticode self-assigned this May 10, 2022
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@forkata forkata left a comment

Choose a reason for hiding this comment

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

Thanks @spaghetticode, this is a good change!

@kennyadsl kennyadsl merged commit 9948b01 into solidusio:master May 19, 2022
@kennyadsl kennyadsl deleted the spaghetticode/remove-bang-method branch May 19, 2022 11:19
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Aug 11, 2023
solidusio#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Aug 14, 2023
solidusio#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.
github-actions bot pushed a commit that referenced this pull request Aug 14, 2023
#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.

(cherry picked from commit 82c7bdf)
github-actions bot pushed a commit that referenced this pull request Aug 14, 2023
#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.

(cherry picked from commit 82c7bdf)
github-actions bot pushed a commit that referenced this pull request Aug 14, 2023
#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.

(cherry picked from commit 82c7bdf)
github-actions bot pushed a commit that referenced this pull request Aug 14, 2023
#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.

(cherry picked from commit 82c7bdf)
github-actions bot pushed a commit that referenced this pull request Aug 14, 2023
#4369 introduced a regression
where when calling `Spree::Order#restart_checkout_flow` on an empty
order, an error would be added to the order. That happened when calling
`#next` and the validation failed because no line items were present.

We restore the previous behavior where we only try going to the
`"address"` state if the order has line items.

(cherry picked from commit 82c7bdf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants