Skip to content

fix: restore upstream msgids in ACE email templates so translations apply - #44

Open
ZamanChaudhary wants to merge 1 commit into
develop-ulmofrom
zaman/fix-ace-email-translation-msgids
Open

fix: restore upstream msgids in ACE email templates so translations apply#44
ZamanChaudhary wants to merge 1 commit into
develop-ulmofrom
zaman/fix-ace-email-translation-msgids

Conversation

@ZamanChaudhary

Copy link
Copy Markdown

EDLYPRODUCT-8318

Problem

Users with a non-English account language (e.g. Arabic) receive the
password reset email in English on Ulmo, while it was translated on Koa.

Root cause: gettext matches msgids exactly. The Indigo theme's ACE email
templates reworded upstream strings ("You are receiving..." vs upstream
"You're receiving...", "Change my Password" vs "Reset my password"), so
they match no entry in the community translation catalogs that atlas
ships into every image — Django silently falls back to English.

Fix

Restore the translatable strings to exact upstream wording (styling
untouched). Translations then come from the community catalogs with zero
maintenance for us, in every language the community supports.

Also: fixes the shared email frame (mailing-address msgid, unsubscribe
line), tags one untranslatable hardcoded line in the OTP email
(forward-looking — its catalog is a separate change), and cleans a stray
space in the certificate email subject msgid.

Testing

  • Rendered the password reset email end-to-end on a local Tutor dev
    stack with pref-lang=ar: subject, RTL body and CTA all render in
    Arabic (previously body was English).
  • diff against upstream templates confirms msgids are byte-identical.
  • lms/cms base_body copies verified identical.

Notes for reviewers

  • Zero core changes: only theme templates in this repo.
  • Needs an openedx image rebuild to reach staging.
  • Any future rewording of a translatable string in these templates will
    silently break its translation again — keep msgids identical to
    upstream, or ship a catalog for the new wording.

@muhammadali286 muhammadali286 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Automated review by Claude (Opus 5), run by @muhammadali286

Reviewed against EDLYPRODUCT-8318, alongside its companion PR https://github.com/edly-io/edly-panel-edx-app/pull/339.

Verified — the diagnosis and the fix are both correct

I diffed the changed msgids against upstream edly-io/edx-platform@develop-ulmo. Six of the seven changed strings match byte-for-byte:

String Upstream location Match
"Password reset" openedx/core/djangoapps/user_authn/templates/user_authn/edx_ace/passwordreset/email/body.html
"You're receiving this e-mail because you requested a password reset for your user account at {{ platform_name }}." same
"If you didn't request this change, you can disregard this email - we have not yet reset your password." same
"Reset my password" same
"Our mailing address is:" — colon inside the msgid openedx/core/djangoapps/ace_common/templates/ace_common/edx_ace/common/base_body.html
"Unsubscribe from these emails." same

The root-cause analysis holds exactly as written: gettext matches msgids literally, Indigo had reworded upstream strings, so nothing matched the atlas-shipped community catalogs and Django fell back to English silently. Restoring the upstream wording buys translations in every community-supported language at zero maintenance cost — the right fix, and a much better one than shipping our own catalogs for reworded strings.

Worth calling out that the "Our mailing address is:" change is the easy one to get backwards (the colon moving into the msgid looks like a typo unless you check upstream) — it's correct here.


🟡 F3 · medium · the ticket isn't closable on this PR + #339 alone

EDLYPRODUCT-8318 names enrollment confirmations, certificate notifications, password resets and program updates. Between the two PRs, what's actually fixed is: password reset, the shared email frame, and three panel emails.

Certificate notifications specifically are not fixed by this diff. The change to edly_features_app/edx_ace/certificategeneration/email/subject.txt only strips a leading space — and since that template is Edly-owned rather than upstream, there is no community catalog entry for "Learner has earned a new certificate {{ platform_name }}" in any case. That string stays English before and after. The cleanup is fine, but it reads in the PR summary as part of the translation fix, and it isn't.

Enrollment confirmations and program updates aren't touched at all.

Since the mechanism here is generic and cheap, a diff of every Indigo ACE template against its upstream counterpart would either close the ticket properly or give a precise scope for what remains. Right now the ticket can't be closed on these two PRs, which is worth knowing before it moves to QA.

🟡 F1 · medium (cross-PR, sequencing) · your own warning applies to another open PR

The reviewer note here says:

Any future rewording of a translatable string in these templates will silently break its translation again — keep msgids identical to upstream, or ship a catalog for the new wording.

That is exactly right, and #47 (EDLYPRODUCT-8352, currently open, same repo) is doing it: it replaces "Invitation to Join " and "An account has been created for you in %(platform_name)s..." — both of which #339 is simultaneously shipping ar/fr/id/zh_CN translations for — and introduces three new English msgids with no catalog entry anywhere.

Flagging it here as well as on #339 because whoever merges these needs to sequence them deliberately rather than discover it afterwards. Details on the #339 review.

F5 · low · mixed-language emails after this lands

Once this merges, the passwordreset subject will translate via the community catalog, while the panel email subjects stay English by design (#339, Koa parity). Net effect is inconsistent subject-line behavior across platform emails, and for panel emails a translated body under an English subject. Probably a product call rather than something for this PR.

F6 · low · nothing guards against recurrence

Both PRs fix the same failure mode — msgid drift or missing catalog producing a silent English fallback, with no error anywhere. A cheap regression test (activate ar, render each ACE template, assert the output differs from the English render) would have surfaced the #47 collision in F1 automatically.

F8 · nit · duplicated template

base_body.html is edited identically in both the lms/ and cms/ trees. You verified they're identical, which is the right check — noting only that it's a copy-paste hazard for the next person.


One clarification, so it isn't misread as a security finding

Upstream wraps these strings as {% trans "..." as tmsg %}{{ tmsg | force_escape }} and {% filter force_escape %}...{% endfilter %}; the Indigo copies omit that. This does not affect the msgid, so it's irrelevant to this PR's goal, and Django's autoescape already covers these HTML templates — so it is not an XSS issue. Mentioning it only because "byte-identical to upstream" is true of the msgids but not of the surrounding template code, in case full parity is ever a goal.


Verdict: COMMENT — I found no defect in this diff, and the part that mattered most (upstream msgid parity) I was able to verify directly. F3 is about whether the ticket is done, not whether this code is right.

@ZamanChaudhary

Copy link
Copy Markdown
Author

Went through this review finding-by-finding — no code changes on this PR, and here's why for each:

🟡 F3 · scope gap acknowledged, not fixable here

Correct that certificate/enrollment/program-update emails aren't covered by this PR + #339. That's real scope, not something to patch with a code change on this diff — would need its own investigation of each remaining template's msgid parity against upstream.

🟡 F1 · resolved, not by sequencing

Rather than requiring #47 to merge first, #339 was updated to ship translations additively: the new msgids #47 introduces now have ar/fr/id/zh_CN entries alongside the old ones, extracted verbatim via django-admin makemessages against #47's actual templates. So there's no merge-order dependency between this PR, #47, and #339 anymore — verified live in a devstack render. Details on #339.

F5 · product call, no action

Agreed — inconsistent subject/body translation behavior across email types is a product decision, not something this PR should change.

F6 · addressed, but in #339 not here

Added a catalog-level regression test (test_translations.py) in edly-panel-edx-app. It doesn't belong in this repo — tutor-indigo has no Python test infrastructure at all (Makefile only runs lint/types/format), and there's nothing here to render against. Note it verifies catalog resolution, not cross-repo msgid drift between this repo's templates and panel-edx-app's catalogs — no test today can check that, since neither repo can see the other's files.

F8 · nit acknowledged, no action

Agreed it's a copy-paste hazard; not deduplicating the lms/cms template split in this PR — that's a theme-structure change, not a translation fix.

Escaping note

Agreed this isn't a defect (autoescape covers it) — nothing to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants