Packaging fails if multiple stock locations are required to fulfill quantity

[jhawthorn]

Discovered this while writing tests for #566.

Because of the way the Stock Coordinator, Packer and Prioritizer work, it won't properly split inventory across stock locations if multiple locations are required to fulfill the quantity of a variant (ie. no stock location has the full amount requested, but combined they do). I believe this was a regression in spree 2.4

Example scenario

Consider packaging an order with 2 of the same variant. In this example we have two stock locations, each with 1 count_on_hand of the variant.

• In the Stock::Coordinator we have two unallocated_inventory_units, we'll call them unit1 and unit2.
• Next a Sock::Packer is made for each stock location to generate the candidate packages. They both create the identical package, containing only unit1. Neither contains unit2 because each only has enough stock for one.
• The Stock::Prioritizer removes the unit from the second candidate package and then removes the empty package entirely.
• The Stock::Coordinator finally returns a single package with the single inventory unit.

[jordan-brough]

Great find. The 2.4 breakage you're talking about would be this commit from this pull request right? I'm going to go back and try to understand the old code better but it does seem like something fundamental needs to change here.

[kennyadsl]

@jhawthorn @jordan-brough I made this commit with @DanielePalombo on a solidus fork branch. This solves splitting across multiple stock locations.

Anyway I'm afraid this generates 2 ready packages, not identical, since with this logic what's inside the first package cannot be inside the second one. At a first look this makes Stock::Prioritizer and Stock::Adjuster useless. Stock::Prioritizer will just sort packages, but when it tries to adjust them removing duplicate inventory units across packages it will never perform any action.

Also, at the moment Stock::Prioritizer is not sorting anything.

A couple of crazy ideas to solve this:

• completely remove Stock::Prioritizer and Stock::Adjuster. With this commit I cannot find a scenario when they will be actually used.
• when Stock::Coordinator build packages, it should give precedence to inventory units not yet allocated. I think this will solve even if I cannot be 100% sure it works in any possible scenario at the moment. This will create 2 packages, but not identical if there are other inventory units for the same variant which are not yet been taken into another package.
• something like passing the unallocated inventory units to Prioritizer (probably we already have all inventory units there, so it could be easy to find unallocated ones). When it adjusts packages, it tries to substitute the removed inventory unit with an unallocated one if there are an unallocated inventory unit eligible for that package.

I'd love to hear some feedback before trying to go in one of the above directions.

[bradwbradw]

i'm not sure about removing Stock::Prioritizer entirely, since there's lots of documentation out there that says that we should override sort_packages with our own business logic based on geography, logistics, minimum thresholds and what-not. At minimum I'd suggest to at least keep sort_packages should anyone want to override it