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.

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

Add plural form for flower farming variant unit names #13038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chahmedejaz
Copy link
Collaborator

@chahmedejaz chahmedejaz commented Dec 14, 2024

⚠️ Please use clockify code #12476 Flower Farms

What? Why?

What should we test?

  • As mentioned in the issue

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

This PR also adds the variant unit names in English locales that are being used in the staging environments i.e. AU, FR or UK because as per our current architecture, if we add changes to en.yml only, then we won't be able to view these unit changes in staging because they use their use en_AU, en_FR or en_GB respectively.
Our pluralization code only uses the country-based English locale only rather than considering the fallback locale en.yml. For reference please review this file:

# frozen_string_literal: true
module OpenFoodNetwork
# Pluralize or singularize words.
#
# We store some inflection data in locales and use a reverse lookup of a word
# to find the plural or singular of the same word.
#
# Here is one example with a French user:
#
# - We have a product with the variant unit name "bouquet".
# - The I18n.locale is set to :fr.
# - The French locale contains:
# bunch:
# one: "bouquet"
# other: "bouquets"
# - We create a table containing:
# "bouquet" => "bunch"
# "bouquets" => "bunch"
# - Looking up "bouquet" gives us the I18n key "bunch".
# - We find the right plural by calling I18n:
#
# I18n.t("inflections.bunch", count: 2, default: "bouquet")
#
# - This returns the correct plural "bouquets".
# - It returns the original "bouquet" if the word is missing from the locale.
module I18nInflections
# Make this a singleton to cache lookup tables.
extend self
def pluralize(word, count)
return word if count.nil?
key = i18n_key(word)
return word unless key
I18n.t(key, scope: "inflections", count:, default: word)
end
private
def i18n_key(word)
@lookup ||= {}
# The user may switch the locale. `I18n.t` is always using the current
# locale and we need a lookup table for each of them.
unless @lookup.key?(I18n.locale)
@lookup[I18n.locale] = build_i18n_key_lookup
end
@lookup[I18n.locale][word.downcase]
end
def build_i18n_key_lookup
lookup = {}
I18n.t("inflections")&.each do |key, translations|
translations.each_value do |translation|
lookup[translation.downcase] = key
end
end
lookup
end
end
end

Hence to make it work for testing in staging, added the respective changes in the staging English locales.

However, if we still want to avoid these locale changes in other files then I think we may validate it in release testing when all the Transifex changes are merged. Or maybe we could validate it in production?

ping @openfoodfoundation/reviewers

@chahmedejaz
Copy link
Collaborator Author

FYI: Failing specs are in the master as well.

@chahmedejaz chahmedejaz added the user facing changes Thes pull requests affect the user experience label Dec 14, 2024
@chahmedejaz chahmedejaz marked this pull request as ready for review December 14, 2024 20:25
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 As long as it doesn't mess up Transifex, we should be good with updating the other locals.

@rioug
Copy link
Collaborator

rioug commented Dec 16, 2024

FYI: Failing specs are in the master as well.

This will be fixed by : #13039

Copy link
Member

@dacook dacook left a 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, but I think it will be ok, and I can see the advantage of being able to test.

I think what could happen is if someone changed a value next to one of these before the next release, it would cause a git conflict. But that seems unlikely and I'm sure we can handle if we need to.

@dacook dacook force-pushed the task/13032-plural-variant-unit-names branch from d17ea41 to 0bd92a0 Compare December 16, 2024 05:06
@RachL
Copy link
Contributor

RachL commented Dec 16, 2024

@chahmedejaz For other translation PR, we validate the page still works and we check translation in production. I'm happy to do the same here.

However I've noticed that the FR translation previously set up for variant plural do not work in production (comment #13032 (comment)). Is this something that will be fixed as part of this PR as well?

@chahmedejaz
Copy link
Collaborator Author

chahmedejaz commented Dec 16, 2024

However I've noticed that the FR translation previously set up for variant plural do not work in production (comment #13032 (comment)). Is this something that will be fixed as part of this PR as well?

Hi @RachL - Sorry I missed that part of the comment. I just looked into it and here are the findings:

  • All the mentioned locales in the comment here are missing in the Belgium version of French on Belgium instances only.
  • I validated these locales on the FR staging, and they work. I think this should work for production FR as well except for the Belgium.
    Screenshot from 2024-12-16 19-50-16
  • Moreover, we are missing the translation for crate in all application locales. I'll that now for the English version here and after that we can add the respective translation for the other locales including the FR ones. Thanks.

@chahmedejaz chahmedejaz force-pushed the task/13032-plural-variant-unit-names branch from 0bd92a0 to 71ee7d5 Compare December 16, 2024 19:16
@chahmedejaz
Copy link
Collaborator Author

chahmedejaz commented Dec 16, 2024

Moreover, we are missing the translation for crate in all application locales. I'll that now for the English version here and after that we can add the respective translation for the other locales including the FR ones. Thanks.

@RachL - crate is added in the en.yml, now this can be translated by Transifex to the all the other locales i.e. FR.

@chahmedejaz
Copy link
Collaborator Author

For other translation PR, we validate the page still works and we check translation in production. I'm happy to do the same here.

Sure, Rachel.
@openfoodfoundation/testers - This can be tested in production only if we have the respective translations in all the English-based locales i.e. en_AU, en_FR, en_GB, etc..
So, please make sure that the respective Transifex changes are merged before testing. Otherwise, we won't have the new plurals.
Also, please make sure to add the missing plurals for the fr_BE as per this.

Please let me know if you have any questions on this. Thanks

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

Regarding changing other locales: In this case, it's okay to add to other locales for testing purposes. There's only a very small chance of a merge conflict. But I also think that other English locales should fallback to the default English if they don't have a translation yet. Therefore we don't need to update other locales. And since we have only new keys, we can also test in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prod-test user facing changes Thes pull requests affect the user experience
Projects
Status: Test Ready 🧪
Development

Successfully merging this pull request may close these issues.

Add plural form for flower farming variant unit names
5 participants