[Spree Upgrade] Scope Variants to VariantOverrides when order is changed

[luisramos0]

Original title: InventoryUnit.increase is gone in Spree 2.0 (related to variant scoping)

OFN 2-0-STABLE build errors, for example:

bundle exec rspec ./spec/controllers/admin/bulk_line_items_controller_spec.rb:14

`Admin::BulkLineItemsController#index` as an administrator when no ransack params are passed in retrieves a list of `line_items` with appropriate attributes, including line items with appropriate attributes

Failure/Error: Spree::InventoryUnit.increase(order, variant, quantity)
NoMethodError: undefined method `increase' for #<Class:0x007f913c1d4488>

The InventoryUnit.increase and decrease methods were managing "both variant.count_on_hand and inventory unit creation". This was moved to stock locations and shipments in spree 2.
This is related to Spree 2 Multiple Shipments epic #2011.

There are two different calls to this increase method. Both for scoping of variant to hub/shop when managing stock in inventory.

/app/models/spree/inventory_unit_decorator.rb

In InventoryUnit.class_eval - assign_opening_inventory
When finalizing an order we were extending InventoryUnit to scope each variant before managing stock.
Added statements to the original Spree code:
scoper = OpenFoodNetwork::ScopeVariantToHub.new(order.distributor)
scoper.scope(line_item.variant)

This assign_opening_inventory was removed and the stock movement is now handled in a new shipment.finalize where stock locations are updated (manifest_unstock).

Possible solution is to:

• drop inventory_unit_decorator
• insert the variant scoping somewhere in the shipment.finalize method

Shipment.class_eval do
def finalize!
--- do the scoping of variants in inventory_units here
InventoryUnit.finalize_units!(inventory_units)
manifest.each { |item| manifest_unstock(item) }
end

/app/models/spree/line_item_decorator.rb

On update_inventory and remove_inventory, the spree code is copied and the variant is scoped.

Possible solution:

• remove monkey patching line_item_decorator.rb
• patch for scoping on new class order_inventory

In both cases, the first step is to remove the existing patching with the scoping.

[luisramos0]

Right now I am not sure how/where to do the variant scoping. I think it might need further group discussion. I'll raise this one in the next catch up.
Hopefully we find a better way than .class_eval...ing the spree models...

[luisramos0]

This issue is related to this wiki page:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Spree-Upgrade:-Scoping-Variants-Products-to-Hubs-and-VariantOverrides

[luisramos0]

Moving this to the Stock Management epic as this is only related to inventory/stock.

[luisramos0]

The scoping issue was thoroughly discussed in the recent Porto's gathering.
We need to document how Spree 2 handles stock, basically, as an order is finalized, stock_item.count_on_hand is updated together with the creation of a stock movement.

Additionally, when a completed order is altered and line_item.update_inventory is executed, order_inventory.verify will unstock/restock if needed also updating stock_item.count_on_hand through stock movements.

In both these cases, we need to SCOPE the variant being manipulated so that if there is a VariantOverride, it gets used instead. There are two possibilities to be explored:

• create stock_items for variant_overrides and make scoped variants use it (new column stock_item.variant_override_id)
• make stock item stock updates go directly to variant_overrides table

[luisramos0]

Useful related comment: #2014 (comment)

[luisramos0]

ScopeProductToHub

Looks like ScopeProductToHub can remain unchanged for the spree upgrade. This scoper is only scoping the variants in the product for the product_renderer, so, no stock management going on here.

ScopeVariantsForSearch

The same for ScopeVariantsForSearch where it's only scoping variants to run the search. Same as above, only reading data from scoped variants.

ScopeVariantToHub

This is where all the fun happens particularly in decrement! and then vo.decrement_stock!
So, the first step is to plug the scoping into the right place for spree 2 code and then adapting these stock management methods in the vo.

[luisramos0]

So, I'll explain here what has changed before I write any code.

Quick clarification: scoping a variant means to override the variant object if the hub in question is not the producer of the variant so that the variant behaves like a variant override, i.e., hub's stock is used instead of producers stock (hubs stock is managed in the variant_override.count_on_hand field and not in variant.count_on_hand field). The other fields overridden are price and on_demand.

There are two main stock management cases:

• completing an order
• changing a completed order

Stock management in spree v1.3

When completing an order

As the checkout workflow is moved to completed, order.finalize! calls inventory_unit.assign_opening_inventory that calls InventoryUnit.increase and is patched in OFN for scoping in inventory_unit_decorator.assign_opening_inventory.

InventoryUnit.increase and InventoryUnit.decrease will change stock levels by doing two things:

• update variant.count_on_hand value
• update order.inventory_unit structure

When changing a completed order

Line items are saved through controllers (line_items_controller for example), calling line_item.update_inventory before_save hook that calls InventoryUnit.increase and is patched for scoping in line_item_decorator.update_inventory.
The same happens for respective line_item.remove_inventory before_destroy hook that calls InventoryUnit.decrease and is patched in line_item_decorator.remove_inventory.

Stock management in spree v2.0.4

When completing an order

As the same checkout workflow is moved to completed, order.finalize! calls shipment.finalize! that changes inventory_unit.state to pending: false and unstocks each shipment manifest item (prepared from the shipment.inventory_units) by calling stock_location.unstock.

The stock_location.unstock will change stock levels by doing two things:

• creating a stock movement related to variant's stock item
• update the stock_item.count_on_hand value

When changing a completed order

Line items are saved through controllers (line_items_controller for example), calling line_item.update_inventory before_save hook that calls order_inventory.verify.

This verify method basically updates order.shipments.inventory_units according to changes on order.line_items.variants and quantities. If values don't match between order.line_items and order.shipments.inventory_units, like when completing an order, this class will unstock added items or restock removed items by calling stock_location.unstock or stock_location.restock and by updating order.shipments.inventory_units, for example, adding new inventory_units.

the million dollar question

In spree 2.0.4, where is the best place to scope variants?

[mkllnk]

A VariantOverride holds more information than just stock:

  create_table "variant_overrides", :force => true do |t|
    t.integer  "variant_id",                                          :null => false
    t.integer  "hub_id",                                              :null => false
    t.decimal  "price",                 :precision => 8, :scale => 2
    t.integer  "count_on_hand"
    t.integer  "default_stock"
    t.boolean  "resettable"
    t.string   "sku"
    t.boolean  "on_demand"
    t.datetime "permission_revoked_at"
    t.datetime "import_date"
  end

So basically it's about overriding price, stock and sku. Price and sku seem to be no problem so far, but we struggle with the stock management, because stock is not stored on the variant any more.

Maybe it's time to extract the stock management from the VariantOverride. Are your thoughts going in the same direction? So the million dollar question is: Where is the best place to override the stock management in Spree 2?

This is very difficult to answer. Here are some brainstorm ideas:

1. Create stock locations for each enterprise with permissions to override stock. We can introduce a separate table enterprise_stock_locations. Then we need to select the right stock location for the unstock and restock calls.
2. Create a StockItemOverride similar to the VariantOverride. Sounds ugly to me, but could be easy to apply by re-using the existing pattern.
3. Introduce service classes for stock management that we can replace.
4. Override StockLocation to consider enterprises.
5. Introduce "The Network" ™️ to replace variant overrides. This is probably what we want to do long term, but it may be too big for the Spree Upgrade. Can we find a way to do this only for stock? Could the first option be part of The Network?

Maybe it would be beneficial to draw out The Network in more detail to see if there is a small part we can implement to solve this problem.

[luisramos0]

Hey @mkllnk thanks for your feedback! (I numbered your options for discussion).

Have you seen the draft PR? I don't think you listed the option I follow in the PR. Maybe the closest one is number 4, but I dont really understand what you mean by number 4. Can you add more detail to option 4?

I have considered option 2 but I believe the straight forward approach sketched in the PR is better, easier and is not extending even further the bad abstraction called variant overrides.

I think I understand options 1 and 3, but I dont like them, they just add more complexity within spree and mixing things up even more.

Re option 5, from our discussions in Porto, it sounded clear that the network feature will have to respect what spree does and let spree do what it does, i.e., we have to stop messing up with spree and build on top and around, not within. So, looks like the best approach to build the network feature will be connecting products in terms of relationships between products but letting the spree relationships of product/variant/stockItem, etc live as untouched as possible. Basically, product network will kill the inventory and variant overrides. What do you think about this? (this part of the conversation deserves a discourse thread on its own).
So, I think option 5 is the best but too big for the spree upgrade.

And because of this vision in option 5, my answer to
"it's time to extract the stock management from the VariantOverride"
is no, I think we can keep it as simple as possible until we move on to something better.

Looking forward getting your feedback on the PR!

[mkllnk]

I hadn't seen the draft PR when I wrote this, sorry. Now I reviewed it and as I said there, I completely agree with your approach and it's basically my option 4. Just that we need to override more than the stock location.

I do agree that this approach is very reasonable to get the Spree upgrade done. We then need to iterate and start the Network feature before the next big Spree upgrade. Smaller upgrades could be okay, but bigger upgrades will be really difficult if we don't sort this out first.

[luisramos0]

all right!
I don't think there's any other large data model change like this in spree after v2.0. v3.0 is a heavy UI/frontend change.

[luisramos0]

I think the basic scope of this issue was covered in #3147 and #3158
I have created #3172 for the follow up and review of all other non-basic scoping processes. There are quite a few things we need to test in there. I added a note to #3172 mentioning that included in #3172 must be the validation that scoping is working properly not just for count_on_hand and on_demand but also price, default_stock, resettable, sku and lastly, permissions.