From 152b678e62c038507ece293c548500059c8f532c Mon Sep 17 00:00:00 2001 From: Eemaan Amir <57627710+eemaanamir@users.noreply.github.com> Date: Mon, 12 Aug 2024 12:04:20 +0500 Subject: [PATCH] feat: save author pronoun separately for notification to prevent info loss (#35234) * feat: save author pronoun separately for notification to prevent info loss * fix: missing pronoun in comment on followed post * test: updated tests for new comment notifications --- .../rest_api/discussions_notifications.py | 18 ++++++++-- .../discussion/rest_api/tests/test_tasks.py | 10 ++++-- .../notifications/base_notification.py | 9 +++-- .../notifications/notification_content.py | 35 +++++++++++++++++++ 4 files changed, 65 insertions(+), 7 deletions(-) create mode 100644 openedx/core/djangoapps/notifications/notification_content.py diff --git a/lms/djangoapps/discussion/rest_api/discussions_notifications.py b/lms/djangoapps/discussion/rest_api/discussions_notifications.py index f72271f3a6..7249d086cc 100644 --- a/lms/djangoapps/discussion/rest_api/discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/discussions_notifications.py @@ -118,9 +118,10 @@ class DiscussionNotificationSender: self.parent_response and self.creator.id != int(self.thread.user_id) ): + author_name = f"{self.parent_response.username}'s" # use your if author of response is same as author of post. # use 'their' if comment author is also response author. - author_name = ( + author_pronoun = ( # Translators: Replier commented on "your" response to your post _("your") if self._response_and_thread_has_same_creator() @@ -129,10 +130,12 @@ class DiscussionNotificationSender: _("their") if self._response_and_comment_has_same_creator() else f"{self.parent_response.username}'s" + ) ) context = { "author_name": str(author_name), + "author_pronoun": str(author_pronoun), } self._send_notification([self.thread.user_id], "new_comment", extra_context=context) @@ -189,10 +192,21 @@ class DiscussionNotificationSender: if not self.parent_id: self._send_notification(users, "response_on_followed_post") else: + author_name = f"{self.parent_response.username}'s" + # use 'their' if comment author is also response author. + author_pronoun = ( + # Translators: Replier commented on "their" response in a post you're following + _("their") + if self._response_and_comment_has_same_creator() + else f"{self.parent_response.username}'s" + ) self._send_notification( users, "comment_on_followed_post", - extra_context={"author_name": self.parent_response.username} + extra_context={ + "author_name": str(author_name), + "author_pronoun": str(author_pronoun), + } ) def _create_cohort_course_audience(self): diff --git a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py index f3f2a68ae6..28cfe3395c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py @@ -338,6 +338,7 @@ class TestSendResponseNotifications(DiscussionAPIViewTestMixin, ModuleStoreTestC 'replier_name': self.user_3.username, 'post_title': self.thread.title, 'author_name': 'dummy\'s', + 'author_pronoun': 'dummy\'s', 'course_name': self.course.display_name, 'sender_id': self.user_3.id } @@ -399,7 +400,8 @@ class TestSendResponseNotifications(DiscussionAPIViewTestMixin, ModuleStoreTestC expected_context = { 'replier_name': self.user_3.username, 'post_title': self.thread.title, - 'author_name': 'your', + 'author_name': 'dummy\'s', + 'author_pronoun': 'your', 'course_name': self.course.display_name, 'sender_id': self.user_3.id, } @@ -441,7 +443,8 @@ class TestSendResponseNotifications(DiscussionAPIViewTestMixin, ModuleStoreTestC 'sender_id': self.user_2.id, } if parent_id: - expected_context['author_name'] = 'dummy' + expected_context['author_name'] = 'dummy\'s' + expected_context['author_pronoun'] = 'dummy\'s' self.assertDictEqual(args.context, expected_context) self.assertEqual( args.content_url, @@ -531,7 +534,8 @@ class TestSendCommentNotification(DiscussionAPIViewTestMixin, ModuleStoreTestCas send_response_notifications(thread.id, str(self.course.id), self.user_2.id, parent_id=response.id) handler.assert_called_once() context = handler.call_args[1]['notification_data'].context - self.assertEqual(context['author_name'], 'their') + self.assertEqual(context['author_name'], 'dummy\'s') + self.assertEqual(context['author_pronoun'], 'their') @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index fb558dfec0..2c696ec60d 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -7,6 +7,7 @@ from .email_notifications import EmailCadence from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole from .utils import find_app_in_normalized_apps, find_pref_in_normalized_prefs from ..django_comment_common.models import FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA +from .notification_content import get_notification_type_content_function FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE = 'filter_audit_expired_users_with_no_role' @@ -109,8 +110,9 @@ COURSE_NOTIFICATION_TYPES = { 'is_core': True, 'info': '', 'non_editable': [], - 'content_template': _('<{p}><{strong}>{replier_name} commented on {author_name}\'s response in ' - 'a post you’re following <{strong}>{post_title}'), + 'content_template': _('<{p}><{strong}>{replier_name} commented on <{strong}>{author_name}' + ' response in a post you’re following <{strong}>{post_title}' + ''), 'content_context': { 'post_title': 'Post title', 'author_name': 'author name', @@ -465,10 +467,13 @@ def get_notification_content(notification_type, context): 'strong': 'strong', 'p': 'p', } + content_function = get_notification_type_content_function(notification_type) if notification_type == 'course_update': notification_type = 'course_updates' notification_type = NotificationTypeManager().notification_types.get(notification_type, None) if notification_type: + if content_function: + return content_function(notification_type, context) notification_type_content_template = notification_type.get('content_template', None) if notification_type_content_template: return notification_type_content_template.format(**context, **html_tags_context) diff --git a/openedx/core/djangoapps/notifications/notification_content.py b/openedx/core/djangoapps/notifications/notification_content.py new file mode 100644 index 0000000000..1dcdc4fb5a --- /dev/null +++ b/openedx/core/djangoapps/notifications/notification_content.py @@ -0,0 +1,35 @@ +""" +Helper functions for overriding notification content for given notification type. +""" + + +def get_notification_type_content_function(notification_type): + """ + Returns the content function for the given notification if it exists. + """ + try: + return globals()[f"get_{notification_type}_notification_content"] + except KeyError: + return None + + +def get_notification_content_with_author_pronoun(notification_type, context): + """ + Helper function to get notification content with author's pronoun. + """ + html_tags_context = { + 'strong': 'strong', + 'p': 'p', + } + notification_type_content_template = notification_type.get('content_template', None) + if 'author_pronoun' in context: + context['author_name'] = context['author_pronoun'] + if notification_type_content_template: + return notification_type_content_template.format(**context, **html_tags_context) + return '' + + +# Returns notification content for the new_comment notification. +get_new_comment_notification_content = get_notification_content_with_author_pronoun +# Returns notification content for the comment_on_followed_post notification. +get_comment_on_followed_post_notification_content = get_notification_content_with_author_pronoun