simpify mail pref changing by make synchronous function instead of async task

Author: Johnetordoff

api/users/serializers.py

@@ -38,8 +38,7 @@
 from osf.models.user_message import MessageTypes
 from osf.models.provider import AbstractProviderGroupObjectPermission
 from osf.utils.requests import string_type_request_headers
-from website import settings
-from website.profile.views import update_mailchimp_subscription
+from website import settings, mailchimp_utils
 from website.settings import MAILCHIMP_GENERAL_LIST, OSF_HELP_LIST, CONFIRM_REGISTRATIONS_BY_EMAIL
 from website.util import api_v2_url
 
@@ -546,7 +545,17 @@ def update_email_preferences(self, instance, attr, value):
         if self.MAP_MAIL[attr] == OSF_HELP_LIST:
             instance.osf_mailing_lists[settings.OSF_HELP_LIST] = value
         elif self.MAP_MAIL[attr] == MAILCHIMP_GENERAL_LIST:
-            update_mailchimp_subscription(instance, self.MAP_MAIL[attr], value)
+            if value:
+                mailchimp_utils.subscribe_mailchimp(
+                    self.MAP_MAIL[attr],
+                    instance._id,
+                )
+            else:
+                mailchimp_utils.unsubscribe_mailchimp(
+                    self.MAP_MAIL[attr],
+                    instance._id,
+                    username=instance.username,
+                )
         else:
             raise exceptions.ValidationError(detail='Invalid email preference.')
 
@@ -587,14 +596,6 @@ def to_representation(self, instance):
         """
         context = self.context
 
-        # special case to show that we sent a request to subscribe, but don't
-        # change actual value until the task executes in case it doesn't and they need to try again
-        if getattr(instance, 'assume_mailchimp_completed_request', False):
-            instance.mailchimp_mailing_lists[MAILCHIMP_GENERAL_LIST] = True
-            instance.save = lambda _: NotImplementedError(
-                'Cannot save read comments in UserSettingsUpdateSerializer.to_representation',
-            )
-
         return UserSettingsSerializer(instance=instance, context=context).data
 
     def update(self, instance, validated_data):
@@ -611,11 +612,6 @@ def update(self, instance, validated_data):
             elif attr in self.MAP_MAIL.keys():
                 self.update_email_preferences(instance, attr, value)
 
-                # special case to show that we sent a request to subscribe, but don't
-                # change actual value until the task executes in case it doesn't and they need to try again
-                if value and self.MAP_MAIL[attr] == MAILCHIMP_GENERAL_LIST:
-                    instance.assume_mailchimp_completed_request = True
-
         return instance
 
 

api_tests/users/views/test_user_settings_detail.py

@@ -4,7 +4,7 @@
 from osf_tests.factories import (
     AuthUserFactory,
 )
-from website.settings import MAILCHIMP_GENERAL_LIST, OSF_HELP_LIST
+from website.settings import OSF_HELP_LIST
 
 
 @pytest.fixture()
@@ -200,16 +200,15 @@ def bad_payload(self, user_one):
             }
         }
 
-    @mock.patch('api.users.serializers.update_mailchimp_subscription')
-    def test_authorized_patch_200(self, mailchimp_mock, app, user_one, payload, url):
+    @mock.patch('website.mailchimp_utils.get_mailchimp_api')
+    def test_authorized_patch_200(self, mock_mailchimp_client, app, user_one, payload, url):
         res = app.patch_json_api(url, payload, auth=user_one.auth)
         assert res.status_code == 200
 
         user_one.refresh_from_db()
         assert res.json['data']['attributes']['subscribe_osf_help_email'] is False
         assert user_one.osf_mailing_lists[OSF_HELP_LIST] is False
-        mailchimp_mock.assert_called_with(user_one, MAILCHIMP_GENERAL_LIST, True)
-        assert res.json['data']['attributes']['subscribe_osf_general_email'] is True
+        mock_mailchimp_client.assert_called_with()
 
     def test_bad_payload_patch_400(self, app, user_one, bad_payload, url):
         res = app.patch_json_api(url, bad_payload, auth=user_one.auth, expect_errors=True)

tests/test_configure_mailing_list_view.py

@@ -45,7 +45,6 @@ def test_get_notifications(self):
         res = self.app.get(url, auth=user.auth)
         assert mailing_lists == res.json['mailing_lists']
 
-    @unittest.skipIf(settings.USE_CELERY, 'Subscription must happen synchronously for this test')
     @mock.patch('website.mailchimp_utils.get_mailchimp_api')
     def test_user_choose_mailing_lists_updates_user_dict(self, mock_get_mailchimp_api):
         user = AuthUserFactory()

website/mailchimp_utils.py

@@ -39,37 +39,35 @@ def get_list_name_from_id(list_id):
 @queued_task
 @app.task
 @transaction.atomic
+def subscribe_mailchimp_async(list_name, user_id):
+    return subscribe_mailchimp(list_name=list_name, user_id=user_id)
+
 def subscribe_mailchimp(list_name, user_id):
     user = OSFUser.load(user_id)
-    user_hash = hashlib.md5(user.username.lower().encode()).hexdigest()
-    m = get_mailchimp_api()
-    list_id = get_list_id_from_name(list_name=list_name)
+    if not user:
+        raise OSFError('User not found.')
 
     if user.mailchimp_mailing_lists is None:
         user.mailchimp_mailing_lists = {}
 
-    try:
-        m.lists.members.create_or_update(
-            list_id=list_id,
-            subscriber_hash=user_hash,
-            data={
-                'status': 'subscribed',
-                'status_if_new': 'subscribed',
-                'email_address': user.username,
-                'merge_fields': {
-                    'FNAME': user.given_name,
-                    'LNAME': user.family_name
-                }
+    get_mailchimp_api().lists.members.create_or_update(
+        list_id=get_list_id_from_name(list_name=list_name),
+        subscriber_hash=hashlib.md5(
+            user.username.lower().encode()
+        ).hexdigest(),
+        data={
+            'status': 'subscribed',
+            'status_if_new': 'subscribed',
+            'email_address': user.username,
+            'merge_fields': {
+                'FNAME': user.given_name,
+                'LNAME': user.family_name
             }
-        )
-    except MailChimpError as error:
-        sentry.log_exception(error)
-        sentry.log_message(error)
-        user.mailchimp_mailing_lists[list_name] = False
-    else:
-        user.mailchimp_mailing_lists[list_name] = True
-    finally:
-        user.save()
+        }
+    )
+
+    user.mailchimp_mailing_lists[list_name] = True
+    user.save()
 
 
 def unsubscribe_mailchimp(list_name, user_id, username=None):

website/profile/views.py

@@ -519,10 +519,7 @@ def update_mailchimp_subscription(user, list_name, subscription):
     :param boolean subscription: true if user is subscribed
     """
     if subscription:
-        try:
-            mailchimp_utils.subscribe_mailchimp(list_name, user._id)
-        except (MailChimpError, OSFError):
-            pass
+        mailchimp_utils.subscribe_mailchimp(list_name, user._id)
     else:
         try:
             mailchimp_utils.unsubscribe_mailchimp_async(list_name, user._id, username=user.username)