From 77cbe98f0549e3cb42992a684d86e812af998a30 Mon Sep 17 00:00:00 2001 From: benvand Date: Thu, 18 Oct 2018 14:52:55 +0100 Subject: [PATCH 1/2] Allow but log when a user signs up twice to the mailing list --- dmutils/email/dm_mailchimp.py | 35 ++++++++++++++------------- tests/email/test_dm_mailchimp.py | 41 +++++++++++++++++++++++++++----- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/dmutils/email/dm_mailchimp.py b/dmutils/email/dm_mailchimp.py index 697198f9..76f509f1 100644 --- a/dmutils/email/dm_mailchimp.py +++ b/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 diff --git a/tests/email/test_dm_mailchimp.py b/tests/email/test_dm_mailchimp.py index 640b7168..b7593e1c 100644 --- a/tests/email/test_dm_mailchimp.py +++ b/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', 'example@example.com') 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 = "user@example.com 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', 'example@example.com') + + 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( From 8f2135347bf0e7fb12cdf269deac26cefe3960ed Mon Sep 17 00:00:00 2001 From: benvand Date: Thu, 18 Oct 2018 15:36:32 +0100 Subject: [PATCH 2/2] Version bump from 44.8.1 to 44.8.2 --- dmutils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dmutils/__init__.py b/dmutils/__init__.py index 1e567d73..352c429e 100644 --- a/dmutils/__init__.py +++ b/dmutils/__init__.py @@ -2,4 +2,4 @@ from .flask_init import init_app, init_manager -__version__ = '44.8.1' +__version__ = '44.8.2'