fix: Correction to the language code for Português, Italian and German #903

heldersepu · 2023-10-12T03:45:13Z

Started researching into this issues because of this issue:
openedx/wg-build-test-release#267

For a few languages we have multiple files:

missing translation:

proper translation:

This PR adds the correct one in the options

openedx-webhooks · 2023-10-12T03:45:19Z

Thanks for the pull request, @heldersepu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

heldersepu · 2023-10-15T23:35:06Z

@openedx/cla-problems I completed the Contributor License Agreement form (twice) it said I should expect to see an email from DocuSign with 1 business day, but no email from DocuSign

mphilbrick211 · 2023-10-16T17:15:49Z

@openedx/cla-problems I completed the Contributor License Agreement form (twice) it said I should expect to see an email from DocuSign with 1 business day, but no email from DocuSign

Hi @heldersepu! I will look into this for you.

heldersepu · 2023-10-16T18:48:26Z

Hi @heldersepu! I will look into this for you.

Thanks @mphilbrick211 I received the email from DocuSing, I signed it and now waiting for the requirement to clear

mphilbrick211 · 2023-10-16T18:56:10Z

Hi @heldersepu! My colleague sent you the CLA this afternoon for your review / signature. Could you please confirm if you received it?

mphilbrick211 · 2023-10-16T19:44:55Z

Hi @heldersepu! I will look into this for you.

Thanks @mphilbrick211 I received the email from DocuSing, I signed it and now waiting for the requirement to clear

Great! Once you rebase it should turn green. It can take 24 hours to go through. If you're still having any issues, please let me know!

codecov · 2023-10-17T15:49:21Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f098fe1) 46.44% compared to head (960f902) 46.51%.
Report is 9 commits behind head on master.

❗ Current head 960f902 differs from pull request most recent head 5151b13. Consider uploading reports for the commit 5151b13 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #903      +/-   ##
==========================================
+ Coverage   46.44%   46.51%   +0.06%     
==========================================
  Files         116      117       +1     
  Lines        2405     2408       +3     
  Branches      640      640              
==========================================
+ Hits         1117     1120       +3     
  Misses       1214     1214              
  Partials       74       74

Files	Coverage Δ
src/account-settings/site-language/constants.js	`100.00% <ø> (ø)`

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

e0d · 2023-10-17T18:44:21Z

Hi, we need to have a valid Contributor License Agreement (CLA) in place for all contributions. See the welcome message above for the details about how to enroll. The process is different depending upon whether you are making this contribution as an individual or on behalf of your employer.

heldersepu · 2023-10-17T20:41:16Z

Hi, we need to have a valid Contributor License Agreement (CLA) in place for all contributions. See the welcome message above for the details about how to enroll. The process is different depending upon whether you are making this contribution as an individual or on behalf of your employer.

Hello @e0d I believe that requirement has been satisfied:

e0d · 2023-10-18T01:04:23Z

Looks good

heldersepu · 2023-10-19T14:16:11Z

Looks good

@e0d ... I still see the review is required

e0d · 2023-10-19T14:18:45Z

I've updated the status to ensure that the reviewing team sees this is ready.

e0d · 2023-10-19T14:19:39Z

@OmarIthawi would you be able to review this PR?

OmarIthawi

Thanks @heldersepu for spotting the bug.

I've checked the list of supported languages and it aligns with this pull request.

However, looking at the French Candian locale which I'm assuming is correct, I see the codes need to be all lower case.

frontend-app-account/src/account-settings/site-language/constants.js

Lines 97 to 101 in 98083b1

    
           { 
        
             code: 'fr-ca', 
        
             name: 'French (CA)', 
        
             released: true, 
        
           },

Please update it as suggested and it should be good to go.

Thanks @e0d for the ping :)

src/account-settings/site-language/constants.js

heldersepu · 2023-10-19T14:58:59Z

@OmarIthawi thanks for the review... I just tested and it looks like the codes are case insensitive, but I like your suggestion to keep them all the same (lowercase)
I merged your suggestions ... and now the build is failing something about lint:
https://github.com/openedx/frontend-app-account/actions/runs/6576412813/job/17865793511?pr=903

OmarIthawi · 2023-10-19T15:04:55Z

@OmarIthawi thanks for the review... I just tested and it looks like the codes are case insensitive, but I like your suggestion to keep them all the same (lowercase) I merged your suggestions ... and now the build is failing something about lint: https://github.com/openedx/frontend-app-account/actions/runs/6576412813/job/17865793511?pr=903

Great @heldersepu! The tests are failing due to commit messages. Please squash them and it should be ready to merge.

fix: Correction to the language code for Português, Italian and German lowercase code for DE lowercase code for IT Co-Authored-By: Omar Al-Ithawi <[email protected]>

heldersepu · 2023-10-19T15:18:49Z

@OmarIthawi I squashed the commits... I think it now requires an approval

OmarIthawi · 2023-10-19T15:21:53Z

I can't approve though. But @mphilbrick211 should be able to. Would you mind helping?

brian-smith-tcril · 2023-10-19T15:45:03Z

I remember having conversations in the Translations working group about use of specific (de-DE) vs generic (de) language codes before. In the new (post OEP-58) system, we have a de.json file, but no it.json or pt.json.

I know we had issues with browser language not being supported when using the specific codes (for example, someone in Austria with a browser reporting de-AT would get de, but not de-DE).

My concern is that this PR would break the intended behavior (allowing fallbacks to the generic language code when the country code doesn't match).

heldersepu · 2023-10-19T16:07:22Z

I know we had issues with browser language not being supported when using the specific codes (for example, someone in Austria with a browser reporting de-AT would get de, but not de-DE).

@brian-smith-tcril the counter example to what you describe is spanish:

  {
    code: 'es-419',
    name: 'Español (Latinoamérica)',
    released: true,
  }

there is no code: 'es' in the constants or in the language files

In my tests the language is only shown correctly when the user selects it in the account preferences

an alternative ...
instead of updating the language code in the account-settings/site-language/constants.js
we could rename the language files to have the root/generic language on those that we have only one

OmarIthawi · 2023-10-19T16:17:33Z

I remember having conversations in the Translations working group about use of specific (de-DE) vs generic (de) language codes before. In the new (post OEP-58) system, we have a de.json file, but no it.json or pt.json.

I know we had issues with browser language not being supported when using the specific codes (for example, someone in Austria with a browser reporting de-AT would get de, but not de-DE).

My concern is that this PR would break the intended behavior (allowing fallbacks to the generic language code when the country code doesn't match).

@brian-smith-tcril I presumed that we maintain a canonical list of the languages in the following list of supported languages.

It be wrong, if that table is incorrect then we should fix it and probably move it into docs.openedx.org so also it's publicly accessible.

For the fallback mechanism, I'm afraid I don't have a good answer on that!

https://github.com/openedx/openedx-translations/blob/main/translations/frontend-app-account/src/i18n/messages/de.json

I've added this language and later removed it in Transifex after finding out that it's not what Translators would use.

The Transifex bot don't delete stale resoruces, so we should probably do that ourselves.

Anyway, this isn't to say which one is right. I don't know to be honest.

heldersepu · 2023-10-19T16:21:40Z

@OmarIthawi that list is missing plain French:

OmarIthawi · 2023-10-19T16:23:46Z

@OmarIthawi that list () is missing plain French:

https://github.com/openedx/frontend-app-account/blob/open-release/palm.master/src/i18n/messages/fr.json

https://github.com/openedx/frontend-app-account/blob/open-release/palm.master/src/i18n/messages/fr_CA.json

I don't have a quick answer for that. We should consult the wider Open edX Transifex Working Group.

heldersepu · 2023-10-19T16:28:08Z

I was doing some digging to my proposed alternative...

The only language( in the src/i18n/messages) with a true multi country code translation is french

the files de.json, it.json and pt.json are all in english, no translation there

heldersepu · 2023-10-19T16:44:03Z

@brian-smith-tcril you make the claim:

PR would break the intended behavior (fallbacks to the generic language code when the country code doesn't match)

I do not see in the code where the const siteLanguageList is used in "browser language" I see it only on <EditableSelectField name="siteLanguage"
https://github.com/openedx/frontend-app-account/blob/open-release/palm.master/src/account-settings/AccountSettingsPage.jsx#L744

@brian-smith-tcril maybe I'm missing something... Where in the code is the siteLanguageList used for that behavior?

brian-smith-tcril · 2023-10-19T18:06:10Z

@heldersepu you touched on my concern when you mentioned

the files de.json, it.json and pt.json are all in english, no translation there

I see the problem this PR is addressing (the siteLanguageList languages don't match the translation files) as having a few possible solutions:

Update the siteLanguageList to use the .json files as they exist now
Update the language codes so the translations live in .json files that match what's in siteLanguageList
Some combination of both (keep some with country codes but switch some to not use country codes)

I want to make sure that we don't want to address this via 2 or 3 before merging 1.

heldersepu · 2023-10-19T20:12:08Z

Sorry @brian-smith-tcril I really don't understand what you mean in 2

Update the siteLanguageList ...
Update the language codes ...

The language codes are in the siteLanguageList, I think both of those options are the same

I tested changing the language in my browser to see what will be the impact for a new user
The registration page seem to have the correct language ...

French (Swiz)

French (Canadian)

Spanish (Argentinian)

I tested a few other languages (clearing cache in between) and same pattern authn/register shows correct language, but the /accounts page always shows content in english until we select and save a language in the Site Preferences.

As far as I can see the const siteLanguageList is used only on <EditableSelectField name="siteLanguage"
...and the registration form code is not part of this repo (frontend-app-account)

heldersepu · 2023-10-19T23:52:32Z

@brian-smith-tcril I was doing some debugging to see what happens the first time a user with a valid language like fr goes into the account page, I set a breakpoint at function getLocale this is called a few times when the page is loading

First time we can see no cookie (as expected) and it gets the lang from navigator.language correctly

But on following calls we do have a cookie and it has been set to en ?!?!

Now I see your point ... let's see if I can untangle this

brian-smith-tcril · 2023-10-20T16:03:01Z

Sorry @brian-smith-tcril I really don't understand what you mean in 2

Update the siteLanguageList ...

Update the language codes ...

The language codes are in the siteLanguageList, I think both of those options are the same

I'm sorry I wasn't more clear in my previous message. To clarify what I meant I'll use German as an example.

For 1 (update the siteLanguageList) this would just mean doing what this PR does. The siteLanguageList would have de-de instead of de.

For 2 (update the language codes) this would mean ensuring all the proper German translations live in de.json. This way the de that currently exists in siteLanguageList would be accurate.

However, after bringing this up in the #wg-translations channel on Slack, it seems that option 2 is not straightforward.

I believe I also see why you're getting back en as the cookie. This is happening in findSupportedLocale

`findSupportedLocale` examples

Logic for `fr`:

This shows the issue with using fr, as we only have fr_CA

if (messages[locale] !== undefined) {
  return locale;
}

we don't have messages for fr, so we continue on to

if (messages[getPrimaryLanguageSubtag(locale)] !== undefined) {
  return getPrimaryLanguageSubtag(locale);
}

the primary subtag of fr is still fr, so we still don't have messages. we continue on to

return 'en';

Logic for `de_AT`:

This also demonstrates the issue with using de_DE instead of de. If someone had their browser set to de_AT, we would see:

if (messages[locale] !== undefined) {
  return locale;
}

we don't have messages for de_AT, so we continue on to

if (messages[getPrimaryLanguageSubtag(locale)] !== undefined) {
  return getPrimaryLanguageSubtag(locale);
}

the primary subtag of de_AT is de, so we still don't have messages. we continue on to

return 'en';

Logic for `de_AT` (if we switched to using `de` instead of `de_DE`)

If, however, we used de instead of de_DE, we would see:

if (messages[locale] !== undefined) {
  return locale;
}

we don't have messages for de_AT, so we continue on to

if (messages[getPrimaryLanguageSubtag(locale)] !== undefined) {
  return getPrimaryLanguageSubtag(locale);
}

the primary subtag of de_AT is de. We have messages! We return de!

brian-smith-tcril

While I believe the best solution to this would be to use generic language codes in transifex instead of country specific ones, this PR addresses the immediate issue effectively.

This brings the language codes in the dropdown in line with the language codes in the supported languages table, and will address the worst of the issues.

Without this change it would be impossible for someone to use the dropdown to override undesired browser behavior.

Before this PR

User has browser set to de_AT, sees English because we don't have de_AT or de translations.
User tries to change language using dropdown, selects German (de), sees English because we don't have de translations.

After this PR

User has browser set to de_AT, sees English because we don't have de_AT or de translations.
User tries to change language using dropdown, selects German (de_DE), sees German because we have de_DE (German - Germany) translations.

heldersepu · 2023-10-22T02:35:30Z

@brian-smith-tcril I see all the checks are green thank you!
but I do not have a button to merge, likely because that is an action reserved for approved reviews

... about the fallback mechanism (or generic language) looks like the spanish (es_419.json) was coded to be the fallback in the registration page:

We should probably do that for all that do not have a proper translation, that can be a next PR I could work on, I will raise it up in the wider Open edX Transifex Working Group as suggested by @OmarIthawi

heldersepu · 2023-10-22T02:46:01Z

@OmarIthawi I do not have access to the slack Open edX Transifex Working Group if you can invite me I can run point on that, if not just raise the questions yourself see what they have to say ...

things that need answers :

Delete stale resources, such as it.json and de.json
Languages not in the list of supported languages.
Is it OK to set language as fallback (like spanish in registration page) for languages that we have only one translation

OmarIthawi · 2023-10-23T08:17:07Z

@OmarIthawi I do not have access to the slack Open edX Transifex Working Group if you can invite me I can run point on that, if not just raise the questions yourself see what they have to say ...

@heldersepu get a invite via this link: https://openedx.org/slack

things that need answers :

Delete stale resources, such as it.json and de.json

Languages not in the list of supported languages.

Is it OK to set language as fallback (like spanish in registration page) for languages that we have only one translation

I can't answer those questions here, let's discuss them with the group.

openedx-webhooks · 2023-10-23T20:10:28Z

@heldersepu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 12, 2023

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 12, 2023

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Oct 17, 2023

OmarIthawi reviewed Oct 19, 2023

View reviewed changes

src/account-settings/site-language/constants.js Outdated Show resolved Hide resolved

src/account-settings/site-language/constants.js Outdated Show resolved Hide resolved

src/account-settings/site-language/constants.js Outdated Show resolved Hide resolved

fix: Correction to the language code for Português, Italian and German

5151b13

fix: Correction to the language code for Português, Italian and German lowercase code for DE lowercase code for IT Co-Authored-By: Omar Al-Ithawi <[email protected]>

heldersepu force-pushed the master branch from 960f902 to 5151b13 Compare October 19, 2023 15:09

OmarIthawi approved these changes Oct 19, 2023

View reviewed changes

mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 20, 2023

brian-smith-tcril approved these changes Oct 20, 2023

View reviewed changes

brian-smith-tcril removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 23, 2023

brian-smith-tcril merged commit 5f49bed into openedx:master Oct 23, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Correction to the language code for Português, Italian and German #903

fix: Correction to the language code for Português, Italian and German #903

heldersepu commented Oct 12, 2023 •

edited

Loading

openedx-webhooks commented Oct 12, 2023 •

edited

Loading

heldersepu commented Oct 15, 2023

mphilbrick211 commented Oct 16, 2023

heldersepu commented Oct 16, 2023

mphilbrick211 commented Oct 16, 2023

mphilbrick211 commented Oct 16, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

e0d commented Oct 17, 2023

heldersepu commented Oct 17, 2023

e0d commented Oct 18, 2023

heldersepu commented Oct 19, 2023

e0d commented Oct 19, 2023

e0d commented Oct 19, 2023

OmarIthawi left a comment

heldersepu commented Oct 19, 2023

OmarIthawi commented Oct 19, 2023

heldersepu commented Oct 19, 2023

OmarIthawi commented Oct 19, 2023

brian-smith-tcril commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

OmarIthawi commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

OmarIthawi commented Oct 19, 2023

heldersepu commented Oct 19, 2023

heldersepu commented Oct 19, 2023

brian-smith-tcril commented Oct 19, 2023

heldersepu commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

brian-smith-tcril commented Oct 20, 2023

brian-smith-tcril left a comment •

edited

Loading

heldersepu commented Oct 22, 2023

heldersepu commented Oct 22, 2023 •

edited

Loading

OmarIthawi commented Oct 23, 2023 •

edited

Loading

openedx-webhooks commented Oct 23, 2023

fix: Correction to the language code for Português, Italian and German #903

fix: Correction to the language code for Português, Italian and German #903

Conversation

heldersepu commented Oct 12, 2023 • edited Loading

openedx-webhooks commented Oct 12, 2023 • edited Loading

heldersepu commented Oct 15, 2023

mphilbrick211 commented Oct 16, 2023

heldersepu commented Oct 16, 2023

mphilbrick211 commented Oct 16, 2023

mphilbrick211 commented Oct 16, 2023

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

e0d commented Oct 17, 2023

heldersepu commented Oct 17, 2023

e0d commented Oct 18, 2023

heldersepu commented Oct 19, 2023

e0d commented Oct 19, 2023

e0d commented Oct 19, 2023

OmarIthawi left a comment

Choose a reason for hiding this comment

heldersepu commented Oct 19, 2023

OmarIthawi commented Oct 19, 2023

heldersepu commented Oct 19, 2023

OmarIthawi commented Oct 19, 2023

brian-smith-tcril commented Oct 19, 2023 • edited Loading

heldersepu commented Oct 19, 2023 • edited Loading

OmarIthawi commented Oct 19, 2023 • edited Loading

heldersepu commented Oct 19, 2023 • edited Loading

OmarIthawi commented Oct 19, 2023

heldersepu commented Oct 19, 2023

heldersepu commented Oct 19, 2023

brian-smith-tcril commented Oct 19, 2023

heldersepu commented Oct 19, 2023 • edited Loading

French (Swiz)

French (Canadian)

Spanish (Argentinian)

heldersepu commented Oct 19, 2023 • edited Loading

brian-smith-tcril commented Oct 20, 2023

findSupportedLocale examples

Logic for fr:

Logic for de_AT:

Logic for de_AT (if we switched to using de instead of de_DE)

brian-smith-tcril left a comment • edited Loading

Choose a reason for hiding this comment

Before this PR

After this PR

heldersepu commented Oct 22, 2023

heldersepu commented Oct 22, 2023 • edited Loading

OmarIthawi commented Oct 23, 2023 • edited Loading

openedx-webhooks commented Oct 23, 2023

heldersepu commented Oct 12, 2023 •

edited

Loading

openedx-webhooks commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

brian-smith-tcril commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

OmarIthawi commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

heldersepu commented Oct 19, 2023 •

edited

Loading

`findSupportedLocale` examples

Logic for `fr`:

Logic for `de_AT`:

Logic for `de_AT` (if we switched to using `de` instead of `de_DE`)

brian-smith-tcril left a comment •

edited

Loading

heldersepu commented Oct 22, 2023 •

edited

Loading

OmarIthawi commented Oct 23, 2023 •

edited

Loading