-
-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Inconsistent Behavior When Editing Products to mg Units in Hungarian Locale #12826
Fix Inconsistent Behavior When Editing Products to mg Units in Hungarian Locale #12826
Conversation
@@ -26,9 +26,15 @@ def system | |||
def self.variant_unit_options | |||
available_units_sorted.flat_map do |measurement, measurement_info| | |||
measurement_info.filter_map do |scale, unit_info| | |||
# Our code is based upon English based number formatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
# Some language locales like +hu+ uses a comma(,) for decimal separator | ||
# While in English, decimal separator is represented by a period. | ||
# e.g. en: 0.001, hu: 0,001 | ||
# Hence the results become "weight_0,001" for hu while or code recognizes "weight_0.001" | ||
scale_clean = | ||
ActiveSupport::NumberHelper.number_to_rounded(scale, precision: nil, significant: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😓
FYI: The failing specs are unrelated. (also failing in master). |
scale_clean = | ||
ActiveSupport::NumberHelper.number_to_rounded(scale, precision: nil, significant: false, | ||
strip_insignificant_zeros: true) | ||
strip_insignificant_zeros: true, | ||
locale: :en) | ||
[ | ||
"#{I18n.t(measurement)} (#{unit_info['name']})", # Label (eg "Weight (g)") | ||
"#{measurement}_#{scale_clean}", # Scale ID (eg "weight_1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I think the weights and measures code in the system is pretty complicated and would like to see it simplified somehow.. |
Thanks, David 👍
Yeah, agreed 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Exactly, @mkllnk agreed. Sure would love to work on this tech debt. Thanks. :) |
I'll look into these and get back to you whether the locales for these actually use commas in Rails app. Thanks |
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: 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. All good! |
98eabc9
into
openfoodfoundation:master
@mkllnk - I just investigated this, and the issue is reproducible using the French locale however, on the German one it's working fine. |
What? Why?
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):