Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

RFC for adding association between shipping method and stock locatoin#4

Open
ono wants to merge 2 commits into
spree-contrib:masterfrom
ono:feature/shipping-method-stock-location-association
Open

RFC for adding association between shipping method and stock locatoin#4
ono wants to merge 2 commits into
spree-contrib:masterfrom
ono:feature/shipping-method-stock-location-association

Conversation

@ono

@ono ono commented Jul 27, 2015

Copy link
Copy Markdown

Following up a discussion here, I wrote RFC for the requirement. Let me know if you have any questions. I am happy to discuss more. Lost My Name team is also happy to help implementing the feature too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

grammar: but -> except

@JDutil

JDutil commented Jul 28, 2015

Copy link
Copy Markdown
Member

While the reasoning behind this change is sound it's still not really clear what needs to change in the Spree::Stock classes to accomplish this. Changes are going to need to occur in the Spree::Stock::Coordinator or related classes to accomplish this, but what exactly & what consequences will that have?

@alexstoick

Copy link
Copy Markdown

Hello @JDutil

Having a look at Spree::Stock::Coordinator it looks like the required changes
would not impact much of it. As @ono mentioned in the PR - the change would be
to reject any shipping_methods.

There is already a method in Spree::Stock::Estimator which is used to select the available shipping methods for a package. The function I'm referring to is https://github.com/spree/spree/blob/master/core/app/models/spree/stock/estimator.rb#L49. It feels like adding the condition for the package.stock_location to match shipping_method.stock_location is in this place.

Looking at the rest of the code - it doesn't feel like this will touch any other parts of the system. Spree will work just fine if the shipping_rates provided for the package are correct - so there will be no other change required.

Let me know what you think!

@JDutil

JDutil commented Aug 5, 2015

Copy link
Copy Markdown
Member

@alexstoick that looks right if you want to give it a shot updating your PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants