ECOM-5065 Don't treat invalid email response from Sailthru as retryable error

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
 
 

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

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,