Stock location sorters #2783

aldesantis · 2018-06-23T13:49:02Z

This introduces the concept of a stock location sorter, which takes a collection of stock locations and returns them in the right order for inventory allocation.

It also updates SimpleCoordinator#allocate_inventory to respect the order of stock locations returned by the sorter.

core/app/models/spree/stock/simple_coordinator.rb

aldesantis · 2018-07-10T14:06:02Z

Improved this with a better, configurable design that doesn't require opening and modifying SimpleCoordinator.

kennyadsl

Great change, just left some thoughts. Thanks!

kennyadsl · 2018-07-11T07:45:30Z

core/app/models/spree/stock/simple_coordinator.rb

@@ -23,7 +23,7 @@ def initialize(order, inventory_units = nil)
        @order = order
        @inventory_units = inventory_units || InventoryUnitBuilder.new(order).units
        @splitters = Spree::Config.environment.stock_splitters
-        @stock_locations = Spree::StockLocation.active
+        @stock_locations = Spree::Config.stock_location_sorter_class.new(Spree::StockLocation.active).sort


I'd use another variable or method @sorted_stock_lcoations for that. It should also help backward compatibility if someone extended this class and was using the @stock_locations ivar. what do you think?

@kennyadsl the problem with that is if someone overrides @stock_locations and we sort in a different variable (let's say @sorted_stock_locations), they also have to override @sorted_stock_locations, otherwise it will contain the result of the sorting of the original @stock_locations variable.

Yes, you are right. One option could be creating another @active_stock_locations ivar, use that in the whole class instead of @stock_locations which could be deprecated with DeprecatedInstanceVariableProxy. I feel like it's too complex though, maybe a changelog line is enough here. I'd say, let's see what others think before.

Talked with @gmacdougall about this. Since the default sorted is returning the same thing as before maybe no one will have unexpected behavior with this, so I'm ok leaving it as is.

core/app/models/spree/stock/simple_coordinator.rb

aldesantis · 2018-07-12T10:57:06Z

@kennyadsl this was documented in the guides. Please let me know how they look.

kennyadsl · 2018-07-12T12:18:58Z

guides/source/developers/shipments/stock-location-sorters.html.md

+  class Application < Rails::Application
+    # ...
+
+    initializer 'spree.register.calculators' do |app|


why calculators?

@kennyadsl fixed.

kennyadsl · 2018-07-12T13:16:47Z

guides/source/developers/shipments/stock-location-sorters.html.md

+
+### Switching the sorter
+
+Once you have created the logic for the new shipping calculator, you need to


maybe this text as well?

Ops, there are failing specs, can you take a look please?

kennyadsl · 2018-09-17T09:14:49Z

@aldesantis can you please rebase against master here?

aldesantis · 2018-09-17T11:21:37Z

@kennyadsl done.

gmacdougall

This looks great! Thanks for the hard work on this one.

gmacdougall · 2018-09-17T19:49:04Z

There's just a failing spec that needs to be addressed.

aldesantis · 2018-09-18T08:23:04Z

@gmacdougall addressed.

tvdeyen

Very great work. I like the concept 👍

IMO the configuration and the module namespace should be reflecting that we are sorting stock locations, like your PR says.

tvdeyen · 2018-09-18T21:09:34Z

core/app/models/spree/stock/simple_coordinator.rb

@@ -23,7 +23,7 @@ def initialize(order, inventory_units = nil)
        @order = order
        @inventory_units = inventory_units || InventoryUnitBuilder.new(order).units
        @splitters = Spree::Config.environment.stock_splitters
-        @stock_locations = Spree::StockLocation.active
+        @stock_locations = Spree::Config.stock.sorter_class.new(Spree::StockLocation.active).sort


We should name this

Spree::Config.stock.location_sorter_class

tvdeyen · 2018-09-18T21:11:13Z

core/app/models/spree/stock/sorter/base.rb

+
+module Spree
+  module Stock
+    module Sorter


The Sorter module should be renamed

Spree::Stock::LocationSorter::Base

It would also be really good to add some top-level docs as well.

tvdeyen · 2018-09-18T21:11:22Z

core/app/models/spree/stock/sorter/default_first.rb

+
+module Spree
+  module Stock
+    module Sorter


tvdeyen · 2018-09-18T21:11:47Z

core/lib/spree/core/stock_configuration.rb

@@ -5,6 +5,7 @@ module Core
    class StockConfiguration
      attr_writer :coordinator_class
      attr_writer :estimator_class
+      attr_writer :sorter_class


location_sorter_class

Can also do:

attr_writer :coordinator_class, :estimator_class, :sorter_class

tvdeyen · 2018-09-18T21:12:03Z

core/lib/spree/core/stock_configuration.rb

@@ -15,6 +16,11 @@ def estimator_class
        @estimator_class ||= '::Spree::Stock::Estimator'
        @estimator_class.constantize
      end
+
+      def sorter_class


tvdeyen · 2018-09-18T21:12:12Z

core/spec/lib/spree/core/stock_configuration_spec.rb

@@ -37,4 +37,21 @@
      end
    end
  end
+  describe '#sorter_class' do


Missing newline

BenMorganIO · 2018-09-21T17:31:11Z

core/spec/models/spree/stock/sorter/default_first_spec.rb

+module Spree
+  module Stock
+    module Sorter
+      RSpec.describe DefaultFirst, type: :model do


This looks weird. Why not just do:

describe Spree::Stock::Sorter::DefaultFirst do end

Why the need for type: :model?

@BenMorganIO I agree that it's weird, but this is how specs are written for the other models too (including type: :model), so I chose to stick with the convention here.

aldesantis · 2018-09-28T07:43:53Z

@BenMorganIO @tvdeyen @kennyadsl I addressed the requested changes (sorry for the delay!) in separate commits so you can look at them. Let me know if it all looks good and I will squash the commits together.

EDIT: Nevermind, squashed now per @kennyadsl's request.

kennyadsl

Thanks!

CHANGELOG.md

aldesantis · 2018-10-12T18:22:23Z

@tvdeyen @BenMorganIO I think this might be ready for merging?

kennyadsl · 2018-10-12T19:11:03Z

@aldesantis specs failures seem related, can you please take a look?

Specs are failing

This makes it easier to change the order of @stock_locations (e.g. by shipping from the default location first) in SimpleCoordinator by introducing the concept of stock location sorter and ensuring the order of stock locations is preserved when allocating inventory.

aldesantis · 2018-10-13T09:40:21Z

@kennyadsl sorry about that, it should be okay now.

aldesantis · 2018-10-17T08:12:28Z

🎉

BenMorganIO reviewed Jul 3, 2018

View reviewed changes

core/app/models/spree/stock/simple_coordinator.rb Show resolved Hide resolved

kennyadsl added the WIP label Jul 9, 2018

aldesantis force-pushed the simple-coordinator-extension branch from 1c3688e to b38e58e Compare July 10, 2018 14:05

aldesantis force-pushed the simple-coordinator-extension branch 3 times, most recently from abdeaf8 to 622e2cf Compare July 10, 2018 14:13

aldesantis changed the title ~~Make it easier to extend @stock_locations in SimpleCoordinator~~ Stock location sorters Jul 10, 2018

aldesantis force-pushed the simple-coordinator-extension branch from 622e2cf to 788c2c8 Compare July 10, 2018 14:16

kennyadsl reviewed Jul 11, 2018

View reviewed changes

kennyadsl removed the WIP label Jul 11, 2018

aldesantis force-pushed the simple-coordinator-extension branch from 788c2c8 to 4943026 Compare July 12, 2018 10:56

aldesantis force-pushed the simple-coordinator-extension branch from 4943026 to 5d555ef Compare July 12, 2018 11:18

kennyadsl reviewed Jul 12, 2018

View reviewed changes

aldesantis force-pushed the simple-coordinator-extension branch from 5d555ef to a383c08 Compare July 12, 2018 13:14

kennyadsl reviewed Jul 12, 2018

View reviewed changes

aldesantis force-pushed the simple-coordinator-extension branch 2 times, most recently from a4192c1 to 193c89e Compare July 12, 2018 13:23

kennyadsl previously approved these changes Jul 12, 2018

View reviewed changes

aldesantis force-pushed the simple-coordinator-extension branch 2 times, most recently from 9b53177 to 39cdde4 Compare July 12, 2018 14:37

kennyadsl assigned aldesantis Sep 17, 2018

aldesantis force-pushed the simple-coordinator-extension branch from 39cdde4 to 93e30af Compare September 17, 2018 11:21

gmacdougall approved these changes Sep 17, 2018

View reviewed changes

aldesantis force-pushed the simple-coordinator-extension branch from 93e30af to 0ddbd8d Compare September 18, 2018 08:21

ericsaupe approved these changes Sep 18, 2018

View reviewed changes

tvdeyen reviewed Sep 18, 2018

View reviewed changes

BenMorganIO reviewed Sep 21, 2018

View reviewed changes

aldesantis force-pushed the simple-coordinator-extension branch 3 times, most recently from 22cf6e0 to f2a476f Compare September 28, 2018 08:01

kennyadsl approved these changes Sep 28, 2018

View reviewed changes

kennyadsl requested changes Sep 28, 2018

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

aldesantis force-pushed the simple-coordinator-extension branch from f2a476f to 60ec029 Compare September 28, 2018 08:59

aldesantis force-pushed the simple-coordinator-extension branch 2 times, most recently from df533a4 to 76db15d Compare October 12, 2018 18:21

kennyadsl previously approved these changes Oct 12, 2018

View reviewed changes

aldesantis added 2 commits October 13, 2018 11:32

Write stock location sorters guide

c0bdf54

aldesantis force-pushed the simple-coordinator-extension branch from 76db15d to c0bdf54 Compare October 13, 2018 09:32

kennyadsl merged commit 5ecf43f into solidusio:master Oct 17, 2018

aldesantis deleted the simple-coordinator-extension branch October 17, 2018 08:12

kennyadsl mentioned this pull request Nov 15, 2018

Add acts_as_list to Spree::StockLocation #2953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stock location sorters #2783

Stock location sorters #2783

aldesantis commented Jun 23, 2018 •

edited

Loading

aldesantis commented Jul 10, 2018 •

edited

Loading

kennyadsl left a comment

kennyadsl Jul 11, 2018

aldesantis Jul 11, 2018 •

edited

Loading

kennyadsl Jul 11, 2018

kennyadsl Jul 11, 2018

aldesantis commented Jul 12, 2018

kennyadsl Jul 12, 2018

aldesantis Jul 12, 2018

kennyadsl Jul 12, 2018

kennyadsl commented Sep 17, 2018

aldesantis commented Sep 17, 2018

gmacdougall left a comment

gmacdougall commented Sep 17, 2018

aldesantis commented Sep 18, 2018

tvdeyen left a comment

tvdeyen Sep 18, 2018

tvdeyen Sep 18, 2018

BenMorganIO Sep 21, 2018

tvdeyen Sep 18, 2018

tvdeyen Sep 18, 2018

BenMorganIO Sep 21, 2018

tvdeyen Sep 18, 2018

tvdeyen Sep 18, 2018

BenMorganIO Sep 21, 2018

BenMorganIO Sep 21, 2018

aldesantis Sep 28, 2018

aldesantis commented Sep 28, 2018 •

edited

Loading

kennyadsl left a comment

aldesantis commented Oct 12, 2018

kennyadsl commented Oct 12, 2018

aldesantis commented Oct 13, 2018

aldesantis commented Oct 17, 2018


		### Switching the sorter

		Once you have created the logic for the new shipping calculator, you need to

Stock location sorters #2783

Stock location sorters #2783

Conversation

aldesantis commented Jun 23, 2018 • edited Loading

aldesantis commented Jul 10, 2018 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldesantis Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldesantis commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Sep 17, 2018

aldesantis commented Sep 17, 2018

gmacdougall left a comment

Choose a reason for hiding this comment

gmacdougall commented Sep 17, 2018

aldesantis commented Sep 18, 2018

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldesantis commented Sep 28, 2018 • edited Loading

kennyadsl left a comment

Choose a reason for hiding this comment

aldesantis commented Oct 12, 2018

kennyadsl commented Oct 12, 2018

aldesantis commented Oct 13, 2018

aldesantis commented Oct 17, 2018

aldesantis commented Jun 23, 2018 •

edited

Loading

aldesantis commented Jul 10, 2018 •

edited

Loading

aldesantis Jul 11, 2018 •

edited

Loading

aldesantis commented Sep 28, 2018 •

edited

Loading