Skip to content
This repository has been 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
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ono
Copy link

@ono ono commented Jul 27, 2015

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.

- Place delivered to
- Items delivering

Spree currently considers last two factors but the first factor.
Copy link
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
Copy link
Member

JDutil commented Jul 28, 2015

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

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
Copy link
Member

JDutil commented Aug 5, 2015

@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