From e31919182fd896e060ff1c7542e9cfb900f393a7 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Sun, 17 Mar 2024 17:28:51 -0400 Subject: [PATCH 01/18] add account changes messaging to osf.io --- framework/celery_tasks/routers.py | 2 ++ website/settings/defaults.py | 1 + 2 files changed, 3 insertions(+) diff --git a/framework/celery_tasks/routers.py b/framework/celery_tasks/routers.py index 5335012dbf9..1b85855ab1b 100644 --- a/framework/celery_tasks/routers.py +++ b/framework/celery_tasks/routers.py @@ -14,6 +14,8 @@ def match_by_module(task_path): return CeleryConfig.task_high_queue if task_subpath in CeleryConfig.remote_computing_modules: return CeleryConfig.task_remote_computing_queue + if task_subpath in CeleryConfig.account_status_changes: + return CeleryConfig.account_status_changes return CeleryConfig.task_default_queue diff --git a/website/settings/defaults.py b/website/settings/defaults.py index 001e52f0a3c..cfac88c2e0d 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -411,6 +411,7 @@ class CeleryConfig: task_med_queue = 'med' task_high_queue = 'high' task_remote_computing_queue = 'remote' + account_status_changes = 'account_status_changes' remote_computing_modules = { 'addons.boa.tasks.submit_to_boa', From 0c734c7542535ffd9f3ab4c9ee63c503509f1004 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Sun, 17 Mar 2024 17:36:45 -0400 Subject: [PATCH 02/18] add queue and signals --- framework/auth/signals.py | 4 +- osf/external/messages/celery_publishers.py | 47 +++++++++++++++++++++ osf/management/commands/test_publish.py | 39 ++++++++++++++++++ osf/models/user.py | 13 ++---- osf_tests/test_user.py | 4 +- website/profile/views.py | 48 +++++++++++++++++++++- website/settings/defaults.py | 1 + website/signals.py | 7 +++- 8 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 osf/external/messages/celery_publishers.py create mode 100644 osf/management/commands/test_publish.py diff --git a/framework/auth/signals.py b/framework/auth/signals.py index 2387ea2b4d0..15f279520e5 100644 --- a/framework/auth/signals.py +++ b/framework/auth/signals.py @@ -6,6 +6,8 @@ user_registered = signals.signal('user-registered') user_confirmed = signals.signal('user-confirmed') user_email_removed = signals.signal('user-email-removed') -user_merged = signals.signal('user-merged') +user_account_merged = signals.signal('user-account-merged') +user_account_deactivated = signals.signal('user-account-deactivated') +user_account_reactivated = signals.signal('user-account-reactivated') unconfirmed_user_created = signals.signal('unconfirmed-user-created') diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py new file mode 100644 index 00000000000..724b57df731 --- /dev/null +++ b/osf/external/messages/celery_publishers.py @@ -0,0 +1,47 @@ +from kombu import Exchange +from framework.celery_tasks import app as celery_app +from framework.celery_tasks.handlers import enqueue_task + + +def publish_deactivated_user(user): + enqueue_task( + _publish_user_status_change.s( + body={ + 'action': 'deactivate', + 'user_uri': user.url, + }, + ) + ) + + +def publish_reactivate_user(user): + enqueue_task( + _publish_user_status_change.s( + body={ + 'action': 'reactivate', + 'user_uri': user.url, + }, + ) + ) + + +def publish_merged_user(user): + enqueue_task( + _publish_user_status_change.s( + body={ + 'action': 'merge', + 'user_uri': user.url, + 'merged_user_uri': user.merged_by.url, + }, + ) + ) + + +@celery_app.task() +def _publish_user_status_change(body: dict): + with celery_app.producer_pool.acquire() as producer: + producer.publish( + body=body, + exchange=Exchange(celery_app.conf.account_status_changes), + serializer='json' + ) diff --git a/osf/management/commands/test_publish.py b/osf/management/commands/test_publish.py new file mode 100644 index 00000000000..33b9a82de86 --- /dev/null +++ b/osf/management/commands/test_publish.py @@ -0,0 +1,39 @@ +from django.core.management.base import BaseCommand +from osf.models import OSFUser +from osf.external.messages.celery_publishers import ( + publish_deactivated_user, + publish_reactivate_user, + publish_merged_user, +) + + +class Command(BaseCommand): + help = 'Sends a message to manage a user state, for test purposes only.' + + def add_arguments(self, parser): + parser.add_argument('user_guid', type=str, help='URI of the user to post.') + # Adding a new argument to specify the action to perform + parser.add_argument( + 'action', + type=str, + help='The action to perform on the user (deactivate, reactivate, merge).', + choices=['deactivate', 'reactivate', 'merge'] + ) + + def handle(self, *args, **options): + user_guid = options['user_guid'] + user = OSFUser.objects.get(guids___id=user_guid) + action = options['action'] + + # Using a mapping of action to function to simplify the control flow + actions_to_functions = { + 'deactivate': publish_deactivated_user, + 'reactivate': publish_reactivate_user, + 'merge': publish_merged_user, + } + + if action in actions_to_functions: + actions_to_functions[action](user) # Call the appropriate function + self.stdout.write(self.style.SUCCESS(f'Successfully {action} message for user: {user._id}')) + else: + self.stdout.write(self.style.ERROR('Invalid action specified.')) diff --git a/osf/models/user.py b/osf/models/user.py index 6f7c7ae33d3..4969654b28d 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -746,15 +746,6 @@ def merge_user(self, user): notifications_configured = user.notifications_configured.copy() notifications_configured.update(self.notifications_configured) self.notifications_configured = notifications_configured - if not website_settings.RUNNING_MIGRATION: - for key, value in user.mailchimp_mailing_lists.items(): - # subscribe to each list if either user was subscribed - subscription = value or self.mailchimp_mailing_lists.get(key) - signals.user_merged.send(self, list_name=key, subscription=subscription) - - # clear subscriptions for merged user - signals.user_merged.send(user, list_name=key, subscription=False, send_goodbye=False) - for target_id, timestamp in user.comments_viewed_timestamp.items(): if not self.comments_viewed_timestamp.get(target_id): self.comments_viewed_timestamp[target_id] = timestamp @@ -872,6 +863,8 @@ def merge_user(self, user): user.merged_by = self user.save() + signals.user_account_merged.send(user) + signals.user_account_deactivated.send(self) def _merge_users_preprints(self, user): """ @@ -993,6 +986,7 @@ def deactivate_account(self): if isinstance(req, FlaskRequest): logout() remove_sessions_for_user(self) + signals.user_account_deactivated.send(self) def reactivate_account(self): """ @@ -1002,6 +996,7 @@ def reactivate_account(self): self.requested_deactivation = False from website.mailchimp_utils import subscribe_on_confirm subscribe_on_confirm(self) + signals.user_account_reactivated.send(self) def update_is_active(self): """Update ``is_active`` to be consistent with the fields that diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 2c4d961f77b..a2b1912f548 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -17,7 +17,7 @@ from importlib import import_module from framework.auth.exceptions import ExpiredTokenError, InvalidTokenError, ChangePasswordError -from framework.auth.signals import user_merged +from framework.auth.signals import user_account_merged from framework.analytics import get_total_activity_count from framework.exceptions import PermissionsError from framework.celery_tasks import handlers @@ -1507,7 +1507,7 @@ def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe) with capture_signals() as mock_signals: merge_dupe() - assert mock_signals.signals_sent() == set([user_merged]) + assert mock_signals.signals_sent() == set([user_account_merged]) @pytest.mark.enable_enqueue_task @mock.patch('website.mailchimp_utils.get_mailchimp_api') diff --git a/website/profile/views.py b/website/profile/views.py index c929c5a3c20..bee7e2699c2 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -16,7 +16,11 @@ from framework.auth.decorators import must_be_confirmed from framework.auth.exceptions import ChangePasswordError from framework.auth.views import send_confirm_email -from framework.auth.signals import user_merged +from framework.auth.signals import ( + user_account_merged, + user_account_deactivated, + user_account_reactivated +) from framework.exceptions import HTTPError, PermissionsError from framework.flask import redirect # VOL-aware redirect from framework.status import push_status_message @@ -36,6 +40,12 @@ from website.util import api_v2_url, web_url_for, paths from website.util.sanitize import escape_html from addons.base import utils as addon_utils +from osf.external.messages.celery_publishers import ( + publish_reactivate_user, + publish_deactivated_user, + publish_merged_user +) + from api.waffle.utils import storage_i18n_flag_active @@ -506,7 +516,6 @@ def user_choose_mailing_lists(auth, **kwargs): return {'message': 'Successfully updated mailing lists', 'result': all_mailing_lists}, 200 -@user_merged.connect def update_mailchimp_subscription(user, list_name, subscription, send_goodbye=True): """ Update mailing list subscription in mailchimp. @@ -527,6 +536,41 @@ def update_mailchimp_subscription(user, list_name, subscription, send_goodbye=Tr pass +@user_account_merged.connect +def send_account_merged_message(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been merged.""" + publish_merged_user(user) + + +@user_account_deactivated.connect +def send_mailchimp_unsubscribe(user): + """ Tells mailchimp a to unsubscribe a merged user """ + if not settings.RUNNING_MIGRATION: + for key, value in user.mailchimp_mailing_lists.items(): + update_mailchimp_subscription(user, list_name=key, subscription=False, send_goodbye=False) + + +@user_account_merged.connect +def send_mailchimp_merge(user): + """ Tells mailchimp a to merge a users subscriptions """ + if not settings.RUNNING_MIGRATION: + for key, value in user.merged_by.mailchimp_mailing_lists.items(): + subscription = value or user.merged_by.mailchimp_mailing_lists.get(key) + update_mailchimp_subscription(user, list_name=key, subscription=subscription, send_goodbye=False) + + +@user_account_deactivated.connect +def send_account_deactivation_message(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" + publish_deactivated_user(user) + + +@user_account_reactivated.connect +def send_account_reactivation_message(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been reactivated.""" + publish_reactivate_user(user) + + def mailchimp_get_endpoint(**kwargs): """Endpoint that the mailchimp webhook hits to check that the OSF is responding""" return {}, http_status.HTTP_200_OK diff --git a/website/settings/defaults.py b/website/settings/defaults.py index cfac88c2e0d..610fc219e3b 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -489,6 +489,7 @@ class CeleryConfig: routing_key=task_med_queue, consumer_arguments={'x-priority': 1}), Queue(task_high_queue, Exchange(task_high_queue), routing_key=task_high_queue, consumer_arguments={'x-priority': 10}), + Queue(account_status_changes, Exchange(account_status_changes), routing_key=account_status_changes) ) task_default_exchange_type = 'direct' diff --git a/website/signals.py b/website/signals.py index ff4988c3885..c1b8660dcd4 100644 --- a/website/signals.py +++ b/website/signals.py @@ -6,7 +6,8 @@ from website.conferences import signals as conference from website.reviews import signals as reviews -ALL_SIGNALS = [ + +ALL_SIGNALS = [ # TODO: Fix project.comment_added, project.mention_added, project.unreg_contributor_added, @@ -17,7 +18,9 @@ auth.user_confirmed, auth.user_email_removed, auth.user_registered, - auth.user_merged, + auth.user_account_deactivated, + auth.user_account_reactivated, + auth.user_account_merged, auth.unconfirmed_user_created, event.file_updated, conference.osf4m_user_created, From 1454e579be7006e945eecec6215f64275afdcb0b Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 18 Mar 2024 10:57:34 -0400 Subject: [PATCH 03/18] remove deactivate call --- osf/external/messages/__init__.py | 0 osf/models/user.py | 1 - 2 files changed, 1 deletion(-) create mode 100644 osf/external/messages/__init__.py diff --git a/osf/external/messages/__init__.py b/osf/external/messages/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/osf/models/user.py b/osf/models/user.py index 4969654b28d..ae56a8ceb11 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -986,7 +986,6 @@ def deactivate_account(self): if isinstance(req, FlaskRequest): logout() remove_sessions_for_user(self) - signals.user_account_deactivated.send(self) def reactivate_account(self): """ From 025bcf790171268c69673ae4a6ff1c6a71ea0021 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 18 Mar 2024 11:08:00 -0400 Subject: [PATCH 04/18] try fixing tests by adding enqueue decorators --- osf/models/user.py | 1 + osf_tests/test_notable_domains.py | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/osf/models/user.py b/osf/models/user.py index ae56a8ceb11..4969654b28d 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -986,6 +986,7 @@ def deactivate_account(self): if isinstance(req, FlaskRequest): logout() remove_sessions_for_user(self) + signals.user_account_deactivated.send(self) def reactivate_account(self): """ diff --git a/osf_tests/test_notable_domains.py b/osf_tests/test_notable_domains.py index b89e97c3c58..205f0c19037 100644 --- a/osf_tests/test_notable_domains.py +++ b/osf_tests/test_notable_domains.py @@ -320,6 +320,7 @@ def ignored_notable_domain(self): note=NotableDomain.Note.IGNORED, ) + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_unknown_one_spam_domain(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -338,6 +339,7 @@ def test_from_spam_to_unknown_one_spam_domain(self, factory, spam_notable_domain assert obj_one.spam_status == SpamStatus.UNKNOWN assert len(obj_one.spam_data['domains']) == 0 + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_unknown_two_spam_domains(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_two = factory() @@ -356,6 +358,7 @@ def test_from_spam_to_unknown_two_spam_domains(self, factory, spam_notable_domai assert obj_two.spam_status == SpamStatus.SPAM assert set(obj_two.spam_data['domains']) == set([self.spam_domain_two.netloc]) + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_unknown_marked_by_external(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_three = factory() @@ -376,6 +379,7 @@ def test_from_spam_to_unknown_marked_by_external(self, factory, spam_notable_dom assert obj_three.spam_status == SpamStatus.SPAM assert len(obj_three.spam_data['domains']) == 0 + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_ignored_one_spam_domain(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -394,6 +398,7 @@ def test_from_spam_to_ignored_one_spam_domain(self, factory, spam_notable_domain assert obj_one.spam_status == SpamStatus.UNKNOWN assert len(obj_one.spam_data['domains']) == 0 + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_ignored_two_spam_domains(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_two = factory() @@ -412,6 +417,7 @@ def test_from_spam_to_ignored_two_spam_domains(self, factory, spam_notable_domai assert obj_two.spam_status == SpamStatus.SPAM assert set(obj_two.spam_data['domains']) == set([self.spam_domain_two.netloc]) + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_ignored_makred_by_external(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_three = factory() @@ -432,6 +438,7 @@ def test_from_spam_to_ignored_makred_by_external(self, factory, spam_notable_dom assert obj_three.spam_status == SpamStatus.SPAM assert len(obj_three.spam_data['domains']) == 0 + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_unknown_to_spam_unknown_plus_ignored(self, factory, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -450,6 +457,7 @@ def test_from_unknown_to_spam_unknown_plus_ignored(self, factory, unknown_notabl assert obj_one.spam_status == SpamStatus.SPAM assert set(obj_one.spam_data['domains']) == set([self.unknown_domain.netloc]) + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_unknown_to_spam_unknown_only(self, factory, unknown_notable_domain, ignored_notable_domain): obj_two = factory() @@ -468,6 +476,7 @@ def test_from_unknown_to_spam_unknown_only(self, factory, unknown_notable_domain assert obj_two.spam_status == SpamStatus.SPAM assert set(obj_two.spam_data['domains']) == set([self.unknown_domain.netloc]) + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_ignored_to_spam_unknown_plus_ignored(self, factory, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -486,6 +495,7 @@ def test_from_ignored_to_spam_unknown_plus_ignored(self, factory, unknown_notabl assert obj_one.spam_status == SpamStatus.SPAM assert set(obj_one.spam_data['domains']) == set([self.ignored_domain.netloc]) + @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_ignored_to_spam_ignored_only(self, factory, unknown_notable_domain, ignored_notable_domain): obj_two = factory() From 098c044479cb2a81108d3a5acde418677b1dfe81 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 18 Mar 2024 11:41:58 -0400 Subject: [PATCH 05/18] move mailchimp signals --- framework/auth/signals.py | 1 + osf/models/user.py | 12 +++++++++++- osf_tests/test_notable_domains.py | 10 ---------- website/mailchimp_utils.py | 6 +++--- website/profile/views.py | 27 +++++---------------------- 5 files changed, 20 insertions(+), 36 deletions(-) diff --git a/framework/auth/signals.py b/framework/auth/signals.py index 15f279520e5..4d3dfffff56 100644 --- a/framework/auth/signals.py +++ b/framework/auth/signals.py @@ -11,3 +11,4 @@ user_account_reactivated = signals.signal('user-account-reactivated') unconfirmed_user_created = signals.signal('unconfirmed-user-created') +user_update_mailchimp_subscription = signals.signal('user-update-mailchimp-subscription') diff --git a/osf/models/user.py b/osf/models/user.py index 4969654b28d..3d133abc644 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -746,6 +746,15 @@ def merge_user(self, user): notifications_configured = user.notifications_configured.copy() notifications_configured.update(self.notifications_configured) self.notifications_configured = notifications_configured + if not website_settings.RUNNING_MIGRATION: + for key, value in user.mailchimp_mailing_lists.items(): + # subscribe to each list if either user was subscribed + subscription = value or self.mailchimp_mailing_lists.get(key) + signals.user_update_mailchimp_subscription.send(self, list_name=key, subscription=subscription) + + # clear subscriptions for merged user + signals.user_update_mailchimp_subscription.send(user, list_name=key, subscription=False) + for target_id, timestamp in user.comments_viewed_timestamp.items(): if not self.comments_viewed_timestamp.get(target_id): self.comments_viewed_timestamp[target_id] = timestamp @@ -979,6 +988,8 @@ def deactivate_account(self): # Call to `unsubscribe` above saves, and can lead to stale data self.reload() self.is_disabled = True + self.save() + signals.user_account_deactivated.send(self) # we must call both methods to ensure the current session is cleared and all existing # sessions are revoked. @@ -986,7 +997,6 @@ def deactivate_account(self): if isinstance(req, FlaskRequest): logout() remove_sessions_for_user(self) - signals.user_account_deactivated.send(self) def reactivate_account(self): """ diff --git a/osf_tests/test_notable_domains.py b/osf_tests/test_notable_domains.py index 205f0c19037..b89e97c3c58 100644 --- a/osf_tests/test_notable_domains.py +++ b/osf_tests/test_notable_domains.py @@ -320,7 +320,6 @@ def ignored_notable_domain(self): note=NotableDomain.Note.IGNORED, ) - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_unknown_one_spam_domain(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -339,7 +338,6 @@ def test_from_spam_to_unknown_one_spam_domain(self, factory, spam_notable_domain assert obj_one.spam_status == SpamStatus.UNKNOWN assert len(obj_one.spam_data['domains']) == 0 - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_unknown_two_spam_domains(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_two = factory() @@ -358,7 +356,6 @@ def test_from_spam_to_unknown_two_spam_domains(self, factory, spam_notable_domai assert obj_two.spam_status == SpamStatus.SPAM assert set(obj_two.spam_data['domains']) == set([self.spam_domain_two.netloc]) - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_unknown_marked_by_external(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_three = factory() @@ -379,7 +376,6 @@ def test_from_spam_to_unknown_marked_by_external(self, factory, spam_notable_dom assert obj_three.spam_status == SpamStatus.SPAM assert len(obj_three.spam_data['domains']) == 0 - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_ignored_one_spam_domain(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -398,7 +394,6 @@ def test_from_spam_to_ignored_one_spam_domain(self, factory, spam_notable_domain assert obj_one.spam_status == SpamStatus.UNKNOWN assert len(obj_one.spam_data['domains']) == 0 - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_ignored_two_spam_domains(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_two = factory() @@ -417,7 +412,6 @@ def test_from_spam_to_ignored_two_spam_domains(self, factory, spam_notable_domai assert obj_two.spam_status == SpamStatus.SPAM assert set(obj_two.spam_data['domains']) == set([self.spam_domain_two.netloc]) - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_spam_to_ignored_makred_by_external(self, factory, spam_notable_domain_one, spam_notable_domain_two, unknown_notable_domain, ignored_notable_domain): obj_three = factory() @@ -438,7 +432,6 @@ def test_from_spam_to_ignored_makred_by_external(self, factory, spam_notable_dom assert obj_three.spam_status == SpamStatus.SPAM assert len(obj_three.spam_data['domains']) == 0 - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_unknown_to_spam_unknown_plus_ignored(self, factory, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -457,7 +450,6 @@ def test_from_unknown_to_spam_unknown_plus_ignored(self, factory, unknown_notabl assert obj_one.spam_status == SpamStatus.SPAM assert set(obj_one.spam_data['domains']) == set([self.unknown_domain.netloc]) - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_unknown_to_spam_unknown_only(self, factory, unknown_notable_domain, ignored_notable_domain): obj_two = factory() @@ -476,7 +468,6 @@ def test_from_unknown_to_spam_unknown_only(self, factory, unknown_notable_domain assert obj_two.spam_status == SpamStatus.SPAM assert set(obj_two.spam_data['domains']) == set([self.unknown_domain.netloc]) - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_ignored_to_spam_unknown_plus_ignored(self, factory, unknown_notable_domain, ignored_notable_domain): obj_one = factory() @@ -495,7 +486,6 @@ def test_from_ignored_to_spam_unknown_plus_ignored(self, factory, unknown_notabl assert obj_one.spam_status == SpamStatus.SPAM assert set(obj_one.spam_data['domains']) == set([self.ignored_domain.netloc]) - @pytest.mark.enable_enqueue_task @pytest.mark.parametrize('factory', [NodeFactory, CommentFactory, PreprintFactory, RegistrationFactory, UserFactory]) def test_from_ignored_to_spam_ignored_only(self, factory, unknown_notable_domain, ignored_notable_domain): obj_two = factory() diff --git a/website/mailchimp_utils.py b/website/mailchimp_utils.py index 2a9aa32ec54..58cad24e70d 100644 --- a/website/mailchimp_utils.py +++ b/website/mailchimp_utils.py @@ -74,7 +74,7 @@ def subscribe_mailchimp(list_name, user_id): user.save() -def unsubscribe_mailchimp(list_name, user_id, username=None, send_goodbye=True): +def unsubscribe_mailchimp(list_name, user_id, username=None): """Unsubscribe a user from a mailchimp mailing list given its name. :param str list_name: mailchimp mailing list name @@ -113,10 +113,10 @@ def unsubscribe_mailchimp(list_name, user_id, username=None, send_goodbye=True): @queued_task @app.task @transaction.atomic -def unsubscribe_mailchimp_async(list_name, user_id, username=None, send_goodbye=True): +def unsubscribe_mailchimp_async(list_name, user_id, username=None): """ Same args as unsubscribe_mailchimp, used to have the task be run asynchronously """ - unsubscribe_mailchimp(list_name=list_name, user_id=user_id, username=username, send_goodbye=send_goodbye) + unsubscribe_mailchimp(list_name=list_name, user_id=user_id, username=username) @user_confirmed.connect def subscribe_on_confirm(user): diff --git a/website/profile/views.py b/website/profile/views.py index bee7e2699c2..892cf4cbadd 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -19,7 +19,8 @@ from framework.auth.signals import ( user_account_merged, user_account_deactivated, - user_account_reactivated + user_account_reactivated, + user_update_mailchimp_subscription ) from framework.exceptions import HTTPError, PermissionsError from framework.flask import redirect # VOL-aware redirect @@ -46,7 +47,6 @@ publish_merged_user ) - from api.waffle.utils import storage_i18n_flag_active logger = logging.getLogger(__name__) @@ -515,8 +515,8 @@ def user_choose_mailing_lists(auth, **kwargs): all_mailing_lists.update(user.osf_mailing_lists) return {'message': 'Successfully updated mailing lists', 'result': all_mailing_lists}, 200 - -def update_mailchimp_subscription(user, list_name, subscription, send_goodbye=True): +@user_update_mailchimp_subscription.connect +def update_mailchimp_subscription(user, list_name, subscription): """ Update mailing list subscription in mailchimp. :param obj user: current user @@ -530,7 +530,7 @@ def update_mailchimp_subscription(user, list_name, subscription, send_goodbye=Tr pass else: try: - mailchimp_utils.unsubscribe_mailchimp_async(list_name, user._id, username=user.username, send_goodbye=send_goodbye) + mailchimp_utils.unsubscribe_mailchimp_async(list_name, user._id, username=user.username) except (MailChimpError, OSFError): # User has already unsubscribed, so nothing to do pass @@ -542,23 +542,6 @@ def send_account_merged_message(user): publish_merged_user(user) -@user_account_deactivated.connect -def send_mailchimp_unsubscribe(user): - """ Tells mailchimp a to unsubscribe a merged user """ - if not settings.RUNNING_MIGRATION: - for key, value in user.mailchimp_mailing_lists.items(): - update_mailchimp_subscription(user, list_name=key, subscription=False, send_goodbye=False) - - -@user_account_merged.connect -def send_mailchimp_merge(user): - """ Tells mailchimp a to merge a users subscriptions """ - if not settings.RUNNING_MIGRATION: - for key, value in user.merged_by.mailchimp_mailing_lists.items(): - subscription = value or user.merged_by.mailchimp_mailing_lists.get(key) - update_mailchimp_subscription(user, list_name=key, subscription=subscription, send_goodbye=False) - - @user_account_deactivated.connect def send_account_deactivation_message(user): """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" From 5163c0476305c53ea4a677f3ed62f073e94e885f Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 18 Mar 2024 13:15:12 -0400 Subject: [PATCH 06/18] revert mailchimp signals --- framework/auth/signals.py | 4 +-- osf/external/messages/celery_publishers.py | 41 +++++++++------------- osf/models/user.py | 16 +++++---- osf_tests/test_user.py | 4 +-- website/profile/views.py | 34 +++--------------- website/signals.py | 4 +-- 6 files changed, 34 insertions(+), 69 deletions(-) diff --git a/framework/auth/signals.py b/framework/auth/signals.py index 4d3dfffff56..c538c0bc279 100644 --- a/framework/auth/signals.py +++ b/framework/auth/signals.py @@ -6,9 +6,7 @@ user_registered = signals.signal('user-registered') user_confirmed = signals.signal('user-confirmed') user_email_removed = signals.signal('user-email-removed') -user_account_merged = signals.signal('user-account-merged') -user_account_deactivated = signals.signal('user-account-deactivated') -user_account_reactivated = signals.signal('user-account-reactivated') +user_merged = signals.signal('user-account-merged') unconfirmed_user_created = signals.signal('unconfirmed-user-created') user_update_mailchimp_subscription = signals.signal('user-update-mailchimp-subscription') diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index 724b57df731..b203712302b 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -1,43 +1,36 @@ from kombu import Exchange from framework.celery_tasks import app as celery_app -from framework.celery_tasks.handlers import enqueue_task def publish_deactivated_user(user): - enqueue_task( - _publish_user_status_change.s( - body={ - 'action': 'deactivate', - 'user_uri': user.url, - }, - ) + _publish_user_status_change( + body={ + 'action': 'deactivate', + 'user_uri': user.url, + }, ) def publish_reactivate_user(user): - enqueue_task( - _publish_user_status_change.s( - body={ - 'action': 'reactivate', - 'user_uri': user.url, - }, - ) + _publish_user_status_change( + body={ + 'action': 'reactivate', + 'user_uri': user.url, + }, ) def publish_merged_user(user): - enqueue_task( - _publish_user_status_change.s( - body={ - 'action': 'merge', - 'user_uri': user.url, - 'merged_user_uri': user.merged_by.url, - }, - ) + assert user.merged_by, 'User received merge signal, but has no `merged_by` reference.' + _publish_user_status_change( + body={ + 'action': 'merge', + 'user_uri': user.url, + 'merged_user_uri': user.merged_by.url, + }, ) -@celery_app.task() def _publish_user_status_change(body: dict): with celery_app.producer_pool.acquire() as producer: producer.publish( diff --git a/osf/models/user.py b/osf/models/user.py index 3d133abc644..f1e6576c155 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -750,10 +750,10 @@ def merge_user(self, user): for key, value in user.mailchimp_mailing_lists.items(): # subscribe to each list if either user was subscribed subscription = value or self.mailchimp_mailing_lists.get(key) - signals.user_update_mailchimp_subscription.send(self, list_name=key, subscription=subscription) + signals.user_merged.send(self, list_name=key, subscription=subscription) # clear subscriptions for merged user - signals.user_update_mailchimp_subscription.send(user, list_name=key, subscription=False) + signals.user_merged.send(user, list_name=key, subscription=False) for target_id, timestamp in user.comments_viewed_timestamp.items(): if not self.comments_viewed_timestamp.get(target_id): @@ -872,8 +872,9 @@ def merge_user(self, user): user.merged_by = self user.save() - signals.user_account_merged.send(user) - signals.user_account_deactivated.send(self) + from osf.external.messages.celery_publishers import publish_deactivated_user, publish_merged_user + publish_deactivated_user(self) + publish_merged_user(self) def _merge_users_preprints(self, user): """ @@ -988,8 +989,6 @@ def deactivate_account(self): # Call to `unsubscribe` above saves, and can lead to stale data self.reload() self.is_disabled = True - self.save() - signals.user_account_deactivated.send(self) # we must call both methods to ensure the current session is cleared and all existing # sessions are revoked. @@ -997,6 +996,8 @@ def deactivate_account(self): if isinstance(req, FlaskRequest): logout() remove_sessions_for_user(self) + from osf.external.messages.celery_publishers import publish_deactivated_user + publish_deactivated_user(self) def reactivate_account(self): """ @@ -1006,7 +1007,8 @@ def reactivate_account(self): self.requested_deactivation = False from website.mailchimp_utils import subscribe_on_confirm subscribe_on_confirm(self) - signals.user_account_reactivated.send(self) + from osf.external.messages.celery_publishers import publish_reactivate_user + publish_reactivate_user(self) def update_is_active(self): """Update ``is_active`` to be consistent with the fields that diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index a2b1912f548..2c4d961f77b 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -17,7 +17,7 @@ from importlib import import_module from framework.auth.exceptions import ExpiredTokenError, InvalidTokenError, ChangePasswordError -from framework.auth.signals import user_account_merged +from framework.auth.signals import user_merged from framework.analytics import get_total_activity_count from framework.exceptions import PermissionsError from framework.celery_tasks import handlers @@ -1507,7 +1507,7 @@ def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe) with capture_signals() as mock_signals: merge_dupe() - assert mock_signals.signals_sent() == set([user_account_merged]) + assert mock_signals.signals_sent() == set([user_merged]) @pytest.mark.enable_enqueue_task @mock.patch('website.mailchimp_utils.get_mailchimp_api') diff --git a/website/profile/views.py b/website/profile/views.py index 892cf4cbadd..18e0bd655b0 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -16,12 +16,8 @@ from framework.auth.decorators import must_be_confirmed from framework.auth.exceptions import ChangePasswordError from framework.auth.views import send_confirm_email -from framework.auth.signals import ( - user_account_merged, - user_account_deactivated, - user_account_reactivated, - user_update_mailchimp_subscription -) +from framework.auth.signals import user_merged + from framework.exceptions import HTTPError, PermissionsError from framework.flask import redirect # VOL-aware redirect from framework.status import push_status_message @@ -41,11 +37,6 @@ from website.util import api_v2_url, web_url_for, paths from website.util.sanitize import escape_html from addons.base import utils as addon_utils -from osf.external.messages.celery_publishers import ( - publish_reactivate_user, - publish_deactivated_user, - publish_merged_user -) from api.waffle.utils import storage_i18n_flag_active @@ -515,7 +506,8 @@ def user_choose_mailing_lists(auth, **kwargs): all_mailing_lists.update(user.osf_mailing_lists) return {'message': 'Successfully updated mailing lists', 'result': all_mailing_lists}, 200 -@user_update_mailchimp_subscription.connect + +@user_merged.connect def update_mailchimp_subscription(user, list_name, subscription): """ Update mailing list subscription in mailchimp. @@ -536,24 +528,6 @@ def update_mailchimp_subscription(user, list_name, subscription): pass -@user_account_merged.connect -def send_account_merged_message(user): - """ Sends a message using Celery messaging to alert other services that an osf.io user has been merged.""" - publish_merged_user(user) - - -@user_account_deactivated.connect -def send_account_deactivation_message(user): - """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" - publish_deactivated_user(user) - - -@user_account_reactivated.connect -def send_account_reactivation_message(user): - """ Sends a message using Celery messaging to alert other services that an osf.io user has been reactivated.""" - publish_reactivate_user(user) - - def mailchimp_get_endpoint(**kwargs): """Endpoint that the mailchimp webhook hits to check that the OSF is responding""" return {}, http_status.HTTP_200_OK diff --git a/website/signals.py b/website/signals.py index c1b8660dcd4..2ae3bb4efd4 100644 --- a/website/signals.py +++ b/website/signals.py @@ -18,9 +18,7 @@ auth.user_confirmed, auth.user_email_removed, auth.user_registered, - auth.user_account_deactivated, - auth.user_account_reactivated, - auth.user_account_merged, + auth.user_merged, auth.unconfirmed_user_created, event.file_updated, conference.osf4m_user_created, From 378ea483bb8e1ed06fd86ebbbf9ba4372b52016a Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Mon, 18 Mar 2024 13:33:02 -0400 Subject: [PATCH 07/18] mock publisher for messaging --- conftest.py | 4 +-- framework/auth/signals.py | 4 ++- osf/external/messages/celery_publishers.py | 40 +++++++++++++--------- osf/models/user.py | 15 ++++---- osf_tests/test_user.py | 4 +-- website/profile/views.py | 33 ++++++++++++++++-- website/signals.py | 5 ++- 7 files changed, 71 insertions(+), 34 deletions(-) diff --git a/conftest.py b/conftest.py index 34c9bdfc036..edf96d46ba9 100644 --- a/conftest.py +++ b/conftest.py @@ -90,8 +90,8 @@ def fake(): 'mark': 'enable_search', 'replacement': mock.MagicMock() }, - 'website.search.elastic_search': { - 'mark': 'enable_search', + 'osf.external.messages.celery_publishers._publish_user_status_change': { + 'mark': 'enable_account_status_messaging', 'replacement': mock.MagicMock() } } diff --git a/framework/auth/signals.py b/framework/auth/signals.py index c538c0bc279..4d3dfffff56 100644 --- a/framework/auth/signals.py +++ b/framework/auth/signals.py @@ -6,7 +6,9 @@ user_registered = signals.signal('user-registered') user_confirmed = signals.signal('user-confirmed') user_email_removed = signals.signal('user-email-removed') -user_merged = signals.signal('user-account-merged') +user_account_merged = signals.signal('user-account-merged') +user_account_deactivated = signals.signal('user-account-deactivated') +user_account_reactivated = signals.signal('user-account-reactivated') unconfirmed_user_created = signals.signal('unconfirmed-user-created') user_update_mailchimp_subscription = signals.signal('user-update-mailchimp-subscription') diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index b203712302b..2e33324b2fd 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -1,36 +1,44 @@ from kombu import Exchange from framework.celery_tasks import app as celery_app +from framework.celery_tasks.handlers import enqueue_task def publish_deactivated_user(user): - _publish_user_status_change( - body={ - 'action': 'deactivate', - 'user_uri': user.url, - }, + enqueue_task( + _publish_user_status_change.s( + body={ + 'action': 'deactivate', + 'user_uri': user.url, + }, + ) ) def publish_reactivate_user(user): - _publish_user_status_change( - body={ - 'action': 'reactivate', - 'user_uri': user.url, - }, + enqueue_task( + _publish_user_status_change.s( + body={ + 'action': 'reactivate', + 'user_uri': user.url, + }, + ) ) def publish_merged_user(user): assert user.merged_by, 'User received merge signal, but has no `merged_by` reference.' - _publish_user_status_change( - body={ - 'action': 'merge', - 'user_uri': user.url, - 'merged_user_uri': user.merged_by.url, - }, + enqueue_task( + _publish_user_status_change.s( + body={ + 'action': 'merge', + 'user_uri': user.url, + 'merged_user_uri': user.merged_by.url, + }, + ) ) +@celery_app.task() def _publish_user_status_change(body: dict): with celery_app.producer_pool.acquire() as producer: producer.publish( diff --git a/osf/models/user.py b/osf/models/user.py index f1e6576c155..6ad8702716a 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -750,10 +750,10 @@ def merge_user(self, user): for key, value in user.mailchimp_mailing_lists.items(): # subscribe to each list if either user was subscribed subscription = value or self.mailchimp_mailing_lists.get(key) - signals.user_merged.send(self, list_name=key, subscription=subscription) + signals.user_update_mailchimp_subscription.send(self, list_name=key, subscription=subscription) # clear subscriptions for merged user - signals.user_merged.send(user, list_name=key, subscription=False) + signals.user_update_mailchimp_subscription.send(user, list_name=key, subscription=False) for target_id, timestamp in user.comments_viewed_timestamp.items(): if not self.comments_viewed_timestamp.get(target_id): @@ -872,9 +872,8 @@ def merge_user(self, user): user.merged_by = self user.save() - from osf.external.messages.celery_publishers import publish_deactivated_user, publish_merged_user - publish_deactivated_user(self) - publish_merged_user(self) + signals.user_account_merged.send(user) + signals.user_account_deactivated.send(self) def _merge_users_preprints(self, user): """ @@ -989,6 +988,7 @@ def deactivate_account(self): # Call to `unsubscribe` above saves, and can lead to stale data self.reload() self.is_disabled = True + signals.user_account_deactivated.send(self) # we must call both methods to ensure the current session is cleared and all existing # sessions are revoked. @@ -996,8 +996,6 @@ def deactivate_account(self): if isinstance(req, FlaskRequest): logout() remove_sessions_for_user(self) - from osf.external.messages.celery_publishers import publish_deactivated_user - publish_deactivated_user(self) def reactivate_account(self): """ @@ -1007,8 +1005,7 @@ def reactivate_account(self): self.requested_deactivation = False from website.mailchimp_utils import subscribe_on_confirm subscribe_on_confirm(self) - from osf.external.messages.celery_publishers import publish_reactivate_user - publish_reactivate_user(self) + signals.user_account_reactivated.send(self) def update_is_active(self): """Update ``is_active`` to be consistent with the fields that diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 2c4d961f77b..147460de051 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -17,7 +17,7 @@ from importlib import import_module from framework.auth.exceptions import ExpiredTokenError, InvalidTokenError, ChangePasswordError -from framework.auth.signals import user_merged +from framework.auth.signals import user_account_merged, user_account_deactivated, user_update_mailchimp_subscription from framework.analytics import get_total_activity_count from framework.exceptions import PermissionsError from framework.celery_tasks import handlers @@ -1507,7 +1507,7 @@ def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe) with capture_signals() as mock_signals: merge_dupe() - assert mock_signals.signals_sent() == set([user_merged]) + assert mock_signals.signals_sent() == set([user_account_merged, user_account_deactivated, user_update_mailchimp_subscription]) @pytest.mark.enable_enqueue_task @mock.patch('website.mailchimp_utils.get_mailchimp_api') diff --git a/website/profile/views.py b/website/profile/views.py index 18e0bd655b0..810ce90b21e 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -16,8 +16,12 @@ from framework.auth.decorators import must_be_confirmed from framework.auth.exceptions import ChangePasswordError from framework.auth.views import send_confirm_email -from framework.auth.signals import user_merged - +from framework.auth.signals import ( + user_account_merged, + user_account_deactivated, + user_account_reactivated, + user_update_mailchimp_subscription +) from framework.exceptions import HTTPError, PermissionsError from framework.flask import redirect # VOL-aware redirect from framework.status import push_status_message @@ -37,6 +41,11 @@ from website.util import api_v2_url, web_url_for, paths from website.util.sanitize import escape_html from addons.base import utils as addon_utils +from osf.external.messages.celery_publishers import ( + publish_reactivate_user, + publish_deactivated_user, + publish_merged_user +) from api.waffle.utils import storage_i18n_flag_active @@ -507,7 +516,7 @@ def user_choose_mailing_lists(auth, **kwargs): return {'message': 'Successfully updated mailing lists', 'result': all_mailing_lists}, 200 -@user_merged.connect +@user_update_mailchimp_subscription.connect def update_mailchimp_subscription(user, list_name, subscription): """ Update mailing list subscription in mailchimp. @@ -528,6 +537,24 @@ def update_mailchimp_subscription(user, list_name, subscription): pass +@user_account_merged.connect +def send_account_merged_message(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been merged.""" + publish_merged_user(user) + + +@user_account_deactivated.connect +def send_account_deactivation_message(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" + publish_deactivated_user(user) + + +@user_account_reactivated.connect +def send_account_reactivation_message(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been reactivated.""" + publish_reactivate_user(user) + + def mailchimp_get_endpoint(**kwargs): """Endpoint that the mailchimp webhook hits to check that the OSF is responding""" return {}, http_status.HTTP_200_OK diff --git a/website/signals.py b/website/signals.py index 2ae3bb4efd4..8cef25cbf29 100644 --- a/website/signals.py +++ b/website/signals.py @@ -18,7 +18,10 @@ auth.user_confirmed, auth.user_email_removed, auth.user_registered, - auth.user_merged, + auth.user_account_deactivated, + auth.user_account_reactivated, + auth.user_account_merged, + auth.user_update_mailchimp_subscription, auth.unconfirmed_user_created, event.file_updated, conference.osf4m_user_created, From 7875c8efba90f2fad54908c76e2f2dd555cff9ac Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 13:18:24 -0400 Subject: [PATCH 08/18] move mailchimp calls into signals --- framework/auth/signals.py | 1 - osf/models/user.py | 9 --------- osf_tests/test_user.py | 4 ++-- website/profile/views.py | 17 +++++++++++++++-- website/signals.py | 3 +-- 5 files changed, 18 insertions(+), 16 deletions(-) diff --git a/framework/auth/signals.py b/framework/auth/signals.py index 4d3dfffff56..15f279520e5 100644 --- a/framework/auth/signals.py +++ b/framework/auth/signals.py @@ -11,4 +11,3 @@ user_account_reactivated = signals.signal('user-account-reactivated') unconfirmed_user_created = signals.signal('unconfirmed-user-created') -user_update_mailchimp_subscription = signals.signal('user-update-mailchimp-subscription') diff --git a/osf/models/user.py b/osf/models/user.py index 6ad8702716a..df443f952fa 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -746,15 +746,6 @@ def merge_user(self, user): notifications_configured = user.notifications_configured.copy() notifications_configured.update(self.notifications_configured) self.notifications_configured = notifications_configured - if not website_settings.RUNNING_MIGRATION: - for key, value in user.mailchimp_mailing_lists.items(): - # subscribe to each list if either user was subscribed - subscription = value or self.mailchimp_mailing_lists.get(key) - signals.user_update_mailchimp_subscription.send(self, list_name=key, subscription=subscription) - - # clear subscriptions for merged user - signals.user_update_mailchimp_subscription.send(user, list_name=key, subscription=False) - for target_id, timestamp in user.comments_viewed_timestamp.items(): if not self.comments_viewed_timestamp.get(target_id): self.comments_viewed_timestamp[target_id] = timestamp diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index 147460de051..a304f3765ca 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -17,7 +17,7 @@ from importlib import import_module from framework.auth.exceptions import ExpiredTokenError, InvalidTokenError, ChangePasswordError -from framework.auth.signals import user_account_merged, user_account_deactivated, user_update_mailchimp_subscription +from framework.auth.signals import user_account_merged, user_account_deactivated from framework.analytics import get_total_activity_count from framework.exceptions import PermissionsError from framework.celery_tasks import handlers @@ -1507,7 +1507,7 @@ def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe) with capture_signals() as mock_signals: merge_dupe() - assert mock_signals.signals_sent() == set([user_account_merged, user_account_deactivated, user_update_mailchimp_subscription]) + assert mock_signals.signals_sent() == set([user_account_merged, user_account_deactivated]) @pytest.mark.enable_enqueue_task @mock.patch('website.mailchimp_utils.get_mailchimp_api') diff --git a/website/profile/views.py b/website/profile/views.py index 810ce90b21e..e126b637a5d 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -20,7 +20,6 @@ user_account_merged, user_account_deactivated, user_account_reactivated, - user_update_mailchimp_subscription ) from framework.exceptions import HTTPError, PermissionsError from framework.flask import redirect # VOL-aware redirect @@ -516,7 +515,6 @@ def user_choose_mailing_lists(auth, **kwargs): return {'message': 'Successfully updated mailing lists', 'result': all_mailing_lists}, 200 -@user_update_mailchimp_subscription.connect def update_mailchimp_subscription(user, list_name, subscription): """ Update mailing list subscription in mailchimp. @@ -543,12 +541,27 @@ def send_account_merged_message(user): publish_merged_user(user) +@user_account_merged.connect +def unsubscribe_merge_account_from_mailchimp(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" + for key, value in user.merged_by.mailchimp_mailing_lists.items(): + if value: + mailchimp_utils.subscribe_mailchimp(key, user._id, username=user.username) + + @user_account_deactivated.connect def send_account_deactivation_message(user): """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" publish_deactivated_user(user) +@user_account_deactivated.connect +def unsubscribe_deactivated_account_from_mailchimp(user): + """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" + for key, value in user.mailchimp_mailing_lists.items(): + mailchimp_utils.unsubscribe_mailchimp_async(key, user._id, username=user.username) + + @user_account_reactivated.connect def send_account_reactivation_message(user): """ Sends a message using Celery messaging to alert other services that an osf.io user has been reactivated.""" diff --git a/website/signals.py b/website/signals.py index 8cef25cbf29..ac78f2e463f 100644 --- a/website/signals.py +++ b/website/signals.py @@ -21,8 +21,7 @@ auth.user_account_deactivated, auth.user_account_reactivated, auth.user_account_merged, - auth.user_update_mailchimp_subscription, - auth.unconfirmed_user_created, +` auth.unconfirmed_user_created, event.file_updated, conference.osf4m_user_created, reviews.reviews_email From 96f153fbdd94fcd81120e1a9c27e354028944e55 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 13:21:44 -0400 Subject: [PATCH 09/18] refactor queue names/urls --- framework/celery_tasks/routers.py | 4 ++-- osf/external/messages/celery_publishers.py | 10 +++++----- website/settings/defaults.py | 4 ++-- website/signals.py | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/framework/celery_tasks/routers.py b/framework/celery_tasks/routers.py index 1b85855ab1b..10696c32655 100644 --- a/framework/celery_tasks/routers.py +++ b/framework/celery_tasks/routers.py @@ -14,8 +14,8 @@ def match_by_module(task_path): return CeleryConfig.task_high_queue if task_subpath in CeleryConfig.remote_computing_modules: return CeleryConfig.task_remote_computing_queue - if task_subpath in CeleryConfig.account_status_changes: - return CeleryConfig.account_status_changes + if task_subpath in CeleryConfig.task_account_status_changes_queue: + return CeleryConfig.task_account_status_changes_queue return CeleryConfig.task_default_queue diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index 2e33324b2fd..f9bbb1490e1 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -8,7 +8,7 @@ def publish_deactivated_user(user): _publish_user_status_change.s( body={ 'action': 'deactivate', - 'user_uri': user.url, + 'user_uri': user.get_semantic_iri(), }, ) ) @@ -19,7 +19,7 @@ def publish_reactivate_user(user): _publish_user_status_change.s( body={ 'action': 'reactivate', - 'user_uri': user.url, + 'user_uri': user.get_semantic_iri(), }, ) ) @@ -31,8 +31,8 @@ def publish_merged_user(user): _publish_user_status_change.s( body={ 'action': 'merge', - 'user_uri': user.url, - 'merged_user_uri': user.merged_by.url, + 'user_uri': user.get_semantic_iri(), + 'merged_user_uri': user.merged_by.get_semantic_iri(), }, ) ) @@ -43,6 +43,6 @@ def _publish_user_status_change(body: dict): with celery_app.producer_pool.acquire() as producer: producer.publish( body=body, - exchange=Exchange(celery_app.conf.account_status_changes), + exchange=Exchange(celery_app.conf.task_account_status_changes_queue), serializer='json' ) diff --git a/website/settings/defaults.py b/website/settings/defaults.py index 610fc219e3b..fc598bf794c 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -411,7 +411,7 @@ class CeleryConfig: task_med_queue = 'med' task_high_queue = 'high' task_remote_computing_queue = 'remote' - account_status_changes = 'account_status_changes' + task_account_status_changes_queue = 'account_status_changes' remote_computing_modules = { 'addons.boa.tasks.submit_to_boa', @@ -489,7 +489,7 @@ class CeleryConfig: routing_key=task_med_queue, consumer_arguments={'x-priority': 1}), Queue(task_high_queue, Exchange(task_high_queue), routing_key=task_high_queue, consumer_arguments={'x-priority': 10}), - Queue(account_status_changes, Exchange(account_status_changes), routing_key=account_status_changes) + Queue(task_account_status_changes_queue, Exchange(task_account_status_changes_queue), routing_key=task_account_status_changes_queue) ) task_default_exchange_type = 'direct' diff --git a/website/signals.py b/website/signals.py index ac78f2e463f..c1b8660dcd4 100644 --- a/website/signals.py +++ b/website/signals.py @@ -21,7 +21,7 @@ auth.user_account_deactivated, auth.user_account_reactivated, auth.user_account_merged, -` auth.unconfirmed_user_created, + auth.unconfirmed_user_created, event.file_updated, conference.osf4m_user_created, reviews.reviews_email From 3a63bfebf02731cfee684c8e8ba74b36b154b5f5 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 16:01:36 -0400 Subject: [PATCH 10/18] clean up and refactor test management command. --- .../{test_publish.py => publish_account_changes.py} | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) rename osf/management/commands/{test_publish.py => publish_account_changes.py} (81%) diff --git a/osf/management/commands/test_publish.py b/osf/management/commands/publish_account_changes.py similarity index 81% rename from osf/management/commands/test_publish.py rename to osf/management/commands/publish_account_changes.py index 33b9a82de86..994eef4ec94 100644 --- a/osf/management/commands/test_publish.py +++ b/osf/management/commands/publish_account_changes.py @@ -6,12 +6,18 @@ publish_merged_user, ) +actions_to_functions = { + 'deactivate': publish_deactivated_user, + 'reactivate': publish_reactivate_user, + 'merge': publish_merged_user, +} + class Command(BaseCommand): help = 'Sends a message to manage a user state, for test purposes only.' def add_arguments(self, parser): - parser.add_argument('user_guid', type=str, help='URI of the user to post.') + parser.add_argument('user_guid', type=str, help='use the guid of the user to post.') # Adding a new argument to specify the action to perform parser.add_argument( 'action', @@ -26,11 +32,6 @@ def handle(self, *args, **options): action = options['action'] # Using a mapping of action to function to simplify the control flow - actions_to_functions = { - 'deactivate': publish_deactivated_user, - 'reactivate': publish_reactivate_user, - 'merge': publish_merged_user, - } if action in actions_to_functions: actions_to_functions[action](user) # Call the appropriate function From fb472d4df7f9284a69d196b14354a48a79d81939 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 16:02:12 -0400 Subject: [PATCH 11/18] combine and designal mailchimp --- website/profile/views.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/website/profile/views.py b/website/profile/views.py index e126b637a5d..bf2a8c1f11a 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -542,11 +542,17 @@ def send_account_merged_message(user): @user_account_merged.connect -def unsubscribe_merge_account_from_mailchimp(user): - """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" - for key, value in user.merged_by.mailchimp_mailing_lists.items(): - if value: - mailchimp_utils.subscribe_mailchimp(key, user._id, username=user.username) +def unsubscribe_old_merged_account_from_mailchimp(user): + """ This is a merged account (an old account that was merged into an active one) so it needs to be unsubscribed + from mailchimp.""" + if not settings.RUNNING_MIGRATION: + for key, value in user.merged_by.mailchimp_mailing_lists.items(): + # subscribe to each list if either user was subscribed + subscription = value or user.mailchimp_mailing_lists.get(key) + update_mailchimp_subscription(user, list_name=key, subscription=subscription) + + # clear subscriptions for merged user + update_mailchimp_subscription(user.merged_by, list_name=key, subscription=False) @user_account_deactivated.connect @@ -555,13 +561,6 @@ def send_account_deactivation_message(user): publish_deactivated_user(user) -@user_account_deactivated.connect -def unsubscribe_deactivated_account_from_mailchimp(user): - """ Sends a message using Celery messaging to alert other services that an osf.io user has been deactivated.""" - for key, value in user.mailchimp_mailing_lists.items(): - mailchimp_utils.unsubscribe_mailchimp_async(key, user._id, username=user.username) - - @user_account_reactivated.connect def send_account_reactivation_message(user): """ Sends a message using Celery messaging to alert other services that an osf.io user has been reactivated.""" From fb10ff830ece3d3a19f74234c7aa485aea3e93dd Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 16:13:07 -0400 Subject: [PATCH 12/18] fix merge signals --- website/profile/views.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/website/profile/views.py b/website/profile/views.py index bf2a8c1f11a..933d16c647a 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -546,12 +546,9 @@ def unsubscribe_old_merged_account_from_mailchimp(user): """ This is a merged account (an old account that was merged into an active one) so it needs to be unsubscribed from mailchimp.""" if not settings.RUNNING_MIGRATION: - for key, value in user.merged_by.mailchimp_mailing_lists.items(): - # subscribe to each list if either user was subscribed - subscription = value or user.mailchimp_mailing_lists.get(key) - update_mailchimp_subscription(user, list_name=key, subscription=subscription) - - # clear subscriptions for merged user + for key, value in user.mailchimp_mailing_lists.items(): + if value: + update_mailchimp_subscription(user, list_name=key, subscription=key) update_mailchimp_subscription(user.merged_by, list_name=key, subscription=False) From 2c42e10edffdbb82636c6999b43790d8851aeeae Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 17:45:55 -0400 Subject: [PATCH 13/18] Remove vestigial RUNNING_MIGRATION setting value --- website/profile/views.py | 9 ++++----- website/search/search.py | 2 +- website/settings/defaults.py | 4 ---- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/website/profile/views.py b/website/profile/views.py index 933d16c647a..4f9bbd64fb7 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -545,11 +545,10 @@ def send_account_merged_message(user): def unsubscribe_old_merged_account_from_mailchimp(user): """ This is a merged account (an old account that was merged into an active one) so it needs to be unsubscribed from mailchimp.""" - if not settings.RUNNING_MIGRATION: - for key, value in user.mailchimp_mailing_lists.items(): - if value: - update_mailchimp_subscription(user, list_name=key, subscription=key) - update_mailchimp_subscription(user.merged_by, list_name=key, subscription=False) + for key, value in user.mailchimp_mailing_lists.items(): + if value: + update_mailchimp_subscription(user, list_name=key, subscription=key) + update_mailchimp_subscription(user.merged_by, list_name=key, subscription=False) @user_account_deactivated.connect diff --git a/website/search/search.py b/website/search/search.py index 2e5c54f5d01..9195f2ea4c2 100644 --- a/website/search/search.py +++ b/website/search/search.py @@ -14,7 +14,7 @@ def requires_search(func): def wrapped(*args, **kwargs): - if search_engine is not None and not settings.RUNNING_MIGRATION: + if search_engine is not None: return func(*args, **kwargs) return wrapped diff --git a/website/settings/defaults.py b/website/settings/defaults.py index fc598bf794c..882a5713592 100644 --- a/website/settings/defaults.py +++ b/website/settings/defaults.py @@ -767,10 +767,6 @@ class CeleryConfig: # Used for gathering meta information about the current build GITHUB_API_TOKEN = None -# switch for disabling things that shouldn't happen during -# the modm to django migration -RUNNING_MIGRATION = False - # External Identity Provider EXTERNAL_IDENTITY_PROFILE = { 'OrcidProfile': 'ORCID', From 7b0f611f451066cc0b9f613f8a1a7fb1c652fa57 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 17:48:41 -0400 Subject: [PATCH 14/18] don't send deactivate signal on merge --- osf/models/user.py | 1 - 1 file changed, 1 deletion(-) diff --git a/osf/models/user.py b/osf/models/user.py index df443f952fa..eb15a791330 100644 --- a/osf/models/user.py +++ b/osf/models/user.py @@ -864,7 +864,6 @@ def merge_user(self, user): user.save() signals.user_account_merged.send(user) - signals.user_account_deactivated.send(self) def _merge_users_preprints(self, user): """ From b4a6403512444509ff513221695a44d65080aebb Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 19 Mar 2024 18:26:45 -0400 Subject: [PATCH 15/18] fix merge user mailchimp subscriptions --- osf/external/messages/celery_publishers.py | 4 ++-- osf_tests/test_user.py | 4 ++-- website/profile/views.py | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index f9bbb1490e1..430fee84798 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -31,8 +31,8 @@ def publish_merged_user(user): _publish_user_status_change.s( body={ 'action': 'merge', - 'user_uri': user.get_semantic_iri(), - 'merged_user_uri': user.merged_by.get_semantic_iri(), + 'into_user_uri': user.get_semantic_iri(), + 'from_user_uri': user.merged_by.get_semantic_iri(), }, ) ) diff --git a/osf_tests/test_user.py b/osf_tests/test_user.py index a304f3765ca..a2b1912f548 100644 --- a/osf_tests/test_user.py +++ b/osf_tests/test_user.py @@ -17,7 +17,7 @@ from importlib import import_module from framework.auth.exceptions import ExpiredTokenError, InvalidTokenError, ChangePasswordError -from framework.auth.signals import user_account_merged, user_account_deactivated +from framework.auth.signals import user_account_merged from framework.analytics import get_total_activity_count from framework.exceptions import PermissionsError from framework.celery_tasks import handlers @@ -1507,7 +1507,7 @@ def test_send_user_merged_signal(self, mock_get_mailchimp_api, dupe, merge_dupe) with capture_signals() as mock_signals: merge_dupe() - assert mock_signals.signals_sent() == set([user_account_merged, user_account_deactivated]) + assert mock_signals.signals_sent() == set([user_account_merged]) @pytest.mark.enable_enqueue_task @mock.patch('website.mailchimp_utils.get_mailchimp_api') diff --git a/website/profile/views.py b/website/profile/views.py index 4f9bbd64fb7..27498860079 100644 --- a/website/profile/views.py +++ b/website/profile/views.py @@ -546,9 +546,9 @@ def unsubscribe_old_merged_account_from_mailchimp(user): """ This is a merged account (an old account that was merged into an active one) so it needs to be unsubscribed from mailchimp.""" for key, value in user.mailchimp_mailing_lists.items(): - if value: - update_mailchimp_subscription(user, list_name=key, subscription=key) - update_mailchimp_subscription(user.merged_by, list_name=key, subscription=False) + subscription = value or user.merged_by.mailchimp_mailing_lists.get(key) + update_mailchimp_subscription(user.merged_by, list_name=key, subscription=subscription) + update_mailchimp_subscription(user, list_name=key, subscription=False) @user_account_deactivated.connect From b8fe1b7de74c9585cda37144736e313228368e40 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Thu, 21 Mar 2024 10:31:12 -0400 Subject: [PATCH 16/18] clean up merge naming and user loading --- osf/external/messages/celery_publishers.py | 4 ++-- osf/management/commands/publish_account_changes.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index 430fee84798..6b3323cb6c1 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -31,8 +31,8 @@ def publish_merged_user(user): _publish_user_status_change.s( body={ 'action': 'merge', - 'into_user_uri': user.get_semantic_iri(), - 'from_user_uri': user.merged_by.get_semantic_iri(), + 'into_user_uri': user.merged_by.get_semantic_iri(), + 'from_user_uri': user.get_semantic_iri(), }, ) ) diff --git a/osf/management/commands/publish_account_changes.py b/osf/management/commands/publish_account_changes.py index 994eef4ec94..577558e8ae6 100644 --- a/osf/management/commands/publish_account_changes.py +++ b/osf/management/commands/publish_account_changes.py @@ -28,7 +28,7 @@ def add_arguments(self, parser): def handle(self, *args, **options): user_guid = options['user_guid'] - user = OSFUser.objects.get(guids___id=user_guid) + user = OSFUser.load(user_guid) action = options['action'] # Using a mapping of action to function to simplify the control flow From dcbc57ae151083e6355a43574498240447c51e53 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Thu, 28 Mar 2024 15:45:19 -0400 Subject: [PATCH 17/18] add waffling for celery messaging queue, add tests --- osf/external/messages/celery_publishers.py | 22 ++++---- tests/test_utils.py | 66 ++++++++++++++++++++++ 2 files changed, 76 insertions(+), 12 deletions(-) diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index 6b3323cb6c1..05db32d6f07 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -1,44 +1,42 @@ +import waffle from kombu import Exchange from framework.celery_tasks import app as celery_app -from framework.celery_tasks.handlers import enqueue_task +from website import settings +from osf import features def publish_deactivated_user(user): - enqueue_task( - _publish_user_status_change.s( + if settings.USE_CELERY and waffle.switch_is_active(features.ENABLE_GV): + _publish_user_status_change( body={ 'action': 'deactivate', 'user_uri': user.get_semantic_iri(), - }, + } ) - ) def publish_reactivate_user(user): - enqueue_task( - _publish_user_status_change.s( + if settings.USE_CELERY and waffle.switch_is_active(features.ENABLE_GV): + _publish_user_status_change( body={ 'action': 'reactivate', 'user_uri': user.get_semantic_iri(), }, ) - ) def publish_merged_user(user): assert user.merged_by, 'User received merge signal, but has no `merged_by` reference.' - enqueue_task( - _publish_user_status_change.s( + if settings.USE_CELERY and waffle.switch_is_active(features.ENABLE_GV): + _publish_user_status_change( body={ 'action': 'merge', 'into_user_uri': user.merged_by.get_semantic_iri(), 'from_user_uri': user.get_semantic_iri(), }, ) - ) -@celery_app.task() def _publish_user_status_change(body: dict): with celery_app.producer_pool.acquire() as producer: producer.publish( diff --git a/tests/test_utils.py b/tests/test_utils.py index 60528d2176e..239319995d1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -13,6 +13,11 @@ from tests.base import OsfTestCase, DbTestCase from osf_tests.factories import RegistrationFactory, UserFactory, fake_email +from framework.auth.signals import ( + user_account_deactivated, + user_account_reactivated, + user_account_merged +) from framework.auth.utils import generate_csl_given_name from framework.routing import Rule, json_renderer @@ -453,3 +458,64 @@ def test_build_create_user_time_conflict(self): user_one_create = UserFactory() user_two_create = UserFactory() assert user_one_create.username != user_two_create.username + + +# Add this import at the top of your file +from django.dispatch import receiver + + +@pytest.mark.django_db +class TestUserSignals: + + @pytest.fixture + def user(self, db): + return UserFactory() + + @pytest.fixture + def old_user(self, db): + return UserFactory() + + @pytest.fixture + def deactivated_user(self, db): + user = UserFactory() + user.deactivate_account() + return user + + @mock.patch('osf.external.messages.celery_publishers.publish_deactivated_user') + def test_user_account_deactivated_signal(self, mock_publish_deactivated_user, user): + # Connect a mock receiver to the signal for testing + @receiver(user_account_deactivated) + def mock_receiver(user, **kwargs): + return mock_publish_deactivated_user(user) + + # Trigger the signal + user.deactivate_account() + + # Verify that the mock receiver was called + mock_publish_deactivated_user.assert_called_once_with(user) + + @mock.patch('osf.external.messages.celery_publishers.publish_merged_user') + def test_user_account_merged_signal(self, mock_publish_merged_user, user, old_user): + # Connect a mock receiver to the signal for testing + @receiver(user_account_merged) + def mock_receiver(user, **kwargs): + return mock_publish_merged_user(user) + + # Trigger the signal + user.merge_user(old_user) + + # Verify that the mock receiver was called + mock_publish_merged_user.assert_called_once_with(old_user) + + @mock.patch('osf.external.messages.celery_publishers.publish_reactivate_user') + def test_user_account_deactivate_signal(self, mock_publish_reactivate_user, deactivated_user): + # Connect a mock receiver to the signal for testing + @receiver(user_account_reactivated) + def mock_receiver(user, **kwargs): + return mock_publish_reactivate_user(user) + + # Trigger the signal + deactivated_user.reactivate_account() + + # Verify that the mock receiver was called + mock_publish_reactivate_user.assert_called_once_with(deactivated_user) From 92502bf9c993b9c7fefe3ccdae09594e8232c239 Mon Sep 17 00:00:00 2001 From: John Tordoff <> Date: Tue, 2 Apr 2024 09:38:14 -0400 Subject: [PATCH 18/18] improve tests and mocks --- osf/external/messages/celery_publishers.py | 55 +++++++++-------- tests/test_utils.py | 69 ++++++++++++++++++++-- 2 files changed, 92 insertions(+), 32 deletions(-) diff --git a/osf/external/messages/celery_publishers.py b/osf/external/messages/celery_publishers.py index 05db32d6f07..3c3c90f9d37 100644 --- a/osf/external/messages/celery_publishers.py +++ b/osf/external/messages/celery_publishers.py @@ -3,44 +3,43 @@ from framework.celery_tasks import app as celery_app from website import settings from osf import features +from osf.utils.requests import get_current_request def publish_deactivated_user(user): - if settings.USE_CELERY and waffle.switch_is_active(features.ENABLE_GV): - _publish_user_status_change( - body={ - 'action': 'deactivate', - 'user_uri': user.get_semantic_iri(), - } - ) + _publish_user_status_change( + body={ + 'action': 'deactivate', + 'user_uri': user.get_semantic_iri(), + } + ) def publish_reactivate_user(user): - if settings.USE_CELERY and waffle.switch_is_active(features.ENABLE_GV): - _publish_user_status_change( - body={ - 'action': 'reactivate', - 'user_uri': user.get_semantic_iri(), - }, - ) + _publish_user_status_change( + body={ + 'action': 'reactivate', + 'user_uri': user.get_semantic_iri(), + }, + ) def publish_merged_user(user): assert user.merged_by, 'User received merge signal, but has no `merged_by` reference.' - if settings.USE_CELERY and waffle.switch_is_active(features.ENABLE_GV): - _publish_user_status_change( - body={ - 'action': 'merge', - 'into_user_uri': user.merged_by.get_semantic_iri(), - 'from_user_uri': user.get_semantic_iri(), - }, - ) + _publish_user_status_change( + body={ + 'action': 'merge', + 'into_user_uri': user.merged_by.get_semantic_iri(), + 'from_user_uri': user.get_semantic_iri(), + }, + ) def _publish_user_status_change(body: dict): - with celery_app.producer_pool.acquire() as producer: - producer.publish( - body=body, - exchange=Exchange(celery_app.conf.task_account_status_changes_queue), - serializer='json' - ) + if settings.USE_CELERY and waffle.flag_is_active(get_current_request(), features.ENABLE_GV): + with celery_app.producer_pool.acquire() as producer: + producer.publish( + body=body, + exchange=Exchange(celery_app.conf.task_account_status_changes_queue), + serializer='json' + ) diff --git a/tests/test_utils.py b/tests/test_utils.py index 239319995d1..689f937e7ce 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -6,6 +6,8 @@ import time import unittest from django.utils import timezone +from django.dispatch import receiver + from flask import Flask from nose.tools import * # noqa (PEP8 asserts) @@ -24,6 +26,7 @@ from framework.utils import secure_filename, throttle_period_expired from api.base.utils import waterbutler_api_url_for from osf.utils.functional import rapply +from waffle.testutils import override_flag from website.routes import process_rules, OsfWebRenderer from website import settings from website.util import paths @@ -31,6 +34,10 @@ from website.project import utils as project_utils from website.profile import utils as profile_utils +from osf import features + +from kombu import Exchange + try: import magic # noqa LIBMAGIC_AVAILABLE = True @@ -460,10 +467,6 @@ def test_build_create_user_time_conflict(self): assert user_one_create.username != user_two_create.username -# Add this import at the top of your file -from django.dispatch import receiver - - @pytest.mark.django_db class TestUserSignals: @@ -481,6 +484,10 @@ def deactivated_user(self, db): user.deactivate_account() return user + @pytest.fixture + def account_status_changes_exchange(self): + return Exchange('account_status_changes') + @mock.patch('osf.external.messages.celery_publishers.publish_deactivated_user') def test_user_account_deactivated_signal(self, mock_publish_deactivated_user, user): # Connect a mock receiver to the signal for testing @@ -519,3 +526,57 @@ def mock_receiver(user, **kwargs): # Verify that the mock receiver was called mock_publish_reactivate_user.assert_called_once_with(deactivated_user) + + @pytest.mark.enable_account_status_messaging + @mock.patch('osf.external.messages.celery_publishers.celery_app.producer_pool.acquire') + def test_publish_body_on_deactivation(self, mock_publish_user_status_change, user, account_status_changes_exchange): + with mock.patch.object(settings, 'USE_CELERY', True): + with override_flag(features.ENABLE_GV, active=True): + user.deactivate_account() + + mock_publish_user_status_change().__enter__().publish.assert_called_once_with( + body={'action': 'deactivate', 'user_uri': f'http://localhost:5000/{user._id}'}, + exchange=account_status_changes_exchange, + serializer='json', + ) + + @pytest.mark.enable_account_status_messaging + @mock.patch('osf.external.messages.celery_publishers.celery_app.producer_pool.acquire') + def test_publish_body_on_reactivation( + self, + mock_publish_user_status_change, + deactivated_user, + account_status_changes_exchange + ): + with mock.patch.object(settings, 'USE_CELERY', True): + with override_flag(features.ENABLE_GV, active=True): + deactivated_user.reactivate_account() + + mock_publish_user_status_change().__enter__().publish.assert_called_once_with( + body={'action': 'reactivate', 'user_uri': f'http://localhost:5000/{deactivated_user._id}'}, + exchange=account_status_changes_exchange, + serializer='json', + ) + + @pytest.mark.enable_account_status_messaging + @mock.patch('osf.external.messages.celery_publishers.celery_app.producer_pool.acquire') + def test_publish_body_on_merger( + self, + mock_publish_user_status_change, + user, + old_user, + account_status_changes_exchange + ): + with mock.patch.object(settings, 'USE_CELERY', True): + with override_flag(features.ENABLE_GV, active=True): + user.merge_user(old_user) + + mock_publish_user_status_change().__enter__().publish.assert_called_once_with( + body={ + 'action': 'merge', + 'into_user_uri': f'http://localhost:5000/{user._id}', + 'from_user_uri': f'http://localhost:5000/{old_user._id}' + }, + exchange=account_status_changes_exchange, + serializer='json', + ) \ No newline at end of file