Fix Inconsistent Behavior When Editing Products to mg Units in Hungarian Locale

[chahmedejaz]

What? Why?

• Closes Editing products with mL as units leads to inconsistent behavior with Hungarian locale  #12815
• Please review the comment in the code, I think it'd better explain the change.

What should we test?

• This needs to be validated with the Hungarian language (Magyar)
• Visit the products page
• Update product unit scale to different values
• It's better to validate it with all the available unit scales in the application.

Release notes

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

[chahmedejaz]

Ref:

openfoodnetwork/spec/services/weights_and_measures_spec.rb

Lines 102 to 128 in b2e15f5

	describe "#variant_unit_options" do
	let(:available_units) { "mg,g,kg,T,mL,cL,dL,L,kL,lb,oz,gal" }
	subject { WeightsAndMeasures.variant_unit_options }
	before do
	allow(Spree::Config).to receive(:available_units).and_return(available_units)
	end
	it "returns options for each unit" do
	expected_array = [
	["Weight (mg)", "weight_0.001"],
	["Weight (g)", "weight_1"],
	["Weight (oz)", "weight_28.35"],
	["Weight (lb)", "weight_453.6"],
	["Weight (kg)", "weight_1000"],
	["Weight (T)", "weight_1000000"],
	["Volume (mL)", "volume_0.001"],
	["Volume (cL)", "volume_0.01"],
	["Volume (dL)", "volume_0.1"],
	["Volume (L)", "volume_1"],
	["Volume (gal)", "volume_4.54609"],
	["Volume (kL)", "volume_1000"],
	["Items", "items"],
	]
	expect(subject).to match_array expected_array # diff each element
	expect(subject).to eq expected_array # test ordering also
	end

[chahmedejaz]

I believe this helper should only be used to round numbers when we want to display the output to the end user as it is highly influenced by the locale being used.

Upon further investigation (from old UI), I think it's being an alternative to Javascript's parseFloat method which behaves just like this helper. And here Rails being Rails, it just does so much under the hood that it feels like magic and these odd behaviors can be missed easily 😓

[chahmedejaz]

FYI: The failing specs are unrelated. (also failing in master).

[chahmedejaz]

I'm not sure exactly, we can tweak the keys of the UNITS hash such that we don't need to use the helper to round off-scale values here. i.e. already round off keys here. But I'm reluctant to do so because it's being used in other places and not sure if it's a good idea to do it this quickly.
https://github.com/chahmedejaz/openfoodnetwork/blob/a50be52cded204db8ba67e6dee4bdb4888d2d4a1/app/services/weights_and_measures.rb#L65-L86

The current solution works just like we would have expected in the old UI. So, maybe we can go with it for now. 🤷

[dacook]

Thanks so much for looking into this Ahmed. This looks like the perfect quick fix.
As it is dealing with backend codes that are not visible to the user, it's appropriate to force the locale here. I like that the comment explains why in weights_and_measures, so suggest we also have that in products.rb

[dacook]

I think the weights and measures code in the system is pretty complicated and would like to see it simplified somehow..
But I don't have a suggestion for that right now!

[chahmedejaz]

I like that the comment explains why in weights_and_measures, so suggest we also have that in products.rb

Thanks, David 👍

I think the weights and measures code in the system is pretty complicated and would like to see it simplified somehow..

Yeah, agreed 😅

[mkllnk]

Interesting. I wonder why this didn't fail in Germany and France before. They use a comma, too, right?

I think that the underlying problem is that we compute identifiers for the units. We should have a fixed set for identifiers. Then we don't need to convert anything which could be affected by a locale.

@chahmedejaz, would you like to work on a follow-up contribution? Now that you fixed the problem, it's just tech debt.

[chahmedejaz]

I think that the underlying problem is that we compute identifiers for the units. We should have a fixed set for identifiers. Then we don't need to convert anything which could be affected by a locale.

Exactly, @mkllnk agreed. Sure would love to work on this tech debt. Thanks. :)

[chahmedejaz]

Interesting. I wonder why this didn't fail in Germany and France before. They use a comma, too, right?

I'll look into these and get back to you whether the locales for these actually use commas in Rails app. Thanks

[filipefurtad0]

Hey @chahmedejaz ,

Thanks so much for this blazing fast fix.

First I've followed the steps described here, to enable the HU locale on staging-UK. Then I've reproduced the steps leading to the bug, and could now observe that the units are changed consistently:

From mL:

To mg:

Edited the name:

I've also made some smoke tests around the EN locale, with other units like Kg, and changing these to L, or items. Looks all good.
Since it looks like the bug relates to how commas and dots are used as separators, I've changed this setting in the server and repeated the tests above.

All good!
Merging.

[chahmedejaz]

Interesting. I wonder why this didn't fail in Germany and France before. They use a comma, too, right?

I'll look into these and get back to you whether the locales for these actually use commas in Rails app. Thanks

@mkllnk - I just investigated this, and the issue is reproducible using the French locale however, on the German one it's working fine.
I believe the gem that provides these locales-related formatting data in the app (rails-i18n) has a comma as a decimal separator for the French locale and a period for the German.