From 4f08575e9915b0f3d147286a21ae4fa47797d99f Mon Sep 17 00:00:00 2001 From: Benjamin Gill Date: Tue, 19 Apr 2022 11:53:59 +0100 Subject: [PATCH 1/2] Log stack trace for failure to remove email from Mailchimp Unlike all the other methods on the mailchimp client, this one is only called from scripts. This means that logging is unstructured, so we lose the information in `extra`. Switch to `logger.exception` so that we get the stack trace, including the error details. This should make future errors easier to debug. Since emails are hashed, there's no risk of leaking PII. --- dmutils/email/dm_mailchimp.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dmutils/email/dm_mailchimp.py b/dmutils/email/dm_mailchimp.py index d7c924a5..02cfac95 100644 --- a/dmutils/email/dm_mailchimp.py +++ b/dmutils/email/dm_mailchimp.py @@ -229,8 +229,7 @@ def permanently_remove_email_from_list(self, email_address: str, list_id: str) - ) return True except (RequestException, MailChimpError) as e: - self.logger.error( + self.logger.exception( f"Mailchimp failed to permanently remove user ({hashed_email}) from list ({list_id})", - extra={"error": str(e), "mailchimp_response": get_response_from_exception(e)}, ) return False From 4caa811ce316c769aba17b4ea5ec02f91c3564aa Mon Sep 17 00:00:00 2001 From: Benjamin Gill Date: Tue, 19 Apr 2022 12:04:03 +0100 Subject: [PATCH 2/2] Count removal as success if already removed Now that we've got two environments (1.5 and 1.0), we're running the data retention script for both. The environments have different databases, but share Mailchimp lists. This means that we have the potential for race conditions - one environment's job can delete a user from a list before the other tries to. When this happens, we get a MailChimpError with a status of 404. So count this particular circumstance as a success. --- dmutils/__init__.py | 2 +- dmutils/email/dm_mailchimp.py | 3 +++ tests/email/test_dm_mailchimp.py | 24 ++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/dmutils/__init__.py b/dmutils/__init__.py index fbe22cdd..06c8d864 100644 --- a/dmutils/__init__.py +++ b/dmutils/__init__.py @@ -2,4 +2,4 @@ from .flask_init import init_app -__version__ = '60.7.1' +__version__ = '60.8.0' diff --git a/dmutils/email/dm_mailchimp.py b/dmutils/email/dm_mailchimp.py index 02cfac95..04fca07a 100644 --- a/dmutils/email/dm_mailchimp.py +++ b/dmutils/email/dm_mailchimp.py @@ -229,6 +229,9 @@ def permanently_remove_email_from_list(self, email_address: str, list_id: str) - ) return True except (RequestException, MailChimpError) as e: + if get_response_from_exception(e).get('status') == 404: + self.logger.info(f"User ({hashed_email}) not found in list ({list_id})") + return True self.logger.exception( f"Mailchimp failed to permanently remove user ({hashed_email}) from list ({list_id})", ) diff --git a/tests/email/test_dm_mailchimp.py b/tests/email/test_dm_mailchimp.py index 72a4393d..b3b687f8 100644 --- a/tests/email/test_dm_mailchimp.py +++ b/tests/email/test_dm_mailchimp.py @@ -603,3 +603,27 @@ def test_permanently_remove_email_from_list_failure(self, exception): subscriber_hash="ee5ae5f54bdf3394d48ea4e79e6d0e39", ), ] + + def test_permanently_remove_email_from_list_already_removed(self): + dm_mailchimp_client = DMMailChimpClient('username', DUMMY_MAILCHIMP_API_KEY, logging.getLogger('mailchimp')) + with mock.patch.object( + dm_mailchimp_client._client.lists.members, + 'delete_permanent', + autospec=True, + ) as del_perm: + del_perm.side_effect = MailChimpError({"status": 404}) + + with assert_external_service_log_entry(successful_call=False, extra_modules=['mailchimp'], count=1): + result = dm_mailchimp_client.permanently_remove_email_from_list( + "trousers.potato@purse.net", + "modern_society", + ) + + assert result is True + + assert del_perm.call_args_list == [ + mock.call( + list_id="modern_society", + subscriber_hash="ee5ae5f54bdf3394d48ea4e79e6d0e39", + ), + ]