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

Stock location <--> Shipping method relationship #412

Merged
merged 7 commits into from Jan 20, 2016

Conversation

Projects
None yet
6 participants
@alexstoick
Contributor

alexstoick commented Oct 2, 2015

Starting point for addressing #391

Show outdated Hide outdated core/app/models/spree/shipping_method_stock_location.rb
@@ -0,0 +1,6 @@
class ShippingMethodStockLocation
self.table_name = :spree_shipping_methods_stock_locations

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

This should be Spree namespaced (eyeroll for not writing Solidus) and the table name should be spree_shipping_method_stock_locations.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

This should be Spree namespaced (eyeroll for not writing Solidus) and the table name should be spree_shipping_method_stock_locations.

This comment has been minimized.

@athal7

athal7 Oct 2, 2015

Contributor

We could potentially start namespacing new models with Solidus. Not sure what @jhawthorn has in mind for the rename

@athal7

athal7 Oct 2, 2015

Contributor

We could potentially start namespacing new models with Solidus. Not sure what @jhawthorn has in mind for the rename

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 5, 2015

Contributor

@athal7 pros and cons came to mind. I would start the rename here and in the rail engine add:

Spree = Solidus

This way Spree starts to become a second class citizen and its Solidus.

@BenMorganIO

BenMorganIO Oct 5, 2015

Contributor

@athal7 pros and cons came to mind. I would start the rename here and in the rail engine add:

Spree = Solidus

This way Spree starts to become a second class citizen and its Solidus.

Show outdated Hide outdated .../migrate/20151001121454_create_spree_shipping_methods_stock_locations.rb
def change
create_table :spree_shipping_methods_stock_locations do |t|
t.belongs_to :shipping_method
t.belongs_to :stock_location

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

Can you add foreign keys for referential integrity? Its available in rails now, so we might as well add it in. I also feel like these should have index: true on them.

@gmacdougall thoughts on this?

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

Can you add foreign keys for referential integrity? Its available in rails now, so we might as well add it in. I also feel like these should have index: true on them.

@gmacdougall thoughts on this?

Show outdated Hide outdated core/app/models/spree/stock/package.rb
@@ -108,7 +108,10 @@ def shipping_categories
# @return [Array<Spree::ShippingMethod>] the shipping methods available
# for this pacakges shipping categories
def shipping_methods

This comment has been minimized.

@athal7

athal7 Oct 2, 2015

Contributor

this is the intersection of the variant's shipping categories' shipping methods and the stock location's shipping methods right?

i wonder if something along these lines might be a bit more expressive:

shipping_method_sets = shipping_categories.map(&:shipping_methods)
shipping_method_sets << stock_location.shipping_methods
shipping_method_sets.reduce(:&).to_a

the method comment could probably use an update as well

@athal7

athal7 Oct 2, 2015

Contributor

this is the intersection of the variant's shipping categories' shipping methods and the stock location's shipping methods right?

i wonder if something along these lines might be a bit more expressive:

shipping_method_sets = shipping_categories.map(&:shipping_methods)
shipping_method_sets << stock_location.shipping_methods
shipping_method_sets.reduce(:&).to_a

the method comment could probably use an update as well

This comment has been minimized.

@alexstoick

alexstoick Oct 2, 2015

Contributor

👍

@alexstoick

alexstoick Oct 2, 2015

Contributor

👍

@@ -0,0 +1,8 @@
class CreateSpreeShippingMethodStockLocations < ActiveRecord::Migration
def change
create_table :spree_shipping_method_stock_locations do |t|

This comment has been minimized.

@athal7

athal7 Oct 2, 2015

Contributor

i think timestamps would be helpful as well here, possibly indexes as well

@athal7

athal7 Oct 2, 2015

Contributor

i think timestamps would be helpful as well here, possibly indexes as well

This comment has been minimized.

@alexstoick

alexstoick Oct 2, 2015

Contributor

👍 on both

@alexstoick

alexstoick Oct 2, 2015

Contributor

👍 on both

@athal7

This comment has been minimized.

Show comment
Hide comment
@athal7

athal7 Oct 2, 2015

Contributor

this is a great start!

Contributor

athal7 commented Oct 2, 2015

this is a great start!

Show outdated Hide outdated core/lib/spree/testing_support/factories/shipping_method_factory.rb
@@ -10,6 +10,12 @@
end
end
after(:create) do |shipping_method, evaluator|
if shipping_method.stock_locations.empty?
shipping_method.stock_locations << (Spree::StockLocation.all.to_a)

This comment has been minimized.

@jhawthorn

jhawthorn Oct 2, 2015

Member

is this necessary? We've had a lot of issues with factories which depend on the state of the database when they are created

@jhawthorn

jhawthorn Oct 2, 2015

Member

is this necessary? We've had a lot of issues with factories which depend on the state of the database when they are created

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

Also, is the #to_a needed? I think rails actually calls it internally. That's how it does the DB request at the end. Calling #to_a here just hands you back an Array object instead of an ActiveRecord::Relation object. I would always prefer an AR object over a simple array object.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

Also, is the #to_a needed? I think rails actually calls it internally. That's how it does the DB request at the end. Calling #to_a here just hands you back an Array object instead of an ActiveRecord::Relation object. I would always prefer an AR object over a simple array object.

This comment has been minimized.

@alexstoick

alexstoick Oct 5, 2015

Contributor

@jhawthorn This is quite an issue, and I was just trying to get my specs to go green.

However, since I have looked around and it's a difficult one to find. This is because with this PR we now have a dependency between: shipping_method and stock_location. In most factories these get created separately and for the code to work, we need to link them.

So now we have two solutions:

  1. If we don't do this here, we'd have to replicate it through most factories, and my gut feeling is that this will be quite difficult in some particular cases (ie: factories which call other factories...)

  2. Do this here and try and find a way to clean it up.


@alexstoick

alexstoick Oct 5, 2015

Contributor

@jhawthorn This is quite an issue, and I was just trying to get my specs to go green.

However, since I have looked around and it's a difficult one to find. This is because with this PR we now have a dependency between: shipping_method and stock_location. In most factories these get created separately and for the code to work, we need to link them.

So now we have two solutions:

  1. If we don't do this here, we'd have to replicate it through most factories, and my gut feeling is that this will be quite difficult in some particular cases (ie: factories which call other factories...)

  2. Do this here and try and find a way to clean it up.


@jhawthorn

This comment has been minimized.

Show comment
Hide comment
@jhawthorn

jhawthorn Oct 2, 2015

Member

How do we want to represent a shipping method with all stock locations set? Do we want to make it explicit and require the join record for each, or would we like to allow no stock locations to mean all?

For the former, which is what this currently implements, we'll need a migration to add all existing shipping methods to all stock locations.

Member

jhawthorn commented Oct 2, 2015

How do we want to represent a shipping method with all stock locations set? Do we want to make it explicit and require the join record for each, or would we like to allow no stock locations to mean all?

For the former, which is what this currently implements, we'll need a migration to add all existing shipping methods to all stock locations.

Show outdated Hide outdated core/app/models/spree/shipping_method.rb
@@ -15,6 +15,9 @@ class ShippingMethod < Spree::Base
has_many :shipping_method_zones
has_many :zones, through: :shipping_method_zones
has_many :shipping_method_stock_locations, class_name: Spree::ShippingMethodStockLocation

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

If I destroy a shipping method, what should happen?

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

If I destroy a shipping method, what should happen?

This comment has been minimized.

@jhawthorn

jhawthorn Oct 13, 2015

Member

As Ben says, this should be dependent: :destroy. Also, class_name should always be a string.

@jhawthorn

jhawthorn Oct 13, 2015

Member

As Ben says, this should be dependent: :destroy. Also, class_name should always be a string.

Show outdated Hide outdated core/app/models/spree/stock_location.rb
@@ -12,6 +12,9 @@ class InvalidMovementError < StandardError; end
belongs_to :state, class_name: 'Spree::State'
belongs_to :country, class_name: 'Spree::Country'
has_many :shipping_method_stock_locations, class_name: Spree::ShippingMethodStockLocation

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

I'm shutting down a stock location and destroying it in the admin. What should happen?

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

I'm shutting down a stock location and destroying it in the admin. What should happen?

This comment has been minimized.

@jhawthorn

jhawthorn Oct 13, 2015

Member

As Ben says, this should be dependent: :destroy. Also, class_name should always be a string.

@jhawthorn

jhawthorn Oct 13, 2015

Member

As Ben says, this should be dependent: :destroy. Also, class_name should always be a string.

class AddForeignKeyToShippingMethodStockLocation < ActiveRecord::Migration
def change
add_foreign_key :spree_shipping_method_stock_locations, :spree_shipping_methods, column: :shipping_method_id
add_foreign_key :spree_shipping_method_stock_locations, :spree_stock_locations, column: :stock_location_id

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

Oh man... Can't wait till rails 5 is out.

t.belongs_to :shipping_method, foreign_key: true
@BenMorganIO

BenMorganIO Oct 2, 2015

Contributor

Oh man... Can't wait till rails 5 is out.

t.belongs_to :shipping_method, foreign_key: true
@jhawthorn

This comment has been minimized.

Show comment
Hide comment
@jhawthorn

jhawthorn Oct 13, 2015

Member

@alexstoick and others, still hoping for discussion on how we want to consider a ShippingMethod with no associated stock locations.

I think no associated stock locations should mean the same as being tied to all stock locations. This means that:

  • We don't need a migration
  • any existing code/tests will work the same (no restrictions on stock location)
  • we avoid the factory issue discussed above

Thoughts?

Member

jhawthorn commented Oct 13, 2015

@alexstoick and others, still hoping for discussion on how we want to consider a ShippingMethod with no associated stock locations.

I think no associated stock locations should mean the same as being tied to all stock locations. This means that:

  • We don't need a migration
  • any existing code/tests will work the same (no restrictions on stock location)
  • we avoid the factory issue discussed above

Thoughts?

@athal7

This comment has been minimized.

Show comment
Hide comment
@athal7

athal7 Oct 13, 2015

Contributor

@jhawthorn personally I would lean towards explicitness here rather than ease of migration.

Contributor

athal7 commented Oct 13, 2015

@jhawthorn personally I would lean towards explicitness here rather than ease of migration.

@jhawthorn jhawthorn added this to the 1.2.0 milestone Oct 22, 2015

@alexstoick

This comment has been minimized.

Show comment
Hide comment
@alexstoick

alexstoick Oct 23, 2015

Contributor

@athal7 @jhawthorn I'm also 👍 on being explicit about something like this. I think it can be risky to leave this sort of logic hidden, especially as it represents one of the very central pieces of the system.

At LostMyName we have quite a complex shipping matrix, and having this sort of explicitness embedded in the system can sometimes makes things a bit more difficult to get off the ground, but in the long run we found it very valuable.


Contributor

alexstoick commented Oct 23, 2015

@athal7 @jhawthorn I'm also 👍 on being explicit about something like this. I think it can be risky to leave this sort of logic hidden, especially as it represents one of the very central pieces of the system.

At LostMyName we have quite a complex shipping matrix, and having this sort of explicitness embedded in the system can sometimes makes things a bit more difficult to get off the ground, but in the long run we found it very valuable.


t.belongs_to :shipping_method
t.belongs_to :stock_location
t.timestamps

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

For Rails 5:

t.timestamps null: false
@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

For Rails 5:

t.timestamps null: false
Show outdated Hide outdated core/app/models/spree/shipping_method_stock_location.rb
@@ -0,0 +1,6 @@
class Spree::ShippingMethodStockLocation < ActiveRecord::Base

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

This should probably become nested. That way people know that Spree is a module... Makes code investigation for newcomers easier.

Also, it should extend from Spree::Base, not ActiveRecord::Base. This way we get that spree_ table prefix.

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

This should probably become nested. That way people know that Spree is a module... Makes code investigation for newcomers easier.

Also, it should extend from Spree::Base, not ActiveRecord::Base. This way we get that spree_ table prefix.

Show outdated Hide outdated core/app/models/spree/shipping_method_stock_location.rb
@@ -0,0 +1,6 @@
class Spree::ShippingMethodStockLocation < ActiveRecord::Base
self.table_name = :spree_shipping_method_stock_locations

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

I think when you extend from Spree::Base, this line can disappear :D

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

I think when you extend from Spree::Base, this line can disappear :D

This comment has been minimized.

@alexstoick

alexstoick Oct 24, 2015

Contributor

TIL about Spree::Base

@alexstoick

alexstoick Oct 24, 2015

Contributor

TIL about Spree::Base

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 25, 2015

Contributor

:D

@BenMorganIO
Show outdated Hide outdated core/app/models/spree/stock_location.rb
@@ -12,6 +12,9 @@ class InvalidMovementError < StandardError; end
belongs_to :state, class_name: 'Spree::State'
belongs_to :country, class_name: 'Spree::Country'
has_many :shipping_method_stock_locations, dependent: :destroy, class_name: "Spree::ShippingMethodStockLocation"

This comment has been minimized.

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

Out of curiosity, was the :class_name required?

@BenMorganIO

BenMorganIO Oct 24, 2015

Contributor

Out of curiosity, was the :class_name required?

@athal7

This comment has been minimized.

Show comment
Hide comment
@athal7

athal7 Oct 28, 2015

Contributor

@alexstoick a few of the folks on the core team chatted about the question re explicit association to stock locations, and this is what we propose:

add a boolean to spree_shipping_methods that, when true, allows this shipping method to be used by all stock locations. We can make that boolean true by default, and have the UI of selecting stock locations for a shipping method only visible when that boolean is unchecked.

How does that sound?

Contributor

athal7 commented Oct 28, 2015

@alexstoick a few of the folks on the core team chatted about the question re explicit association to stock locations, and this is what we propose:

add a boolean to spree_shipping_methods that, when true, allows this shipping method to be used by all stock locations. We can make that boolean true by default, and have the UI of selecting stock locations for a shipping method only visible when that boolean is unchecked.

How does that sound?

@alexstoick

This comment has been minimized.

Show comment
Hide comment
@alexstoick

alexstoick Jan 11, 2016

Contributor

@athal7 Made these changes to the code but I can't work out a suitable name for said boolean - using available_for_all at the minute.

Also another headache-y bit is represented by the shipping_methods method which now gets a bit messy. I have to approaches there (active one + commented out one), but I'm open to suggestions on how to make either nicer!

It'd be good to get your thoughts on this before going to Admin side of things!

Contributor

alexstoick commented Jan 11, 2016

@athal7 Made these changes to the code but I can't work out a suitable name for said boolean - using available_for_all at the minute.

Also another headache-y bit is represented by the shipping_methods method which now gets a bit messy. I have to approaches there (active one + commented out one), but I'm open to suggestions on how to make either nicer!

It'd be good to get your thoughts on this before going to Admin side of things!

@jhawthorn jhawthorn modified the milestones: 1.3.0, 1.2.0 Jan 13, 2016

@athal7

This comment has been minimized.

Show comment
Hide comment
@athal7

athal7 Jan 13, 2016

Contributor

@alexstoick thanks!

re the boolean, maybe something a bit more explicit would be an improvement? e.g. supported_by_all_stock_locations

re the shipping_methods method, did the suggestion I provided not work for you?

Contributor

athal7 commented Jan 13, 2016

@alexstoick thanks!

re the boolean, maybe something a bit more explicit would be an improvement? e.g. supported_by_all_stock_locations

re the shipping_methods method, did the suggestion I provided not work for you?

@cbrunsdon

This comment has been minimized.

Show comment
Hide comment
@cbrunsdon

cbrunsdon Jan 18, 2016

Member

I think I'm 👍 on this, and would like to move on this as early in the 1.3 window as possible.

Thanks a lot for your work @alexstoick, really appreciate the follow-through on this.

Member

cbrunsdon commented Jan 18, 2016

I think I'm 👍 on this, and would like to move on this as early in the 1.3 window as possible.

Thanks a lot for your work @alexstoick, really appreciate the follow-through on this.

@jhawthorn

This comment has been minimized.

Show comment
Hide comment
@jhawthorn

jhawthorn Jan 20, 2016

Member

👍

Member

jhawthorn commented Jan 20, 2016

👍

jhawthorn added a commit that referenced this pull request Jan 20, 2016

Merge pull request #412 from Lostmyname/feature/stock-location-shippi…
…ng-method-relationship

Stock location <--> Shipping method relationship

@jhawthorn jhawthorn merged commit ac2fd7b into solidusio:master Jan 20, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@@ -14,6 +14,9 @@ class ShippingMethod < Spree::Base
has_many :zones, through: :shipping_method_zones
belongs_to :tax_category, -> { with_deleted }, :class_name => 'Spree::TaxCategory'
has_many :shipping_method_stock_locations, class_name: Spree::ShippingMethodStockLocation

This comment has been minimized.

@BenMorganIO

BenMorganIO Jan 21, 2016

Contributor

It looks like this was a duplicate.

@BenMorganIO

BenMorganIO Jan 21, 2016

Contributor

It looks like this was a duplicate.

@sebfie

This comment has been minimized.

Show comment
Hide comment
@sebfie

sebfie Jun 29, 2016

Is the backend view missing to link a stock to a shipping method? I can not find options in my backend.

sebfie commented Jun 29, 2016

Is the backend view missing to link a stock to a shipping method? I can not find options in my backend.

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