Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Behaviour should not depend on has_checkout_step? #2502

Closed
jhawthorn opened this issue Jan 12, 2018 · 3 comments
Closed

Behaviour should not depend on has_checkout_step? #2502

jhawthorn opened this issue Jan 12, 2018 · 3 comments
Labels
type:enhancement Proposed or newly added feature

Comments

@jhawthorn
Copy link
Contributor

jhawthorn commented Jan 12, 2018

We have a few places where the names of checkout steps influence behaviour. We take things like has_checkout_step?("delivery") to mean "this order doesn't need shipments". This is a bad way to do this. A store might want to create shipments, but not have a delivery step.

However this behaviour has been allowed for a long time and is being depended upon. See #626 and others.

$ ag 'has_checkout_step\?\'
frontend/app/views/spree/shared/_order_details.html.erb
3:  <% if order.has_checkout_step?("address") %>
10:    <% if order.has_checkout_step?("delivery") %>
31:  <% if order.has_checkout_step?("payment") %>

core/app/models/spree/line_item.rb
195:      if (saved_changes? || target_shipment.present?) && order.has_checkout_step?("delivery")

core/app/models/spree/order.rb
845:      if has_checkout_step?("delivery")

Removing this is essential to making the state machine actually customizable, as well as reducing our dependence on the state machine (which we'd really like to do, as fewer and fewer stores are using a wizard-style checkout)

@jhawthorn
Copy link
Contributor Author

One option is to introduce a new method like Order#shipments_required?, and use that to determine whether or not we should be building shipments. I don't like this because I don't think it would ever be well tested.

Another option is to always require shipments. I'm hesitant to do this because people clearly rely on the existing behaviour (building a store which doesn't ship). On the other hand, it's hard to properly test something like this. I would have the most confidence we would do this correctly.

@jarednorman
Copy link
Member

I imagine that stores that drop the "delivery" state in order to not build shipments must be depending on other customization beyond just removing the state. If it's something you want to support in some meaningful way, then maybe we need to add make this a configuration option of some kind. If not, then I'm fine with just a new method.

@kennyadsl
Copy link
Member

I think we can just resume #2673 and address latest change requests.

@solidusio solidusio locked and limited conversation to collaborators Sep 7, 2022
@waiting-for-dev waiting-for-dev converted this issue into discussion #4600 Sep 7, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
type:enhancement Proposed or newly added feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants