Skip to content

Add ability to send a notification email on review of community libra… #6050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marcellamaki wants to merge 1 commit into
base: hotfixes
Choose a base branch
from
Open

Add ability to send a notification email on review of community libra… #6050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions contentcuration/contentcuration/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
from django.db.models.query_utils import DeferredAttribute
from django.db.models.sql import Query
from django.dispatch import receiver
from django.template.loader import render_to_string
from django.urls import reverse
from django.utils import timezone
from django.utils.translation import gettext as _
from django_cte import CTEManager
Expand Down Expand Up @@ -86,7 +88,9 @@
from contentcuration.db.models.manager import CustomContentNodeTreeManager
from contentcuration.db.models.manager import CustomManager
from contentcuration.utils.cache import delete_public_channel_cache_keys
from contentcuration.utils.messages import get_messages
from contentcuration.utils.parser import load_json_string
from contentcuration.utils.urls import canonical_url
from contentcuration.viewsets.sync.constants import ALL_CHANGES
from contentcuration.viewsets.sync.constants import ALL_TABLES
from contentcuration.viewsets.sync.constants import PUBLISHABLE_CHANGE_TABLES
Expand Down Expand Up @@ -3049,6 +3053,53 @@ def notify_update_to_channel_editors(self, exclude_user_id=None):

User.notify_users(editors, date=self.date_updated)

def send_resolution_email(self):
"""
Send an email to the submission author letting them know their
Community Library submission has been resolved (approved or
rejected).
"""
is_approved = self.status == community_library_submission.STATUS_APPROVED

community_strings = get_messages().get("CommunityChannelsStrings", {})

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

status_message = (
community_strings["approvedStatus"]
if is_approved
else community_strings["flaggedStatus"]
)
subject_text = "{}: {}".format(

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

community_strings["communityLibrarySubmissionLabel"], status_message
)

subject = render_to_string(
"registration/custom_email_subject.txt",
{"subject": subject_text},
)
subject = "".join(subject.splitlines())

message = render_to_string(
"community_library/submission_resolved_email.html",
{
"name": self.author.get_full_name(),
"channel": self.channel,
"channel_url": canonical_url(
reverse("channel", kwargs={"channel_id": self.channel.pk})
),
"approved": is_approved,

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: approved is never read by the template — the branch is fully resolved in Python via status_message. Dead context key.

"status_message": (
community_strings["availableStatus"]

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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["needsChangesPrimaryInfo"]

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

),
"feedback_notes_label": community_strings["feedbackNotesLabel"],
"feedback_notes": self.feedback_notes,
},
)

self.author.email_user(
subject, message, settings.DEFAULT_FROM_EMAIL, html_message=message
)

@classmethod
def filter_view_queryset(cls, queryset, user):
if user.is_anonymous:
Expand Down
27 changes: 27 additions & 0 deletions contentcuration/contentcuration/templates/community_library/submission_resolved_email.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<!DOCTYPE html>
{% load i18n %}
<html>

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 %}">

<head>
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
</head>
<body>
{% autoescape off %}

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

<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>

@rtibblesbot rtibblesbot Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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.


<p>{{ status_message }}</p>

{% if feedback_notes %}
<p>{{ feedback_notes_label }}: {{ feedback_notes }}</p>
{% endif %}

<p>
{% translate "Thanks for using Kolibri Studio!" %}
<br>
{% translate "The Learning Equality Team" %}
</p>
{% endautoescape %}
</body>
</html>
16 changes: 16 additions & 0 deletions contentcuration/contentcuration/tests/viewsets/test_community_library_submission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from unittest import mock

import pytz
from django.core import mail
from django.urls import reverse

from contentcuration.constants import (
Expand Down Expand Up @@ -731,6 +732,13 @@ def test_resolve_submission__accept_correct(self, apply_task_mock):
channel_id=self.submission.channel.id,
)

self.assertEqual(len(mail.outbox), 1)

@rtibblesbot rtibblesbot Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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.

sent_email = mail.outbox[0]
self.assertEqual(sent_email.to, [self.submission.author.email])
self.assertIn("approved", sent_email.subject.lower())

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

self.assertIn("available in community library", sent_email.body.lower())
self.assertIn(self.submission.channel.name, sent_email.body)

@mock.patch(
"contentcuration.viewsets.community_library_submission.apply_channel_changes_task"
)
Expand Down Expand Up @@ -770,6 +778,14 @@ def test_resolve_submission__reject_correct(self, apply_task_mock):
)
apply_task_mock.fetch_or_enqueue.assert_not_called()

self.assertEqual(len(mail.outbox), 1)
sent_email = mail.outbox[0]
self.assertEqual(sent_email.to, [self.submission.author.email])
self.assertIn("needs changes", sent_email.subject.lower())
self.assertIn("needs changes", sent_email.body.lower())
self.assertIn(self.submission.channel.name, sent_email.body)
self.assertIn(self.feedback_notes, sent_email.body)

def test_resolve_submission__reject_missing_resolution_reason(self):
self.client.force_authenticate(user=self.admin_user)
metadata = self.resolve_reject_metadata.copy()
Expand Down
2 changes: 2 additions & 0 deletions contentcuration/contentcuration/viewsets/community_library_submission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,6 @@ def resolve(self, request, pk=None):
published_version.id
)

submission.send_resolution_email()

@rtibblesbot rtibblesbot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


return Response(self.serialize_object())
Loading