Merge pull request #469 from alphagov/480-overnight-error-adding-emai…

…l-to-mailchimp-list

480 overnight error adding email to mailchimp list

dmutils/__init__.py

@@ -2,4 +2,4 @@
 from .flask_init import init_app, init_manager
 
 
-__version__ = '44.8.1'
+__version__ = '44.8.2'

dmutils/email/dm_mailchimp.py

@@ -100,7 +100,7 @@ def send_campaign(self, campaign_id):
         return False
 
     def subscribe_new_email_to_list(self, list_id, email_address):
-        """ Will subscribe email address to list if they do not already exist in that list else do nothing"""
+        """Will subscribe email address to list if they do not already exist in that list else do nothing"""
         hashed_email = self.get_email_hash(email_address)
         try:
             with log_external_request(service='Mailchimp'):
@@ -113,31 +113,32 @@ def subscribe_new_email_to_list(self, list_id, email_address):
                     }
                 )
         except RequestException as e:
+            # Some errors we don't care about but do want to log. Find and log them here.
             response = get_response_from_request_exception(e)
 
-            # As defined in mailchimp API documentation, this particular error message may arise if a user has requested
-            # mailchimp to never add them to mailchimp lists. In this case, we resort to allowing a failed API call (but
-            # log) as a user of this method would unlikely be able to do anything as we have no control over this
-            # behaviour.
             if "looks fake or invalid, please enter a real email address." in response.get("detail", ""):
+                # As defined in mailchimp API documentation, this particular error message may arise if a user has
+                # requested mailchimp to never add them to mailchimp lists. In this case, we resort to allowing a
+                # failed API call (but log) as a user of this method would unlikely be able to do anything as we have
+                # no control over this behaviour.
                 self.logger.error(
                     f"Expected error: Mailchimp failed to add user ({hashed_email}) to list ({list_id}). "
                     "API error: The email address looks fake or invalid, please enter a real email address.",
-                    extra={
-                        "error": str(e),
-                        "mailchimp_response": response,
-                    }
+                    extra={"error": str(e), "mailchimp_response": response}
                 )
                 return True
+            elif 'is already a list member.' in response.get("detail", ""):
+                # If a user is already a list member we receive a 400 error as documented in the tests for this error
+                self.logger.warning(
+                    f"Expected error: Mailchimp failed to add user ({hashed_email}) to list ({list_id}). "
+                    "API error: This email address is already subscribed.",
+                    extra={"error": str(e), "mailchimp_response": response}
+                )
+                return True
+            # Otherwise this was an unexpected error and should be logged as such
             self.logger.error(
-                "Mailchimp failed to add user ({}) to list ({})".format(
-                    hashed_email,
-                    list_id
-                ),
-                extra={
-                    "error": str(e),
-                    "mailchimp_response": response,
-                }
+                f"Mailchimp failed to add user ({hashed_email}) to list ({list_id})",
+                extra={"error": str(e), "mailchimp_response": response}
             )
             return False
 

tests/email/test_dm_mailchimp.py

@@ -132,7 +132,7 @@ def test_log_error_message_if_error_subscribing_email_to_list(self, get_email_ha
             assert log_catcher.records[1].levelname == 'ERROR'
 
     @mock.patch("dmutils.email.dm_mailchimp.DMMailChimpClient.get_email_hash", return_value="foo")
-    def test_returns_true_if_expected_error_subscribing_email_to_list(self, get_email_hash):
+    def test_returns_true_if_expected_invalid_email_error_subscribing_email_to_list(self, get_email_hash):
         dm_mailchimp_client = DMMailChimpClient('username', DUMMY_MAILCHIMP_API_KEY, logging.getLogger('mailchimp'))
         with mock.patch.object(
                 dm_mailchimp_client._client.lists.members, 'create_or_update', autospec=True) as create_or_update:
@@ -144,12 +144,42 @@ def test_returns_true_if_expected_error_subscribing_email_to_list(self, get_emai
                 res = dm_mailchimp_client.subscribe_new_email_to_list('list_id', '[email protected]')
 
             assert res is True
-            assert log_catcher.records[1].msg == "Expected error: Mailchimp failed to add user (foo) to list (" \
-                                                 "list_id). API error: The email address looks fake or invalid, " \
-                                                 "please enter a real email address."
+            assert log_catcher.records[1].msg == (
+                "Expected error: Mailchimp failed to add user (foo) to list (list_id). "
+                "API error: The email address looks fake or invalid, please enter a real email address."
+            )
             assert log_catcher.records[1].error == "error sending"
             assert log_catcher.records[1].levelname == 'ERROR'
 
+    @mock.patch("dmutils.email.dm_mailchimp.DMMailChimpClient.get_email_hash", return_value="foo")
+    def test_returns_true_if_expected_already_subscribed_email_error(self, get_email_hash):
+        dm_mailchimp_client = DMMailChimpClient('username', DUMMY_MAILCHIMP_API_KEY, logging.getLogger('mailchimp'))
+
+        with mock.patch.object(
+            dm_mailchimp_client._client.lists.members, 'create_or_update', autospec=True
+        ) as create_or_update:
+
+            response = mock.MagicMock(__bool__=False)
+            expected_error = "[email protected] is already a list member. Use PUT to insert or update list members."
+
+            response.status_code = 400
+            response.message = (
+                "Bad Request for url: https://us5.api.mailchimp.com/3.0/lists/list_id/members/member_id"
+            )
+            response.json.return_value = {"detail": expected_error}
+            create_or_update.side_effect = HTTPError("400 Client Error", response=response)
+
+            with assert_external_service_log_entry(successful_call=False, extra_modules=['mailchimp']) as log_catcher:
+                res = dm_mailchimp_client.subscribe_new_email_to_list('list_id', '[email protected]')
+
+            assert res is True
+            assert log_catcher.records[1].msg == (
+                "Expected error: Mailchimp failed to add user (foo) to list (list_id). "
+                "API error: This email address is already subscribed."
+            )
+            assert log_catcher.records[1].error == "400 Client Error"
+            assert log_catcher.records[1].levelname == 'WARNING'
+
     @mock.patch("dmutils.email.dm_mailchimp.DMMailChimpClient.get_email_hash", return_value="foo")
     def test_handles_responses_with_invalid_json(self, get_email_hash):
         dm_mailchimp_client = DMMailChimpClient('username', DUMMY_MAILCHIMP_API_KEY, logging.getLogger('mailchimp'))
@@ -169,8 +199,7 @@ def test_handles_responses_with_invalid_json(self, get_email_hash):
 
     def test_subscribe_new_emails_to_list(self):
         dm_mailchimp_client = DMMailChimpClient('username', DUMMY_MAILCHIMP_API_KEY, mock.MagicMock())
-        with mock.patch.object(dm_mailchimp_client, 'subscribe_new_email_to_list', autospec=True):
-            dm_mailchimp_client.subscribe_new_email_to_list.return_value = True
+        with mock.patch.object(dm_mailchimp_client, 'subscribe_new_email_to_list', autospec=True, return_value=True):
 
             with assert_external_service_log_entry(count=2):
                 res = dm_mailchimp_client.subscribe_new_emails_to_list(