Openfoodnetwork: Able to end up with an invalid variant through web UI

[kristinalim]

Through the web UI alone, it is possible to end up with an invalid variant. This has big potential to mess up related code.

Description

We normally presume variant.save and variant.update_attributes would succeed if we do not make changes the record that would make it invalid. When we have an already invalid variant, even if we don't change it and then attempt to save it, it would fail.

I haven't done more investigation on the consequences. I'm still just documenting at this point.

Expected Behavior

It should not be possible to end up with an invalid variant in the database.

Actual Behavior

I can create an invalid variant in the web UI by performing a series of actions.

Steps to Reproduce

As enterprise manager:

1. Create a new product with "Items" as unit size.
2. Go to the "Bulk Edit Products" page.
3. Add a new variant for the product. Set unit to a non-numeric value, e.g. ~"1 med"~ "med".
4. Save the changes
5. Change the product "Unit" to weight, and save.

The variant at this point fails validation.

Context

Some possibly related issues:

• #2769
• #2764
• #2774
• #2739

Severity

S2

[lin-d-hop]

Just wondering how this issue is going. We have weird problems popping up all over the place on UK prod and I suspect this might be related to all of them. Any updates on this s2?

[kristinalim]

Sorry @lin-d-hop, there is no solution for this yet. But for now it doesn't seem to me like this is the root cause of other issues reported recently. I extracted a list of variants in UK production which the application considers invalid - none are under producers in screenshots that have been provided.

I found that there are 12 out of 22,923 variants in the database (including master variants which are automatically created for each product) which the software considers invalid:

• 12 variants - Complaining of missing unit value
• 1 of the 12 variants - Complaining of missing unit description

It would be best to get these into a valid state - the "Unit" cell for these variants in the "Bulk Edit Products" page should be in the format "3" or "3 modifier".

Should I email you the list of these suppliers, product, and variants, @lin-d-hop?

[sauloperez]

Ok, this is how I managed to reproduce this.

Steps

Issuing a POST http://localhost:3000/admin/products/bulk_update with the following body

{
"products": [
  {
    "id": 11,
    "variants_attributes": [
      {
    "unit_value": null,
    "unit_description": "med",
    "display_name": "test variant 3"
      }
    ]
  }
],
"filters": []
}

and then another POST http://localhost:3000/admin/products/bulk_update with the following body

{
"products": [
  {
    "id": 11,
    "variant_unit": "weight",
    "variant_unit_scale": 1
  }
],
"filters": []
}

Then, when checking from the rails console I get

[13] pry(main)> product = Spree::Product.find 11
[14] pry(main)> product.variants.last.valid?
=> false
[15] pry(main)> product.variants.last.errors
=> #<ActiveModel::Errors:0x00564ee5173060
 @base=
  #<Spree::Variant id: 24, sku: "", weight: nil, height: nil, width: nil, depth: nil, deleted_at: nil, is_master: false, product_id: 11, count_on_hand: 0, cost_price: nil, position: nil, lock_version: 0, on_demand: false, cost_currency: "AUD", unit_value: nil, unit_description: "med", display_name: "test variant 3", display_as: nil, import_date: nil>,
 @messages={:unit_value=>["can't be blank"]}>

Then, when I update that invalid variant from the "Bulk Edit Products" page, although the UI says "Saved changes" they are not. The changes don't show on the UI.

Problem

As @kristinalim points out (sorry, it took me sometime to fully understand what you meant), we don't take into account the impact of a product update may have on its variants. In this case, changing the product's variant_unit makes the variant's unit_value invalid.

Solution

This requires two things. First, fix the issue and prevent this from happening again by checking if the variant is valid when changing the product. And two, fix currently invalid variants by preparing a data migration to be run in all instances.

[sauloperez]

This is going to be a bit tough... there's not a single unit test for the ProductSet class, the one responsible for this... :clap:

EDIT What a piece of legacy code. You can't even touch it with a stick. Still working on the test coverage.

[sauloperez]

Done. We can now prepare a release to fix this one and the other S2's we managed to fix.

For UK and others to fix current invalid variants https://github.com/openfoodfoundation/openfoodnetwork/pull/2845/files?utf8=%E2%9C%93&diff=unified#diff-37a146315916653cc326306cb9dc4346 will be automatically executed. Data migrations are rather error-prone, so please, keep an eye on the log file that gets generated.

:+1: ?