From cb2dc70867a96c2d39175e09b1da64779e20f6ff Mon Sep 17 00:00:00 2001 From: Ostap Zherebetskyi Date: Tue, 30 Dec 2025 16:06:54 +0200 Subject: [PATCH] fix: update legacy notification migration logic --- .../commands/migrate_notifications.py | 39 ++--- osf/models/notification_type.py | 3 +- .../test_migrate_notifications.py | 141 +++++++++++------- 3 files changed, 112 insertions(+), 71 deletions(-) diff --git a/osf/management/commands/migrate_notifications.py b/osf/management/commands/migrate_notifications.py index 49866d3faa2..ff38ca0d927 100644 --- a/osf/management/commands/migrate_notifications.py +++ b/osf/management/commands/migrate_notifications.py @@ -25,26 +25,13 @@ EVENT_NAME_TO_NOTIFICATION_TYPE = { # Provider notifications - 'new_pending_withdraw_requests': NotificationType.Type.PROVIDER_NEW_PENDING_WITHDRAW_REQUESTS, - 'contributor_added_preprint': NotificationType.Type.PREPRINT_CONTRIBUTOR_ADDED_DEFAULT, - 'new_pending_submissions': NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS, - 'moderator_added': NotificationType.Type.PROVIDER_MODERATOR_ADDED, - 'reviews_submission_confirmation': NotificationType.Type.PROVIDER_REVIEWS_SUBMISSION_CONFIRMATION, - 'reviews_resubmission_confirmation': NotificationType.Type.PROVIDER_REVIEWS_RESUBMISSION_CONFIRMATION, - 'confirm_email_moderation': NotificationType.Type.PROVIDER_CONFIRM_EMAIL_MODERATION, 'global_reviews': NotificationType.Type.REVIEWS_SUBMISSION_STATUS, # Node notifications 'file_updated': NotificationType.Type.NODE_FILE_UPDATED, - # Collection submissions - 'collection_submission_submitted': NotificationType.Type.COLLECTION_SUBMISSION_SUBMITTED, - 'collection_submission_accepted': NotificationType.Type.COLLECTION_SUBMISSION_ACCEPTED, - 'collection_submission_rejected': NotificationType.Type.COLLECTION_SUBMISSION_REJECTED, - 'collection_submission_removed_admin': NotificationType.Type.COLLECTION_SUBMISSION_REMOVED_ADMIN, - 'collection_submission_removed_moderator': NotificationType.Type.COLLECTION_SUBMISSION_REMOVED_MODERATOR, - 'collection_submission_removed_private': NotificationType.Type.COLLECTION_SUBMISSION_REMOVED_PRIVATE, - 'collection_submission_cancel': NotificationType.Type.COLLECTION_SUBMISSION_CANCEL, + # User notifications + 'global_file_updated': NotificationType.Type.USER_FILE_UPDATED, } @@ -110,11 +97,25 @@ def migrate_legacy_notification_subscriptions( ): logger.info('Starting legacy notification subscription migration...') - legacy_qs = NotificationSubscriptionLegacy.objects.filter(id__gte=start_id).order_by('id') - total = legacy_qs.count() - if total == 0: + legacy_qs = NotificationSubscriptionLegacy.objects.filter(id__gte=start_id, event_name__in=EVENT_NAME_TO_NOTIFICATION_TYPE.keys()).order_by('id') + legacy_qs_ids = legacy_qs.values_list('id', flat=True) + if legacy_qs_ids.count() != 0: + with connection.cursor() as cursor: + cursor.execute('SELECT COUNT(*) FROM osf_notificationsubscriptionlegacy_none where notificationsubscription_id IN %s', [tuple(legacy_qs_ids)]) + none_count = cursor.fetchone()[0] + cursor.execute('SELECT COUNT(*) FROM osf_notificationsubscriptionlegacy_email_digest where notificationsubscription_id IN %s', [tuple(legacy_qs_ids)]) + digest_count = cursor.fetchone()[0] + cursor.execute('SELECT COUNT(*) FROM osf_notificationsubscriptionlegacy_email_transactional where notificationsubscription_id IN %s', [tuple(legacy_qs_ids)]) + transactional_count = cursor.fetchone()[0] + + legacy_expanded_total = none_count + digest_count + transactional_count + else: + legacy_expanded_total = 0 + + if legacy_expanded_total == 0: logger.info('No legacy subscriptions to migrate.') return + logger.info(f"Total legacy subscriptions to process: {legacy_expanded_total}") notiftype_map = dict(NotificationType.objects.values_list('name', 'id')) existing_keys = build_existing_keys() @@ -128,7 +129,7 @@ def migrate_legacy_notification_subscriptions( for batch_range in tqdm(list(iter_batches(first_id, last_id, batch_size)), desc='Processing', unit='batch'): batch = list( NotificationSubscriptionLegacy.objects - .filter(id__range=batch_range) + .filter(id__range=batch_range, event_name__in=EVENT_NAME_TO_NOTIFICATION_TYPE.keys()) .order_by('id') .select_related('provider', 'node', 'user') ) diff --git a/osf/models/notification_type.py b/osf/models/notification_type.py index 32b5e7a2f29..ea41262a52a 100644 --- a/osf/models/notification_type.py +++ b/osf/models/notification_type.py @@ -277,7 +277,7 @@ def get_group_frequency_or_default(self, user, subscribed_object, content_type): NotificationType.Type.FOLDER_CREATED.value, ] - if self.name in _global_file_updated and subscribed_object != ContentType.objects.get_for_model(AbstractNode): + if self.name in _global_file_updated and content_type != ContentType.objects.get_for_model(AbstractNode): frequency_data = NotificationSubscription.objects.filter( user=user, content_type=content_type, @@ -289,7 +289,6 @@ def get_group_frequency_or_default(self, user, subscribed_object, content_type): elif self.name in _global_reviews: frequency_data = NotificationSubscription.objects.filter( user=user, - content_type=content_type, notification_type__name__in=_global_reviews, ).distinct('message_frequency').values_list('message_frequency', flat=True) if frequency_data.exists() and len(frequency_data) == 1: diff --git a/osf_tests/management_commands/test_migrate_notifications.py b/osf_tests/management_commands/test_migrate_notifications.py index 2578e1b9798..1ea906d0d17 100644 --- a/osf_tests/management_commands/test_migrate_notifications.py +++ b/osf_tests/management_commands/test_migrate_notifications.py @@ -55,31 +55,7 @@ def node(self): ).delete() return project - ALL_PROVIDER_EVENTS = [ - 'new_pending_withdraw_requests', - 'contributor_added_preprint', - 'new_pending_submissions', - 'moderator_added', - 'reviews_submission_confirmation', - 'reviews_resubmission_confirmation', - 'confirm_email_moderation', - ] - - ALL_NODE_EVENTS = [ - 'file_updated', - ] - - ALL_COLLECTION_EVENTS = [ - 'collection_submission_submitted', - 'collection_submission_accepted', - 'collection_submission_rejected', - 'collection_submission_removed_admin', - 'collection_submission_removed_moderator', - 'collection_submission_removed_private', - 'collection_submission_cancel', - ] - - ALL_EVENT_NAMES = ALL_PROVIDER_EVENTS + ALL_NODE_EVENTS + ALL_COLLECTION_EVENTS + ALL_EVENT_NAMES = ['global_reviews', 'global_file_updated', 'file_updated'] def create_legacy_sub(self, event_name, users=None, user=None, provider=None, node=None): """ @@ -118,12 +94,12 @@ def create_legacy_sub(self, event_name, users=None, user=None, provider=None, no return subscription_id def test_migrate_provider_subscription(self, users, provider, provider2): - self.create_legacy_sub(event_name='new_pending_submissions', users=users, provider=provider) - self.create_legacy_sub(event_name='new_pending_submissions', users=users, provider=provider2) - self.create_legacy_sub(event_name='new_pending_submissions', users=users, provider=RegistrationProvider.get_default()) + self.create_legacy_sub(event_name='global_reviews', users=users, provider=provider) + self.create_legacy_sub(event_name='global_reviews', users=users, provider=provider2) + self.create_legacy_sub(event_name='global_reviews', users=users, provider=RegistrationProvider.get_default()) migrate_legacy_notification_subscriptions() subs = NotificationSubscription.objects.filter( - notification_type__name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS + notification_type__name=NotificationType.Type.REVIEWS_SUBMISSION_STATUS ) assert subs.count() == 9 for obj in [provider, provider2, RegistrationProvider.get_default()]: @@ -158,21 +134,15 @@ def test_idempotent_migration(self, users, user, node, provider): ) def test_migrate_all_subscription_types(self, users, user, provider, provider2, node): - providers = [provider, provider2] - for event_name in self.ALL_EVENT_NAMES: - if event_name in self.ALL_PROVIDER_EVENTS: - self.create_legacy_sub(event_name=event_name, users=users, user=user, node=node, provider=provider) - self.create_legacy_sub(event_name=event_name, users=users, user=user, node=node, provider=provider2) - else: - self.create_legacy_sub(event_name=event_name, users=users, user=user, node=node) + self.create_legacy_sub(event_name='global_reviews', users=users, user=user) + self.create_legacy_sub(event_name='file_updated', users=users, node=node) + self.create_legacy_sub(event_name='global_file_updated', users=users, user=user) # Run migration the first time migrate_legacy_notification_subscriptions() subs = NotificationSubscription.objects.all() # Calculate expected total - expected_total = len(self.ALL_PROVIDER_EVENTS) * len(providers) \ - + len(self.ALL_NODE_EVENTS) \ - + len(self.ALL_COLLECTION_EVENTS) + expected_total = 9 # 3 event names x 3 users each assert subs.count() >= expected_total # Run migration again to test deduplication migrate_legacy_notification_subscriptions() @@ -185,14 +155,7 @@ def test_migrate_all_subscription_types(self, users, user, provider, provider2, notification_type__name=nt_name ) assert nt_objs.exists() - # Verify subscriptions belong to correct objects - for provider in providers: - content_type = ContentType.objects.get_for_model(provider.__class__) - assert NotificationSubscription.objects.filter( - notification_type=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance, - content_type=content_type, - object_id=provider.id - ).exists() + node_ct = ContentType.objects.get_for_model(node.__class__) assert NotificationSubscription.objects.filter( notification_type=NotificationType.Type.NODE_FILE_UPDATED.instance, @@ -202,7 +165,7 @@ def test_migrate_all_subscription_types(self, users, user, provider, provider2, def test_migrate_rolls_back_on_runtime_error(self, users, user, node, provider): user = AuthUserFactory() - self.create_legacy_sub(event_name='collection_submission_submitted', users=users, user=user, node=node, provider=provider) + self.create_legacy_sub(event_name='global_reviews', users=users, user=user, node=node, provider=provider) def failing_migration(): with transaction.atomic(): @@ -221,7 +184,7 @@ def test_migrate_skips_invalid_data(self, users, user, node, provider): def test_migrate_batch_with_valid_and_invalid(self, users, user, node, provider): # Valid subscription self.create_legacy_sub( - event_name='reviews_resubmission_confirmation', + event_name='global_reviews', users=users, user=user, node=node, @@ -238,7 +201,7 @@ def test_migrate_batch_with_valid_and_invalid(self, users, user, node, provider) ) migrate_legacy_notification_subscriptions() assert NotificationSubscription.objects.filter( - notification_type__name=NotificationType.Type.PROVIDER_REVIEWS_RESUBMISSION_CONFIRMATION + notification_type__name=NotificationType.Type.REVIEWS_SUBMISSION_STATUS ).count() == 3 def test_migrate_subscription_frequencies_none(self, user, django_db_blocker): @@ -310,3 +273,81 @@ def test_migrate_node_subscription_frequencies_daily(self, user, node, django_db ) assert subs.count() == 1 assert subs.get().message_frequency == 'daily' + + def test_node_subscription_copy_group_frequency(self, user, node, django_db_blocker): + self.create_legacy_sub( + event_name='file_updated', + users={'email_digest': user}, + node=node + ) + + migrate_legacy_notification_subscriptions() + + NotificationType.Type.FILE_UPDATED.instance.emit( + user=user, + subscribed_object=node, + event_context={ + 'user_fullname': user.fullname, + }, + is_digest=True, + ) + + nt = NotificationSubscription.objects.get( + user=user, + notification_type__name=NotificationType.Type.FILE_UPDATED, + content_type=ContentType.objects.get_for_model(node), + object_id=node.id, + ) + assert nt.message_frequency == 'daily' + + def test_user_subscription_copy_group_frequency(self, user, node, django_db_blocker): + self.create_legacy_sub( + event_name='global_file_updated', + users={'none': user}, + user=user + ) + + migrate_legacy_notification_subscriptions() + + NotificationType.Type.FILE_UPDATED.instance.emit( + user=user, + subscribed_object=user, + event_context={ + 'user_fullname': user.fullname, + }, + is_digest=True, + ) + + nt = NotificationSubscription.objects.get( + user=user, + notification_type__name=NotificationType.Type.FILE_UPDATED, + content_type=ContentType.objects.get_for_model(user), + object_id=user.id, + ) + assert nt.message_frequency == 'none' + + def test_provider_subscription_copy_group_frequency(self, user, node, provider): + self.create_legacy_sub( + event_name='global_reviews', + users={'none': user}, + user=user + ) + + migrate_legacy_notification_subscriptions() + + NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance.emit( + user=user, + subscribed_object=provider, + event_context={ + 'user_fullname': user.fullname, + }, + is_digest=True, + ) + + nt = NotificationSubscription.objects.get( + user=user, + notification_type__name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS, + content_type=ContentType.objects.get_for_model(provider, for_concrete_model=False), + object_id=provider.id, + ) + assert nt.message_frequency == 'none'