Merge pull request #11329 from Johnetordoff/fix/update-v2-subscriptions

[ENG-9005] Fix/update v2 subscriptions

Author: Johnetordoff

api/subscriptions/serializers.py

@@ -37,7 +37,11 @@ def get_absolute_url(self, obj):
         return obj.absolute_api_v2_url
 
     def update(self, instance, validated_data):
-        instance.message_frequency = validated_data.get['frequency']
+        freq = validated_data.get('message_frequency')
+        if freq is None:
+            freq = validated_data.get('frequency')
+        instance.message_frequency = freq
+        instance.save()
         return instance
 
 

api/subscriptions/views.py

@@ -1,4 +1,4 @@
-from django.db.models import Value, When, Case, F, Q, OuterRef, Subquery
+from django.db.models import Value, When, Case, Q, OuterRef, Subquery
 from django.db.models.fields import CharField, IntegerField
 from django.db.models.functions import Concat, Cast
 from django.contrib.contenttypes.models import ContentType
@@ -56,35 +56,38 @@ def get_queryset(self):
             id=Cast(OuterRef('object_id'), IntegerField()),
         ).values('guids___id')[:1]
 
-        return NotificationSubscription.objects.filter(user=self.request.user).annotate(
+        return NotificationSubscription.objects.filter(
+            notification_type__in=[
+                NotificationType.Type.USER_FILE_UPDATED.instance,
+                NotificationType.Type.NODE_FILE_UPDATED.instance,
+                NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
+            ],
+            user=self.request.user,
+        ).annotate(
             event_name=Case(
                 When(
-                    notification_type__name=NotificationType.Type.NODE_FILES_UPDATED.value,
+                    notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
                     then=Value('files_updated'),
                 ),
                 When(
-                    notification_type__name=NotificationType.Type.USER_FILE_UPDATED.value,
-                    then=Value('global_file_updated'),
+                    notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
+                    then=Value(f'{user_guid}_global_file_updated'),
                 ),
-                default=F('notification_type__name'),
-                output_field=CharField(),
             ),
             legacy_id=Case(
                 When(
-                    notification_type__name=NotificationType.Type.NODE_FILES_UPDATED.value,
+                    notification_type=NotificationType.Type.NODE_FILE_UPDATED.instance,
                     then=Concat(Subquery(node_subquery), Value('_file_updated')),
                 ),
                 When(
-                    notification_type__name=NotificationType.Type.USER_FILE_UPDATED.value,
-                    then=Value(f'{user_guid}_global'),
+                    notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
+                    then=Value(f'{user_guid}_global_file_updated'),
                 ),
                 When(
-                    Q(notification_type__name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.value) &
+                    Q(notification_type=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance) &
                     Q(content_type=provider_ct),
                     then=Concat(Subquery(provider_subquery), Value('_new_pending_submissions')),
                 ),
-                default=F('notification_type__name'),
-                output_field=CharField(),
             ),
         )
 
@@ -137,7 +140,11 @@ def get_object(self):
             obj_filter = Q(
                 object_id=getattr(subscription_obj, 'id', None),
                 content_type=ContentType.objects.get_for_model(subscription_obj.__class__),
-                notification_type__name__icontains=event,
+                notification_type__in=[
+                    NotificationType.Type.USER_FILE_UPDATED.instance,
+                    NotificationType.Type.NODE_FILE_UPDATED.instance,
+                    NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
+                ],
             )
         else:
             obj_filter = Q()
@@ -146,13 +153,13 @@ def get_object(self):
             obj = NotificationSubscription.objects.annotate(
                 legacy_id=Case(
                     When(
-                        notification_type__name=NotificationType.Type.NODE_FILES_UPDATED.value,
+                        notification_type__name=NotificationType.Type.NODE_FILE_UPDATED.value,
                         content_type=node_ct,
                         then=Concat(Subquery(node_subquery), Value('_file_updated')),
                     ),
                     When(
                         notification_type__name=NotificationType.Type.USER_FILE_UPDATED.value,
-                        then=Value(f'{user_guid}_global'),
+                        then=Value(f'{user_guid}_global_file_updated'),
                     ),
                     When(
                         notification_type__name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.value,

api_tests/subscriptions/views/test_subscriptions_detail.py

@@ -1,6 +1,8 @@
 import pytest
+from django.contrib.contenttypes.models import ContentType
 
 from api.base.settings.defaults import API_BASE
+from osf.models import NotificationType
 from osf_tests.factories import (
     AuthUserFactory,
     NotificationSubscriptionFactory
@@ -19,7 +21,12 @@ def user_no_auth(self):
 
     @pytest.fixture()
     def notification(self, user):
-        return NotificationSubscriptionFactory(user=user)
+        return NotificationSubscriptionFactory(
+            notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
+            object_id=user.id,
+            content_type_id=ContentType.objects.get_for_model(user).id,
+            user=user
+        )
 
     @pytest.fixture()
     def url(self, notification):
@@ -75,7 +82,7 @@ def test_subscription_detail_valid_user(
         res = app.get(url, auth=user.auth)
         notification_id = res.json['data']['id']
         assert res.status_code == 200
-        assert notification_id == f'{user._id}_global'
+        assert notification_id == f'{user._id}_global_file_updated'
 
     def test_subscription_detail_invalid_notification_id_no_user(
         self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid

api_tests/subscriptions/views/test_subscriptions_list.py

@@ -1,4 +1,5 @@
 import pytest
+from django.contrib.contenttypes.models import ContentType
 
 from api.base.settings.defaults import API_BASE
 from osf.models.notification_type import NotificationType
@@ -31,21 +32,26 @@ def node(self, user):
     def global_user_notification(self, user):
         return NotificationSubscriptionFactory(
             notification_type=NotificationType.Type.USER_FILE_UPDATED.instance,
+            object_id=user.id,
+            content_type_id=ContentType.objects.get_for_model(user).id,
             user=user,
         )
 
     @pytest.fixture()
     def file_updated_notification(self, node, user):
         return NotificationSubscriptionFactory(
-            notification_type=NotificationType.Type.NODE_FILES_UPDATED.instance,
-            subscribed_object=node,
+            notification_type=NotificationType.Type.NODE_FILE_UPDATED.instance,
+            object_id=node.id,
+            content_type_id=ContentType.objects.get_for_model(node).id,
             user=user,
         )
 
     @pytest.fixture()
     def provider_notification(self, provider, user):
         return NotificationSubscriptionFactory(
             notification_type=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance,
+            object_id=provider.id,
+            content_type_id=ContentType.objects.get_for_model(provider).id,
             subscribed_object=provider,
             user=user,
         )
@@ -60,16 +66,14 @@ def test_list_complete(
             user,
             provider,
             node,
-            global_user_notification,
-            provider_notification,
             file_updated_notification,
             url
     ):
         res = app.get(url, auth=user.auth)
         notification_ids = [item['id'] for item in res.json['data']]
         # There should only be 3 notifications: users' global, node's file updates and provider's preprint added.
         assert len(notification_ids) == 3
-        assert f'{user._id}_global' in notification_ids
+        assert f'{user._id}_global_file_updated' in notification_ids
         assert f'{provider._id}_new_pending_submissions' in notification_ids
         assert f'{node._id}_file_updated' in notification_ids
 
@@ -87,7 +91,7 @@ def test_cannot_post_patch_put_or_delete(self, app, url, user):
         assert put_res.status_code == 405
         assert delete_res.status_code == 405
 
-    def test_multiple_values_filter(self, app, url, global_user_notification, file_updated_notification, user):
+    def test_multiple_values_filter(self, app, url, file_updated_notification, user):
         res = app.get(url + '?filter[event_name]=comments,file_updated', auth=user.auth)
         assert len(res.json['data']) == 2
         for subscription in res.json['data']:

osf/models/notification_subscription.py

@@ -4,9 +4,10 @@
 from django.contrib.contenttypes.fields import GenericForeignKey
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ValidationError
-from osf.models.notification_type import get_default_frequency_choices
+from osf.models.notification_type import get_default_frequency_choices, NotificationType
 from osf.models.notification import Notification
 from api.base import settings
+from api.base.utils import absolute_reverse
 
 from .base import BaseModel
 
@@ -106,29 +107,24 @@ def emit(
 
     @property
     def absolute_api_v2_url(self):
-        from api.base.utils import absolute_reverse
-        return absolute_reverse('institutions:institution-detail', kwargs={'institution_id': self._id, 'version': 'v2'})
+        return absolute_reverse(
+            'subscriptions:notification-subscription-detail',
+            kwargs={
+                'subscription_id': self._id, 'version': 'v2'
+            }
+        )
 
     @property
     def _id(self):
         """
         Legacy subscription id for API compatibility.
-        Provider: <short_name>_<event>
-        User/global: <user_id>_global_<event>
-        Node/etc: <guid>_<event>
         """
-        # Safety checks
-        event = self.notification_type.name
-        ct = self.notification_type.object_content_type
-        match getattr(ct, 'model', None):
-            case 'preprintprovider' | 'collectionprovider' | 'registrationprovider':
-                # Providers: use subscribed_object._id (which is the provider short name, e.g. 'mindrxiv')
+        match  self.notification_type.name:
+            case NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.value:
                 return f'{self.subscribed_object._id}_new_pending_submissions'
-            case 'node' | 'collection' | 'preprint':
-                # Node-like objects: use object_id (guid)
-                return f'{self.subscribed_object._id}_{event}'
-            case 'osfuser' | 'user' | None:
-                # Global: <user_id>_global
-                return f'{self.user._id}_global'
+            case NotificationType.Type.USER_FILE_UPDATED.value:
+                return f'{self.user._id}_file_updated'
+            case NotificationType.Type.NODE_FILE_UPDATED.value:
+                return f'{self.subscribed_object._id}_file_updated'
             case _:
                 raise NotImplementedError()