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] Replace Order#shipping_method with Order#shipments #2009

Closed
sauloperez opened this issue Dec 21, 2017 · 6 comments
Closed
Assignees
Labels
spike A time-boxed investigation/implementation to access suitability of a tool or feature

Comments

@sauloperez
Copy link
Contributor

sauloperez commented Dec 21, 2017

Order#shipping_method is gone in Spree 2.0, so any line of code that uses it in OFN will fail. We can provide the same behaviour but being spree-2.0 compatible without having to wait for a specific spree-2.0 branch 🤘

The work has already been started in: #1651

@sauloperez sauloperez changed the title Replace order.shipments with order.shipping_method Replace Order#shipments with Order#shipping_method Dec 21, 2017
@sauloperez sauloperez changed the title Replace Order#shipments with Order#shipping_method Replace Order#shipping_method with Order#shipments May 14, 2018
@sauloperez sauloperez added the spike A time-boxed investigation/implementation to access suitability of a tool or feature label Jul 5, 2018
@sauloperez
Copy link
Contributor Author

It might to big of a PR to get all this done in one shot. Let's first investigate, post the outcomes and then we decide. We could create individual issues for each them and fix them bit by bit.

@luisramos0
Copy link
Contributor

Note from #2523 - when removing line items from a completed/finalised order, the shipments/stock_locations are updated in the order.contents.remove_variant method. This needs to be adapted. See #2523 for more details.

@luisramos0 luisramos0 changed the title Replace Order#shipping_method with Order#shipments [Spree 2 Upgrade] Replace Order#shipping_method with Order#shipments Aug 24, 2018
@luisramos0 luisramos0 self-assigned this Sep 8, 2018
@luisramos0
Copy link
Contributor

luisramos0 commented Sep 9, 2018

Actions from my investigation:

I have created separate items to keep all issues focused and all PRs small.

I need validation of the approach for #2654 (and #2669), please see here.

@daniellemoorhead
Copy link
Contributor

Hey @luisramos0 the PR attached to this issue is merged, yet this is still in the code review column...can it be closed, or are you waiting on #2662 to be merged before it's done?

If you're waiting, then suggest disconnecting #2654 and connecting #2662 instead. Or, if as it says in #2654 that it closes #2009 then go right ahead and close this 😄

@luisramos0
Copy link
Contributor

Thanks for the heads up @daniellemoorhead

I disconnected this from #2654 , 2654 is starting but not closing this one.

There's more work to be done and it's not just 2662. Actually 2662 may not be necessary.

What's still not done in this issue is to go through all the changes in the original PR #1651 and see what's still valid. This issue acceptance criteria is the absence of errors related to order#shipping_method.

@luisramos0
Copy link
Contributor

Ok, so I have reviewed everything in #1651 and created separate issues. Mostly around order.shipping_method_id and not order.shipping_method.
Issues created: #2678, #2679, #2681, #2682, #2683 and #2684 🎉

What remains to be done as part of this issue is to fix the order_spec.
I changed my mind. For clarity I am closing this one. I have opened #2685 to fix the fee calculations in the order_spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spike A time-boxed investigation/implementation to access suitability of a tool or feature
Projects
None yet
Development

No branches or pull requests

3 participants