diff --git a/api/subscriptions/views.py b/api/subscriptions/views.py index 953e2a66aec..dcbeb28d3fd 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,92 @@ 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) 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('_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.REVIEWS_SUBMISSION_STATUS.value, + content_type=user_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('_files_updated'): + notification_type = node_file_updated_nt + content_type = node_ct + node_guid = subscription_id[:-len('_files_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) + sentry.log_message(f'Missing default subscription has been created: [user={user_guid}], node={node_guid} type={notification_type}, legacy_id={subscription_id}]') - except ObjectDoesNotExist: - raise NotFound - - obj = obj.filter(user=self.request.user).first() - if not obj: + if missing_subscription_created: + # Note: must use `annotated_obj_qs` to insert `legacy_id` so that `SubscriptionSerializer` can build data + # properly; in addition, there should be only one result + subscription = annotated_obj_qs.get(legacy_id=subscription_id) + else: + subscription = existing_subscriptions.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..f13ab1c3777 100644 --- a/api_tests/subscriptions/views/test_subscriptions_detail.py +++ b/api_tests/subscriptions/views/test_subscriptions_detail.py @@ -1,11 +1,18 @@ import pytest + from django.contrib.contenttypes.models import ContentType from api.base.settings.defaults import API_BASE -from osf.models import NotificationType +from osf.models import ( + AbstractNode, + NotificationSubscription, + NotificationType, + OSFUser +) from osf_tests.factories import ( AuthUserFactory, - NotificationSubscriptionFactory + NodeFactory, + NotificationSubscriptionFactory, ) @pytest.mark.django_db @@ -16,22 +23,94 @@ def user(self): return AuthUserFactory() @pytest.fixture() - def user_no_auth(self): + def user_missing_subscriptions(self): + return AuthUserFactory() + + @pytest.fixture() + def user_no_permission(self): return AuthUserFactory() @pytest.fixture() - def notification(self, user): + def node(self, user): + return NodeFactory(creator=user) + + @pytest.fixture() + def node_missing_subscriptions(self, user_missing_subscriptions): + node = NodeFactory(creator=user_missing_subscriptions) + subscription = NotificationSubscription.objects.get( + user=user_missing_subscriptions, + notification_type__name=NotificationType.Type.NODE_FILE_UPDATED.value, + object_id=node.id, + content_type=ContentType.objects.get_for_model(AbstractNode) + ) + subscription.delete() + return node + + @pytest.fixture() + def notification_user_global_file_updated(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 url(self, user): + def notification_user_global_reviews(self, user): + return NotificationSubscriptionFactory( + notification_type=NotificationType.Type.REVIEWS_SUBMISSION_STATUS.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_user_global_file_updated(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_user_global_file_updated_missing(self, user_missing_subscriptions): + return f'/{API_BASE}subscriptions/{user_missing_subscriptions._id}_global_file_updated/' + + @pytest.fixture() + def url_user_global_reviews_missing(self, user_missing_subscriptions): + return f'/{API_BASE}subscriptions/{user_missing_subscriptions._id}_global_reviews/' + + @pytest.fixture() + def url_node_file_updated(self, node): + return f'/{API_BASE}subscriptions/{node._id}_files_updated/' + + @pytest.fixture() + def url_node_file_updated_not_found(self): + return f'/{API_BASE}subscriptions/12345_files_updated/' + + @pytest.fixture() + def url_node_file_updated_without_permission(self, node_without_permission): + return f'/{API_BASE}subscriptions/{node_without_permission._id}_files_updated/' + + @pytest.fixture() + def url_node_file_updated_missing(self, node_missing_subscriptions): + return f'/{API_BASE}subscriptions/{node_missing_subscriptions._id}_files_updated/' + @pytest.fixture() def url_invalid(self): return f'/{API_BASE}subscriptions/invalid-notification-id/' @@ -58,40 +137,162 @@ def payload_invalid(self): } } - def test_subscription_detail_invalid_user(self, app, user, user_no_auth, notification, url, payload): - res = app.get( - url, - auth=user_no_auth.auth, - expect_errors=True - ) + def test_user_global_subscription_detail_permission_denied( + self, + app, + user, + user_no_permission, + notification_user_global_file_updated, + notification_user_global_reviews, + url_user_global_file_updated, + url_user_global_reviews + ): + res = app.get(url_user_global_file_updated, auth=user_no_permission.auth, expect_errors=True) + assert res.status_code == 403 + res = app.get(url_user_global_reviews, auth=user_no_permission.auth, expect_errors=True) assert res.status_code == 403 - def test_subscription_detail_no_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + def test_user_global_subscription_detail_forbidden( + self, + app, + user, + user_no_permission, + notification_user_global_file_updated, + notification_user_global_reviews, + url_user_global_file_updated, + url_user_global_reviews ): - res = app.get( - url, - expect_errors=True - ) + res = app.get(url_user_global_file_updated, expect_errors=True) + assert res.status_code == 401 + res = app.get(url_user_global_reviews, expect_errors=True) assert res.status_code == 401 - def test_subscription_detail_valid_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + def test_user_global_subscription_detail_success( + self, + app, + user, + user_no_permission, + notification_user_global_file_updated, + notification_user_global_reviews, + url_user_global_file_updated, + url_user_global_reviews ): - - res = app.get(url, auth=user.auth) + res = app.get(url_user_global_file_updated, auth=user.auth) notification_id = res.json['data']['id'] assert res.status_code == 200 assert notification_id == f'{user._id}_global_file_updated' + res = app.get(url_user_global_reviews, auth=user.auth) + notification_id = res.json['data']['id'] + assert res.status_code == 200 + assert notification_id == f'{user._id}_global_reviews' + + def test_user_global_file_updated_subscription_detail_missing_and_created( + self, + app, + user_missing_subscriptions, + url_user_global_file_updated_missing, + ): + assert not NotificationSubscription.objects.filter( + user=user_missing_subscriptions, + notification_type__name=NotificationType.Type.USER_FILE_UPDATED.value, + object_id=user_missing_subscriptions.id, + content_type=ContentType.objects.get_for_model(OSFUser) + ).exists() + res = app.get(url_user_global_file_updated_missing, auth=user_missing_subscriptions.auth) + notification_id = res.json['data']['id'] + assert res.status_code == 200 + assert notification_id == f'{user_missing_subscriptions._id}_global_file_updated' + + def test_user_global_reviews_subscription_detail_missing_and_created( + self, + app, + user_missing_subscriptions, + url_user_global_reviews_missing, + ): + assert not NotificationSubscription.objects.filter( + user=user_missing_subscriptions, + notification_type__name=NotificationType.Type.REVIEWS_SUBMISSION_STATUS.value, + object_id=user_missing_subscriptions.id, + content_type=ContentType.objects.get_for_model(OSFUser) + ).exists() + res = app.get(url_user_global_reviews_missing, auth=user_missing_subscriptions.auth) + notification_id = res.json['data']['id'] + assert res.status_code == 200 + assert notification_id == f'{user_missing_subscriptions._id}_global_reviews' + + 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}_files_updated' + + def test_node_file_updated_subscription_detail_missing_and_created( + self, + app, + user_missing_subscriptions, + node_missing_subscriptions, + url_node_file_updated_missing, + ): + assert not NotificationSubscription.objects.filter( + user=user_missing_subscriptions, + notification_type__name=NotificationType.Type.NODE_FILE_UPDATED.value, + object_id=node_missing_subscriptions.id, + content_type=ContentType.objects.get_for_model(AbstractNode) + ).exists() + res = app.get(url_node_file_updated_missing, auth=user_missing_subscriptions.auth) + notification_id = res.json['data']['id'] + assert res.status_code == 200 + assert notification_id == f'{node_missing_subscriptions._id}_files_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_permission_denied( + self, + app, + user, + user_no_permission, + node, + notification_node_file_updated, + url_node_file_updated + ): + res = app.get(url_node_file_updated, auth=user_no_permission.auth, expect_errors=True) + assert res.status_code == 403 + + def test_node_file_updated_subscription_detail_forbidden( + self, + app, + user, + node, + notification_node_file_updated, + url_node_file_updated + ): + res = app.get(url_node_file_updated, expect_errors=True) + assert res.status_code == 401 def test_subscription_detail_invalid_notification_id_no_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): res = app.get(url_invalid, expect_errors=True) assert res.status_code == 404 def test_subscription_detail_invalid_notification_id_existing_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): res = app.get( url_invalid, @@ -101,22 +302,22 @@ def test_subscription_detail_invalid_notification_id_existing_user( assert res.status_code == 404 def test_subscription_detail_invalid_payload_403( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): - res = app.patch_json_api(url, payload_invalid, auth=user_no_auth.auth, expect_errors=True) + res = app.patch_json_api(url_user_global_file_updated, payload_invalid, auth=user_no_permission.auth, expect_errors=True) assert res.status_code == 403 def test_subscription_detail_invalid_payload_401( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): - res = app.patch_json_api(url, payload_invalid, expect_errors=True) + res = app.patch_json_api(url_user_global_file_updated, payload_invalid, expect_errors=True) assert res.status_code == 401 def test_subscription_detail_invalid_payload_400( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): res = app.patch_json_api( - url, + url_user_global_file_updated, payload_invalid, auth=user.auth, expect_errors=True, @@ -126,33 +327,33 @@ def test_subscription_detail_invalid_payload_400( assert res.json['errors'][0]['detail'] == ('"invalid-frequency" is not a valid choice.') def test_subscription_detail_patch_invalid_notification_id_no_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): res = app.patch_json_api(url_invalid, payload, expect_errors=True) assert res.status_code == 404 def test_subscription_detail_patch_invalid_notification_id_existing_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): res = app.patch_json_api(url_invalid, payload, auth=user.auth, expect_errors=True) assert res.status_code == 404 def test_subscription_detail_patch_invalid_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): - res = app.patch_json_api(url, payload, auth=user_no_auth.auth, expect_errors=True) + res = app.patch_json_api(url_user_global_file_updated, payload, auth=user_no_permission.auth, expect_errors=True) assert res.status_code == 403 def test_subscription_detail_patch_no_user( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): - res = app.patch_json_api(url, payload, expect_errors=True) + res = app.patch_json_api(url_user_global_file_updated, payload, expect_errors=True) assert res.status_code == 401 def test_subscription_detail_patch( - self, app, user, user_no_auth, notification, url, url_invalid, payload, payload_invalid + self, app, user, user_no_permission, notification_user_global_file_updated, url_user_global_file_updated, url_invalid, payload, payload_invalid ): - res = app.patch_json_api(url, payload, auth=user.auth) + res = app.patch_json_api(url_user_global_file_updated, payload, auth=user.auth) assert res.status_code == 200 assert res.json['data']['attributes']['frequency'] == 'none' diff --git a/notifications.yaml b/notifications.yaml index 29d896cdc89..6862927ec56 100644 --- a/notifications.yaml +++ b/notifications.yaml @@ -462,13 +462,6 @@ notification_types: template: 'website/templates/file_updated.html.mako' tests: ['tests/test_events.py'] - - name: node_file_updated - subject: 'File Updated' - __docs__: ... - object_content_type_model_name: abstractnode - template: 'website/templates/file_updated.html.mako' - tests: ['tests/test_events.py'] - - name: node_institutional_access_request subject: 'Institutional Access Request' __docs__: ... diff --git a/osf/models/notification_type.py b/osf/models/notification_type.py index fbbe2524f99..86407ef6e0f 100644 --- a/osf/models/notification_type.py +++ b/osf/models/notification_type.py @@ -83,8 +83,7 @@ class Type(str, Enum): USER_CROSSREF_DOI_PENDING = 'user_crossref_doi_pending' # Node notifications - NODE_FILE_UPDATED = 'node_file_updated' - NODE_FILES_UPDATED = 'node_files_updated' + NODE_FILE_UPDATED = 'node_files_updated' NODE_AFFILIATION_CHANGED = 'node_affiliation_changed' NODE_REQUEST_ACCESS_SUBMITTED = 'node_request_access_submitted' NODE_REQUEST_ACCESS_DENIED = 'node_request_access_denied' diff --git a/tests/test_events.py b/tests/test_events.py index fa79515e021..0d83bfdd072 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -309,7 +309,7 @@ def setUp(self): self.file_sub = factories.NotificationSubscriptionFactory( object_id=self.project.id, content_type=ContentType.objects.get_for_model(self.project), - notification_type=NotificationType.objects.get(name=NotificationType.Type.NODE_FILES_UPDATED) + notification_type=NotificationType.objects.get(name=NotificationType.Type.NODE_FILE_UPDATED) ) self.file_sub.save() @@ -407,7 +407,7 @@ def setUp(self): self.file_sub = factories.NotificationSubscriptionFactory( object_id=self.project.id, content_type=ContentType.objects.get_for_model(self.project), - notification_type=NotificationType.objects.get(name=NotificationType.Type.NODE_FILES_UPDATED) + notification_type=NotificationType.objects.get(name=NotificationType.Type.NODE_FILE_UPDATED) ) self.file_sub.save() 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', + }, )