Pluralize common variant unit names #4709

mkllnk · 2020-01-23T01:06:37Z

What? Why?

This adds the most popular unit names as singular and plural to our
locale for translation. The added Javascript performs a reverse lookup
to find the right singular/plural form of a unit name in that language.

I explained why I chose this solution on the issue: #4172 (comment)

What should we test?

Test in English because we haven't translated this yet.

Go to admin -> products.
Change a product to items instead of weight or volume.
Add a unit name like bunch or loaf.
Change the quantity including 0, 1 and 2 of the variant and observe the change in the description.
Save the changes.
Go to the shopfront and verify the product description there.

Release notes

Unit descriptions like "1 bunch" or "4 boxes" can now be translated more accurately. The previous logic would sometimes fail to find the right plural, especially in languages other than English.

Changelog Category: Changed

luisramos0

It's a ingenious solution Maikel. I am just afraid this will not be easy to understand by future devs. I am not seeing a better alternative, let's go for it!

To confirm I understood correctly (this question is a sign this solution is not obvious to understand), the en.yml file will only contain the english version of the most used unit_name in all languages, right? if in Portugal we have a common unit_name "fardo", we need to add the English version "bale/bales" to en.yml and then translate, right?

Can you add a js unit test to the new js pluralizer?
I think both pluralizers (js and ruby) must be on classes of their own and not inside the option_value_namers.

luisramos0 · 2020-01-23T10:34:19Z

config/locales/en.yml

+  #   puts "      other: \"#{name}s\"";
+  # }
+  unit_names:
+    # Used 379 times.


I dont think we need to keep these numbers in this file. The numbers are useful to see what keys to have here but we I think we should remove these "statistics" for here.

Okay, I thought it could be interesting to compare if we update it one day. But I'll remove them.

lib/open_food_network/option_value_namer.rb

luisramos0 · 2020-01-23T10:40:37Z

lib/open_food_network/option_value_namer.rb

+    # Provides efficient access to unit name inflections.
+    # The singleton property ensures that the init code is run once only.
+    # The OptionValueNamer is instantiated in loops.
+    class I18nUnitNames


I think this breaks the single responsibility rule. We should have this in a separate class.
UnitNamePlural or Pluralizer or I18nUnitNames.
This (including option_value_names) should all go to something like app/services/naming

I agree, I will move it.

mkllnk · 2020-01-23T23:01:08Z

@luisramos0 Did you see the js spec covering the pluralizer?

If we move the pluralizer code (js and ruby) to their own classes, should we make the solution independent of variant_unit_name? In that case, we should not use the scope unit_names in the locale but something more general like inflections.

luisramos0 · 2020-01-23T23:41:44Z

yeah, I'd generalize it. but only if it doesnt add even more complexity.

mkllnk · 2020-01-28T23:30:33Z

@luisramos0 This is ready for review again.

I added the missing words used in French.
I moved the I18n scope from unit_names to inflections to be more general here.
I moved the Ruby to its own lib file. I don't think it depends on our application and therefore doesn't need to be a service.
I realised that I didn't consider the case of people switching locales. In our case that's mainly important for the enterprise user creating variants. They need to create variants in the same language as their locale. Otherwise it won't pluralise. There is no way around this because we can't know which language the user input is and one word can mean different things in different languages. So I added support for multiple locales, almost regretting going down the caching path. It adds so much more complexity.
And I added more comments to explain everything. What do you think?

lib/open_food_network/i18n_inflections.rb

luisramos0

Nice, I dont think it added too much complexity from the initial solution 👍
I think we should extract pluralize from option_value_namer.js.coffee. Maybe next time around.
Otherwise, I think this is enough effort for the "small but not easy" problem we are trying to solve here.
Great job Maikel!

This adds the most popular unit names as singular and plural to our locale for translation. The added Javascript performs a reverse lookup to find the right singular/plural form of a unit name in that language.

This code will be used for the shop front and reports.

I considered moving the code to a service but I think that this code can be completely independent of the Open Food Network use case. It would be easy to move to a gem. The downcasing may need reconsidering for general use.

Matt-Yorkley

Nice! 😄

RachL · 2020-02-17T19:32:32Z

@mkllnk I staged this on 2 different staging servers, but I still end up with an error when trying to test

Change a product to items instead of weight or volume.

I've tried this on Melon Farm and Suma Whole Foods (Cocoa powder) on Uk staging.

And my latest release test Kimchi Farm 2 on staging FR

luisramos0 · 2020-02-17T19:41:15Z

@RachL when you change to items you must specific the variant name
is that the problem?

luisramos0 · 2020-02-17T19:42:01Z

btw PR #4780 will bring meaningful error messages back to this page!

RachL · 2020-02-19T16:15:24Z

@luisramos0 ah indeed this is working if I add a unit name before saving. 🤦‍♀ Moving to ready to go!

mkllnk self-assigned this Jan 23, 2020

mkllnk marked this pull request as ready for review January 23, 2020 06:37

luisramos0 suggested changes Jan 23, 2020

View reviewed changes

luisramos0 reviewed Jan 29, 2020

View reviewed changes

lib/open_food_network/i18n_inflections.rb Outdated Show resolved Hide resolved

luisramos0 reviewed Jan 29, 2020

View reviewed changes

lib/open_food_network/i18n_inflections.rb Show resolved Hide resolved

luisramos0 approved these changes Jan 29, 2020

View reviewed changes

mkllnk added 5 commits January 31, 2020 09:48

Pluralize common variant unit names

98b5528

This adds the most popular unit names as singular and plural to our locale for translation. The added Javascript performs a reverse lookup to find the right singular/plural form of a unit name in that language.

Use our unit name pluralization in Ruby

1cce106

This code will be used for the shop front and reports.

Remove usage count comments

2476050

Add unit names commonly used in French

6f8bb79

Make pluralisation code an independent lib

9535c56

I considered moving the code to a service but I think that this code can be completely independent of the Open Food Network use case. It would be easy to move to a gem. The downcasing may need reconsidering for general use.

Matt-Yorkley approved these changes Feb 17, 2020

View reviewed changes

RachL self-assigned this Feb 17, 2020

RachL added pr-staged-uk staging.openfoodnetwork.org.uk feedback-needed labels Feb 17, 2020

RachL removed the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 17, 2020

RachL removed the feedback-needed label Feb 19, 2020

luisramos0 merged commit d99cba3 into openfoodfoundation:master Feb 19, 2020

mkllnk deleted the 4172-js-pluralize branch March 4, 2020 00:25

tschumilas mentioned this pull request Dec 11, 2024

Add plural form for flower farming variant unit names #13032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pluralize common variant unit names #4709

Pluralize common variant unit names #4709

mkllnk commented Jan 23, 2020 •

edited

Loading

luisramos0 left a comment

luisramos0 Jan 23, 2020

mkllnk Jan 23, 2020

luisramos0 Jan 23, 2020 •

edited

Loading

mkllnk Jan 23, 2020

mkllnk commented Jan 23, 2020

luisramos0 commented Jan 23, 2020

mkllnk commented Jan 28, 2020

luisramos0 left a comment

Matt-Yorkley left a comment

RachL commented Feb 17, 2020

luisramos0 commented Feb 17, 2020

luisramos0 commented Feb 17, 2020

RachL commented Feb 19, 2020

Pluralize common variant unit names #4709

Pluralize common variant unit names #4709

Conversation

mkllnk commented Jan 23, 2020 • edited Loading

What? Why?

What should we test?

Release notes

luisramos0 left a comment

Choose a reason for hiding this comment

luisramos0 Jan 23, 2020

Choose a reason for hiding this comment

mkllnk Jan 23, 2020

Choose a reason for hiding this comment

luisramos0 Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

mkllnk Jan 23, 2020

Choose a reason for hiding this comment

mkllnk commented Jan 23, 2020

luisramos0 commented Jan 23, 2020

mkllnk commented Jan 28, 2020

luisramos0 left a comment

Choose a reason for hiding this comment

Matt-Yorkley left a comment

Choose a reason for hiding this comment

RachL commented Feb 17, 2020

luisramos0 commented Feb 17, 2020

luisramos0 commented Feb 17, 2020

RachL commented Feb 19, 2020

mkllnk commented Jan 23, 2020 •

edited

Loading

luisramos0 Jan 23, 2020 •

edited

Loading