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

Merged
marcellamaki merged 2 commits into from
Aug 5, 2026
Merged

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
2 commits
Select commit Hold shift + click to select a range
d0b294b
Add ability to send a notification email on review of community libra…
marcellamaki Jul 21, 2026
3fab968
Simplify strings and update the place in the workflow that the mail i…
marcellamaki Aug 4, 2026
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
48 changes: 48 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,7 +48,10 @@
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 import translation
from django.utils.translation import gettext as _
from django_cte import CTEManager
from django_cte import CTEQuerySet
Expand Down Expand Up @@ -86,7 +89,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.i18n import closest_supported_locale
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 +3054,49 @@ 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

channel_language = self.channel.language
locale_code = (
closest_supported_locale(channel_language.lang_code)
if channel_language
else None
) or settings.LANGUAGE_CODE
with translation.override(locale_code):
if is_approved:
subject_text = _("Your Community Library submission has been approved")

@rtibblesbot rtibblesbot Aug 4, 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 two subjects and the three template strings are net-new msgids — grep -c "Community Library" contentcuration/locale/es_ES/LC_MESSAGES/django.po returns 0. The reused boilerplate (Hello %(name)s,, Thanks for using Kolibri Studio!, The Learning Equality Team) is in the catalogs.

Since this PR now deliberately activates a non-English locale, a Spanish-language channel author gets Spanish greeting and sign-off wrapped around an English body — more jarring than an all-English mail. i18n-upload.yml is workflow_dispatch-only so nothing structurally blocks a Crowdin round-trip on hotfixes. Is one planned before this ships? If not, consider limiting the override to locales that have these msgids.

else:
subject_text = _("Your Community Library submission needs changes")

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,
"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
31 changes: 31 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,31 @@
<!DOCTYPE html>
{% load i18n %}
{% get_current_language as LANGUAGE_CODE %}
{% get_current_language_bidi as LANGUAGE_BIDI %}
<html lang="{{ LANGUAGE_CODE }}" dir="{% if LANGUAGE_BIDI %}rtl{% else %}ltr{% endif %}">

@rtibblesbot rtibblesbot Aug 4, 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.

praise: Setting lang/dir from get_current_language/get_current_language_bidi matters now that the locale is variable, and registration/channel_published_email.html doesn't do it. Dropping that template's blanket {% autoescape off %} is right too — feedback_notes is free-form admin input.

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

<p><a href="{{ channel_url }}" target="_blank">{{ channel.name }}</a> ({{ channel_url }})</p>

{% if approved %}
<p>{% translate "Your submission has been approved and will be added to the Community Library soon." %}</p>
{% else %}
<p>{% translate "Your submission needs changes. Please review the notes below and resubmit after all feedback has been addressed." %}</p>
{% endif %}

{% if feedback_notes %}
<p>{% translate "Notes from the reviewer" %}: {{ feedback_notes }}</p>

@rtibblesbot rtibblesbot Aug 4, 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: Two issues on this line.

The : sits outside the msgid. Colon spacing is locale-dependent (French uses a narrow no-break space before it) and translators can't reorder around it — move it inside: {% translate "Notes from the reviewer:" %}.

feedback_notes is a TextField and is required for rejections, so any newlines the reviewer typed collapse into one run-on line in HTML. {{ feedback_notes|linebreaks }} preserves them. The notes are the whole point of the "needs changes" email.

@rtibbles rtibbles Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're merging this to hotfixes to start with, we can come back to this during fuller string review.

@rtibblesbot rtibblesbot Aug 5, 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.

Understood — deferring to the fuller string review. Thanks.

{% endif %}

<p>
{% translate "Thanks for using Kolibri Studio!" %}
<br>
{% translate "The Learning Equality Team" %}
</p>
</body>
</html>
68 changes: 68 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 All @@ -16,6 +17,7 @@
from contentcuration.tests import testdata
from contentcuration.tests.base import StudioAPITestCase
from contentcuration.tests.helpers import reverse_with_query
from contentcuration.utils.urls import canonical_url
from contentcuration.viewsets.sync.constants import ADDED_TO_COMMUNITY_LIBRARY


Expand Down Expand Up @@ -731,6 +733,58 @@ 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 •
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.

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("approved", sent_email.body.lower())
self.assertIn(self.submission.channel.name, sent_email.body)
self.assertIn(
canonical_url(
reverse("channel", kwargs={"channel_id": self.submission.channel.pk})
),
sent_email.body,
)

@mock.patch(
"contentcuration.viewsets.community_library_submission.apply_channel_changes_task"
)
@mock.patch(
"contentcuration.models.CommunityLibrarySubmission.send_resolution_email",
side_effect=Exception("SMTP is down"),
)
def test_resolve_submission__accept_correct_when_email_fails(
self, send_email_mock, apply_task_mock
):
"""A failure to notify the author shouldn't undo or fail the resolution."""
self.client.force_authenticate(user=self.admin_user)
response = self.client.post(
reverse(
"admin-community-library-submission-resolve",
args=[self.submission.id],
),
self.resolve_approve_metadata,
format="json",
)
self.assertEqual(response.status_code, 200, response.content)

resolved_submission = CommunityLibrarySubmission.objects.get(
id=self.submission.id
)
self.assertEqual(
resolved_submission.status,
community_library_submission_constants.STATUS_APPROVED,
)
Change.objects.get(
channel=self.submission.channel,
change_type=ADDED_TO_COMMUNITY_LIBRARY,
)
apply_task_mock.fetch_or_enqueue.assert_called_once_with(
self.admin_user,
channel_id=self.submission.channel.id,
)
self.assertEqual(len(mail.outbox), 0)

@rtibblesbot rtibblesbot Aug 4, 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: len(mail.outbox) == 0 is also true if send_resolution_email() were never called. send_email_mock.assert_called_once() pins the exception path rather than relying on the sibling test to catch a removed call.


@mock.patch(
"contentcuration.viewsets.community_library_submission.apply_channel_changes_task"
)
Expand Down Expand Up @@ -770,6 +824,20 @@ 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(
canonical_url(
reverse("channel", kwargs={"channel_id": self.submission.channel.pk})
),
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
14 changes: 14 additions & 0 deletions contentcuration/contentcuration/utils/i18n.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,20 @@ def _get_language_info():
LANGUAGE_INFO = _get_language_info()


def closest_supported_locale(lang_code):

@rtibblesbot rtibblesbot Aug 4, 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 coverage for this helper or the non-en path. testdata.channel() hardcodes language_id="en" (tests/testdata.py:246), so locale_code is always "en" and the override is a no-op across the suite. The region-stripping match and the None/unsupported fallback to settings.LANGUAGE_CODE are exactly the logic added this round.

A direct test ("es" → "es-es", "sw" → None, "" → None) plus one resolve test on a Spanish-language channel asserting a translated greeting would cover it.

@rtibbles rtibbles Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Relying on the caller to pass in the lang_code here is a little fragile, but as there is only one caller this is fine.

"""
Given a content language's primary code (e.g. "es", "fr"), return the
Studio UI locale in SUPPORTED_LANGUAGES that matches it, ignoring region,
or None if Studio has no UI translation for that language.
"""
if not lang_code:
return None
for supported in SUPPORTED_LANGUAGES:
if supported.split("-")[0] == lang_code:
return supported
return None


def language_globals():
language_code = get_language()
lang_dir = "rtl" if get_language_bidi() else "ltr"
Expand Down
13 changes: 13 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
@@ -1,3 +1,5 @@
import logging

from django.db.models import OuterRef
from django.db.models import Subquery
from django_filters import BaseInFilter
Expand Down Expand Up @@ -36,6 +38,8 @@
)
from contentcuration.viewsets.user import IsAdminUser

logger = logging.getLogger(__name__)


class ChoiceInFilter(BaseInFilter, ChoiceFilter):
"""
Expand Down Expand Up @@ -358,4 +362,13 @@ def resolve(self, request, pk=None):
published_version.id
)

try:
submission.send_resolution_email()
except Exception:
# The resolution itself has already been committed; a failure to
# notify the author shouldn't turn that into a 500 response.
logger.exception(
"Failed to send resolution email for submission %s", submission.pk
)

return Response(self.serialize_object())
Loading