Raise InsufficientStock if and only if there is insufficient stock #566

jhawthorn · 2015-12-04T02:20:57Z

Previously, InsufficientStock was raised if any StockLocations were fully out of inventory. This was incorrect because it was possible other stock locations could have fulfilled the inventory. This was also incorrect because the stock location could have some, but insufficient inventory, and not raise the exception (an incomplete package would be returned). Now the coordinator checks that the package is complete and raises InsufficientStock if it is incomplete for any reason.

Stock::Packer currently raises InsufficientStock under specific circumstances:

There is a stock item for a variant in the stock location.
There is exactly 0 inventory remaining for that variant in the stock location.
The variant does not allow backordering.

This is odd, because it doesn't raise InsufficientStock when there is some but insufficient inventory (it adds the amount available to the package), or if the stock item doesn't exist at all in that stock location. These three cases should probably be handled in the same way.

The way the Packer is used is to build the fullest possible package for this stock location. As it's possible to fulfill an variant half from one location and half from another, this method should never raise InsufficientStock because it has insufficient information to do so.

gmacdougall · 2015-12-04T12:35:43Z

👍 I agree with your logic.

richardnuno · 2015-12-04T16:12:11Z

I'm concerned this will break the behavior this commit was trying to correct.

I think that if we remove this we need to add a check further down the line that everything in the order can be shipped when generating shipments.

cbrunsdon · 2015-12-04T22:06:01Z

@richardnuno I don't 100% follow what that check is guarding against.

If you could outline what a test would look like that would cover the case you're worried about, I think either Hawth or myself could write it up.

richardnuno · 2015-12-04T22:10:13Z

@cbrunsdon sure! The test would ensure that you cannot transition an order to the delivery state if any of the order's contents couldn't be shipped.

jhawthorn · 2015-12-04T23:55:19Z

I wrote some specs to demonstrate the behaviour of that check. This asserts that InsufficientStock is raised whenever there isn't enough inventory.

Here is the result:

Spree::Stock::Coordinator  with no backordering
    with a single stock location
      with no inventory
        behaves like an unfulfillable package
          raises exception
      with insufficient inventory
        behaves like an unfulfillable package
          raises exception (FAILED - 1)
      with sufficient inventory
        behaves like a fulfillable package
          packages correctly
    with two stock locations
      with no inventory
        behaves like an unfulfillable package
          raises exception
      with some but insufficient inventory in each location
        behaves like an unfulfillable package
          raises exception (FAILED - 2)
      has sufficient inventory in the first location
        behaves like a fulfillable package
          packages correctly (FAILED - 3)
      has sufficient inventory in the second location
        behaves like a fulfillable package
          packages correctly (FAILED - 4)
      with sufficient inventory across both locations
        behaves like a fulfillable package
          packages correctly (PENDING: This is broken. The coordinator packages this incorrectly)
      with sufficient inventory in both locations
        behaves like a fulfillable package
          packages correctly

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) Spree::Stock::Coordinator with no backordering with two stock locations with sufficient inventory across both locations behaves like a fulfillable package packages correctly
     # This is broken. The coordinator packages this incorrectly
     Failure/Error: expect(packages.map(&:quantity).sum).to eq(5)

       expected: 5
            got: 3

       (compared using ==)
     Shared Example Group: "a fulfillable package" called from ./spec/models/spree/stock/coordinator_spec.rb:170
     # ./spec/models/spree/stock/coordinator_spec.rb:107:in `block (4 levels) in <module:Stock>'

Failures:

  1) Spree::Stock::Coordinator with no backordering with a single stock location with insufficient inventory behaves like an unfulfillable package raises exception
     Failure/Error: expect{ packages }.to raise_error(Spree::Order::InsufficientStock)
       expected Spree::Order::InsufficientStock but nothing was raised
     Shared Example Group: "an unfulfillable package" called from ./spec/models/spree/stock/coordinator_spec.rb:126
     # ./spec/models/spree/stock/coordinator_spec.rb:113:in `block (4 levels) in <module:Stock>'

  2) Spree::Stock::Coordinator with no backordering with two stock locations with some but insufficient inventory in each location behaves like an unfulfillable package raises exception
     Failure/Error: expect{ packages }.to raise_error(Spree::Order::InsufficientStock)
       expected Spree::Order::InsufficientStock but nothing was raised
     Shared Example Group: "an unfulfillable package" called from ./spec/models/spree/stock/coordinator_spec.rb:151
     # ./spec/models/spree/stock/coordinator_spec.rb:113:in `block (4 levels) in <module:Stock>'

  3) Spree::Stock::Coordinator with no backordering with two stock locations has sufficient inventory in the first location behaves like a fulfillable package packages correctly
     Failure/Error: let(:packages) { subject.packages }
     Spree::Order::InsufficientStock:
       Spree::Order::InsufficientStock
     Shared Example Group: "a fulfillable package" called from ./spec/models/spree/stock/coordinator_spec.rb:157
     # ./app/models/spree/stock/packer.rb:26:in `block in default_package'
     # ./app/models/spree/stock/packer.rb:22:in `each'
     # ./app/models/spree/stock/packer.rb:22:in `default_package'
     # ./app/models/spree/stock/packer.rb:16:in `packages'
     # ./app/models/spree/stock/coordinator.rb:61:in `block in build_packages'
     # ./app/models/spree/stock/coordinator.rb:58:in `build_packages'
     # ./app/models/spree/stock/coordinator.rb:20:in `packages'
     # ./spec/models/spree/stock/coordinator_spec.rb:102:in `block (3 levels) in <module:Stock>'
     # ./spec/models/spree/stock/coordinator_spec.rb:106:in `block (4 levels) in <module:Stock>'

  4) Spree::Stock::Coordinator with no backordering with two stock locations has sufficient inventory in the second location behaves like a fulfillable package packages correctly
     Failure/Error: let(:packages) { subject.packages }
     Spree::Order::InsufficientStock:
       Spree::Order::InsufficientStock
     Shared Example Group: "a fulfillable package" called from ./spec/models/spree/stock/coordinator_spec.rb:163
     # ./app/models/spree/stock/packer.rb:26:in `block in default_package'
     # ./app/models/spree/stock/packer.rb:22:in `each'
     # ./app/models/spree/stock/packer.rb:22:in `default_package'
     # ./app/models/spree/stock/packer.rb:16:in `packages'
     # ./app/models/spree/stock/coordinator.rb:61:in `block in build_packages'
     # ./app/models/spree/stock/coordinator.rb:58:in `build_packages'
     # ./app/models/spree/stock/coordinator.rb:20:in `packages'
     # ./spec/models/spree/stock/coordinator_spec.rb:102:in `block (3 levels) in <module:Stock>'

The existing check works in a minority of cases, only when the stock location is fully out of stock. It will silently makes a package with insufficient inventory if there is some, but insufficient stock.

This also raises the error incorrectly if one stock location is empty, but the other has sufficient inventory.

These new specs also presented another issue: packaging seems to be broken when it need inventory from multiple stock locations to be fulfilled. This is the pending spec here.

We probably do want to check for this elsewhere (maybe at the end of package coordination) and raise exceptions.

jhawthorn · 2015-12-15T22:48:10Z

@richardnuno does this cover the case you are concerned about?

I'm concerned that neither of these changes really broke any tests. I suspect there is not good coverage of the various rescue_from Spree::Order::InsufficientStock sprinkled through controllers.

richardnuno · 2015-12-16T14:19:40Z

@jhawthorn yes this covers it, thanks!

jhawthorn · 2015-12-21T22:09:07Z

Rebased against latest master

jhawthorn · 2016-01-06T22:33:07Z

Rebased again and added changelog entry

Stock::Packer currently raises InsufficientStock under specific circumstances: * There is a stock item for a variant in the stock location. * There is exactly 0 inventory remaining for that variant in the stock location. * The variant does not allow backordering. This is odd, because it doesn't raise InsufficientStock when there is some but insufficient inventory (it adds the amount available to the package), or if the stock item doesn't exist at all in that stock location. These three cases should probably be handled in the same way. The way the Packer is used is to build the fullest possible package for this stock location. As it's possible to fulfill an variant half from one location and half from another, this method should not be raising InsufficientStock because it has insufficient information to do so.

Instead of raising this inside of the Packer (and only in some cases), raise it any time our final package didn't contain the full requsted quantity.

Since the previous spec having stock locations with insufficient stock failed (and is marked pending) I've added two more examples which do pass.

cbrunsdon · 2016-01-14T19:42:12Z

Those tests are magnificent. Thanks @jhawthorn, I don't have any objections to the PR, and its definitely moving us in the right direction.

If it has any unintended side-effects I can't think of where, but we'll have to deal with them if and when they come up.

Also from now on I'm going to use these as a reference for writing clean tests in these types of situations.

Raise InsufficientStock if and only if there is insufficient stock

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch from d834fcb to b3fd0b3 Compare December 4, 2015 23:52

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch from b3fd0b3 to 16f7893 Compare December 14, 2015 19:52

jhawthorn mentioned this pull request Dec 15, 2015

Packaging fails if multiple stock locations are required to fulfill quantity #583

Closed

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch from 16f7893 to eb0008d Compare December 15, 2015 22:36

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch from eb0008d to 77c647c Compare December 21, 2015 22:08

jhawthorn changed the title ~~Stock::Packer should not raise InsufficientStock~~ Fix checks which raise InsufficientStock when packaging Jan 6, 2016

jhawthorn changed the title ~~Fix checks which raise InsufficientStock when packaging~~ Raise InsufficientStock if and only if there is insufficient stock Jan 6, 2016

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch from 77c647c to 18fce61 Compare January 6, 2016 22:32

jhawthorn added the type:bug Error, flaw or fault label Jan 6, 2016

jhawthorn added this to the 1.2.0 milestone Jan 6, 2016

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch 2 times, most recently from de25d2b to 9705017 Compare January 13, 2016 01:06

jhawthorn added 4 commits January 13, 2016 22:02

Coordinator to raise InsufficientStock on error

d3e5ced

Instead of raising this inside of the Packer (and only in some cases), raise it any time our final package didn't contain the full requsted quantity.

Add additional coordinator tests

2ad32e3

Since the previous spec having stock locations with insufficient stock failed (and is marked pending) I've added two more examples which do pass.

Add changelog entry for InsufficientStock change

c1bac98

jhawthorn force-pushed the packer_dont_raise_none_on_hand branch from 9705017 to c1bac98 Compare January 14, 2016 06:02

cbrunsdon added a commit that referenced this pull request Jan 14, 2016

Merge pull request #566 from jhawthorn/packer_dont_raise_none_on_hand

eda305e

Raise InsufficientStock if and only if there is insufficient stock

cbrunsdon merged commit eda305e into solidusio:master Jan 14, 2016

jhawthorn deleted the packer_dont_raise_none_on_hand branch January 14, 2016 19:43

DanielePalombo mentioned this pull request Jul 29, 2016

Rescue Spree::Order::InsufficientStock in backend #1359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise InsufficientStock if and only if there is insufficient stock #566

Raise InsufficientStock if and only if there is insufficient stock #566

jhawthorn commented Dec 4, 2015

gmacdougall commented Dec 4, 2015

richardnuno commented Dec 4, 2015

cbrunsdon commented Dec 4, 2015

richardnuno commented Dec 4, 2015

jhawthorn commented Dec 4, 2015

jhawthorn commented Dec 15, 2015

richardnuno commented Dec 16, 2015

jhawthorn commented Dec 21, 2015

jhawthorn commented Jan 6, 2016

cbrunsdon commented Jan 14, 2016

Raise InsufficientStock if and only if there is insufficient stock #566

Raise InsufficientStock if and only if there is insufficient stock #566

Conversation

jhawthorn commented Dec 4, 2015

gmacdougall commented Dec 4, 2015

richardnuno commented Dec 4, 2015

cbrunsdon commented Dec 4, 2015

richardnuno commented Dec 4, 2015

jhawthorn commented Dec 4, 2015

jhawthorn commented Dec 15, 2015

richardnuno commented Dec 16, 2015

jhawthorn commented Dec 21, 2015

jhawthorn commented Jan 6, 2016

cbrunsdon commented Jan 14, 2016