Fix oppia#19858: Make MailChimp Error Handling Comprehensive (oppia#2…

…0272)

* 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

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

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(
                     '[email protected]', {}, '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()