add new notificationsubscription class to views

Johnetordoff · bodintsov · commit b2e5ae1438c7 · 2025-06-11T14:43:46.000+03:00
diff --git a/api/subscriptions/fields.py b/api/subscriptions/fields.py
@@ -0,0 +1,12 @@
+from rest_framework import serializers as ser
+from osf.models import NotificationSubscription
+
+class FrequencyField(ser.ChoiceField):
+    def __init__(self, **kwargs):
+        super().__init__(choices=['none', 'instantly', 'daily', 'weekly', 'monthly'], **kwargs)
+
+    def to_representation(self, obj: NotificationSubscription):
+        return obj.message_frequency
+
+    def to_internal_value(self, freq):
+        return super().to_internal_value(freq)
diff --git a/api/subscriptions/permissions.py b/api/subscriptions/permissions.py
@@ -1,13 +1,10 @@
 from rest_framework import permissions
 
-from osf.models.notifications import NotificationSubscriptionLegacy
+from osf.models.notification import NotificationSubscription
 
 
 class IsSubscriptionOwner(permissions.BasePermission):
 
     def has_object_permission(self, request, view, obj):
-        assert isinstance(obj, NotificationSubscriptionLegacy), f'obj must be a NotificationSubscriptionLegacy; got {obj}'
-        user_id = request.user.id
-        return obj.none.filter(id=user_id).exists() \
-               or obj.email_transactional.filter(id=user_id).exists() \
-               or obj.email_digest.filter(id=user_id).exists()
+        assert isinstance(obj, NotificationSubscription), f'obj must be a NotificationSubscription; got {obj}'
+        return obj.user == request.user
diff --git a/api/subscriptions/serializers.py b/api/subscriptions/serializers.py
@@ -1,58 +1,43 @@
+from django.contrib.contenttypes.models import ContentType
 from rest_framework import serializers as ser
-from rest_framework.exceptions import ValidationError
 from api.nodes.serializers import RegistrationProviderRelationshipField
 from api.collections_providers.fields import CollectionProviderRelationshipField
 from api.preprints.serializers import PreprintProviderRelationshipField
+from osf.models import Node
 from website.util import api_v2_url
 
 
-from api.base.serializers import JSONAPISerializer, LinksField
-
-NOTIFICATION_TYPES = {
-    'none': 'none',
-    'instant': 'email_transactional',
-    'daily': 'email_digest',
-}
-
-
-class FrequencyField(ser.Field):
-    def to_representation(self, obj):
-        user_id = self.context['request'].user.id
-        if obj.email_transactional.filter(id=user_id).exists():
-            return 'instant'
-        if obj.email_digest.filter(id=user_id).exists():
-            return 'daily'
-        return 'none'
-
-    def to_internal_value(self, frequency):
-        notification_type = NOTIFICATION_TYPES.get(frequency)
-        if notification_type:
-            return {'notification_type': notification_type}
-        raise ValidationError(f'Invalid frequency "{frequency}"')
+from api.base.serializers import JSONAPISerializer
+from .fields import FrequencyField
 
 class SubscriptionSerializer(JSONAPISerializer):
     filterable_fields = frozenset([
         'id',
         'event_name',
+        'frequency',
     ])
 
-    id = ser.CharField(source='_id', read_only=True)
+    id = ser.CharField(read_only=True)
     event_name = ser.CharField(read_only=True)
     frequency = FrequencyField(source='*', required=True)
-    links = LinksField({
-        'self': 'get_absolute_url',
-    })
 
     class Meta:
         type_ = 'subscription'
 
-    def get_absolute_url(self, obj):
-        return obj.absolute_api_v2_url
-
     def update(self, instance, validated_data):
         user = self.context['request'].user
-        notification_type = validated_data.get('notification_type')
-        instance.add_user_to_subscription(user, notification_type, save=True)
+        frequency = validated_data.get('frequency')
+
+        if frequency != 'none' and instance.content_type == ContentType.objects.get_for_model(Node):
+            node = Node.objects.get(
+                id=instance.id,
+                content_type=instance.content_type,
+            )
+            user_subs = node.parent_node.child_node_subscriptions
+            if node._id not in user_subs.setdefault(user._id, []):
+                user_subs[user._id].append(node._id)
+                node.parent_node.save()
+
         return instance
 
 
diff --git a/api/subscriptions/views.py b/api/subscriptions/views.py
@@ -1,8 +1,8 @@
+from pyasn1_modules.rfc5126 import ContentType
 from rest_framework import generics
 from rest_framework import permissions as drf_permissions
 from rest_framework.exceptions import NotFound
 from django.core.exceptions import ObjectDoesNotExist
-from django.db.models import Q
 
 from framework.auth.oauth_scopes import CoreScopes
 from api.base.views import JSONAPIBaseView
@@ -16,13 +16,12 @@
 )
 from api.subscriptions.permissions import IsSubscriptionOwner
 from osf.models import (
-    NotificationSubscription,
     CollectionProvider,
     PreprintProvider,
     RegistrationProvider,
     AbstractProvider,
 )
-from osf.models.notifications import NotificationSubscriptionLegacy
+from osf.models.notification import NotificationSubscription
 
 
 class SubscriptionList(JSONAPIBaseView, generics.ListAPIView, ListFilterMixin):
@@ -38,32 +37,20 @@ class SubscriptionList(JSONAPIBaseView, generics.ListAPIView, ListFilterMixin):
     required_read_scopes = [CoreScopes.SUBSCRIPTIONS_READ]
     required_write_scopes = [CoreScopes.NULL]
 
-    def get_default_queryset(self):
-        user = self.request.user
-        return NotificationSubscriptionLegacy.objects.filter(
-            Q(none=user) |
-            Q(email_digest=user) |
-            Q(
-                email_transactional=user,
-            ),
-        ).distinct()
-
     def get_queryset(self):
-        return self.get_queryset_from_request()
+        return NotificationSubscription.objects.filter(
+            user=self.request.user,
+        )
 
 
 class AbstractProviderSubscriptionList(SubscriptionList):
-    def get_default_queryset(self):
-        user = self.request.user
-        return NotificationSubscriptionLegacy.objects.filter(
-            provider___id=self.kwargs['provider_id'],
-            provider__type=self.provider_class._typedmodels_type,
-        ).filter(
-            Q(none=user) |
-            Q(email_digest=user) |
-            Q(email_transactional=user),
-        ).distinct()
-
+    def get_queryset(self):
+        provider = AbstractProvider.objects.get(_id=self.kwargs['provider_id'])
+        return NotificationSubscription.objects.filter(
+            object_id=provider,
+            provider__type=ContentType.objects.get_for_model(provider.__class__),
+            user=self.request.user,
+        )
 
 class SubscriptionDetail(JSONAPIBaseView, generics.RetrieveUpdateAPIView):
     view_name = 'notification-subscription-detail'
@@ -81,7 +68,7 @@ class SubscriptionDetail(JSONAPIBaseView, generics.RetrieveUpdateAPIView):
     def get_object(self):
         subscription_id = self.kwargs['subscription_id']
         try:
-            obj = NotificationSubscriptionLegacy.objects.get(_id=subscription_id)
+            obj = NotificationSubscription.objects.get(id=subscription_id)
         except ObjectDoesNotExist:
             raise NotFound
         self.check_object_permissions(self.request, obj)
@@ -101,33 +88,6 @@ class AbstractProviderSubscriptionDetail(SubscriptionDetail):
     required_write_scopes = [CoreScopes.SUBSCRIPTIONS_WRITE]
     provider_class = None
 
-    def __init__(self, *args, **kwargs):
-        assert issubclass(self.provider_class, AbstractProvider), 'Class must be subclass of AbstractProvider'
-        super().__init__(*args, **kwargs)
-
-    def get_object(self):
-        subscription_id = self.kwargs['subscription_id']
-        if self.kwargs.get('provider_id'):
-            provider = self.provider_class.objects.get(_id=self.kwargs.get('provider_id'))
-            try:
-                obj = NotificationSubscriptionLegacy.objects.get(
-                    _id=subscription_id,
-                    provider_id=provider.id,
-                )
-            except ObjectDoesNotExist:
-                raise NotFound
-        else:
-            try:
-                obj = NotificationSubscriptionLegacy.objects.get(
-                    _id=subscription_id,
-                    provider__type=self.provider_class._typedmodels_type,
-                )
-            except ObjectDoesNotExist:
-                raise NotFound
-        self.check_object_permissions(self.request, obj)
-        return obj
-
-
 class CollectionProviderSubscriptionDetail(AbstractProviderSubscriptionDetail):
     provider_class = CollectionProvider
     serializer_class = CollectionSubscriptionSerializer
diff --git a/api_tests/subscriptions/views/test_subscriptions_detail.py b/api_tests/subscriptions/views/test_subscriptions_detail.py
@@ -1,8 +1,10 @@
 import pytest
 
 from api.base.settings.defaults import API_BASE
-from osf_tests.factories import AuthUserFactory, NotificationSubscriptionLegacyFactory
-
+from osf_tests.factories import (
+    AuthUserFactory,
+    NotificationSubscriptionFactory
+)
 
 @pytest.mark.django_db
 class TestSubscriptionDetail:
@@ -16,18 +18,18 @@ def user_no_auth(self):
         return AuthUserFactory()
 
     @pytest.fixture()
-    def global_user_notification(self, user):
-        notification = NotificationSubscriptionLegacyFactory(_id=f'{user._id}_global', user=user, event_name='global')
-        notification.add_user_to_subscription(user, 'email_transactional')
-        return notification
+    def notification(self, user):
+        return NotificationSubscriptionFactory(
+            user=user,
+        )
 
     @pytest.fixture()
-    def url(self, global_user_notification):
-        return f'/{API_BASE}subscriptions/{global_user_notification._id}/'
+    def url(self, notification):
+        return f'/{API_BASE}subscriptions/{notification.id}/'
 
     @pytest.fixture()
     def url_invalid(self):
-        return '/{}subscriptions/{}/'.format(API_BASE, 'invalid-notification-id')
+        return f'/{API_BASE}subscriptions/invalid-notification-id/'
 
     @pytest.fixture()
     def payload(self):
@@ -51,56 +53,101 @@ def payload_invalid(self):
             }
         }
 
-    def test_subscription_detail(self, app, user, user_no_auth, global_user_notification, url, url_invalid, payload, payload_invalid):
-        # GET with valid notification_id
-        # Invalid user
-        res = app.get(url, auth=user_no_auth.auth, expect_errors=True)
+    def test_subscription_detail_invalid_user(
+            self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
+        res = app.get(
+            url,
+            auth=user_no_auth.auth,
+            expect_errors=True
+        )
         assert res.status_code == 403
-        # No user
-        res = app.get(url, expect_errors=True)
+
+    def test_subscription_detail_no_user(
+            self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
+        res = app.get(
+            url,
+            expect_errors=True
+        )
         assert res.status_code == 401
-        # Valid user
+
+    def test_subscription_detail_valid_user(
+            self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
+
         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 == str(notification.id)
 
-        # GET with invalid notification_id
-        # No user
+    def test_subscription_detail_invalid_notification_id_no_user(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.get(url_invalid, expect_errors=True)
         assert res.status_code == 404
-        # Existing user
-        res = app.get(url_invalid, auth=user.auth, expect_errors=True)
+
+    def test_subscription_detail_invalid_notification_id_existing_user(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
+        res = app.get(
+            url_invalid,
+            auth=user.auth,
+            expect_errors=True
+        )
         assert res.status_code == 404
 
-        # PATCH with valid notification_id and invalid data
-        # Invalid user
+    def test_subscription_detail_invalid_payload_403(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url, payload_invalid, auth=user_no_auth.auth, expect_errors=True)
         assert res.status_code == 403
-        # No user
+
+    def test_subscription_detail_invalid_payload_401(
+            self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url, payload_invalid, expect_errors=True)
         assert res.status_code == 401
-        # Valid user
-        res = app.patch_json_api(url, payload_invalid, auth=user.auth, expect_errors=True)
+
+    def test_subscription_detail_invalid_payload_400(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
+        res = app.patch_json_api(
+            url,
+            payload_invalid,
+            auth=user.auth,
+            expect_errors=True
+        )
         assert res.status_code == 400
-        assert res.json['errors'][0]['detail'] == 'Invalid frequency "invalid-frequency"'
+        assert res.json['errors'][0]['detail'] == '"invalid-frequency" is not a valid choice.'
 
-        # PATCH with invalid notification_id
-        # No user
+    def test_subscription_detail_patch_invalid_notification_id_no_user(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url_invalid, payload, expect_errors=True)
         assert res.status_code == 404
-        # Existing user
+
+    def test_subscription_detail_patch_invalid_notification_id_existing_user(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url_invalid, payload, auth=user.auth, expect_errors=True)
         assert res.status_code == 404
 
-        # PATCH with valid notification_id and valid data
-        # Invalid user
+    def test_subscription_detail_patch_invalid_user(
+            self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url, payload, auth=user_no_auth.auth, expect_errors=True)
         assert res.status_code == 403
-        # No user
+
+    def test_subscription_detail_patch_no_user(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url, payload, expect_errors=True)
         assert res.status_code == 401
-        # Valid user
+
+    def test_subscription_detail_patch(
+        self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid
+    ):
         res = app.patch_json_api(url, payload, auth=user.auth)
         assert res.status_code == 200
         assert res.json['data']['attributes']['frequency'] == 'none'
diff --git a/osf/models/notification.py b/osf/models/notification.py
@@ -296,6 +296,12 @@ def emit(self, user, subscribed_object=None, event_context=None):
                 event_context=event_context
             )
 
+    @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'})
+
+
 class Notification(models.Model):
     subscription = models.ForeignKey(
         NotificationSubscription,
diff --git a/osf_tests/factories.py b/osf_tests/factories.py
