Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Aug 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b6b0db
12815: use en formatting for unit value conversion
chahmedejaz Aug 28, 2024
54252f5
Add comment
dacook Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/models/spree/product.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,8 @@ def variant_unit_with_scale
scale_clean = ActiveSupport::NumberHelper.number_to_rounded(variant_unit_scale,
dacook marked this conversation as resolved.
Show resolved Hide resolved
precision: nil,
significant: false,
strip_insignificant_zeros: true)
strip_insignificant_zeros: true,
locale: :en)
[variant_unit, scale_clean].compact_blank.join("_")
end

Expand Down
8 changes: 7 additions & 1 deletion app/services/weights_and_measures.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

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,
Copy link
Collaborator Author

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 😓

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")
Copy link
Collaborator Author

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. 🤷

Expand Down
Loading