From 541065c54226509c3731748acc8e57ec352cc2c7 Mon Sep 17 00:00:00 2001 From: Waheed Ahmad Date: Wed, 15 Jun 2022 19:39:23 +0500 Subject: [PATCH] fix: [VAN-980] changing the email address sync with Braze (#30590) Currently, changing the email address in LMS does not reflect in Braze and the transaction emails sent through Braze are delivering to user's old/previous email address. Added a signal/receiver to sync the new email address upon confirm email change. --- common/djangoapps/student/signals/__init__.py | 3 +- .../djangoapps/student/signals/receivers.py | 22 +++++++++++++- common/djangoapps/student/signals/signals.py | 2 ++ common/djangoapps/student/tests/test_email.py | 4 ++- .../student/tests/test_receivers.py | 29 ++++++++++++------- common/djangoapps/student/views/management.py | 3 ++ .../save_for_later/api/v1/tests/test_views.py | 12 ++++++-- lms/djangoapps/save_for_later/helper.py | 12 ++++---- .../tests/test_send_course_reminder_emails.py | 7 ++++- .../test_send_program_reminder_emails.py | 7 ++++- lms/djangoapps/utils.py | 18 ++++++++++++ 11 files changed, 95 insertions(+), 24 deletions(-) diff --git a/common/djangoapps/student/signals/__init__.py b/common/djangoapps/student/signals/__init__.py index 0ae2390011..9d134198f0 100644 --- a/common/djangoapps/student/signals/__init__.py +++ b/common/djangoapps/student/signals/__init__.py @@ -4,5 +4,6 @@ from common.djangoapps.student.signals.signals import ( ENROLL_STATUS_CHANGE, ENROLLMENT_TRACK_UPDATED, REFUND_ORDER, - UNENROLL_DONE + UNENROLL_DONE, + USER_EMAIL_CHANGED ) diff --git a/common/djangoapps/student/signals/receivers.py b/common/djangoapps/student/signals/receivers.py index c05813dc2d..569f4a21dc 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -3,7 +3,8 @@ Signal receivers for the "student" application. """ # pylint: disable=unused-argument - +import logging +from asyncio.log import logger from django.conf import settings from django.contrib.auth import get_user_model from django.db import IntegrityError @@ -11,6 +12,7 @@ from django.db.models.signals import post_save, pre_save from django.dispatch import receiver from lms.djangoapps.courseware.toggles import courseware_mfe_progress_milestones_are_active +from lms.djangoapps.utils import get_braze_client from common.djangoapps.student.helpers import EMAIL_EXISTS_MSG_FMT, USERNAME_EXISTS_MSG_FMT, AccountValidationError from common.djangoapps.student.models import ( CourseEnrollment, @@ -20,8 +22,11 @@ from common.djangoapps.student.models import ( is_username_retired ) from common.djangoapps.student.models_api import confirm_name_change +from common.djangoapps.student.signals import USER_EMAIL_CHANGED from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed +logger = logging.getLogger(__name__) + @receiver(pre_save, sender=get_user_model()) def on_user_updated(sender, instance, **kwargs): @@ -97,3 +102,18 @@ if is_name_affirmation_installed(): # pylint: disable=import-error from edx_name_affirmation.signals import VERIFIED_NAME_APPROVED VERIFIED_NAME_APPROVED.connect(listen_for_verified_name_approved) + + +@receiver(USER_EMAIL_CHANGED) +def _listen_for_user_email_changed(sender, user, **kwargs): + """ If user has changed their email, update that in email Braze. """ + email = user.email + user_id = user.id + attributes = {'email': email, 'external_id': user_id} + + try: + braze_client = get_braze_client() + if braze_client: + braze_client.track_user(attributes=attributes) + except Exception: # pylint: disable=broad-except + logger.warning(f'Unable to sync new email [{email}] with Braze for user [{user_id}]') diff --git a/common/djangoapps/student/signals/signals.py b/common/djangoapps/student/signals/signals.py index 0682bb5ea9..49745ca48c 100644 --- a/common/djangoapps/student/signals/signals.py +++ b/common/djangoapps/student/signals/signals.py @@ -19,3 +19,5 @@ ENROLL_STATUS_CHANGE = Signal() # providing_args=["course_enrollment"] REFUND_ORDER = Signal() + +USER_EMAIL_CHANGED = Signal() diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index 940d54e77a..22f1197576 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -562,6 +562,7 @@ class EmailChangeConfirmationTests(EmailTestMixin, EmailTemplateTagMixin, CacheI @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") @override_settings(MKTG_URLS={'ROOT': 'https://dummy-root', 'CONTACT': '/help/contact-us'}) + @patch('common.djangoapps.student.signals.signals.USER_EMAIL_CHANGED.send') @ddt.data( ('plain_text', False), ('plain_text', True), @@ -569,9 +570,10 @@ class EmailChangeConfirmationTests(EmailTestMixin, EmailTemplateTagMixin, CacheI ('html', True) ) @ddt.unpack - def test_successful_email_change(self, test_body_type, test_marketing_enabled): + def test_successful_email_change(self, test_body_type, test_marketing_enabled, mock_email_change_signal): with patch.dict(settings.FEATURES, {'ENABLE_MKTG_SITE': test_marketing_enabled}): self.assertChangeEmailSent(test_body_type) + assert mock_email_change_signal.called meta = json.loads(UserProfile.objects.get(user=self.user).meta) assert 'old_emails' in meta diff --git a/common/djangoapps/student/tests/test_receivers.py b/common/djangoapps/student/tests/test_receivers.py index 39e654adeb..fb8d285fdb 100644 --- a/common/djangoapps/student/tests/test_receivers.py +++ b/common/djangoapps/student/tests/test_receivers.py @@ -1,18 +1,15 @@ """ Tests for student signal receivers. """ from unittest import skipUnless +from unittest.mock import patch + +from django.conf import settings from edx_toggles.toggles.testutils import override_waffle_flag + +from common.djangoapps.student.models import CourseEnrollmentCelebration, PendingNameChange, UserProfile +from common.djangoapps.student.signals.signals import USER_EMAIL_CHANGED +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory, UserProfileFactory from lms.djangoapps.courseware.toggles import COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES -from common.djangoapps.student.models import ( - CourseEnrollmentCelebration, - PendingNameChange, - UserProfile -) -from common.djangoapps.student.tests.factories import ( - CourseEnrollmentFactory, - UserFactory, - UserProfileFactory -) from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order @@ -73,3 +70,15 @@ class ReceiversTest(SharedModuleStoreTestCase): assert PendingNameChange.objects.count() == 0 profile = UserProfile.objects.get(user=user) assert profile.name == new_name + + @skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") + @patch('common.djangoapps.student.signals.receivers.get_braze_client') + def test_listen_for_user_email_changed(self, mock_get_braze_client): + """ + Ensure that USER_EMAIL_CHANGED signal triggers correct calls to get_braze_client. + """ + user = UserFactory(email='email@test.com', username='jdoe') + + USER_EMAIL_CHANGED.send(sender=None, user=user) + + assert mock_get_braze_client.called diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 1d918ac57a..5ec791ba27 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -79,6 +79,7 @@ from common.djangoapps.student.models import ( # lint-amnesty, pylint: disable= from common.djangoapps.student.signals import REFUND_ORDER from common.djangoapps.util.db import outer_atomic from common.djangoapps.util.json_request import JsonResponse +from common.djangoapps.student.signals import USER_EMAIL_CHANGED from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger("edx.student") @@ -902,6 +903,8 @@ def confirm_email_change(request, key): return response response = render_to_response("email_change_successful.html", address_context) + + USER_EMAIL_CHANGED.send(sender=None, user=user) return response diff --git a/lms/djangoapps/save_for_later/api/v1/tests/test_views.py b/lms/djangoapps/save_for_later/api/v1/tests/test_views.py index bf93c23dc5..038016cd09 100644 --- a/lms/djangoapps/save_for_later/api/v1/tests/test_views.py +++ b/lms/djangoapps/save_for_later/api/v1/tests/test_views.py @@ -39,7 +39,11 @@ class CourseSaveForLaterApiViewTest(ThirdPartyAuthTestMixin, APITestCase): self.course_key = CourseKey.from_string(self.course_id) CourseOverviewFactory.create(id=self.course_key) - @patch('lms.djangoapps.save_for_later.helper.BrazeClient', MagicMock()) + @override_settings( + EDX_BRAZE_API_KEY='test-key', + EDX_BRAZE_API_SERVER='http://test.url' + ) + @patch('lms.djangoapps.utils.BrazeClient', MagicMock()) def test_save_course_using_email(self): """ Test successfully email sent @@ -116,7 +120,11 @@ class ProgramSaveForLaterApiViewTest(ThirdPartyAuthTestMixin, APITestCase): self.uuid = '587f6abe-bfa4-4125-9fbe-4789bf3f97f1' self.program = ProgramFactory(uuid=self.uuid) - @patch('lms.djangoapps.save_for_later.helper.BrazeClient', MagicMock()) + @override_settings( + EDX_BRAZE_API_KEY='test-key', + EDX_BRAZE_API_SERVER='http://test.url' + ) + @patch('lms.djangoapps.utils.BrazeClient', MagicMock()) @patch('lms.djangoapps.save_for_later.api.v1.views.get_programs') def test_save_program_using_email(self, mock_get_programs): """ diff --git a/lms/djangoapps/save_for_later/helper.py b/lms/djangoapps/save_for_later/helper.py index cb69bb1933..e023f203ee 100644 --- a/lms/djangoapps/save_for_later/helper.py +++ b/lms/djangoapps/save_for_later/helper.py @@ -5,12 +5,11 @@ helper functions import logging from datetime import datetime from django.conf import settings -from braze.client import BrazeClient from eventtracking import tracker from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers - from common.djangoapps.course_modes.models import CourseMode +from lms.djangoapps.utils import get_braze_client log = logging.getLogger(__name__) @@ -100,11 +99,10 @@ def send_email(email, data): Send email through Braze """ event_properties = _get_event_properties(data) - braze_client = BrazeClient( - api_key=settings.EDX_BRAZE_API_KEY, - api_url=settings.EDX_BRAZE_API_SERVER, - app_id='', - ) + braze_client = get_braze_client() + + if not braze_client: + return False try: attributes = None diff --git a/lms/djangoapps/save_for_later/management/commands/tests/test_send_course_reminder_emails.py b/lms/djangoapps/save_for_later/management/commands/tests/test_send_course_reminder_emails.py index 6bdc8a6637..2c93492ae3 100644 --- a/lms/djangoapps/save_for_later/management/commands/tests/test_send_course_reminder_emails.py +++ b/lms/djangoapps/save_for_later/management/commands/tests/test_send_course_reminder_emails.py @@ -4,6 +4,7 @@ from unittest.mock import patch import ddt from django.core.management import call_command +from django.test.utils import override_settings from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -29,8 +30,12 @@ class SavedCourseReminderEmailsTest(SharedModuleStoreTestCase): CourseOverviewFactory.create(id=self.saved_course.course_id) CourseOverviewFactory.create(id=self.saved_course_1.course_id) + @override_settings( + EDX_BRAZE_API_KEY='test-key', + EDX_BRAZE_API_SERVER='http://test.url' + ) def test_send_reminder_emails(self): - with patch('lms.djangoapps.save_for_later.helper.BrazeClient') as mock_task: + with patch('lms.djangoapps.utils.BrazeClient') as mock_task: call_command('send_course_reminder_emails', '--batch-size=1') mock_task.assert_called() diff --git a/lms/djangoapps/save_for_later/management/commands/tests/test_send_program_reminder_emails.py b/lms/djangoapps/save_for_later/management/commands/tests/test_send_program_reminder_emails.py index 2501ccdb9f..b3665a0456 100644 --- a/lms/djangoapps/save_for_later/management/commands/tests/test_send_program_reminder_emails.py +++ b/lms/djangoapps/save_for_later/management/commands/tests/test_send_program_reminder_emails.py @@ -5,6 +5,7 @@ from unittest.mock import patch import ddt from django.core.management import call_command +from django.test.utils import override_settings from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -26,10 +27,14 @@ class SavedProgramReminderEmailsTest(SharedModuleStoreTestCase): self.program = ProgramFactory(uuid=self.uuid) self.saved_program = SavedPogramFactory.create(program_uuid=self.uuid) + @override_settings( + EDX_BRAZE_API_KEY='test-key', + EDX_BRAZE_API_SERVER='http://test.url' + ) @patch('lms.djangoapps.save_for_later.management.commands.send_program_reminder_emails.get_programs') def test_send_reminder_emails(self, mock_get_programs): mock_get_programs.return_value = self.program - with patch('lms.djangoapps.save_for_later.helper.BrazeClient') as mock_task: + with patch('lms.djangoapps.utils.BrazeClient') as mock_task: call_command('send_program_reminder_emails', '--batch-size=1') mock_task.assert_called() diff --git a/lms/djangoapps/utils.py b/lms/djangoapps/utils.py index e034b6ed51..182c382ca2 100644 --- a/lms/djangoapps/utils.py +++ b/lms/djangoapps/utils.py @@ -2,6 +2,9 @@ Helper Methods """ +from braze.client import BrazeClient +from django.conf import settings + def _get_key(key_or_id, key_cls): """ @@ -13,3 +16,18 @@ def _get_key(key_or_id, key_cls): if isinstance(key_or_id, str) else key_or_id ) + + +def get_braze_client(): + """ Returns a Braze client. """ + braze_api_key = settings.EDX_BRAZE_API_KEY + braze_api_url = settings.EDX_BRAZE_API_SERVER + + if not braze_api_key or not braze_api_url: + return None + + return BrazeClient( + api_key=braze_api_key, + api_url=braze_api_url, + app_id='', + )