diff --git a/api/users/serializers.py b/api/users/serializers.py index e7e306f9194..ed7f2af833c 100644 --- a/api/users/serializers.py +++ b/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): diff --git a/api_tests/users/views/test_user_settings_detail.py b/api_tests/users/views/test_user_settings_detail.py index cf9194409f6..50da5e6cbfa 100644 --- a/api_tests/users/views/test_user_settings_detail.py +++ b/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) diff --git a/osf/management/commands/update_mailchimp_email.py b/osf/management/commands/update_mailchimp_email.py index 8c39a9b8edf..6b742d4706e 100644 --- a/osf/management/commands/update_mailchimp_email.py +++ b/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 diff --git a/osf/models/user.py b/osf/models/user.py index 600b563dd76..eb17c724d38 100644 --- a/osf/models/user.py +++ b/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 diff --git a/scripts/fix_user_mailchimp.py b/scripts/fix_user_mailchimp.py index d8937f8d7a7..a687725cca2 100644 --- a/scripts/fix_user_mailchimp.py +++ b/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 diff --git a/tests/test_configure_mailing_list_view.py b/tests/test_configure_mailing_list_view.py index e36afec5e02..1f6960a451b 100644 --- a/tests/test_configure_mailing_list_view.py +++ b/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() diff --git a/tests/test_mailchimp.py b/tests/test_mailchimp.py index ac30e2763d4..fb269c09797 100644 --- a/tests/test_mailchimp.py +++ b/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): diff --git a/tests/test_user_profile_view.py b/tests/test_user_profile_view.py index 3e1c455c078..4350bbcd714 100644 --- a/tests/test_user_profile_view.py +++ b/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 diff --git a/website/mailchimp_utils.py b/website/mailchimp_utils.py index c14df5d865e..becc770541a 100644 --- a/website/mailchimp_utils.py +++ b/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) diff --git a/website/profile/views.py b/website/profile/views.py index c4306b92125..c4d49147454 100644 --- a/website/profile/views.py +++ b/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