Add ability to send a notification email on review of community libra… - #6050
Add ability to send a notification email on review of community libra…#6050marcellamaki wants to merge 1 commit into
Conversation
🔵 Review postedLast updated: 2026-07-29 15:14 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6050 adds a resolution-notification email (approved / requires changes) to the community-library resolve flow, cleanly mirroring the channel_published_email pattern and well covered by tests.
One blocking concern plus a few smaller notes:
- blocking — introduces six net-new Django gettext strings on the non-default
hotfixesbranch, whereas issue #5993 scoped this to reusing existing strings. See models.py:3064. Confirm the branch/translation tradeoff is intentional. - suggestion — email is sent before the approval side-effects; a send failure could leave an APPROVED-but-not-added state (viewset:349).
- suggestion —
namereimplementsUser.get_full_name()(models.py:3077). - nitpick —
blocktranswrapping a bare variable adds noise to the catalog (template:12, and the same at line 21).
CI pending. Manual QA was required for this PR but did not run — UI/email rendering was not visually verified.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| is_approved = self.status == community_library_submission.STATUS_APPROVED | ||
|
|
||
| if is_approved: | ||
| subject_text = _("Your Community Library submission has been approved") |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: This email introduces six net-new Django gettext strings — the two subjects here (3064/3066) plus five template strings ("Available in Community Library", "Your previously submitted version needs changes…", "Notes from the reviewer", "Thanks for using Kolibri Studio!", "The Learning Equality Team"). None exist in locale/en/LC_MESSAGES/django.po; the matching frontend contentcuration-messages.json text is a separate ($tr) catalog. Two issues: (1) linked issue #5993 scoped this to reusing existing strings, but in the Django catalog these are new; (2) this PR targets hotfixes, not unstable, so new strings miss the normal extraction/translation cycle and will ship English-only until forward-merged. If English-only on the hotfix is an accepted P1 tradeoff, that's fine — please confirm it's intentional and that extraction also happens on unstable.
| ) | ||
|
|
||
| submission.notify_update_to_channel_editors() | ||
| submission.send_resolution_email() |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: send_resolution_email() runs before the approval side-effects (_mark_previous_pending_submissions_as_superseded / _add_to_community_library / mark_channel_version_as_distributable). email_user only swallows the two Postmark recipient exceptions; any other send/render error propagates. With no transaction.atomic here and ATOMIC_REQUESTS unset, serializer.save() has already committed APPROVED — a send failure would 500 and leave the submission approved but never added to the community library. Move the send after the state-mutation block so a mail failure is the least-damaging failure point.
| message = render_to_string( | ||
| "community_library/submission_resolved_email.html", | ||
| { | ||
| "name": f"{self.author.first_name} {self.author.last_name}".strip(), |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: f"{self.author.first_name} {self.author.last_name}".strip() duplicates User.get_full_name(). Use self.author.get_full_name().
| {% autoescape off %} | ||
| <p>{% blocktrans with name=name %}Hello {{ name }},{% endblocktrans %}</p> | ||
|
|
||
| <p><a href="{{ channel_url }}" target="_blank">{% blocktrans with channel_name=channel.name %}{{ channel_name }}{% endblocktrans %}</a> ({{ channel_url }})</p> |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: {% blocktrans with channel_name=channel.name %}{{ channel_name }}{% endblocktrans %} emits a msgid of just the placeholder — nothing for a translator to act on. Plain {{ channel.name }} is equivalent. Same at line 21 for feedback_notes. Matches the existing channel_published_email.html, so take it or leave it.
| channel_id=self.submission.channel.id, | ||
| ) | ||
|
|
||
| self.assertEqual(len(mail.outbox), 1) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Both paths assert against the real locmem mail backend — recipient, subject, body copy, channel name, and feedback notes — rather than mocking the mail layer. Solid end-to-end coverage of the new path.
63291a2 to
d0b294b
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6050 — delta re-review.
All 5 prior findings are settled (4 resolved, 1 acknowledged); sourcing the copy from CommunityChannelsStrings was the right call and drops the net-new Django msgids to zero. The rewritten send_resolution_email brings new findings, though: the email is rendered in the reviewing admin's locale rather than a fixed one, and a mail failure now 500s a resolution that has already committed and can't be retried.
CI passing. Manual QA did not run for this PR — nothing here was visually verified.
- blocking — email localized to the reviewer's language (models.py:3064); unhandled send failure after commit (community_library_submission.py:361)
- suggestion — approval email claims availability before the async export runs, subject built by concatenation, UI-panel strings reused as email prose, missing
lang/dir, unnecessaryautoescape off, test assertions hard-coding frontend catalogue copy - nitpick — dead
approvedcontext key
Prior-finding status
RESOLVED — contentcuration/contentcuration/models.py:3064 — six net-new Django gettext strings on hotfixes
RESOLVED — contentcuration/contentcuration/viewsets/community_library_submission.py:361 — email sent before the approval side-effects
RESOLVED — contentcuration/contentcuration/models.py:3083 — name reimplements User.get_full_name()
ACKNOWLEDGED — contentcuration/contentcuration/templates/community_library/submission_resolved_email.html:12 — blocktrans wrapping a bare variable
RESOLVED — contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py:735 — locmem mail assertions (praise)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| """ | ||
| is_approved = self.status == community_library_submission.STATUS_APPROVED | ||
|
|
||
| community_strings = get_messages().get("CommunityChannelsStrings", {}) |
There was a problem hiding this comment.
blocking: The email is rendered in the reviewing admin's language, not the recipient's.
get_messages() keys off to_locale(get_language()), and the {% translate %} tags in the template resolve against the same active language. send_resolution_email() runs synchronously inside the admin's resolve request, and KolibriStudioLocaleMiddleware is installed (settings.py:133), so the active locale is whatever the reviewer's session/Accept-Language says. A reviewer browsing Studio in Spanish sends the author a Spanish email.
Studio's other transactional emails avoid this by accident — they render inside Celery tasks (utils/publish.py, tasks.py) where no request locale is active, so they fall back to LANGUAGE_CODE. User has no language preference field, so the recipient's language isn't knowable today; wrapping the subject/body render in translation.override(settings.LANGUAGE_CODE) would at least make this deterministic and consistent with the rest of Studio's mail.
| published_version.id | ||
| ) | ||
|
|
||
| submission.send_resolution_email() |
There was a problem hiding this comment.
blocking: A mail failure here 500s a resolution that has already committed, and the admin can't retry.
User.email_user (models.py:568-574) only swallows PMMailInactiveRecipientException / PMMailUnauthorizedException; any other backend failure propagates. This call sits after serializer.save(), the superseded-status update(), Change.create_change(), apply_channel_changes_task.fetch_or_enqueue() and mark_channel_version_as_distributable() — and ATOMIC_REQUESTS isn't set, so those are separate autocommits that don't roll back. Retrying is also impossible: CommunityLibrarySubmissionResolveSerializer.update rejects anything that isn't PENDING (lines 158-162).
Same path swallows the KeyError risk from models.py:3064 — get_messages().get("CommunityChannelsStrings", {}) returns {} when the locale JSON can't be opened (utils/messages.py:31-32 catches IOError), and every subsequent access is an unguarded [...], so the {} default only converts one exception into another two lines later.
Moving the send into transaction.on_commit(...) or a task (matching the publish/custom-email flows) — or catching and logging around it — fixes both, and keeps SMTP latency off the admin's request path.
| ), | ||
| "approved": is_approved, | ||
| "status_message": ( | ||
| community_strings["availableStatus"] |
There was a problem hiding this comment.
suggestion: The approval email says "Available in Community Library" before the channel actually is. _add_to_community_library only creates a Change and enqueues apply_channel_changes_task; the export and mark_live() happen later in viewsets/channel.py:853-867. At send time the submission is APPROVED, not LIVE. If the task fails or is retried, the author has been told the channel is available and there's no corrective mail. Sending the approval email from mark_live() would make the claim true; the rejection email has no such dependency.
| if is_approved | ||
| else community_strings["flaggedStatus"] | ||
| ) | ||
| subject_text = "{}: {}".format( |
There was a problem hiding this comment.
suggestion: The subject is a user-visible sentence assembled from two independently-translated fragments and a hard-coded ": ". Translators never see the composed string and can't reorder the parts or change the separator (which isn't universal, and reads wrong in RTL). A single catalogue entry with a parameter — the convention used throughout communityChannelsStrings.js, e.g. reasonLabel: 'Reason: {reason}' — keeps punctuation and word order translatable. Same pattern in the template at line 17.
| "status_message": ( | ||
| community_strings["availableStatus"] | ||
| if is_approved | ||
| else community_strings["needsChangesPrimaryInfo"] |
There was a problem hiding this comment.
suggestion: These strings carry a translator context describing a different surface. needsChangesPrimaryInfo is documented as 'Information shown in the "Submit to Community Library" panel when a previous version was not approved' — in an email about the submission that was just resolved, "your previously submitted version" reads oddly; availableStatus and flaggedStatus are documented as status chips. Translators localize to the context they're given, so panel copy will read wrong in every non-English locale.
This is also the first backend use of get_messages() for individual keys rather than dumping the whole catalogue into a page context, so a frontend copy edit now silently rewrites email content. The PR description already flags wording as follow-up — worth widening those context strings (or adding email-specific entries) when it happens.
| @@ -0,0 +1,27 @@ | |||
| <!DOCTYPE html> | |||
| {% load i18n %} | |||
| <html> | |||
There was a problem hiding this comment.
suggestion: No lang/dir on <html>, so RTL locales render LTR. Studio ships Arabic, and locale/ar/.../contentcuration-messages.json has all six keys used here — combined with the locale finding on models.py, this email will be rendered in Arabic whenever the reviewing admin browses in ar. RTLCSS doesn't touch templates and mail clients default to LTR. {% load i18n %} is already present, so this works without the i18n context processor (which doesn't run under render_to_string with no request):
{% get_current_language as LANG %}{% get_current_language_bidi as LANG_BIDI %}
<html lang="{{ LANG }}" dir="{% if LANG_BIDI %}rtl{% else %}ltr{% endif %}">| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| </head> | ||
| <body> | ||
| {% autoescape off %} |
There was a problem hiding this comment.
suggestion: Nothing in this template emits markup from context — name, channel.name, feedback_notes, status_message and feedback_notes_label are all plain text — so autoescape off buys nothing and injects reviewer-authored feedback_notes and author-authored channel.name raw into an HTML email. Inherited from registration/channel_published_email.html, but a new template is a cheap place to stop copying it; dropping lines 9 and 25 should be behaviourally identical.
| "channel_url": canonical_url( | ||
| reverse("channel", kwargs={"channel_id": self.channel.pk}) | ||
| ), | ||
| "approved": is_approved, |
There was a problem hiding this comment.
nitpick: approved is never read by the template — the branch is fully resolved in Python via status_message. Dead context key.
| self.assertEqual(len(mail.outbox), 1) | ||
| sent_email = mail.outbox[0] | ||
| self.assertEqual(sent_email.to, [self.submission.author.email]) | ||
| self.assertIn("approved", sent_email.subject.lower()) |
There was a problem hiding this comment.
suggestion: These literals are the current values of CommunityChannelsStrings.approvedStatus / availableStatus, so a frontend copy edit breaks a backend test with no visible connection to the change. Asserting against the same source the code reads (get_messages()["CommunityChannelsStrings"]["availableStatus"]) checks that the right string was selected rather than which words it happens to contain today.
Two cheap additions while you're here: assert len(mail.outbox) == 0 in the validation-failure cases (nothing currently catches an email leaking out of a rejected request), and assert the channel URL appears in the body — canonical_url(reverse("channel", ...)) is the one piece of new render logic nothing exercises.
2 participants
Summary
This implements a very basic notification email to support communication around community library submissions. It seems from a lack of updates/edits to reviewed channels that users maybe were not seeing their notifications. This repurposes existing strings (therefore, we may want to make future edits to the wording to be more detailed/informative), to send an update about accepted community library channels or requesting edits/updates.
References
Fixes #5993
Reviewer guidance
I think code review here is best, and then manual QA on hotfixes to confirm email end to end. I did some testing of this using assistance from Claude, but it was a bit of a workaround, and I think testing more fully (to a real email address) will be more helpful than trying to replicate that during PR review.
AI usage
Implemented with Claude -- wrote up a spec together with me reviewing and providing inputs and guidance. Claude implementation, and then manual code review and testing in the browser and confirming logged outputs and email functions.