From 986b75ba394bd767612aa480d632933d4e306295 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Wed, 23 Apr 2025 12:54:42 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20personalize=20recording=20?= =?UTF-8?q?notification=20emails=20by=20user=20preferences?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Customize email notifications for recording availability based on each user's language and timezone settings. Improves user experience through localized communications. Prioritize simple, maintainable implementation over complex code that would form subgroups based on user preferences. Note: Changes individual email sending instead of batch processing, which may impact performance for large groups but is acceptable for typical recording access patterns. --- .../core/recording/event/notification.py | 69 +++++++----- .../recording/event/test_notification.py | 103 ++++++++++++++---- 2 files changed, 124 insertions(+), 48 deletions(-) diff --git a/src/backend/core/recording/event/notification.py b/src/backend/core/recording/event/notification.py index 9c01c270..ca6ff480 100644 --- a/src/backend/core/recording/event/notification.py +++ b/src/backend/core/recording/event/notification.py @@ -44,49 +44,68 @@ class NotificationService: page in the frontend where they can access their specific recording. """ - owner_accesses = models.RecordingAccess.objects.select_related("user").filter( - role=models.RoleChoices.OWNER, - recording_id=recording.id, + owner_accesses = ( + models.RecordingAccess.objects.select_related("user") + .filter( + role=models.RoleChoices.OWNER, + recording_id=recording.id, + ) + .order_by("created_at") ) if not owner_accesses: logger.error("No owner found for recording %s", recording.id) return False - language = get_language() - context = { "brandname": settings.EMAIL_BRAND_NAME, "support_email": settings.EMAIL_SUPPORT_EMAIL, "logo_img": settings.EMAIL_LOGO_IMG, "domain": settings.EMAIL_DOMAIN, "room_name": recording.room.name, - "recording_date": recording.created_at.strftime("%A %d %B %Y"), - "recording_time": recording.created_at.strftime("%H:%M"), "link": f"{settings.SCREEN_RECORDING_BASE_URL}/{recording.id}", } - emails = [access.user.email for access in owner_accesses] + has_failures = False - with override(language): - msg_html = render_to_string("mail/html/screen_recording.html", context) - msg_plain = render_to_string("mail/text/screen_recording.txt", context) - subject = str(_("Your recording is ready")) # Force translation - - try: - send_mail( - subject.capitalize(), - msg_plain, - settings.EMAIL_FROM, - emails, - html_message=msg_html, - fail_silently=False, + # We process emails individually rather than in batch because: + # 1. Each email requires personalization (timezone, language) + # 2. The number of recipients per recording is typically small (not thousands) + for access in owner_accesses: + user = access.user + language = user.language or get_language() + with override(language): + personalized_context = { + "recording_date": recording.created_at.astimezone( + user.timezone + ).strftime("%Y-%m-%d"), + "recording_time": recording.created_at.astimezone( + user.timezone + ).strftime("%H:%M"), + **context, + } + msg_html = render_to_string( + "mail/html/screen_recording.html", personalized_context ) - except smtplib.SMTPException as exception: - logger.error("notification could not be sent: %s", exception) - return False + msg_plain = render_to_string( + "mail/text/screen_recording.txt", personalized_context + ) + subject = str(_("Your recording is ready")) # Force translation - return True + try: + send_mail( + subject.capitalize(), + msg_plain, + settings.EMAIL_FROM, + [user.email], + html_message=msg_html, + fail_silently=False, + ) + except smtplib.SMTPException as exception: + logger.error("notification could not be sent: %s", exception) + has_failures = True + + return not has_failures @staticmethod def _notify_summary_service(recording): diff --git a/src/backend/core/tests/recording/event/test_notification.py b/src/backend/core/tests/recording/event/test_notification.py index 7c56af53..641e081e 100644 --- a/src/backend/core/tests/recording/event/test_notification.py +++ b/src/backend/core/tests/recording/event/test_notification.py @@ -4,6 +4,7 @@ Test event notification. # pylint: disable=E1128,W0621,W0613,W0212 +import datetime import smtplib from unittest import mock @@ -74,6 +75,7 @@ def test_notify_external_services_unknown_mode(caplog): assert f"Unknown recording mode unknown for recording {recording.id}" in caplog.text +# pylint: disable=too-many-locals def test_notify_user_by_email_success(mocked_current_site, settings): """Test successful email notification to recording owners.""" settings.EMAIL_BRAND_NAME = "ACME" @@ -84,15 +86,28 @@ def test_notify_user_by_email_success(mocked_current_site, settings): recording = factories.RecordingFactory(room__name="Conference Room A") - owners = [ - factories.UserRecordingAccessFactory( - recording=recording, role=models.RoleChoices.OWNER - ).user, - factories.UserRecordingAccessFactory( - recording=recording, role=models.RoleChoices.OWNER - ).user, - ] - owner_emails = [owner.email for owner in owners] + # Fix recording test to avoid flaky tests + recording.created_at = datetime.datetime(2023, 5, 15, 14, 30, 0) + + french_user = factories.UserFactory( + email="franc@test.com", language="fr-fr", timezone="Europe/Paris" + ) + dutch_user = factories.UserFactory( + email="berry@test.com", language="nl-nl", timezone="Europe/Amsterdam" + ) + english_user = factories.UserFactory( + email="john@test.com", language="en-us", timezone="America/Phoenix" + ) + + factories.UserRecordingAccessFactory( + recording=recording, role=models.RoleChoices.OWNER, user=french_user + ) + factories.UserRecordingAccessFactory( + recording=recording, role=models.RoleChoices.OWNER, user=dutch_user + ) + factories.UserRecordingAccessFactory( + recording=recording, role=models.RoleChoices.OWNER, user=english_user + ) # Create non-owner users to verify they don't receive emails factories.UserRecordingAccessFactory( @@ -108,30 +123,69 @@ def test_notify_user_by_email_success(mocked_current_site, settings): result = notification_service._notify_user_by_email(recording) assert result is True - mock_send_mail.assert_called_once() + assert mock_send_mail.call_count == 3 - subject, body, sender, recipients = mock_send_mail.call_args[0] + call_args_list = mock_send_mail.call_args_list - assert subject == "Your recording is ready" - - # Verify email contains expected content - required_content = [ + base_content = [ "ACME", # Brand name "support@acme.com", # Support email "https://acme.com/logo", # Logo URL f"https://acme.com/recordings/{recording.id}", # Recording link "Conference Room A", # Room name - recording.created_at.strftime("%A %d %B %Y"), # Formatted date - recording.created_at.strftime("%H:%M"), # Formatted time ] - for content in required_content: - assert content in body + # First call verification + subject1, body1, sender1, recipients1 = call_args_list[0][0] + assert subject1 == "Votre enregistrement est prêt" - assert sender == "notifications@acme.com" + # Verify email contains expected content + personalized_content1 = [ + *base_content, + "2023-05-15", # Formatted date + "16:30", # Formatted time + "Votre enregistrement est prêt !", # Intro + ] + for content in personalized_content1: + assert content in body1 - # Verify all owners received the email (order-independent comparison) - assert sorted(recipients) == sorted(owner_emails) + assert recipients1 == ["franc@test.com"] + assert sender1 == "notifications@acme.com" + + # Second call verification + subject2, body2, sender2, recipients2 = call_args_list[1][0] + assert subject2 == "Je opname is klaar" + + # Verify second email content (if needed) + personalized_content2 = [ + *base_content, + "2023-05-15", # Formatted date + "16:30", # Formatted time + "Je opname is klaar!", # Intro + ] + for content in personalized_content2: + assert content in body2 + + assert recipients2 == ["berry@test.com"] + assert sender2 == "notifications@acme.com" + + # Third call verification + subject3, body3, sender3, recipients3 = call_args_list[2][0] + assert subject3 == "Your recording is ready" + + # Verify second email content (if needed) + personalized_content3 = [ + *base_content, + "Conference Room A", # Room name + "2023-05-15", # Formatted date + "07:30", # Formatted time + "Your recording is ready!", # Intro + ] + for content in personalized_content3: + assert content in body3 + + assert recipients3 == ["john@test.com"] + assert sender3 == "notifications@acme.com" def test_notify_user_by_email_no_owners(mocked_current_site, caplog): @@ -153,6 +207,9 @@ def test_notify_user_by_email_smtp_exception(mocked_current_site, caplog): factories.UserRecordingAccessFactory( recording=recording, role=models.RoleChoices.OWNER ) + factories.UserRecordingAccessFactory( + recording=recording, role=models.RoleChoices.OWNER + ) notification_service = NotificationService() @@ -163,5 +220,5 @@ def test_notify_user_by_email_smtp_exception(mocked_current_site, caplog): result = notification_service._notify_user_by_email(recording) assert result is False - mock_send_mail.assert_called_once() + assert mock_send_mail.call_count == 2 assert "notification could not be sent:" in caplog.text