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

[Spree 2 Upgrade] WIP - Added adapted order.ship_method= #2669

Closed

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Sep 8, 2018

What? Why?

Closes #2662

What should we test?

Spree upgrade issues cannot be tested yet.

How is this related to the Spree upgrade?

Part of order multi-shipments epic.

Dependencies

Related to #2654

@luisramos0 luisramos0 changed the title [Spree Upgrade] WIP - Added adapted order.ship_method= [Spree 2 Upgrade] WIP - Added adapted order.ship_method= Sep 8, 2018
@luisramos0 luisramos0 self-assigned this Sep 8, 2018
if shipments.empty?
shipment = Spree::Shipment.new
shipment.order = self
shipment.save
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK we don't need this save as AR persists the order and all its associations when doing a order.save. Correct me if I'm wrong.

shipments << shipment
end
shipments.first.add_shipping_method(shipping_method, true)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we investigated how this works with the way Spree creates shipments in https://github.com/openfoodfoundation/spree/blob/fe0a1311abb097bf3ac8001a24246c6cff5ecdbb/core/app/models/spree/order.rb#L506-L515 ? That's the only place I know in Spree's codebase where shipments are created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I have seen that code. I am closing this PR actually, we need to fix the specs and then we will see if this is still required.

@luisramos0
Copy link
Contributor Author

I am closing this PR for now, I dont think we will need this adapter. We will have to replace all places where shipping_method= is used. Mostly specs as listed in #2662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants