Track (negative) stock for on-demand products and overrides

[mkllnk]

ℹ️ Funded Feature. Please track ALL ASSOCIATED WORK under the associated tracking code #11678 DFC Orders

What? Why?

• Part of [DFC] Orders Part 1: Create order from OFN via DFC #11678
• Retries Allow negative stock levels for on-demand products #12536
• Avoids ActiveRecord::RecordInvalid in checkout#update #12586

We want to place backorders on a wholesaler's platform when we run out of stock in our local OFN store. This complicates the stock logic though. To allow customers to order more than we have in stock, we set the products to on demand. And we change the logic to still count stock, allowing it to go negative, so that we know how much stock we need to order from the wholesaler.

This PR just prepares the change in OFN stock logic without adding the backordering logic. While it seems to be a fairly easy change, there's potential for subtle bugs around products going out of stock. We may have assumptions in our code that I'm not aware of.

What should we test?

• Test the checkout of stock controlled items and on-demand items.
• Also include inventory items with overridden stock or overridden on-demand.
• Test the order creation as admin of those items.
• Test with producer products and with inventory products (using variant overrides).
• When a stock controlled product goes out of stock during checkout, we still need to raise an error.
• When an on-demand product goes out of stock, we should still be able to check out and it should not be marked as out of stock.
• After on-demand products have been ordered, the stock level may go negative. Ensure that order with products with negative stock can still be shipped. All order management actions should be as before.

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[rioug]

Good work ! the commit history make it easy to follow.

[rioug]

The spec description implies that only the override stock should be updated, is this meant to change as well ?
Took me a minute to understand, but if I am not mistaken because of OpenFoodNetwork::ScopeVariantToHub the variant override is "linked" to the variant, so hub_variant.on_hand actually returns the override.count_on_hand

[dacook]

The implementation seems very strange and error-prone to me.. I'm glad that we at least have a spec that demonstrates it now!

[dacook]

Great job, as Gaetan said it's very well structured and easy to follow.

Sorry if it's already covered, but what will happen in the case of existing on-demand products that don't need stock tracking? As this PR shows, there shouldn't be any change in behaviour, but I guess the underlying stock level in the DB will change.

• If count_on_hand was an integer, it will now get reduced.
• If count_on_hand was nil, is it considered 0, and therefore reduced below zero?

Either way, if admins switch their products back off on-demand, they will then probably be confronted with a negative number. This may be confusing at first, but I don't think a problem.

[dacook]

The implementation seems very strange and error-prone to me.. I'm glad that we at least have a spec that demonstrates it now!

[mkllnk]

If count_on_hand was nil, is it considered 0, and therefore reduced below zero?

Yes, that's right. And when you switch to stock tracking then you will see a negative number. That's the reason we didn't do it before. We avoided that negative number. But I don't think it's a big problem.

[RachL]

That's the reason we didn't do it before.

It's funny I thought we already did that. Maybe we did at some point? I've searched around and found this: #4217

Anyway I agree it's not a problem 👍

[mkllnk]

That's an interesting issue you found, @RachL. I added checking shipments to the test list so that we don't introduce that issue again.

[filipefurtad0]

Hey @mkllnk ,

Thank you for outlining the test scenarios.

i) Test the checkout of stock controlled items and on-demand items.

• On demand checkout worked as usual 🟢
• Stock controlled checkout worked as usual:
  • reducing stock during checkout redirects to the cart page and "forces" cart update to comply with available stock 🟢
  • stock is correctly reduced, after placing the order 🟢

ii) Also include inventory items with overridden stock or overridden on-demand.

• On demand checkout worked as usual 🟢
• Stock controlled checkout worked as usual:
  • reducing stock during checkout redirects to the cart page and "forces" cart update to comply with available stock 🟢
  • stock is correctly reduced, after placing the order 🟢

iii) Test the order creation as admin of those items.

Without variant overrides:

• Creating backoffice orders for on demand items worked as usual 🟢
• Creating backoffice orders for stock controlled items worked as usual 🟢
  • stock is correctly reduced, after placing the order 🟢
  • one can't create backoffice orders with a quantity superior to the available stock 🟢

With variant overrides:

• Creating backoffice orders for on demand items worked as usual 🟢
• Creating backoffice orders for stock controlled items worked as usual 🟢
  • stock is correctly reduced, after placing the order 🟢
  • one can't create backoffice orders with a quantity superior to the available stock 🟢

iv) Test with producer products and with inventory products (using variant overrides).
Tested shopfront orders with and without variant overrides. Behavior was found to be the same as i) and ii)

v) When a stock controlled product goes out of stock during checkout, we still need to raise an error -> verified in i) and ii)

vi) When an on-demand product goes out of stock, we should still be able to check out and it should not be marked as out of stock -> indeed, it was noticed that stock goes to negative values, which does not prevent orders from being placed, either in the shopfront or backoffice

vii) After on-demand products have been ordered, the stock level may go negative. Ensure that order with products with negative stock can still be shipped. All order management actions should be as before. 🟢
Other than negative stock values for variants set as on demand, I've found no difference to the current/usual behavior.

All looks good.

I have a question regarding the stock levels for on demand items, with variant overrides. Lets take the following steps:

• set a given variant as on demand - outcome:

• place a shopfront order with this variant as line item, quantity = 2 - outcome:
  

• On the On demand? dropdown, select No - hit Save Changes. Outcome:
  

• Now select back Yes - outcome:
  

• Place another order, with quantity = 2. Outcome:
  

So, each time, a variant is overridden with the No option, the on demand stock level is reset to zero. I would think this value should be persistent, i.e., the outcome on the final step should be -4 instead of -2.

Is this the expected behavior?

[drummer83]

Hi @mkllnk,
Just flagging that there's your feedback needed on this one.
Thanks!

[mkllnk]

Thank you, @drummer83, for reminding me. It slipped through my notifications.

The remaining problematic behaviour is the reset of stock when you change a variant to on_demand = false. That does not seem intuitive but I don't see a way around that at the moment. Well, we could, but it's risky. Let me explain.

Currently, we don't allow negative stock. During checkout, we check that there's enough stock and then complete the order. The order completion triggers stock to be adjusted. And if there was a race condition and someone else checked out at the same time then Rails would raise an error if the stock became negative.

So validating stock to be positive is part of the checkout logic at the moment. We would need to add some additional checks if we allowed negative stock. And we would not be able to check out. For example:

• A product is on demand.
• The stock level goes negative.
• The product is switched to standard stock control but has negative stock.
• The checkout needs to display out of stock and cancel because there's no stock left.

So while preserving the stock level would be nice, I don't think that there's much benefit. If we find a use case for it, we can do it. But any change of the checkout logic is risky because it's complex old code.

[mkllnk]

David just noted as well that on the new products screen, we don't see the stock level unless we switch on-demand off. And then we can't save it until we set it to 0 or a positive value anyway. So there is no automatic, surprising reset. The current logic should be good then.

Long term, I would like to allow negative stock though and make the checkout logic more robust to handle that, not relying on validation errors to fail the checkout.