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

Use order shipments #1651

Closed

Conversation

sauloperez
Copy link
Contributor

@sauloperez sauloperez commented Jun 30, 2017

This is a work in progress to address what we agreed on in https://community.openfoodnetwork.org/t/shipping-rate-model-step7/985/2

We have definitely abused order.shipping_method a lot, but when it comes down to it, I think we can pretty much use order.shipment.shipping_method (or equivalent) as a drop in replacement for order.shipping_method.

The goal here is exactly that: not to use order#shipping_method anymore and go through order#shipments instead.

TODO

  • Add documentation
  • Figure out how to abstract order.shipments.last

How is this related to Spree upgrade?

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 🤘

You can get further details in https://community.openfoodnetwork.org/t/order-model-step7/979?u=sauloperez

These rely on order.shipping_method which needs to be replaced with
order.shipments.shipping_method
Now we only clear the order's shipping_method, and in Spree 2.0 it can
have many shipments.
@sauloperez sauloperez force-pushed the use-order-shipments branch from bd04acf to 8ae36c9 Compare July 1, 2017 11:56
As so far we're only returning a single shipping method id, we get it
from the last shipment in the order.
This again gets the shipping method from the last shipment. It also
extracts partials for delivery and pickup shipping methods.
@sauloperez sauloperez force-pushed the use-order-shipments branch from 0ab4ace to 4fafd25 Compare July 4, 2017 07:27
@sauloperez sauloperez force-pushed the use-order-shipments branch from 4fafd25 to b353d7a Compare July 4, 2017 08:16
def shipping_method_id
return unless object.shipments.any?
object.shipments.last.shipping_method_id
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add some documentation here explaining why we need this and maybe telling that in the future we should move from order.shipments.last to something more robust

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree on the doc adding a TODO. I want to abstract order.shipments.last somehow.

end
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@sauloperez
Copy link
Contributor Author

This also needed for the Spree upgrade

@sauloperez sauloperez closed this Jul 11, 2018
@sauloperez
Copy link
Contributor Author

we totally change the way we deal with the spree upgrade since this was open, so no reason to still exist.

@sauloperez sauloperez deleted the use-order-shipments branch July 11, 2018 11:32
@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented Jul 19, 2018

@sauloperez Is this a good example of where we could test the ideas talked about today in the Spree Upgrade meeting? So reimplementing the current API for order.shipping_method to return order.shipments.first.shipping_method.

@sauloperez
Copy link
Contributor Author

Yep! we could work on this one with @luisramos0

@luisramos0
Copy link
Contributor

ok, I have reviewed all commits here and created issues for each part.

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.

7 participants