fix: restore upstream msgids in ACE email templates so translations apply - #44
fix: restore upstream msgids in ACE email templates so translations apply#44ZamanChaudhary wants to merge 1 commit into
Conversation
muhammadali286
left a comment
There was a problem hiding this comment.
🤖 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.
|
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 hereCorrect 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 sequencingRather 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 F5 · product call, no actionAgreed — inconsistent subject/body translation behavior across email types is a product decision, not something this PR should change. F6 · addressed, but in #339 not hereAdded a catalog-level regression test ( F8 · nit acknowledged, no actionAgreed 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 noteAgreed this isn't a defect (autoescape covers it) — nothing to change. |
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
stack with
pref-lang=ar: subject, RTL body and CTA all render inArabic (previously body was English).
diffagainst upstream templates confirms msgids are byte-identical.Notes for reviewers
silently break its translation again — keep msgids identical to
upstream, or ship a catalog for the new wording.