[ENG-7962] Fix User Setting Response Payload async mailchimp perference change issues

api/users/serializers.py

@@ -38,7 +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.profile.views import update_osf_help_mails_subscription, 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
 
@@ -543,10 +543,24 @@ class UserSettingsUpdateSerializer(UserSettingsSerializer):
 
     def update_email_preferences(self, instance, attr, value):
         if self.MAP_MAIL[attr] == OSF_HELP_LIST:
-            update_osf_help_mails_subscription(user=instance, subscribe=value)
+            instance.osf_mailing_lists[settings.OSF_HELP_LIST] = value
+        elif self.MAP_MAIL[attr] == MAILCHIMP_GENERAL_LIST:
+            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:
-            update_mailchimp_subscription(instance, self.MAP_MAIL[attr], value)
+            raise exceptions.ValidationError(detail='Invalid email preference.')
+
         instance.save()
+        return instance
 
     def update_two_factor(self, instance, value, two_factor_addon):
         if value:
@@ -581,6 +595,7 @@ def to_representation(self, instance):
         Overriding to_representation allows using different serializers for the request and response.
         """
         context = self.context
+
         return UserSettingsSerializer(instance=instance, context=context).data
 
     def update(self, instance, validated_data):

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,14 +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)
+        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)

osf/management/commands/update_mailchimp_email.py

@@ -13,7 +13,7 @@ def update_mailchimp_email():
     for user in OSFUser.objects.filter(deleted__isnull=True):
         for list_name, subscription in user.mailchimp_mailing_lists.items():
             if subscription:
-                mailchimp_utils.subscribe_mailchimp(list_name, user._id)
+                mailchimp_utils.subscribe_mailchimp_async(list_name, user._id)
         users_updated += 1
 
     return users_updated

osf/models/user.py

@@ -1073,7 +1073,7 @@ def save(self, *args, **kwargs):
         if 'username' in dirty_fields:
             for list_name, subscription in self.mailchimp_mailing_lists.items():
                 if subscription:
-                    mailchimp_utils.subscribe_mailchimp(list_name, self._id)
+                    mailchimp_utils.subscribe_mailchimp_async(list_name, self._id)
         return ret
 
     # Legacy methods

scripts/fix_user_mailchimp.py

@@ -9,7 +9,7 @@
 setup_django()
 from osf.models import OSFUser
 from scripts import utils as script_utils
-from website.mailchimp_utils import subscribe_mailchimp
+from website.mailchimp_utils import subscribe_mailchimp_async
 from website import settings
 
 logger = logging.getLogger(__name__)
@@ -31,7 +31,7 @@ def main():
         for user in users:
             if settings.MAILCHIMP_GENERAL_LIST not in user.mailchimp_mailing_lists:
                 if not dry:
-                    subscribe_mailchimp(settings.MAILCHIMP_GENERAL_LIST, user._id)
+                    subscribe_mailchimp_async(settings.MAILCHIMP_GENERAL_LIST, user._id)
                     logger.info(f'User {user._id} has been subscribed to OSF general mailing list')
                 count += 1
 

tests/test_configure_mailing_list_view.py

@@ -11,7 +11,6 @@
     OsfTestCase,
 )
 from website import mailchimp_utils, settings
-from website.profile.views import update_osf_help_mails_subscription
 from website.settings import MAILCHIMP_GENERAL_LIST
 from website.util import api_url_for
 
@@ -46,21 +45,6 @@ def test_get_notifications(self):
         res = self.app.get(url, auth=user.auth)
         assert mailing_lists == res.json['mailing_lists']
 
-    def test_osf_help_mails_subscribe(self):
-        user = UserFactory()
-        user.osf_mailing_lists[settings.OSF_HELP_LIST] = False
-        user.save()
-        update_osf_help_mails_subscription(user, True)
-        assert user.osf_mailing_lists[settings.OSF_HELP_LIST]
-
-    def test_osf_help_mails_unsubscribe(self):
-        user = UserFactory()
-        user.osf_mailing_lists[settings.OSF_HELP_LIST] = True
-        user.save()
-        update_osf_help_mails_subscription(user, False)
-        assert not user.osf_mailing_lists[settings.OSF_HELP_LIST]
-
-    @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()

tests/test_mailchimp.py

@@ -36,7 +36,7 @@ def test_subscribe_called(self, mock_get_mailchimp_api):
         mock_get_mailchimp_api.return_value = mock_client
         mock_client.lists.get.return_value = {'id': 1, 'list_name': list_name}
         list_id = mailchimp_utils.get_list_id_from_name(list_name)
-        mailchimp_utils.subscribe_mailchimp(list_name, user._id)
+        mailchimp_utils.subscribe_mailchimp_async(list_name, user._id)
         handlers.celery_teardown_request()
         mock_client.lists.members.create_or_update.assert_called()
 
@@ -48,10 +48,9 @@ def test_subscribe_fake_email_does_not_throw_validation_error(self, mock_get_mai
         mock_client = mock.MagicMock()
         mock_get_mailchimp_api.return_value = mock_client
         mock_client.lists.members.create_or_update.side_effect = MailChimpError
-        mailchimp_utils.subscribe_mailchimp(list_name, user._id)
-        handlers.celery_teardown_request()
+        mailchimp_utils.subscribe_mailchimp_async(list_name, user._id)
         user.reload()
-        assert not user.mailchimp_mailing_lists[list_name]
+        assert not user.mailchimp_mailing_lists
 
     @mock.patch('website.mailchimp_utils.get_mailchimp_api')
     def test_unsubscribe_called_with_correct_arguments(self, mock_get_mailchimp_api):

tests/test_user_profile_view.py

@@ -1,103 +1,30 @@
 #!/usr/bin/env python3
 """Views tests for the OSF."""
-from unittest.mock import MagicMock, ANY
-from urllib import parse
-
-import datetime as dt
-import time
-import unittest
 from hashlib import md5
-from http.cookies import SimpleCookie
 from unittest import mock
-from urllib.parse import quote_plus
-
 import pytest
-from django.core.exceptions import ValidationError
-from django.utils import timezone
-from flask import request, g
-from lxml import html
-from pytest import approx
 from rest_framework import status as http_status
 
 from addons.github.tests.factories import GitHubAccountFactory
-from addons.osfstorage import settings as osfstorage_settings
-from addons.wiki.models import WikiPage
-from framework import auth
-from framework.auth import Auth, authenticate, cas, core
-from framework.auth.campaigns import (
-    get_campaigns,
-    is_institution_login,
-    is_native_login,
-    is_proxy_login,
-    campaign_url_for
-)
-from framework.auth.exceptions import InvalidTokenError
-from framework.auth.utils import impute_names_model, ensure_external_identity_uniqueness
-from framework.auth.views import login_and_register_handler
 from framework.celery_tasks import handlers
-from framework.exceptions import HTTPError, TemplateHTTPError
-from framework.flask import redirect
-from framework.transactions.handlers import no_auto_transaction
 from osf.external.spam import tasks as spam_tasks
 from osf.models import (
-    Comment,
-    AbstractNode,
-    OSFUser,
-    Tag,
-    SpamStatus,
-    NodeRelation,
     NotableDomain
 )
-from osf.utils import permissions
 from osf_tests.factories import (
     fake_email,
     ApiOAuth2ApplicationFactory,
     ApiOAuth2PersonalTokenFactory,
     AuthUserFactory,
-    CollectionFactory,
-    CommentFactory,
-    NodeFactory,
-    OSFGroupFactory,
-    PreprintFactory,
-    PreprintProviderFactory,
-    PrivateLinkFactory,
-    ProjectFactory,
-    ProjectWithAddonFactory,
-    RegistrationProviderFactory,
-    UserFactory,
-    UnconfirmedUserFactory,
-    UnregUserFactory,
     RegionFactory,
-    DraftRegistrationFactory,
 )
 from tests.base import (
-    assert_is_redirect,
-    capture_signals,
     fake,
-    get_default_metaschema,
     OsfTestCase,
-    assert_datetime_equal,
-    test_app
-)
-from tests.test_cas_authentication import generate_external_user_with_resp
-from tests.utils import run_celery_tasks
-from website import mailchimp_utils, mails, settings, language
-from website.profile.utils import add_contributor_json, serialize_unregistered
-from website.profile.views import update_osf_help_mails_subscription
-from website.project.decorators import check_can_access
-from website.project.model import has_anonymous_link
-from website.project.signals import contributor_added
-from website.project.views.contributor import (
-    deserialize_contributors,
-    notify_added_contributor,
-    send_claim_email,
-    send_claim_registered_email,
 )
-from website.project.views.node import _should_show_wiki_widget, abbrev_authors
+from website import mailchimp_utils
 from website.settings import MAILCHIMP_GENERAL_LIST
 from website.util import api_url_for, web_url_for
-from website.util import rubeus
-from website.util.metrics import OsfSourceTags, OsfClaimedTags, provider_source_tag, provider_claimed_tag
 
 
 @pytest.mark.enable_enqueue_task

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):
@@ -120,4 +118,4 @@ def unsubscribe_mailchimp_async(list_name, user_id, username=None):
 def subscribe_on_confirm(user):
     # Subscribe user to general OSF mailing list upon account confirmation
     if settings.ENABLE_EMAIL_SUBSCRIPTIONS:
-        subscribe_mailchimp(settings.MAILCHIMP_GENERAL_LIST, user._id)
+        subscribe_mailchimp_async(settings.MAILCHIMP_GENERAL_LIST, user._id)

website/profile/views.py

@@ -496,7 +496,7 @@ def user_choose_mailing_lists(auth, **kwargs):
         for list_name, subscribe in json_data.items():
             # TO DO: change this to take in any potential non-mailchimp, something like try: update_subscription(), except IndexNotFound: update_mailchimp_subscription()
             if list_name == settings.OSF_HELP_LIST:
-                update_osf_help_mails_subscription(user=user, subscribe=subscribe)
+                user.osf_mailing_lists[settings.OSF_HELP_LIST] = subscribe
             else:
                 update_mailchimp_subscription(user, list_name, subscribe)
     else:
@@ -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_async(list_name, user._id)
     else:
         try:
             mailchimp_utils.unsubscribe_mailchimp_async(list_name, user._id, username=user.username)
@@ -602,10 +599,6 @@ def impute_names(**kwargs):
     return auth_utils.impute_names(name)
 
 
-def update_osf_help_mails_subscription(user, subscribe):
-    user.osf_mailing_lists[settings.OSF_HELP_LIST] = subscribe
-    user.save()
-
 @must_be_logged_in
 def serialize_names(**kwargs):
     user = kwargs['auth'].user