Uh oh!
There was an error while loading. Please reload this page.
refactor(i18n): move updateLanguage to runtime as shared funtionality - #289
refactor(i18n): move updateLanguage to runtime as shared funtionality#289dcoa wants to merge 7 commits into
Conversation
Thanks for the pull request, @dcoa! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
a7923f1 to
93bf13eCompare…ang attribute in html
4b9b67f to
3432147Compare0236562 to
9e46401Compare9e46401 to
ad58514Compare
arbrandes
left a comment
There was a problem hiding this comment.
Looking pretty good, but I found a bug and a bunch of nits, with Claude's help. Mind taking a look? Thanks!
| const isLocaleSupported = (code) => { | ||
| if (supportedLanguages.length > 0) { | ||
| return supportedLanguages.includes(code) && messages[code] !== undefined; | ||
| } | ||
| return messages[code] !== undefined; | ||
| }; |
There was a problem hiding this comment.
This is the major issue with the PR as is: English disappears when defaultLanguage is not en.
English messages are never in the messages map. isLocaleSupported('en') is therefore always false, so findSupportedLocale('en') returns defaultLanguage. The other half is in getSupportedLanguageList below.
So, for example, on a Spanish-default site an English speaker has no way back to English: the option is absent from the footer menu, and explicitly allowlisting en in supportedLanguages does not restore it. Selecting it programmatically via updateSiteLanguage('en') also silently lands on es-419 while persisting pref-lang=en to the API, leaving the cookie and the UI disagreeing.
The fix is to treat en as always supported, and to keep adding it to the language list alongside defaultLanguage.
| let locales = Object.keys(messages); | ||
| if (!locales.includes(defaultLanguage)) { | ||
| locales.push(defaultLanguage); | ||
| } | ||
| if (supportedLanguages.length > 0) { | ||
| locales = locales.filter((locale) => supportedLanguages.includes(locale)); | ||
| } |
There was a problem hiding this comment.
Other half of the blocking issue on findSupportedLocale above: the old unconditional locales.push('en') was replaced by pushing defaultLanguage, so en is only in the list when it is the default.
| } | ||
| if (messages[locale] !== undefined) { | ||
| const { defaultLanguage, supportedLanguages = [] } = getSiteConfig(); |
There was a problem hiding this comment.
Use the same defaultLanguage = 'en' destructuring default that getSupportedLanguageList does.
| if (user !== null) { | ||
| await patchUserPreferences(user.username, locale); | ||
| } | ||
| await setSessionLanguage(locale); |
There was a problem hiding this comment.
If the preferences PATCH fails, it's also going to skip the cookie write. Suggestion: attempt both calls and aggregate the failures, rather than short-circuiting.
| const formData = new FormData(); | ||
| formData.append('language', locale); | ||
| // Post to the LMS setlang endpoint for server-side persistence. | ||
| // Use the authenticated HTTP client to ensure that the request includes the CSRF token. | ||
| // Works for both authenticated and anonymous users, since the LMS setlang endpoint is public. |
There was a problem hiding this comment.
Delete the FormData construction and update the comment - the request is a JSON PATCH to lang_pref/update_language, not a form post to setlang.
| ************ | ||
| This is a step by step guide to making your React app ready to accept translations. The instructions here are very specific to the edX setup. | ||
| This is a step by step guide to making your React app ready to accept translations. The instructions here are very specific to the Openedx setup. |
There was a problem hiding this comment.
"Openedx" should be "Open edX".
| }); | ||
| it('should filter by supportedLanguages when configured', () => { | ||
| mergeSiteConfig({ supportedLanguages: ['en', 'es-419'] }); |
There was a problem hiding this comment.
Site config is leaking between test blocks. Restore the default in an afterEach so :184 doesn't have to mutate getSiteConfig().supportedLanguages directly.
This mergeSiteConfig in particular is never undone, which also weakens the updateLocale block: the leaked allowlist excludes ar, so "should take precedence over the language preference cookie" isn't testing against a locale the cookie path could actually have returned.
| }); | ||
| handleRtl(); | ||
| expect(setAttribute).toHaveBeenCalledWith('lang', 'es-419'); |
There was a problem hiding this comment.
This test block is called handleRtl, but here it's being removed. These assertions now fire on setAttribute calls that only configureI18n triggers, so the function under test is exercised only incidentally.
| <Dropdown.Item key={language.code} onClick={handleClick}> | ||
| <Dropdown.Item | ||
| key={language.code} | ||
| className={isActive ? 'active' : ''} |
There was a problem hiding this comment.
Use the active prop instead of the class. className={isActive ? 'active' : ''} produces the same markup as active={isActive} in this react-bootstrap version, but the prop is the intended API.
| const mockUpdateLocale = updateLocale as jest.MockedFunction<typeof updateLocale>; | ||
| describe('updateSiteLanguage', () => { | ||
| const mockAuthHttpClient = { patch: jest.fn(), post: jest.fn() }; |
There was a problem hiding this comment.
Nit: post is left over from the setlang POST and is never used.
Description
The current PR aims to consolidate a reusable feature that enables updating the language preference across different UI components, ensuring consistent behavior.
What changes were implemented?
Moved the
updateSiteLanguagefunction fromshell/footertoruntime/i18n, making it reusable across any component in the site. For example, it can now be used by theLanguageMenuin the Footer and the Language Preference Selector in the Account App.Added two optional configuration properties to the i18n runtime:
defaultLanguage(string, default:'en'): Defines the fallback locale when a user's preferred locale is not supported. Open edX provides theLANGUAGE_CODEsetting to configure the platform's default language (for any non-English site), but this setting is not currently read by the client-side code. This property provides equivalent flexibility on the client side.supportedLanguages(string[], default:[]): Defines an allowlist of supported language codes. When non-empty, only these languages are displayed in the language picker and considered supported during locale resolution. An empty array means that all loaded locales are considered supported.Updated
handleRtl()to set both thelanganddirattributes on<html>. Previously, thelangattribute was not being updated. Updatinglangensures browsers, screen readers, and other accessibility tools correctly identify the page's language and provide appropriate language-specific behavior.Performs a optimistic UI update,
updateLocalechanges the UI, then the user preference and the cookie is updated via API.Displays a Toast message when the preference language API or user preference API fails.
How to test
test-sitei18nfolder to expose the translations (usemake extract_translationsor you can get support of AI). For example:defaultLanguageScreencast.from.2026-08-14.23-51-01.webm
Additional Information
LLM use
Supported by Opencode - Deepseek V4