diff --git a/lms/djangoapps/email_marketing/signals.py b/lms/djangoapps/email_marketing/signals.py index 9b44e202af..9adcbc4d22 100644 --- a/lms/djangoapps/email_marketing/signals.py +++ b/lms/djangoapps/email_marketing/signals.py @@ -125,7 +125,8 @@ def add_email_marketing_cookies(sender, response=None, user=None, log.error("No cookie returned attempting to obtain cookie from Sailthru for %s", user.email) else: error = sailthru_response.get_error() - log.error("Error attempting to obtain cookie from Sailthru: %s", error.get_message()) + # generally invalid email address + log.info("Error attempting to obtain cookie from Sailthru: %s", error.get_message()) return response diff --git a/lms/djangoapps/email_marketing/tasks.py b/lms/djangoapps/email_marketing/tasks.py index aa8f9a5b56..15230f7d4c 100644 --- a/lms/djangoapps/email_marketing/tasks.py +++ b/lms/djangoapps/email_marketing/tasks.py @@ -47,10 +47,11 @@ def update_user(self, sailthru_vars, email, new_user=False, activation=False): if not sailthru_response.is_ok(): error = sailthru_response.get_error() - # put out error and schedule retry log.error("Error attempting to add/update user in Sailthru: %s", error.get_message()) - raise self.retry(countdown=email_config.sailthru_retry_interval, - max_retries=email_config.sailthru_max_retries) + if _retryable_sailthru_error(error): + raise self.retry(countdown=email_config.sailthru_retry_interval, + max_retries=email_config.sailthru_max_retries) + return # if activating user, send welcome email if activation and email_config.sailthru_activation_template: @@ -66,8 +67,10 @@ def update_user(self, sailthru_vars, email, new_user=False, activation=False): if not sailthru_response.is_ok(): error = sailthru_response.get_error() - # probably a disabled template, just put out error message log.error("Error attempting to send welcome email to user in Sailthru: %s", error.get_message()) + if _retryable_sailthru_error(error): + raise self.retry(countdown=email_config.sailthru_retry_interval, + max_retries=email_config.sailthru_max_retries) # pylint: disable=not-callable @@ -103,8 +106,9 @@ def update_user_email(self, new_email, old_email): if not sailthru_response.is_ok(): error = sailthru_response.get_error() log.error("Error attempting to update user email address in Sailthru: %s", error.get_message()) - raise self.retry(countdown=email_config.sailthru_retry_interval, - max_retries=email_config.sailthru_max_retries) + if _retryable_sailthru_error(error): + raise self.retry(countdown=email_config.sailthru_retry_interval, + max_retries=email_config.sailthru_max_retries) def _create_sailthru_user_parm(sailthru_vars, email, new_user, email_config): @@ -254,7 +258,7 @@ def _record_purchase(sailthru_client, email, item, message_id, options): if not sailthru_response.is_ok(): error = sailthru_response.get_error() log.error("Error attempting to record purchase in Sailthru: %s", error.get_message()) - return False + return not _retryable_sailthru_error(error) except SailthruClientError as exc: log.error("Exception attempting to record purchase for %s in Sailthru - %s", email, unicode(exc)) @@ -305,8 +309,8 @@ def _update_unenrolled_list(sailthru_client, email, course_url, unenroll): sailthru_response = sailthru_client.api_get("user", {"id": email, "fields": {"vars": 1}}) if not sailthru_response.is_ok(): error = sailthru_response.get_error() - log.error("Error attempting to read user record from Sailthru: %s", error.get_message()) - return False + log.info("Error attempting to read user record from Sailthru: %s", error.get_message()) + return not _retryable_sailthru_error(error) response_json = sailthru_response.json @@ -333,8 +337,8 @@ def _update_unenrolled_list(sailthru_client, email, course_url, unenroll): if not sailthru_response.is_ok(): error = sailthru_response.get_error() - log.error("Error attempting to update user record in Sailthru: %s", error.get_message()) - return False + log.info("Error attempting to update user record in Sailthru: %s", error.get_message()) + return not _retryable_sailthru_error(error) # everything worked return True @@ -342,3 +346,16 @@ def _update_unenrolled_list(sailthru_client, email, course_url, unenroll): except SailthruClientError as exc: log.error("Exception attempting to update user record for %s in Sailthru - %s", email, unicode(exc)) return False + + +def _retryable_sailthru_error(error): + """ Return True if error should be retried. + + 9: Retryable internal error + 43: Rate limiting response + others: Not retryable + + See: https://getstarted.sailthru.com/new-for-developers-overview/api/api-response-errors/ + """ + code = error.get_error_code() + return code == 9 or code == 43 diff --git a/lms/djangoapps/email_marketing/tests/test_signals.py b/lms/djangoapps/email_marketing/tests/test_signals.py index 9fcfa658f7..646424371f 100644 --- a/lms/djangoapps/email_marketing/tests/test_signals.py +++ b/lms/djangoapps/email_marketing/tests/test_signals.py @@ -172,6 +172,30 @@ class EmailMarketingTests(TestCase): update_user.delay({}, self.user.email, activation=True) self.assertTrue(mock_log_error.called) + @patch('email_marketing.tasks.update_user.retry') + @patch('email_marketing.tasks.log.error') + @patch('email_marketing.tasks.SailthruClient.api_post') + def test_update_user_error_retryable(self, mock_sailthru, mock_log_error, mock_retry): + """ + Ensure that retryable error is retried + """ + mock_sailthru.return_value = SailthruResponse(JsonResponse({'error': 43, 'errormsg': 'Got an error'})) + update_user.delay({}, self.user.email) + self.assertTrue(mock_log_error.called) + self.assertTrue(mock_retry.called) + + @patch('email_marketing.tasks.update_user.retry') + @patch('email_marketing.tasks.log.error') + @patch('email_marketing.tasks.SailthruClient.api_post') + def test_update_user_error_nonretryable(self, mock_sailthru, mock_log_error, mock_retry): + """ + Ensure that non-retryable error is not retried + """ + mock_sailthru.return_value = SailthruResponse(JsonResponse({'error': 1, 'errormsg': 'Got an error'})) + update_user.delay({}, self.user.email) + self.assertTrue(mock_log_error.called) + self.assertFalse(mock_retry.called) + @patch('email_marketing.tasks.log.error') @patch('email_marketing.tasks.SailthruClient.api_post') def test_just_return_tasks(self, mock_sailthru, mock_log_error): @@ -284,11 +308,12 @@ class EmailMarketingTests(TestCase): self.assertEquals(userparms['keys']['email'], TEST_EMAIL) @patch('email_marketing.tasks.log.error') + @patch('email_marketing.tasks.log.info') @patch('email_marketing.tasks.SailthruClient.purchase') @patch('email_marketing.tasks.SailthruClient.api_get') @patch('email_marketing.tasks.SailthruClient.api_post') def test_update_course_enrollment(self, mock_sailthru_api_post, - mock_sailthru_api_get, mock_sailthru_purchase, mock_log_error): + mock_sailthru_api_get, mock_sailthru_purchase, mock_log_info, mock_log_error): """ test async method in task posts enrolls and purchases """ @@ -347,14 +372,14 @@ class EmailMarketingTests(TestCase): # test unsupported event mock_sailthru_purchase.side_effect = SailthruClientError - mock_log_error.reset_mock() + mock_log_info.reset_mock() update_course_enrollment.delay(TEST_EMAIL, self.course_url, EnrollStatusChange.upgrade_start, 'verified', course_id=self.course_id_string, message_id='cookie_bid') - self.assertFalse(mock_log_error.called) + self.assertFalse(mock_log_info.called) # test error updating user mock_sailthru_api_get.return_value = SailthruResponse(JsonResponse({'error': 100, 'errormsg': 'Got an error'})) @@ -364,7 +389,7 @@ class EmailMarketingTests(TestCase): 'verified', course_id=self.course_id_string, message_id='cookie_bid') - self.assertTrue(mock_log_error.called) + self.assertTrue(mock_log_info.called) @patch('email_marketing.tasks.SailthruClient') def test_get_course_content(self, mock_sailthru_client): @@ -417,13 +442,13 @@ class EmailMarketingTests(TestCase): # test get error from Sailthru mock_sailthru_client.api_get.return_value = \ - SailthruResponse(JsonResponse({'error': 100, 'errormsg': 'Got an error'})) + SailthruResponse(JsonResponse({'error': 43, 'errormsg': 'Got an error'})) self.assertFalse(_update_unenrolled_list(mock_sailthru_client, TEST_EMAIL, self.course_url, False)) # test post error from Sailthru mock_sailthru_client.api_post.return_value = \ - SailthruResponse(JsonResponse({'error': 100, 'errormsg': 'Got an error'})) + SailthruResponse(JsonResponse({'error': 43, 'errormsg': 'Got an error'})) mock_sailthru_client.api_get.return_value = \ SailthruResponse(JsonResponse({'vars': {'unenrolled': [self.course_url]}})) self.assertFalse(_update_unenrolled_list(mock_sailthru_client, TEST_EMAIL,