Make AvailabilityValidator check inventory per StockLocation instead of per Shipment #1693

jhawthorn · 2017-01-26T22:31:13Z

This fixes a few issues with AvailabilityValidator

Stock was being checked per Shipment rather than per StockLocation. This meant that available inventory could be double counted towards the same order. Quantities is now checked per StockLocation.

If multiple Shipments (or now StockLocations) on one LineItem had insufficient inventory, the same error would be added multiple times to the line item. ex. ["Quantity selected of \"Product #20 - 5904 (Size: S)\" is not available.", "Quantity selected of \"Product #20 - 5904 (Size: S)\" is not available."]

One Shipment record was being queried and loaded for each inventory attached to the line item, which is quite slow and unnecessary. This has been replaced with a single query per LineItem.

Before:

> Spree::Stock::AvailabilityValidator.new.validate(line_item)
SELECT "spree_inventory_units".* FROM "spree_inventory_units" WHERE "spree_inventory_units"."line_item_id" = ?  [["line_item_id", 4]]
SELECT  "spree_shipments".* FROM "spree_shipments" WHERE "spree_shipments"."id" = ? LIMIT ?  [["id", 8], ["LIMIT", 1]]
SELECT  "spree_shipments".* FROM "spree_shipments" WHERE "spree_shipments"."id" = ? LIMIT ?  [["id", 8], ["LIMIT", 1]]
SELECT  "spree_shipments".* FROM "spree_shipments" WHERE "spree_shipments"."id" = ? LIMIT ?  [["id", 9], ["LIMIT", 1]]
SELECT  "spree_shipments".* FROM "spree_shipments" WHERE "spree_shipments"."id" = ? LIMIT ?  [["id", 9], ["LIMIT", 1]]
SELECT  "spree_stock_locations".* FROM "spree_stock_locations" WHERE "spree_stock_locations"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
SELECT  "spree_variants".* FROM "spree_variants" WHERE "spree_variants"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
SELECT SUM("spree_stock_items"."count_on_hand") FROM "spree_stock_items" INNER JOIN "spree_stock_locations" ON "spree_stock_locations"."id" = "spree_stock_items"."stock_location_id" WHERE "spree_stock_items"."deleted_at" IS NULL AND "spree_stock_items"."variant_id" = 1 AND "spree_stock_items"."stock_location_id" = 1
SELECT  "spree_stock_locations".* FROM "spree_stock_locations" WHERE "spree_stock_locations"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
SELECT SUM("spree_stock_items"."count_on_hand") FROM "spree_stock_items" INNER JOIN "spree_stock_locations" ON "spree_stock_locations"."id" = "spree_stock_items"."stock_location_id" WHERE "spree_stock_items"."deleted_at" IS NULL AND "spree_stock_items"."variant_id" = 1 AND "spree_stock_items"."stock_location_id" = 1

After:

> Spree::Stock::AvailabilityValidator.new.validate(line_item)
SELECT  1 AS one FROM "spree_inventory_units" WHERE "spree_inventory_units"."line_item_id" = ? LIMIT ?  [["line_item_id", 4], ["LIMIT", 1]]
SELECT COUNT(*) AS count_all, "stock_location_id" AS stock_location_id FROM "spree_inventory_units" INNER JOIN "spree_shipments" ON "spree_shipments"."id" = "spree_inventory_units"."shipment_id" WHERE "spree_inventory_units"."line_item_id" = ? AND "spree_inventory_units"."pending" = ? GROUP BY "stock_location_id"  [["line_item_id", 4], ["pending", true]]
SELECT  "spree_variants".* FROM "spree_variants" WHERE "spree_variants"."id" = ? LIMIT ?  [["id", 1], ["LIMIT", 1]]
SELECT SUM("spree_stock_items"."count_on_hand") FROM "spree_stock_items" WHERE "spree_stock_items"."deleted_at" IS NULL AND "spree_stock_items"."variant_id" = 1 AND "spree_stock_items"."stock_location_id" = 1
SELECT "spree_stock_items".* FROM "spree_stock_items" WHERE "spree_stock_items"."deleted_at" IS NULL AND "spree_stock_items"."variant_id" = 1 AND "spree_stock_items"."stock_location_id" = 1

vfonic · 2017-02-17T07:10:04Z

core/spec/models/spree/stock/availability_validator_spec.rb

@@ -54,39 +54,85 @@ module Stock
      context "line_item is part of a shipment" do
        let!(:order) { create(:order_with_line_items) }

-        context "has stock in all stock locations" do
+        context "has stock in one stock locations" do


nitpick: "location"

mamhoff · 2017-02-24T20:45:44Z

This is really neat. However, I see some commits do not pass CI. :(

jhawthorn · 2017-02-27T19:57:10Z

This is really neat. However, I see some commits do not pass CI. :(

I believe that was just the second to last commit which I've rebased and amended the last commit fixing the spec failure. I'm running a git rebase -x rspec locally to double check

We can use ActiveRecord to build queries instead of a hash, which allows us to reuse StockLocation.active and makes it a little more readable.

These specs exibit two issues when a line item is split across multiple shipments. If the line item is split across shipments, any availabilty errors will be reported twice. If a shipment is split across shipments with the same stock location, it won't detect insufficient stock if both shipments are able to separately fulfill the inventory.

Previously stock was checked per-shipment which could lead to incorrect results as the same inventory from one stock location could be double-counted across multiple shipments. This also greatly reduces the number of queries, by replacing one Shipment load per inventory_unit with a single query to get the quantity of inventory units per stock location.

We can avoid selecting StockLocation records by just passing their id to Stock::Quantifier

Instead of relying on the behaviour of stock_location_quantities and the entire order state machine, we should just create shipments and inventory units in the state we expect. This adds tests for the line item needing some stock from each stock location. This adds one skipped test which fails because the error on the line item is added twice.

We shouldn't report the same error twice on the same item.

jhawthorn added the Code review needed label Feb 16, 2017

vfonic reviewed Feb 17, 2017

View reviewed changes

mamhoff approved these changes Feb 24, 2017

View reviewed changes

jhawthorn force-pushed the availability_validator branch from 57210f6 to f0777d4 Compare February 27, 2017 19:56

jhawthorn added 7 commits February 27, 2017 12:06

Simplify Stock::Quantifier query building

658a048

We can use ActiveRecord to build queries instead of a hash, which allows us to reuse StockLocation.active and makes it a little more readable.

Avoid unnecessary join in Stock::Quantifier

8f35cd1

Reduce queries in AvailabilityValidator using ids

e6163f2

We can avoid selecting StockLocation records by just passing their id to Stock::Quantifier

Only add errors once in AvailabilityValidator

a2e4ad6

We shouldn't report the same error twice on the same item.

jhawthorn force-pushed the availability_validator branch from f0777d4 to a2e4ad6 Compare February 27, 2017 21:22

jhawthorn changed the title ~~Make InventoryValidator check inventory per StockLocation instead of per Shipment~~ Make AvailabilityValidator check inventory per StockLocation instead of per Shipment Feb 27, 2017

Add AvailabilityValidator change to CHANGELOG

fff20eb

jhawthorn merged commit fb70a41 into solidusio:master Feb 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make AvailabilityValidator check inventory per StockLocation instead of per Shipment #1693

Make AvailabilityValidator check inventory per StockLocation instead of per Shipment #1693

jhawthorn commented Jan 26, 2017 •

edited

Loading

vfonic Feb 17, 2017

mamhoff commented Feb 24, 2017

jhawthorn commented Feb 27, 2017

Make AvailabilityValidator check inventory per StockLocation instead of per Shipment #1693

Make AvailabilityValidator check inventory per StockLocation instead of per Shipment #1693

Conversation

jhawthorn commented Jan 26, 2017 • edited Loading

vfonic Feb 17, 2017

Choose a reason for hiding this comment

mamhoff commented Feb 24, 2017

jhawthorn commented Feb 27, 2017

jhawthorn commented Jan 26, 2017 •

edited

Loading