diff --git a/lms/djangoapps/email_marketing/signals.py b/lms/djangoapps/email_marketing/signals.py index 9adcbc4d22..27b36abe0c 100644 --- a/lms/djangoapps/email_marketing/signals.py +++ b/lms/djangoapps/email_marketing/signals.py @@ -1,20 +1,22 @@ """ This module contains signals needed for email integration """ -import logging -import datetime import crum +import datetime +import logging -from django.dispatch import receiver -from django.core.urlresolvers import reverse from django.conf import settings +from django.core.urlresolvers import reverse +from django.dispatch import receiver from student.models import ENROLL_STATUS_CHANGE from student.cookies import CREATE_LOGON_COOKIE from student.views import REGISTER_USER from email_marketing.models import EmailMarketingConfiguration from util.model_utils import USER_FIELD_CHANGED -from lms.djangoapps.email_marketing.tasks import update_user, update_user_email, update_course_enrollment +from lms.djangoapps.email_marketing.tasks import ( + update_user, update_user_email, update_course_enrollment +) from sailthru.sailthru_client import SailthruClient from sailthru.sailthru_error import SailthruClientError @@ -151,7 +153,9 @@ def email_marketing_register_user(sender, user=None, profile=None, return # perform update asynchronously - update_user.delay(_create_sailthru_user_vars(user, user.profile), user.email, new_user=True) + update_user.delay( + _create_sailthru_user_vars(user, user.profile), user.email, site=_get_current_site(), new_user=True + ) @receiver(USER_FIELD_CHANGED) @@ -187,10 +191,10 @@ def email_marketing_user_field_changed(sender, user=None, table=None, setting=No email_config = EmailMarketingConfiguration.current() if not email_config.enabled: return + # perform update asynchronously, flag if activation - update_user.delay(_create_sailthru_user_vars(user, user.profile), user.email, - new_user=False, - activation=(setting == 'is_active') and new_value is True) + update_user.delay(_create_sailthru_user_vars(user, user.profile), user.email, site=_get_current_site(), + new_user=False, activation=(setting == 'is_active') and new_value is True) elif setting == 'email': # email update is special case @@ -218,3 +222,14 @@ def _create_sailthru_user_vars(user, profile): sailthru_vars['country'] = unicode(profile.country.code) return sailthru_vars + + +def _get_current_site(): + """ + Returns the site for the current request if any. + """ + request = crum.get_current_request() + if not request: + return + + return {'id': request.site.id, 'domain': request.site.domain, 'name': request.site.name} diff --git a/lms/djangoapps/email_marketing/tasks.py b/lms/djangoapps/email_marketing/tasks.py index d83b97aae5..aa4c0e885b 100644 --- a/lms/djangoapps/email_marketing/tasks.py +++ b/lms/djangoapps/email_marketing/tasks.py @@ -6,6 +6,7 @@ import time from celery import task from django.core.cache import cache +from django.conf import settings from email_marketing.models import EmailMarketingConfiguration from student.models import EnrollStatusChange @@ -14,11 +15,12 @@ from sailthru.sailthru_client import SailthruClient from sailthru.sailthru_error import SailthruClientError log = logging.getLogger(__name__) +SAILTHRU_LIST_CACHE_KEY = "email.marketing.cache" # pylint: disable=not-callable @task(bind=True, default_retry_delay=3600, max_retries=24) -def update_user(self, sailthru_vars, email, new_user=False, activation=False): +def update_user(self, sailthru_vars, email, site=None, new_user=False, activation=False): """ Adds/updates Sailthru profile information for a user. Args: @@ -36,8 +38,9 @@ def update_user(self, sailthru_vars, email, new_user=False, activation=False): sailthru_client = SailthruClient(email_config.sailthru_key, email_config.sailthru_secret) try: sailthru_response = sailthru_client.api_post("user", - _create_sailthru_user_parm(sailthru_vars, email, - new_user, email_config)) + _create_email_user_param(sailthru_vars, sailthru_client, + email, new_user, email_config, + site=site)) except SailthruClientError as exc: log.error("Exception attempting to add/update user %s in Sailthru - %s", email, unicode(exc)) @@ -111,7 +114,7 @@ def update_user_email(self, new_email, old_email): max_retries=email_config.sailthru_max_retries) -def _create_sailthru_user_parm(sailthru_vars, email, new_user, email_config): +def _create_email_user_param(sailthru_vars, sailthru_client, email, new_user, email_config, site=None): """ Create sailthru user create/update parms """ @@ -119,8 +122,11 @@ def _create_sailthru_user_parm(sailthru_vars, email, new_user, email_config): sailthru_user['vars'] = dict(sailthru_vars, last_changed_time=int(time.time())) # if new user add to list - if new_user and email_config.sailthru_new_user_list: - sailthru_user['lists'] = {email_config.sailthru_new_user_list: 1} + if new_user: + list_name = _get_or_create_user_list_for_site( + sailthru_client, site=site, default_list_name=email_config.sailthru_new_user_list + ) + sailthru_user['lists'] = {list_name: 1} if list_name else {email_config.sailthru_new_user_list: 1} return sailthru_user @@ -294,6 +300,99 @@ def _get_course_content(course_url, sailthru_client, email_config): return response +def _get_or_create_user_list_for_site(sailthru_client, site=None, default_list_name=None): + """ + Get the user list name from cache if exists else create one and return the name, + callers of this function should perform the enabled check of email config. + :param: sailthru_client + :param: site + :param: default_list_name + :return: list name if exists or created else return None + """ + if site and site.get('id') != settings.SITE_ID: + list_name = site.get('domain', '').replace(".", "_") + "_user_list" + else: + list_name = default_list_name + + sailthru_list = _get_or_create_user_list(sailthru_client, list_name) + return list_name if sailthru_list else default_list_name + + +def _get_or_create_user_list(sailthru_client, list_name): + """ + Get list from sailthru and return if list_name exists else create a new one + and return list data for all lists. + :param sailthru_client + :param list_name + :return sailthru list + """ + sailthru_list_cache = cache.get(SAILTHRU_LIST_CACHE_KEY) + is_cache_updated = False + if not sailthru_list_cache: + sailthru_list_cache = _get_list_from_email_marketing_provider(sailthru_client) + is_cache_updated = True + + sailthru_list = sailthru_list_cache.get(list_name) + + if not sailthru_list: + is_created = _create_user_list(sailthru_client, list_name) + if is_created: + sailthru_list_cache = _get_list_from_email_marketing_provider(sailthru_client) + is_cache_updated = True + sailthru_list = sailthru_list_cache.get(list_name) + + if is_cache_updated: + cache.set(SAILTHRU_LIST_CACHE_KEY, sailthru_list_cache) + + return sailthru_list + + +def _get_list_from_email_marketing_provider(sailthru_client): + """ + Get sailthru list + :param sailthru_client + :return dict of sailthru lists mapped by list name + """ + try: + sailthru_get_response = sailthru_client.api_get("list", {}) + except SailthruClientError as exc: + log.error("Exception attempting to get list from Sailthru - %s", unicode(exc)) + return {} + + if not sailthru_get_response.is_ok(): + error = sailthru_get_response.get_error() + log.info("Error attempting to read list record from Sailthru: %s", error.get_message()) + return {} + + list_map = dict() + for user_list in sailthru_get_response.json['lists']: + list_map[user_list.get('name')] = user_list + + return list_map + + +def _create_user_list(sailthru_client, list_name): + """ + Create list in Sailthru + :param sailthru_client + :param list_name + :return boolean + """ + list_params = {'list': list_name, 'primary': 0, 'public_name': list_name} + try: + sailthru_response = sailthru_client.api_post("list", list_params) + except SailthruClientError as exc: + log.error("Exception attempting to list record for key %s in Sailthru - %s", list_name, unicode(exc)) + return False + + if not sailthru_response.is_ok(): + error = sailthru_response.get_error() + log.error("Error attempting to create list in Sailthru: %s", error.get_message()) + return False + + return True + + def _update_unenrolled_list(sailthru_client, email, course_url, unenroll): """ Maintain a list of courses the user has unenrolled from in the Sailthru user record diff --git a/lms/djangoapps/email_marketing/tests/test_signals.py b/lms/djangoapps/email_marketing/tests/test_signals.py index 646424371f..f953edfa60 100644 --- a/lms/djangoapps/email_marketing/tests/test_signals.py +++ b/lms/djangoapps/email_marketing/tests/test_signals.py @@ -1,9 +1,10 @@ """Tests of email marketing signal handlers.""" -import logging import ddt +import logging from django.test import TestCase from django.contrib.auth.models import AnonymousUser +from django.contrib.sites.models import Site from mock import patch, ANY from util.json_request import JsonResponse @@ -11,8 +12,11 @@ from email_marketing.signals import handle_enroll_status_change, \ email_marketing_register_user, \ email_marketing_user_field_changed, \ add_email_marketing_cookies -from email_marketing.tasks import update_user, update_user_email, update_course_enrollment, \ - _get_course_content, _update_unenrolled_list +from email_marketing.tasks import ( + update_user, update_user_email, update_course_enrollment, + _get_course_content, _update_unenrolled_list, _get_or_create_user_list, + _get_list_from_email_marketing_provider, _create_user_list) + from email_marketing.models import EmailMarketingConfiguration from django.test.client import RequestFactory from student.tests.factories import UserFactory, UserProfileFactory @@ -32,7 +36,7 @@ def update_email_marketing_config(enabled=True, key='badkey', secret='badsecret' """ Enable / Disable Sailthru integration """ - EmailMarketingConfiguration.objects.create( + return EmailMarketingConfiguration.objects.create( enabled=enabled, sailthru_key=key, sailthru_secret=secret, @@ -67,6 +71,10 @@ class EmailMarketingTests(TestCase): self.course_id_string = 'edX/toy/2012_Fall' self.course_id = CourseKey.from_string(self.course_id_string) self.course_url = 'http://testserver/courses/edX/toy/2012_Fall/info' + + self.site = Site.objects.get_current() + self.site_domain = self.site.domain + self.request.site = self.site super(EmailMarketingTests, self).setUp() @patch('email_marketing.signals.crum.get_current_request') @@ -114,15 +122,20 @@ class EmailMarketingTests(TestCase): @patch('email_marketing.tasks.log.error') @patch('email_marketing.tasks.SailthruClient.api_post') - def test_add_user(self, mock_sailthru, mock_log_error): + @patch('email_marketing.tasks.SailthruClient.api_get') + def test_add_user(self, mock_sailthru_get, mock_sailthru_post, mock_log_error): """ test async method in tasks that actually updates Sailthru """ - mock_sailthru.return_value = SailthruResponse(JsonResponse({'ok': True})) - update_user.delay({'gender': 'm', 'username': 'test', 'activated': 1}, TEST_EMAIL, new_user=True) + site_dict = {'id': self.site.id, 'domain': self.site.domain, 'name': self.site.name} + mock_sailthru_post.return_value = SailthruResponse(JsonResponse({'ok': True})) + mock_sailthru_get.return_value = SailthruResponse(JsonResponse({'lists': [{'name': 'new list'}], 'ok': True})) + update_user.delay( + {'gender': 'm', 'username': 'test', 'activated': 1}, TEST_EMAIL, site_dict, new_user=True + ) self.assertFalse(mock_log_error.called) - self.assertEquals(mock_sailthru.call_args[0][0], "user") - userparms = mock_sailthru.call_args[0][1] + self.assertEquals(mock_sailthru_post.call_args[0][0], "user") + userparms = mock_sailthru_post.call_args[0][1] self.assertEquals(userparms['key'], "email") self.assertEquals(userparms['id'], TEST_EMAIL) self.assertEquals(userparms['vars']['gender'], "m") @@ -131,15 +144,36 @@ class EmailMarketingTests(TestCase): self.assertEquals(userparms['lists']['new list'], 1) @patch('email_marketing.tasks.SailthruClient.api_post') - def test_user_activation(self, mock_sailthru): + @patch('email_marketing.tasks.SailthruClient.api_get') + def test_add_user_list_existing_domain(self, mock_sailthru_get, mock_sailthru_post): + """ + test non existing domain name updates Sailthru user lists with default list + """ + existing_site = Site.objects.create(domain='testing.com', name='testing.com') + site_dict = {'id': existing_site.id, 'domain': existing_site.domain, 'name': existing_site.name} + mock_sailthru_post.return_value = SailthruResponse(JsonResponse({'ok': True})) + mock_sailthru_get.return_value = SailthruResponse( + JsonResponse({'lists': [{'name': 'new list'}, {'name': 'testing_com_user_list'}], 'ok': True}) + ) + update_user.delay( + {'gender': 'm', 'username': 'test', 'activated': 1}, TEST_EMAIL, site=site_dict, new_user=True + ) + self.assertEquals(mock_sailthru_post.call_args[0][0], "user") + userparms = mock_sailthru_post.call_args[0][1] + self.assertEquals(userparms['lists']['testing_com_user_list'], 1) + + @patch('email_marketing.tasks.SailthruClient.api_post') + @patch('email_marketing.tasks.SailthruClient.api_get') + def test_user_activation(self, mock_sailthru_get, mock_sailthru_post): """ test send of activation template """ - mock_sailthru.return_value = SailthruResponse(JsonResponse({'ok': True})) + mock_sailthru_post.return_value = SailthruResponse(JsonResponse({'ok': True})) + mock_sailthru_get.return_value = SailthruResponse(JsonResponse({'lists': [{'name': 'new list'}], 'ok': True})) update_user.delay({}, self.user.email, new_user=True, activation=True) # look for call args for 2nd call - self.assertEquals(mock_sailthru.call_args[0][0], "send") - userparms = mock_sailthru.call_args[0][1] + self.assertEquals(mock_sailthru_post.call_args[0][0], "send") + userparms = mock_sailthru_post.call_args[0][1] self.assertEquals(userparms['email'], TEST_EMAIL) self.assertEquals(userparms['template'], "Activation") @@ -156,7 +190,7 @@ class EmailMarketingTests(TestCase): # force Sailthru API exception mock_log_error.reset_mock() mock_sailthru.side_effect = SailthruClientError - update_user.delay({}, self.user.email) + update_user.delay({}, self.user.email, self.site_domain) self.assertTrue(mock_log_error.called) # force Sailthru API exception on 2nd call @@ -459,9 +493,100 @@ class EmailMarketingTests(TestCase): self.assertFalse(_update_unenrolled_list(mock_sailthru_client, TEST_EMAIL, self.course_url, False)) + @patch('email_marketing.tasks.SailthruClient') + def test_get_or_create_sailthru_list(self, mock_sailthru_client): + """ + Test the task the create sailthru lists. + """ + mock_sailthru_client.api_get.return_value = SailthruResponse(JsonResponse({'lists': []})) + _get_or_create_user_list(mock_sailthru_client, 'test1_user_list') + mock_sailthru_client.api_get.assert_called_with("list", {}) + mock_sailthru_client.api_post.assert_called_with( + "list", {'list': 'test1_user_list', 'primary': 0, 'public_name': 'test1_user_list'} + ) + + # test existing user list + mock_sailthru_client.api_get.return_value = \ + SailthruResponse(JsonResponse({'lists': [{'name': 'test1_user_list'}]})) + _get_or_create_user_list(mock_sailthru_client, 'test2_user_list') + mock_sailthru_client.api_get.assert_called_with("list", {}) + mock_sailthru_client.api_post.assert_called_with( + "list", {'list': 'test2_user_list', 'primary': 0, 'public_name': 'test2_user_list'} + ) + + # test get error from Sailthru + mock_sailthru_client.api_get.return_value = \ + SailthruResponse(JsonResponse({'error': 43, 'errormsg': 'Got an error'})) + self.assertEqual(_get_or_create_user_list( + mock_sailthru_client, 'test1_user_list'), None + ) + + # test post error from Sailthru + mock_sailthru_client.api_post.return_value = \ + SailthruResponse(JsonResponse({'error': 43, 'errormsg': 'Got an error'})) + mock_sailthru_client.api_get.return_value = SailthruResponse(JsonResponse({'lists': []})) + self.assertEqual(_get_or_create_user_list(mock_sailthru_client, 'test2_user_list'), None) + + @patch('email_marketing.tasks.SailthruClient') + def test_get_sailthru_list_map_no_list(self, mock_sailthru_client): + """Test when no list returned from sailthru""" + mock_sailthru_client.api_get.return_value = SailthruResponse(JsonResponse({'lists': []})) + self.assertEqual(_get_list_from_email_marketing_provider(mock_sailthru_client), {}) + mock_sailthru_client.api_get.assert_called_with("list", {}) + + @patch('email_marketing.tasks.SailthruClient') + def test_get_sailthru_list_map_error(self, mock_sailthru_client): + """Test when error occurred while fetching data from sailthru""" + mock_sailthru_client.api_get.return_value = SailthruResponse( + JsonResponse({'error': 43, 'errormsg': 'Got an error'}) + ) + self.assertEqual(_get_list_from_email_marketing_provider(mock_sailthru_client), {}) + + @patch('email_marketing.tasks.SailthruClient') + def test_get_sailthru_list_map_exception(self, mock_sailthru_client): + """Test when exception raised while fetching data from sailthru""" + mock_sailthru_client.api_get.side_effect = SailthruClientError + self.assertEqual(_get_list_from_email_marketing_provider(mock_sailthru_client), {}) + + @patch('email_marketing.tasks.SailthruClient') + def test_get_sailthru_list(self, mock_sailthru_client): + """Test fetch list data from sailthru""" + mock_sailthru_client.api_get.return_value = \ + SailthruResponse(JsonResponse({'lists': [{'name': 'test1_user_list'}]})) + self.assertEqual( + _get_list_from_email_marketing_provider(mock_sailthru_client), + {'test1_user_list': {'name': 'test1_user_list'}} + ) + mock_sailthru_client.api_get.assert_called_with("list", {}) + + @patch('email_marketing.tasks.SailthruClient') + def test_create_sailthru_list(self, mock_sailthru_client): + """Test create list in sailthru""" + mock_sailthru_client.api_post.return_value = SailthruResponse(JsonResponse({'ok': True})) + self.assertEqual(_create_user_list(mock_sailthru_client, 'test_list_name'), True) + self.assertEquals(mock_sailthru_client.api_post.call_args[0][0], "list") + listparms = mock_sailthru_client.api_post.call_args[0][1] + self.assertEqual(listparms['list'], 'test_list_name') + self.assertEqual(listparms['primary'], 0) + self.assertEqual(listparms['public_name'], 'test_list_name') + + @patch('email_marketing.tasks.SailthruClient') + def test_create_sailthru_list_error(self, mock_sailthru_client): + """Test error occurrence while creating sailthru list""" + mock_sailthru_client.api_post.return_value = SailthruResponse( + JsonResponse({'error': 43, 'errormsg': 'Got an error'}) + ) + self.assertEqual(_create_user_list(mock_sailthru_client, 'test_list_name'), False) + + @patch('email_marketing.tasks.SailthruClient') + def test_create_sailthru_list_exception(self, mock_sailthru_client): + """Test exception raised while creating sailthru list""" + mock_sailthru_client.api_post.side_effect = SailthruClientError + self.assertEqual(_create_user_list(mock_sailthru_client, 'test_list_name'), False) + @patch('email_marketing.tasks.log.error') @patch('email_marketing.tasks.SailthruClient.api_post') - def test_error_logging1(self, mock_sailthru, mock_log_error): + def test_error_logging(self, mock_sailthru, mock_log_error): """ Ensure that error returned from Sailthru api is logged """ @@ -473,23 +598,35 @@ class EmailMarketingTests(TestCase): update_user_email.delay(self.user.username, "newemail2@test.com") self.assertTrue(mock_log_error.called) + @patch('email_marketing.signals.crum.get_current_request') @patch('lms.djangoapps.email_marketing.tasks.update_user.delay') - def test_register_user(self, mock_update_user): + def test_register_user(self, mock_update_user, mock_get_current_request): + """ + make sure register user call invokes update_user + """ + mock_get_current_request.return_value = self.request + email_marketing_register_user(None, user=self.user, profile=self.profile) + self.assertTrue(mock_update_user.called) + + @patch('lms.djangoapps.email_marketing.tasks.update_user.delay') + def test_register_user_no_request(self, mock_update_user): """ make sure register user call invokes update_user """ email_marketing_register_user(None, user=self.user, profile=self.profile) self.assertTrue(mock_update_user.called) + @patch('email_marketing.signals.crum.get_current_request') @patch('lms.djangoapps.email_marketing.tasks.update_user.delay') @ddt.data(('auth_userprofile', 'gender', 'f', True), ('auth_user', 'is_active', 1, True), ('auth_userprofile', 'shoe_size', 1, False)) @ddt.unpack - def test_modify_field(self, table, setting, value, result, mock_update_user): + def test_modify_field(self, table, setting, value, result, mock_update_user, mock_get_current_request): """ Test that correct fields call update_user """ + mock_get_current_request.return_value = self.request email_marketing_user_field_changed(None, self.user, table=table, setting=setting, new_value=value) self.assertEqual(mock_update_user.called, result)