Openfoodnetwork: missing "unit_value" validation when changing from item -> weight on /products/<product_name>/edit

[filipefurtad0]

Description

Product edits made through the path /products/<product_name>/edit are not being validated on the unit_value field. This can corrupt variants, with consequences on the shopfront, product and order cycle pages, as observed in #6527, #4216, #6368 and #5234.

Until recently, it was possible to create new variants with unit_value = 0. If this variant was from a product which had variant_unit = items and was changed into variant_unit = weight then a corrupt variant could be created. However:

i) a corrupt variant would not be created if this change was done under through the /products page. This pages validates the mandatory fields, and did not allow to create a weight item with unit_value = <empty> .

ii) a corrupt variant would only be created if this change was done under at /products/<product_name>/edit

As I understand this, PR #6369 prevents variants to be created with an empty unit_value. This introduced to prevent this from happening, and automatically introduces a value (1), in these cases.

However, if variants from products with variant_unit = items were created with unit_value = <empty> previous to the deployment of #6369 in production, then these can still be changed from variant_unit = items -> variant_unit = weight, via ii), i.e., through /products/<product_name>/edit where validation for this mandatory field does occur.

I think this might explain the reoccurrence from #6527 - ping @Matt-Yorkley @andrewpbrett

Expected Behavior

unit_value field is not being validated - cannot be empty - when product edits occur via /products/<product_name>/edit.

Actual Behaviour

unit_value field should be validated - cannot be empty - when product edits occur via /products/<product_name>/edit.

Steps to Reproduce

If you can find a product in staging with variant_unit = items and variants with no unit_value just skip to step 6.

1. Create a product with variant_unit = items
2. Create a variant, for this product.
3. After #6369 you need to remove the unit_value: log in to the server and open the SQL shell
4. UPDATE spree_variants SET "unit_value" = NULL WHERE id = ;
5. Go to the /products page and try switching that product from Items -> Weight: you should see the validation error:

Saving failed. Saving failed with the following error(s): Unit value can't be blank

1. Try the same, but under /products/<product_name>/edit: See that no validation occurs - at this step, the corrupt variant is created - the green banner should display

Product "<product name>" has been successfully updated!

See testing notes for more details.

Animated Gif/Screenshot

Workaround

Using the /products page to change this type of products from items -> weight (not sure this is a workaround).

Severity

This would prevent further occurrences from #6527, at least it would prevent some?
taking the liberty to rate this one as an S2 - ping @openfoodfoundation/train-drivers-product-owners

Your Environment

• Version used: v3.5.4
• Browser name and version: Firefox 84
• Operating System and version (desktop or mobile): Desktop Ubuntu 20.04

Possible Fix

[andrewpbrett]

So I'm seeing different behavior than you @filipefurtad0. When I got to the /edit page for a product using items and a single variant whose unit_value I've set to null, if I change from items to weight and click update, I see the green banner that says it's been updated, but then the product is still using items.

I took a long trip down a rabbit hole to look at what all happens when the units are changed. There's a few methods in VariantAndLineItemNaming and OptionValueNamer that look like they might be the source of this issue.

Were you able to confirm in the console that the variant's unit_value was NaN after your test?

Even if not, I think there's a clear takeaway from this, which is that if a variant's unit_value is nil and the product's variant_unit is items, there's no way to change the variant_unit to weight from /products and on the /edit page, the best case is that you get an incorrect error message saying you've updated the product, and the worst case is that you've created a corrupt variant with NaNs and your storefront now doesn't load.

I would propose a data migration to search for any variants where their product is set to use items and unit_value is nil and update them to have a unit_value of 1.

[jaycmb]

This reminds me of the discussion we had around unit prices with @sauloperez & @andrewpbrett , that for when variant_unit is item unit_value could be set to 1 by default (instead of blank) if for items this is the unit value in the majority of cases.

Does #6369 fix only inconsistencies when changing variant_unit from weight to item or both ways?

Wondering about the usecase behind: are users commonly creating products with variant_unit item and then want to change to variant_unit (like for next OC) ?

[filipefurtad0]

When I got to the /edit page for a product using items and a single variant whose unit_value I've set to null, if I change from >items > to weight and click update, I see the green banner that says it's been updated, but then the product is still using items.

Did you create a second variant (step 2.) for that product @andrewpbrett ? Setting the unit_value to null should be done on this newly created variant - i.e., this should done for the variant in which the field is_master has value f. Then, making the change from items -> weight in the /edit page is allowed, but forbidden on the /products page

After this, adding this variant to an OC and trying to clone that OC leads to #4216. I had another look, and NaN is not on the unit_value field, but rather null, as you point out.

So, although the solution you propose on #6792 does not introduce validation on the /products page, it might prevent further occurrences, at least of bug #4216.

[Matt-Yorkley]

I've just noticed a Spree commit which might be related to this: https://github.com/spree/spree/issues/4300

They set variant weight to default to 0.0 at database level, which avoids nil values. Could be relevant/useful?

[filipefurtad0]

Yes, that could be relevant indeed @Matt-Yorkley ,

The corrupt variants from cases I could reproduce were secondary variants (is_master = f), with variant_unit = weight, in which both unit_value is relevant but also the weight field, as seen on this pic (taken from here):

[andrewpbrett]

@Matt-Yorkley good catch. I tacked on a line to https://github.com/openfoodfoundation/openfoodnetwork/pull/6792 to ensure that weight is also never nil. That PR was ready to go so I'll just do a quick dev test and confirm.

[filipefurtad0]

Another hypothesis is that we are feeding the DBs through our product import feature, with :

• corrupt products/variants
• products/variants which can easily be corrupted by changes, after product import (like the present issue, changing items -> weight)

I'm not sure spec/features/admin/product_import_spec.rb has all the necessary validations - at least I could not find anything covering the import of "Items" products/variants (which can then be converted to "weight", with no validation - present issue).

But if we enforce control on the DB level, than I guess we are good? Can you confirm that @Matt-Yorkley @andrewpbrett?

[andrewpbrett]

It seems unlikely to me that this would all be coming from product import; I think someone would have mentioned it when asking them what they were doing. But in any case, enforcing it at the DB level would take care of it even if it is coming from a product import.

[andrewpbrett]

After the dev test, I noticed that I could still force a NaN into the database if I explicitly set it, e.g. variant.unit_weight = Float::NAN (side note: the way I found to get NaN without using that constant is to do 0.0/0.0... might be a clue as to how this is happening at all).

So I added a constraint to check for this at the DB level. I also added a line to the migration to check for any existing variants with unit_value or weight set to NaN and update them. This should have the added benefit that once this runs, we won't have to go searching for any variants out there (including any on non-managed instances) and fix them.