diff --git a/api/subscriptions/views.py b/api/subscriptions/views.py index 953e2a66aec..641490a5786 100644 --- a/api/subscriptions/views.py +++ b/api/subscriptions/views.py @@ -1,13 +1,17 @@ +from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import PermissionDenied from django.db.models import Value, When, Case, 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 + 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, PermissionDenied from rest_framework.response import Response + +from framework import sentry from framework.auth.oauth_scopes import CoreScopes + from api.base.views import JSONAPIBaseView from api.base.filters import ListFilterMixin from api.base import permissions as base_permissions @@ -18,6 +22,7 @@ RegistrationSubscriptionSerializer, ) from api.subscriptions.permissions import IsSubscriptionOwner + from osf.models import ( CollectionProvider, PreprintProvider, @@ -25,6 +30,7 @@ AbstractProvider, AbstractNode, Guid, + OSFUser, ) from osf.models.notification_type import NotificationType from osf.models.notification_subscription import NotificationSubscription @@ -156,46 +162,91 @@ class SubscriptionDetail(JSONAPIBaseView, generics.RetrieveUpdateAPIView): def get_object(self): subscription_id = self.kwargs['subscription_id'] user_guid = self.request.user._id - - provider_ct = ContentType.objects.get(app_label='osf', model='abstractprovider') - node_ct = ContentType.objects.get(app_label='osf', model='abstractnode') + user_file_updated_nt = NotificationType.Type.USER_FILE_UPDATED.instance + reviews_submission_status_nt = NotificationType.Type.REVIEWS_SUBMISSION_STATUS.instance + node_file_updated_nt = NotificationType.Type.NODE_FILE_UPDATED.instance + user_ct = ContentType.objects.get_for_model(OSFUser) + node_ct = ContentType.objects.get_for_model(AbstractNode) + provider_ct = ContentType.objects.get_for_model(AbstractProvider) node_subquery = AbstractNode.objects.filter( id=Cast(OuterRef('object_id'), IntegerField()), ).values('guids___id')[:1] - try: - annotated_obj_qs = NotificationSubscription.objects.filter(user=self.request.user).annotate( - legacy_id=Case( - When( - notification_type__name=NotificationType.Type.NODE_FILE_UPDATED.value, - content_type=node_ct, - then=Concat(Subquery(node_subquery), Value('_files_updated')), - ), - When( - notification_type__name=NotificationType.Type.USER_FILE_UPDATED.value, - then=Value(f'{user_guid}_global_file_updated'), - ), - When( - notification_type__name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.value, - content_type=provider_ct, - then=Value(f'{user_guid}_global_reviews'), - ), - default=Value(f'{user_guid}_global'), - output_field=CharField(), + node_guid = 'n/a' + missing_subscription_created = None + annotated_obj_qs = NotificationSubscription.objects.filter(user=self.request.user).annotate( + legacy_id=Case( + When( + 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_file_updated'), + ), + When( + notification_type__name=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.value, + content_type=provider_ct, + then=Value(f'{user_guid}_global_reviews'), + ), + default=Value(f'{user_guid}_global'), + output_field=CharField(), + ), + ) + existing_subscriptions = annotated_obj_qs.filter(legacy_id=subscription_id) + if not existing_subscriptions.exists(): + # `global_file_updated` and `global_reviews` should exist by default for every user. + # If not found, create them with `none` frequency and `_is_digest=True` as default. + if subscription_id == f'{user_guid}_global_file_updated': + notification_type = user_file_updated_nt + content_type = user_ct + object_id = self.request.user.id + elif subscription_id == f'{user_guid}_global_reviews': + notification_type = reviews_submission_status_nt + content_type = user_ct + object_id = self.request.user.id + elif subscription_id.endswith('_global_file_updated') or subscription_id.endswith('_global_reviews'): + # Mismatched request user and subscription user + sentry.log_message(f'Permission denied: [user={user_guid}, legacy_id={subscription_id}]') + raise PermissionDenied + elif subscription_id.endswith('_file_updated'): + notification_type = node_file_updated_nt + content_type = node_ct + node_guid = subscription_id[:-len('_file_updated')] + node = AbstractNode.objects.filter(guids___id=node_guid, is_deleted=False, type='osf.node').first() + if not node: + # The node in the legacy subscription ID does not exist or is invalid + sentry.log_message(f'Node not found in legacy subscription ID: [user={user_guid}, legacy_id={subscription_id}]') + raise NotFound + if not node.is_contributor(self.request.user): + # The request user is not a contributor of the node + sentry.log_message(f'Permission denied: [user={user_guid}], node={node_guid}, legacy_id={subscription_id}]') + raise PermissionDenied + object_id = node.id + else: + sentry.log_message(f'Subscription not found: [user={user_guid}, legacy_id={subscription_id}]') + raise NotFound + missing_subscription_created = NotificationSubscription.objects.create( + notification_type=notification_type, + user=self.request.user, + content_type=content_type, + object_id=object_id, + _is_digest=True, + message_frequency='none', ) - obj = annotated_obj_qs.filter(legacy_id=subscription_id) - - except ObjectDoesNotExist: - raise NotFound + sentry.log_message(f'Missing default subscription has been created: [user={user_guid}], node={node_guid} type={notification_type}, legacy_id={subscription_id}]') - obj = obj.filter(user=self.request.user).first() - if not obj: - raise PermissionDenied + if missing_subscription_created: + subscription = missing_subscription_created + else: + subscription = existing_subscriptions.filter(user=self.request.user).order_by('id').last() + if not subscription: + raise PermissionDenied - self.check_object_permissions(self.request, obj) - return obj + self.check_object_permissions(self.request, subscription) + return subscription def update(self, request, *args, **kwargs): """ diff --git a/api_tests/subscriptions/views/test_subscriptions_detail.py b/api_tests/subscriptions/views/test_subscriptions_detail.py index f14ca4e2522..765de7c5c56 100644 --- a/api_tests/subscriptions/views/test_subscriptions_detail.py +++ b/api_tests/subscriptions/views/test_subscriptions_detail.py @@ -2,10 +2,11 @@ from django.contrib.contenttypes.models import ContentType from api.base.settings.defaults import API_BASE -from osf.models import NotificationType +from osf.models import NotificationType, OSFUser, AbstractNode from osf_tests.factories import ( AuthUserFactory, - NotificationSubscriptionFactory + NodeFactory, + NotificationSubscriptionFactory, ) @pytest.mark.django_db @@ -15,6 +16,14 @@ class TestSubscriptionDetail: def user(self): return AuthUserFactory() + @pytest.fixture() + def node(self, user): + return NodeFactory(creator=user) + + @pytest.fixture() + def node_without_permission(self): + return NodeFactory() + @pytest.fixture() def user_no_auth(self): return AuthUserFactory() @@ -24,14 +33,54 @@ def 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 + content_type_id=ContentType.objects.get_for_model(OSFUser).id, + user=user, + _is_digest=True, + message_frequency='daily', + ) + + @pytest.fixture() + def notification_user_global_reviews(self, user): + return NotificationSubscriptionFactory( + notification_type=NotificationType.Type.PROVIDER_NEW_PENDING_SUBMISSIONS.instance, + object_id=user.id, + content_type_id=ContentType.objects.get_for_model(OSFUser).id, + user=user, + _is_digest=True, + message_frequency='daily', + ) + + @pytest.fixture() + def notification_node_file_updated(self, node, user): + return NotificationSubscriptionFactory( + notification_type=NotificationType.Type.NODE_FILE_UPDATED.instance, + object_id=node.id, + content_type_id=ContentType.objects.get_for_model(AbstractNode).id, + user=user, + _is_digest=True, + message_frequency='daily', ) @pytest.fixture() def url(self, user): return f'/{API_BASE}subscriptions/{user._id}_global_file_updated/' + @pytest.fixture() + def url_user_global_reviews(self, user): + return f'/{API_BASE}subscriptions/{user._id}_global_reviews/' + + @pytest.fixture() + def url_node_file_updated(self, node): + return f'/{API_BASE}subscriptions/{node._id}_file_updated/' + + @pytest.fixture() + def url_node_file_updated_not_found(self): + return f'/{API_BASE}subscriptions/12345_file_updated/' + + @pytest.fixture() + def url_node_file_updated_without_permission(self, node_without_permission): + return f'/{API_BASE}subscriptions/{node_without_permission._id}_file_updated/' + @pytest.fixture() def url_invalid(self): return f'/{API_BASE}subscriptions/invalid-notification-id/' @@ -84,6 +133,20 @@ def test_subscription_detail_valid_user( assert res.status_code == 200 assert notification_id == f'{user._id}_global_file_updated' + def test_node_file_updated_subscription_detail_success(self, app, user, node, notification_node_file_updated, url_node_file_updated): + res = app.get(url_node_file_updated, auth=user.auth) + notification_id = res.json['data']['id'] + assert res.status_code == 200 + assert notification_id == f'{node._id}_file_updated' + + def test_node_file_updated_subscription_detail_not_found(self, app, user, node, notification_node_file_updated, url_node_file_updated_not_found): + res = app.get(url_node_file_updated_not_found, auth=user.auth, expect_errors=True) + assert res.status_code == 404 + + def test_node_file_updated_subscription_detail_no_permission(self, app, user, node, notification_node_file_updated, url_node_file_updated_without_permission): + res = app.get(url_node_file_updated_without_permission, auth=user.auth, expect_errors=True) + assert res.status_code == 403 + def test_subscription_detail_invalid_notification_id_no_user( self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid ): diff --git a/website/mailchimp_utils.py b/website/mailchimp_utils.py index 7e92a59d275..e3d7a546d0f 100644 --- a/website/mailchimp_utils.py +++ b/website/mailchimp_utils.py @@ -126,7 +126,10 @@ def subscribe_on_confirm(user): notification_type=NotificationType.Type.REVIEWS_SUBMISSION_STATUS.instance, content_type=ContentType.objects.get_for_model(user), object_id=user.id, - defaults={'message_frequency': 'instantly'}, + defaults={ + '_is_digest': True, + 'message_frequency': 'instantly', + }, ) NotificationSubscription.objects.get_or_create( @@ -134,5 +137,8 @@ def subscribe_on_confirm(user): notification_type=NotificationType.Type.USER_FILE_UPDATED.instance, content_type=ContentType.objects.get_for_model(user), object_id=user.id, - defaults={'message_frequency': 'instantly'}, + defaults={ + '_is_digest': True, + 'message_frequency': 'instantly', + }, )