[BUU] Stock settings selection is lost, if there are errors on saving

[filipefurtad0]

Description

Follow-up from #12553.

On the products page, when creating a new variant, the stock options are lost, if there are errors on the form, and the user hits Save changes.

Expected Behavior

If some stock options were already selected, these should be kept, even when there are errors on the form, when saving.

Actual Behaviour

The selected stock options are not kept, when there are errors on the form, when saving.

Steps to Reproduce

1. Click the + sign, to add a Variant
2. Fill in all fields and select a stock option (wither by ticking the On Demand option, or by typing in a numerical value on the On Hand field). Make sure you leave a mandatory field empty (like price or Unit)
3. Click Save changes
4. Notice the changes inserted on the On Hand field were reset

Animated Gif/Screenshot

After hitting Save changes:

Workaround

Fill in the stock options again

Severity

More of a usability issue? bug-s4: it's annoying, but you can use it

Your Environment

• Version used: v4.4.50
• Browser name and version: Firefox 126
• Operating System and version (desktop or mobile): Ubuntu 22.04

Possible Fix

[cyrillefr]

Hello @filipefurtad0 ,

May I work on this issue ?

[cyrillefr]

Hello @dacook ,

I have a problem here. When the save changes button is pressed, there is an attempt to create variant(s). Via the bulk_update method in the products v3 controller. And then in the product_set.rb file: https://github.com/openfoodfoundation/openfoodnetwork/blob/25d375bf8e176788ec89352291c0d89461abd7b0/app/services/sets/product_set.rb#L122-L141

But creating variants must be done without on_hand & on_demand (a prior stock item must exist). So, in case of errors, resulting (incomplete) variant object is returned and then via the controller passed back to the view. That unsaved variant object will always have a nil on_demand (& on_hand).

I could pass back the parameters to the view, but it could be a bit cumbersome. Creating an on_demand property on an unsaved variant raises rightly an error.

What are you thoughts on this ? (I might be missing on something)

[dacook]

I just had a look at this, and found the problem exists on the /admin/products/x/variants/new screen also.

I think it's a rare case. It doesn't seem likely that there will be an error on the new variant. Possible errors:

• the unit value is blank (but it's normally prefilled by default)
• the text inputs are longer than 255 chars

[dacook]

@cyrillefr to fix it, I think we could make on_demand and on_hand as instance variables on the Variant object.

Eg the setter VariantStock#on_demand= can set an @on_demand variable. Then on the getter `VariantStock#on_demand, it can choose to read from that variable if set. If not set, it can continue with the existing default logic. What do you think?

[cyrillefr]

Hello @dacook , I did not dare modify this the last time

  # @raise [StandardError] when the variant has no stock item yet
  def on_demand=(new_value)
    raise_error_if_no_stock_item_available

Should I get rid of the raise ? Then set an instance variable and return this value. It seems this method is not used, I have found only a spec with it. I can definitely do that.

[dacook]

Good point. I think the method is used, with a space as on_demand = (eg in product_set.rb)

It's a bit confusing, it looks like we're assigning an attribute (with VariantStock#on_demand= and #on_hand=), yet it's actually immediately saving an associated record. I think a better solution is to make them behave more like regular field attributes. @mkllnk how does this sound to you?

To do that I think we can:

• make these two attributes as pseudo-fields (perhaps with attr_accessor) that can be set and read at any time
• When saving (probably after_save, after Variant#create_stock_items has completed), attempt to update the stock_item as per on_demand= and on_hand=
  • I thinkraise_error_if_no_stock_item_available is relevant here.
  • It's interesting in some cases we loop through stock_items, others we just access .first. I'm not sure but maybe we currently only support one.

So in the case of an error on the variant, the on_demand/on_hand attributes should retain the value and pre-populate in the edit form again.

[mkllnk]

Your solution sounds good, using standard attributes that get saved later. But it's also risky because a lot of logic depends on it and we may introduce new severe bugs trying to solve an s4 bug. That's not a reason not to do it. I'm just flagging it so that we don't rush the solution.