From 66f3a0803c99cdc14ca809d20cd98a0ee4140783 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Mon, 26 Aug 2024 13:13:47 +0500 Subject: [PATCH] feat: added email content in misc notifications (#35341) --- .../rest_api/discussions_notifications.py | 65 ++++++++++++-- lms/djangoapps/discussion/rest_api/tasks.py | 4 +- .../tests/test_discussions_notifications.py | 84 ++++++++++++++++++- .../discussion/rest_api/tests/test_tasks.py | 67 +++++++++++++-- .../discussion/rest_api/tests/utils.py | 3 +- lms/djangoapps/discussion/signals/handlers.py | 3 +- 6 files changed, 209 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/discussions_notifications.py b/lms/djangoapps/discussion/rest_api/discussions_notifications.py index 21b1e27fcdf8..96e392c35d2a 100644 --- a/lms/djangoapps/discussion/rest_api/discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/discussions_notifications.py @@ -3,7 +3,10 @@ """ import re +from bs4 import BeautifulSoup from django.conf import settings +from django.utils.text import Truncator + from lms.djangoapps.discussion.django_comment_client.permissions import get_team from openedx_events.learning.data import UserNotificationData, CourseNotificationData from openedx_events.learning.signals import USER_NOTIFICATION_REQUESTED, COURSE_NOTIFICATION_REQUESTED @@ -27,13 +30,24 @@ class DiscussionNotificationSender: Class to send notifications to users who are subscribed to the thread. """ - def __init__(self, thread, course, creator, parent_id=None): + def __init__(self, thread, course, creator, parent_id=None, comment_id=None): self.thread = thread self.course = course self.creator = creator self.parent_id = parent_id + self.comment_id = comment_id self.parent_response = None + self.comment = None self._get_parent_response() + self._get_comment() + + def _get_comment(self): + """ + Get comment object + """ + if not self.comment_id: + return + self.comment = Comment(id=self.comment_id).retrieve() def _send_notification(self, user_ids, notification_type, extra_context=None): """ @@ -99,7 +113,10 @@ def send_new_response_notification(self): there is a response to the main thread. """ if not self.parent_id and self.creator.id != int(self.thread.user_id): - self._send_notification([self.thread.user_id], "new_response") + context = { + 'email_content': clean_thread_html_body(self.comment.body), + } + self._send_notification([self.thread.user_id], "new_response", extra_context=context) def _response_and_thread_has_same_creator(self) -> bool: """ @@ -136,6 +153,7 @@ def send_new_comment_notification(self): context = { "author_name": str(author_name), "author_pronoun": str(author_pronoun), + "email_content": clean_thread_html_body(self.comment.body), } self._send_notification([self.thread.user_id], "new_comment", extra_context=context) @@ -149,7 +167,14 @@ def send_new_comment_on_response_notification(self): self.creator.id != int(self.parent_response.user_id) and not self._response_and_thread_has_same_creator() ): - self._send_notification([self.parent_response.user_id], "new_comment_on_response") + context = { + "email_content": clean_thread_html_body(self.comment.body), + } + self._send_notification( + [self.parent_response.user_id], + "new_comment_on_response", + extra_context=context + ) def _check_if_subscriber_is_not_thread_or_content_creator(self, subscriber_id) -> bool: """ @@ -190,7 +215,12 @@ def send_response_on_followed_post_notification(self): # Remove duplicate users from the list of users to send notification users = list(set(users)) if not self.parent_id: - self._send_notification(users, "response_on_followed_post") + self._send_notification( + users, + "response_on_followed_post", + extra_context={ + "email_content": clean_thread_html_body(self.comment.body), + }) else: author_name = f"{self.parent_response.username}'s" # use 'their' if comment author is also response author. @@ -206,6 +236,7 @@ def send_response_on_followed_post_notification(self): extra_context={ "author_name": str(author_name), "author_pronoun": str(author_pronoun), + "email_content": clean_thread_html_body(self.comment.body), } ) @@ -289,7 +320,8 @@ def send_new_thread_created_notification(self): ] context = { 'username': self.creator.username, - 'post_title': self.thread.title + 'post_title': self.thread.title, + "email_content": clean_thread_html_body(self.thread.body), } self._send_course_wide_notification(notification_type, audience_filters, context) @@ -339,3 +371,26 @@ def is_discussion_cohorted(course_key_str): def remove_html_tags(text): clean = re.compile('<.*?>') return re.sub(clean, '', text) + + +def clean_thread_html_body(html_body): + """ + Get post body with tags removed and limited to 500 characters + """ + html_body = BeautifulSoup(Truncator(html_body).chars(500, html=True), 'html.parser') + + tags_to_remove = [ + "a", "link", # Link Tags + "img", "picture", "source", # Image Tags + "video", "track", # Video Tags + "audio", # Audio Tags + "embed", "object", "iframe", # Embedded Content + "script" + ] + + # Remove the specified tags while keeping their content + for tag in tags_to_remove: + for match in html_body.find_all(tag): + match.unwrap() + + return str(html_body) diff --git a/lms/djangoapps/discussion/rest_api/tasks.py b/lms/djangoapps/discussion/rest_api/tasks.py index 45bf41fe905f..a87fafd4ca54 100644 --- a/lms/djangoapps/discussion/rest_api/tasks.py +++ b/lms/djangoapps/discussion/rest_api/tasks.py @@ -33,7 +33,7 @@ def send_thread_created_notification(thread_id, course_key_str, user_id): @shared_task @set_code_owner_attribute -def send_response_notifications(thread_id, course_key_str, user_id, parent_id=None): +def send_response_notifications(thread_id, course_key_str, user_id, comment_id, parent_id=None): """ Send notifications to users who are subscribed to the thread. """ @@ -43,7 +43,7 @@ def send_response_notifications(thread_id, course_key_str, user_id, parent_id=No thread = Thread(id=thread_id).retrieve() user = User.objects.get(id=user_id) course = get_course_with_access(user, 'load', course_key, check_if_enrolled=True) - notification_sender = DiscussionNotificationSender(thread, course, user, parent_id) + notification_sender = DiscussionNotificationSender(thread, course, user, parent_id, comment_id) notification_sender.send_new_comment_notification() notification_sender.send_new_response_notification() notification_sender.send_new_comment_on_response_notification() diff --git a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py index b4d3f3d18a0d..f1a71fd1239e 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py @@ -1,13 +1,14 @@ """ Unit tests for the DiscussionNotificationSender class """ - +import re import unittest from unittest.mock import MagicMock, patch import pytest -from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender +from lms.djangoapps.discussion.rest_api.discussions_notifications import DiscussionNotificationSender, \ + clean_thread_html_body @patch('lms.djangoapps.discussion.rest_api.discussions_notifications.DiscussionNotificationSender' @@ -88,3 +89,82 @@ def test_send_reported_content_notification_for_thread(self, mock_send_notificat self.notification_sender.send_reported_content_notification() self._assert_send_notification_called_with(mock_send_notification, 'thread') + + +class TestCleanThreadHtmlBody(unittest.TestCase): + """ + Tests for the clean_thread_html_body function + """ + + def test_html_tags_removal(self): + """ + Test that the clean_thread_html_body function removes unwanted HTML tags + """ + html_body = """ +

This is a link to a page.

+

Here is an image: image

+

Embedded video:

+

Script test:

+

Some other content that should remain.

+ """ + expected_output = ("

This is a link to a page.

" + "

Here is an image:

" + "

Embedded video:

" + "

Script test: alert('hello');

" + "

Some other content that should remain.

") + + result = clean_thread_html_body(html_body) + + def normalize_html(text): + """ + Normalize the output by removing extra whitespace, newlines, and spaces between tags + """ + text = re.sub(r'\s+', ' ', text).strip() # Replace any sequence of whitespace with a single space + text = re.sub(r'>\s+<', '><', text) # Remove spaces between HTML tags + return text + + normalized_result = normalize_html(result) + normalized_expected_output = normalize_html(expected_output) + + self.assertEqual(normalized_result, normalized_expected_output) + + def test_truncate_html_body(self): + """ + Test that the clean_thread_html_body function truncates the HTML body to 500 characters + """ + html_body = """ +

This is a long text that should be truncated to 500 characters.

+ """ * 20 # Repeat to exceed 500 characters + + result = clean_thread_html_body(html_body) + self.assertGreaterEqual(500, len(result)) + + def test_no_tags_to_remove(self): + """ + Test that the clean_thread_html_body function does not remove any tags if there are no unwanted tags + """ + html_body = "

This paragraph has no tags to remove.

" + expected_output = "

This paragraph has no tags to remove.

" + + result = clean_thread_html_body(html_body) + self.assertEqual(result, expected_output) + + def test_empty_html_body(self): + """ + Test that the clean_thread_html_body function returns an empty string if the input is an empty string + """ + html_body = "" + expected_output = "" + + result = clean_thread_html_body(html_body) + self.assertEqual(result, expected_output) + + def test_only_script_tag(self): + """ + Test that the clean_thread_html_body function removes the script tag and its content + """ + html_body = "" + expected_output = "alert('Hello');" + + result = clean_thread_html_body(html_body) + self.assertEqual(result.strip(), expected_output) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py index 28cfe3395cb6..8efd5cd49cbd 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_tasks.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_tasks.py @@ -273,6 +273,17 @@ def setUp(self): }) self._register_subscriptions_endpoint() + self.comment = ThreadMock(thread_id=4, creator=self.user_2, title='test comment', body='comment body') + self.register_get_comment_response( + { + 'id': self.comment.id, + 'thread_id': self.thread.id, + 'parent_id': None, + 'user_id': self.comment.user_id, + 'body': self.comment.body, + } + ) + def test_basic(self): """ Left empty intentionally. This test case is inherited from DiscussionAPIViewTestMixin @@ -292,7 +303,13 @@ def test_send_notification_to_thread_creator(self): # Post the form or do what it takes to send the signal - send_response_notifications(self.thread.id, str(self.course.id), self.user_2.id, parent_id=None) + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_2.id, + self.comment.id, + parent_id=None + ) self.assertEqual(handler.call_count, 2) args = handler.call_args_list[0][1]['notification_data'] self.assertEqual([int(user_id) for user_id in args.user_ids], [self.user_1.id]) @@ -300,6 +317,7 @@ def test_send_notification_to_thread_creator(self): expected_context = { 'replier_name': self.user_2.username, 'post_title': 'test thread', + 'email_content': self.comment.body, 'course_name': self.course.display_name, 'sender_id': self.user_2.id } @@ -325,7 +343,13 @@ def test_send_notification_to_parent_threads(self): 'user_id': self.thread_2.user_id }) - send_response_notifications(self.thread.id, str(self.course.id), self.user_3.id, parent_id=self.thread_2.id) + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_3.id, + self.comment.id, + parent_id=self.thread_2.id + ) # check if 2 call are made to the handler i.e. one for the response creator and one for the thread creator self.assertEqual(handler.call_count, 2) @@ -337,6 +361,7 @@ def test_send_notification_to_parent_threads(self): expected_context = { 'replier_name': self.user_3.username, 'post_title': self.thread.title, + 'email_content': self.comment.body, 'author_name': 'dummy\'s', 'author_pronoun': 'dummy\'s', 'course_name': self.course.display_name, @@ -355,6 +380,7 @@ def test_send_notification_to_parent_threads(self): expected_context = { 'replier_name': self.user_3.username, 'post_title': self.thread.title, + 'email_content': self.comment.body, 'course_name': self.course.display_name, 'sender_id': self.user_3.id } @@ -372,7 +398,13 @@ def test_no_signal_on_creators_own_thread(self): """ handler = mock.Mock() USER_NOTIFICATION_REQUESTED.connect(handler) - send_response_notifications(self.thread.id, str(self.course.id), self.user_1.id, parent_id=None) + + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_1.id, + self.comment.id, parent_id=None + ) self.assertEqual(handler.call_count, 1) def test_comment_creators_own_response(self): @@ -389,7 +421,13 @@ def test_comment_creators_own_response(self): 'user_id': self.thread_3.user_id }) - send_response_notifications(self.thread.id, str(self.course.id), self.user_3.id, parent_id=self.thread_2.id) + send_response_notifications( + self.thread.id, + str(self.course.id), + self.user_3.id, + parent_id=self.thread_2.id, + comment_id=self.comment.id + ) # check if 1 call is made to the handler i.e. for the thread creator self.assertEqual(handler.call_count, 2) @@ -404,6 +442,7 @@ def test_comment_creators_own_response(self): 'author_pronoun': 'your', 'course_name': self.course.display_name, 'sender_id': self.user_3.id, + 'email_content': self.comment.body } self.assertDictEqual(args_comment.context, expected_context) self.assertEqual( @@ -429,7 +468,13 @@ def test_send_notification_to_followers(self, parent_id, notification_type): USER_NOTIFICATION_REQUESTED.connect(handler) # Post the form or do what it takes to send the signal - notification_sender = DiscussionNotificationSender(self.thread, self.course, self.user_2, parent_id=parent_id) + notification_sender = DiscussionNotificationSender( + self.thread, + self.course, + self.user_2, + parent_id=parent_id, + comment_id=self.comment.id + ) notification_sender.send_response_on_followed_post_notification() self.assertEqual(handler.call_count, 1) args = handler.call_args[1]['notification_data'] @@ -439,6 +484,7 @@ def test_send_notification_to_followers(self, parent_id, notification_type): expected_context = { 'replier_name': self.user_2.username, 'post_title': 'test thread', + 'email_content': self.comment.body, 'course_name': self.course.display_name, 'sender_id': self.user_2.id, } @@ -516,6 +562,7 @@ def test_new_comment_notification(self): thread = ThreadMock(thread_id=1, creator=self.user_1, title='test thread') response = ThreadMock(thread_id=2, creator=self.user_2, title='test response') + comment = ThreadMock(thread_id=3, creator=self.user_2, title='test comment', body='comment body') self.register_get_thread_response({ 'id': thread.id, 'course_id': str(self.course.id), @@ -530,12 +577,20 @@ def test_new_comment_notification(self): 'thread_id': thread.id, 'user_id': response.user_id }) + self.register_get_comment_response({ + 'id': comment.id, + 'parent_id': response.id, + 'user_id': comment.user_id, + 'body': comment.body + }) self.register_get_subscriptions(1, {}) - send_response_notifications(thread.id, str(self.course.id), self.user_2.id, parent_id=response.id) + send_response_notifications(thread.id, str(self.course.id), self.user_2.id, parent_id=response.id, + comment_id=comment.id) handler.assert_called_once() context = handler.call_args[1]['notification_data'].context self.assertEqual(context['author_name'], 'dummy\'s') self.assertEqual(context['author_pronoun'], 'their') + self.assertEqual(context['email_content'], comment.body) @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index 989fd63855d5..27e34705f5df 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -675,12 +675,13 @@ class ThreadMock(object): A mock thread object """ - def __init__(self, thread_id, creator, title, parent_id=None): + def __init__(self, thread_id, creator, title, parent_id=None, body=''): self.id = thread_id self.user_id = str(creator.id) self.username = creator.username self.title = title self.parent_id = parent_id + self.body = body def url_with_id(self, params): return f"http://example.com/{params['id']}" diff --git a/lms/djangoapps/discussion/signals/handlers.py b/lms/djangoapps/discussion/signals/handlers.py index 35288cdbd9be..2aa7d36456c4 100644 --- a/lms/djangoapps/discussion/signals/handlers.py +++ b/lms/djangoapps/discussion/signals/handlers.py @@ -176,8 +176,9 @@ def create_comment_created_notification(*args, **kwargs): comment = kwargs['post'] thread_id = comment.attributes['thread_id'] parent_id = comment.attributes['parent_id'] + comment_id = comment.attributes['id'] course_key_str = comment.attributes['course_id'] - send_response_notifications.apply_async(args=[thread_id, course_key_str, user.id, parent_id]) + send_response_notifications.apply_async(args=[thread_id, course_key_str, user.id, comment_id, parent_id]) @receiver(signals.comment_endorsed)