From 24953db2d77c84aec2f581f5ca2118b51091e81a Mon Sep 17 00:00:00 2001 From: U8N WXD Date: Mon, 20 May 2024 07:21:34 -0700 Subject: [PATCH] Fix #19858: Make MailChimp Error Handling Comprehensive (#20272) * Make MailChimp Error Handling Comprehensive Ensure that all errors from MailChimp while signing a user up for the mailing list are caught, even those errors that arise when trying to handle missing users. * Fix linter --- .../mailchimp_bulk_email_services.py | 84 +++++++++++-------- .../mailchimp_bulk_email_services_test.py | 12 ++- 2 files changed, 56 insertions(+), 40 deletions(-) diff --git a/core/platform/bulk_email/mailchimp_bulk_email_services.py b/core/platform/bulk_email/mailchimp_bulk_email_services.py index 4391468fb73a..60d97f611c40 100644 --- a/core/platform/bulk_email/mailchimp_bulk_email_services.py +++ b/core/platform/bulk_email/mailchimp_bulk_email_services.py @@ -201,6 +201,9 @@ def add_or_update_user_status( Raises: Exception. Raised if the tag or merge fields are invalid. + MailChimpError. Raised if MailChimp throws an error besides a 404 error + for a missing user. Should be caught by outer try-except block so + long as the error thrown by MailChimp inherits from Exception. """ client = _get_mailchimp_class() if not client: @@ -266,42 +269,49 @@ def add_or_update_user_status( merge_fields['NAME']) try: - client.lists.members.get( - feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash) - - # If member is already added to mailchimp list, we cannot permanently - # delete a list member, since they cannot be programmatically added - # back, so we change their status based on preference. - if can_receive_email_updates: - client.lists.members.tags.update( - feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash, tag_data) - client.lists.members.update( - feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash, - subscribed_mailchimp_data) - else: - client.lists.members.update( - feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash, - unsubscribed_mailchimp_data) - - except Exception as error: - # This has to be done since the message can only be accessed from - # MailChimpError by error.message in Python2, but this is deprecated in - # Python3. - # In Python3, the message can be accessed directly by KeyError - # (https://github.com/VingtCinq/python-mailchimp/pull/65), so as a - # workaround for Python2, the 'message' attribute is obtained by - # str() and then it is converted to dict. This works in Python3 as well. - error_message = ast.literal_eval(str(error)) - # Error 404 corresponds to 'User does not exist'. - if error_message['status'] == 404: + try: + client.lists.members.get( + feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash) + + # If member is already added to mailchimp list, we cannot + # permanently delete a list member, since they cannot be + # programmatically added back, so we change their status based on + # preference. if can_receive_email_updates: - user_creation_successful = _create_user_in_mailchimp_db( - client, new_user_mailchimp_data) - if not user_creation_successful: - return False - else: - logging.error( - 'Mailchimp error prevented email signup: %s', - error_message['detail']) - return False + client.lists.members.tags.update( + feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash, tag_data) + client.lists.members.update( + feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash, + subscribed_mailchimp_data) + else: + client.lists.members.update( + feconf.MAILCHIMP_AUDIENCE_ID, subscriber_hash, + unsubscribed_mailchimp_data) + except mailchimpclient.MailChimpError as mailchimp_err: + # This has to be done since the message can only be accessed from + # MailChimpError by error.message in Python2, but this is deprecated + # in Python3. In Python3, the message can be accessed directly by + # KeyError (https://github.com/VingtCinq/python-mailchimp/pull/65), + # so as a workaround for Python2, the 'message' attribute is + # obtained by str() and then it is converted to dict. This works in + # Python3 as well. + error_message = ast.literal_eval(str(mailchimp_err)) + # Error 404 corresponds to 'User does not exist'. + if error_message['status'] == 404: + if can_receive_email_updates: + user_creation_successful = _create_user_in_mailchimp_db( + client, new_user_mailchimp_data) + if not user_creation_successful: + return False + else: + raise mailchimp_err + except Exception as error: + # If our MailChimp operations fail for any reason, we want to still let + # the user complete their operation (e.g. signing-up for Oppia), so we + # log the error message and return False so that the caller can surface + # an error message to the user. Note that this is also where we handle + # the non-404 errors caught in the preceding try-except block, since + # those errors are re-raised in the preceding except block. + logging.error('Mailchimp error prevented email signup: %s', error) + return False return True diff --git a/core/platform/bulk_email/mailchimp_bulk_email_services_test.py b/core/platform/bulk_email/mailchimp_bulk_email_services_test.py index d6ab5d1c0b47..ff4d9b0f38c9 100644 --- a/core/platform/bulk_email/mailchimp_bulk_email_services_test.py +++ b/core/platform/bulk_email/mailchimp_bulk_email_services_test.py @@ -353,7 +353,10 @@ def test_add_or_update_mailchimp_user_status(self) -> None: self.user_email_1, {}, 'Web', can_receive_email_updates=True) self.assertItemsEqual( - ['Mailchimp error prevented email signup: Server Error'], + [ + 'Mailchimp error prevented email signup: ' + '{\'status\': 401, \'detail\': \'Server Error\'}' + ], logs ) @@ -399,11 +402,14 @@ def test_catch_or_raise_errors_when_creating_new_invalid_user(self) -> None: self.assertEqual(len(mailchimp.lists.members.users_data), 3) # Create user raises exception for other errors. - with self.assertRaisesRegex( - Exception, 'Server Issue'): + with self.capture_logging(min_level=logging.ERROR) as logs: mailchimp_bulk_email_services.add_or_update_user_status( 'test5@example.com', {}, 'Web', can_receive_email_updates=True) + self.assertItemsEqual( + ['Mailchimp error prevented email signup: Server Issue'], + logs + ) def test_permanently_delete_user(self) -> None: mailchimp = self.MockMailchimpClass()