Fix variant data inconsistencies

[Matt-Yorkley]

What? Why?

Closes #4216
Closes #6368
Closes #5234

Fixes recurring data inconsistency issue #4216 where a product's variant_unit value is changed from "weight" to "items" and some of the product's variants can be left in an invalid state, which in turn breaks cloning of order cycles (with fatal errors).

The fix in these cases is to find variants in the OC that have a product with variant_unit: "weight" and where the variant's unit_value is nil, and manually change them in the DB or the console from nil to 1. 🙈

What should we test?

1. Create a product that has "weight" as it's unit type, and add some variants.
2. Add those variants to an order cycle.
3. Change the product from "weight" to "items".
4. Try cloning the order cycle.
5. It should be cloned without exploding, no snails 🚫 🐌

Release notes

Fixed a variant weight/items issue leading to OCs that cannot be cloned

Changelog Category: Technical changes

[andrewpbrett]

I updated the specs and the build is now green; it would be really nice to fix this! It might fix the root cause of #6527 as well.

We'll want to have a good think about it to make sure that we're okay changing those tests and that we're not just playing whackamole and going to see another bug pop up because of these changes. I think @sauloperez would be a good person to review since it looks like he wrote some of the original specs.

Sorry about all the force pushing/squashing @Matt-Yorkley

[Matt-Yorkley]

Github won't let me approve this PR as I authored the first bit, but I think we should move this along, we're getting too many errors coming up due to these data inconsistencies and it keeps eating dev time.

[Matt-Yorkley]

I'm guessing @andrewpbrett approves, so I'll move this to test-ready.

[andrewpbrett]

@Matt-Yorkley Your guess was correct, I think I hadn't explicitly approved since it felt like I was partially the author... or maybe I just forgot. In any case, Test Ready! 👍

[filipefurtad0]

Hey @Matt-Yorkley ,

This is quite tricky, this one.

I had a look at the related bugs namely the steps to reproduce on #6368 and your advice on what should we test. I tried this:

• Create a product that has "weight" as it's unit type, and add some variants.
• Add those variants to an order cycle.
• Change the product from "weight" to "items"

But somehow managed to clone the OC with no issues. I was only able to reproduce it by following that procedure but starting off as an "Items" product, following these steps.

• First, create an "Items" product: It plays a role, whether one insert a string or a number on the Value * field:

i) inserting a string, will now allow to change the product from Item to Weight -> which is issue #5234; This prevents the fail cascade. So we need a integer, to continue break the OC cloning, so

ii) inserting a number on the Value * field and

• adding a variant. To observe the bug, this needs to be done under /admin/products/<product>/variants/new. Now, fill in all the fields with strings - no numbers, except on price and on_hand.

iii) Changing the product to "Weight" -> At this point, attempting to change from "Items" to "Weight" on the /products page leads to error 400 -> which is again #5234; To observe the OC bug, this needs to be done under /admin/products/<product>/variants.

iv) After this, we should have a integer on the "Items" variant and a string on the "weight" variant... Adding these variants to the OC, trying to clone the OC -> Error 422 -> which is issue #4216

Also worth mentioning:
v) I observe this error 500: Creating a new "Items" product -> filling in all fields but the Value * field -> Snail: https://app.bugsnag.com/open-food-france/coopcircuits/errors/5fedd818c2cde40017b9bad2?event_id=5ffee08900661b3c48d10000&i=sk&m=nw

vi) Some inconsistencies around the product edit page, on the dropdown selector. An "Items" product displays this unit on the dropdown, but a "Weight" unit does not - see pic below:

All this is the current behaviour. I'll stage your PR and see how it is addressed 👍

[filipefurtad0]

Ok, after staging this PR I noticed, that:

• the Value * field is not mandatory any more for "Item" products. This corresponds to the testcase v) above.

If one chooses "Weight", fills everything but Value * one gets the expected warning:

This would be expected for items as well, right? Or are we making it not mandatory on purpose? In that case, should we remove the * symbol?

• I observed issue Complete product range doesn't load on shop front for UK enterprise #6527 or, to be more precise, issue OFN BE : loading of the page "Products" without result #6608 - I'm not 100% sure they are the same. It triggered this error, and occurs from changing the units from items to weight, and weight to items. However, this only happens, if the edit occurs in the /products page - this message is show:

Saving failed. Save failed due to invalid data:422

and bugsnag is triggered:

• https://app.bugsnag.com/open-food-france/coopcircuits/errors/5fff08a04cdb1a001788a61f?event_id=5fff0c8f00659893e7760000&i=sk&m=fq

This means, the app behaves differently in the pages /products and /products/<product_name>/edit
This makes it quite challenging to test, as the possibilities are quite many... We've experienced differences in the past, in related pages, hence this proposal #6650.

This is the culprit variant:

As to the PR, it seems to bring improvement, as I was not able to create corrupt variants, and reproduce the three underlying issues above.

But it looks like we might be dealing with this, for all previously created variants, in which this change is attempted. So, I I'm not sure it definitively closes #6527...

This and related issues might need further dev/testing but I feel this probably needs to move on. I'd prefer to have some feedback before merging. What do you think about all this @Matt-Yorkley ?

[Matt-Yorkley]

I was not able to create corrupt variants, and reproduce the three underlying issues above

YAY! 🎉 🕺 🥳

we might be dealing with this, for all previously created variants

We've been manually fixing them as we go, which has been crazy. If we spot any others we can fix them manually as well (it's not always a simple case, so we probably can't handle it with a migration), but the big win here is that we won't be creating new ones.

[filipefurtad0]

Thanks for feedback!
Its quite an improvement @Matt-Yorkley - I have to say this one was quite challenging to test, as there are so many different paths to product creation x product edition x product deletion... really lot's of possibilities.
But yeah, let's move it to ready to go 👍