12570 - Fix Variant Unit field is Out of Sync with the Pop-out

[chahmedejaz]

What? Why?

• Closes [BUU] Unit field is out of sync with the popout #12570
• We are creating a new variant object and passing it to the new variant row.
• This variant is empty and hence has no unit_value present in it, due to which display_as field is also empty and variant is unable to be saved.
• This PR creates and uses the helper to build this new variant object such that it has the unit_value of 1 x variant_unit_scale,
• When the variant unit_value is present, then it sets the placeholder value for the display_as field, which was empty

What should we test?

• The expected behavior mentioned in the issue should meet

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

[dacook]

I guess this solves it by ensuring there's value in the underlying field, but I don't see an answer as to why it was out of sync with what's displayed on the screen.

Looking at _variant_row.html.haml, I can see that unit_value_with_description introduces a default of 1, when unit_value is blank.

Your solution ensures unit_value is never blank, with a different default from the last variant. I think 1 is a better default (at least that's the current behaviour).

So I think that your helper should provide a default of 1, and we can remove the 1 from _variant_row.

[dacook]

Oh, it might be event more complicated than that.

We want variant_unit_scale to inherit from the product too. This affects what 1 means. We want the unit_value_with_description to appear as "1", for whatever the variant_unit_scale is (whether 1, or 1000 for example).

Sorry, this system is really complicated, it took me ages to understand it, and I was too tired to try and document it. I've created a screenshot to help: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Product-amounts-units#current-state

Also I have a diagram I drew to show the relationship between fields: (edit: moved diagram to wiki also )

[chahmedejaz]

Sure, thanks, David. I'll dig into this a bit more.

[dacook]

Hi @chahmedejaz , just letting you know there are more conflicts on the way 😞
Gaetan is currently working on move-variant-unit-attributes-to-variant (part of Product Refactor).
His PR won't be ready until at least next week though.

[chahmedejaz]

Hi @chahmedejaz , just letting you know there are more conflicts on the way 😞
Gaetan is currently working on move-variant-unit-attributes-to-variant (part of Product Refactor).
His PR won't be ready until at least next week though.

Thanks for the heads up @dacook. I'll wrap this ASAP.

[chahmedejaz]

I guess this solves it by ensuring there's value in the underlying field, but I don't see an answer as to why it was out of sync with what's displayed on the screen.
Looking at _variant_row.html.haml, I can see that unit_value_with_description introduces a default of 1, when unit_value is blank.

Sorry I just noticed the description of PR is kind of misleading, I've updated it now.
Moreover, here's a brief loom on the issue. Please review and let me know if you have any questions.
https://www.loom.com/share/fcd1e9c7991a456ba55ee6a9f67ba16d

[chahmedejaz]

@dacook - PR is ready for review now. Thanks

[rioug]

That looks good, thanks @chahmedejaz !
It's going to clash with the product refactor but I'll address that in due time.

[dacook]

Yay, thanks for fixing this longrunning issue!

I think this is good, but I have a few questions where I think the code could be simpler.

[chahmedejaz]

Yay, thanks for fixing this longrunning issue!

I think this is good, but I have a few questions where I think the code could be simpler.

@dacook - I've addressed your comments in this commit here: 5b7fbc8 Thanks.

[dacook]

Looks great!

[drummer83]

Hi @chahmedejaz,
Thanks for working on this fix.

Before staging

I could not reproduce the exact original issue:

• After adding a new variant I was able to save without error (as long as any other information was changed).
• The unit value was always set in such way that it will be displayed as 1 g (or 1 L for Volume):
  • 0.001 for Weight (kg)
  • 1 for Weight (g)
  • 1000 for Weight (mg)
• The unit value however is only displayed after changing it (it returns to blank if closing the pop-out without changing the value).

After staging

• The unit value is always set to 1.
• The unit value is immediately displayed after adding the new variant.

Conclusion

I found no issues with your solution.
Ready to go! 🚀 Merging!! 🥳
Thanks again!