Fix File show_as_unviewed behavior (#9960)

* Replace `current_user_has_viewed` SerializerMethodField with `show_as_unviewed` annotation

api/files/annotations.py

@@ -1,8 +1,9 @@
-from django.db.models import Max, Case, When
+from django.db.models import BooleanField, Case, Exists, F, IntegerField, Max, OuterRef, Q, Subquery, Value, When
 from django.db.models.functions.base import Cast
 from django.contrib.postgres.fields.jsonb import KeyTextTransform
 from osf.utils.datetime_aware_jsonfield import DateTimeAwareJSONField
 from osf.utils.fields import NonNaiveDateTimeField
+from osf.models import FileVersion
 
 
 # Get date modified for OSF and non-OSF providers
@@ -28,3 +29,58 @@
         ), output_field=NonNaiveDateTimeField(),
     ),
 )
+
+def make_show_as_unviewed_annotations(user):
+    '''Returns the annotations required to set the current_user_has_viewed attribute.
+
+    Usage:
+    OsfStorageFile.objects.annotate(**make_show_as_unviewed_annotations(request.user))
+
+    show_as_unviewed is only true if the user has not seen the latest version of a file
+    but has looked at it previously. Making this determination requires multiple annotations,
+    which is why this returns a dictionary that must be unpacked into kwargs.
+    '''
+    if user.is_anonymous:
+        return {'show_as_unviewed': Value(False, output_field=BooleanField())}
+
+    seen_versions = FileVersion.objects.annotate(
+        latest_version=Subquery(
+            FileVersion.objects.filter(
+                basefilenode=OuterRef('basefilenode'),
+            ).order_by('-created').values('id')[:1],
+            output_field=IntegerField(),
+        ),
+    ).filter(seen_by=user)
+
+    has_seen_latest = Exists(
+        seen_versions.filter(basefilenode=OuterRef('id')).filter(id=F('latest_version')),
+    )
+    has_previously_seen = Exists(
+        seen_versions.filter(basefilenode=OuterRef('id')).exclude(id=F('latest_version')),
+    )
+    show_as_unviewed = Case(
+        When(Q(has_seen_latest=False) & Q(has_previously_seen=True), then=Value(True)),
+        default=Value(False),
+        output_field=BooleanField(),
+    )
+
+    return {
+        'has_seen_latest': has_seen_latest,
+        'has_previously_seen': has_previously_seen,
+        'show_as_unviewed': show_as_unviewed,
+    }
+
+def check_show_as_unviewed(user, osf_file):
+    '''A separate function for assigning the show_as_unviewed value to a single instance.
+
+    Our logic is not conducive to assigning annotations to a single file, so do it manually
+    in the DetailView case.
+    '''
+    if user.is_anonymous:
+        return False
+
+    latest_version = osf_file.versions.order_by('-created').first()
+    return (
+        osf_file.versions.filter(seen_by=user).exists()
+        and not latest_version.seen_by.filter(id=user.id).exists()
+    )

api/files/serializers.py

@@ -383,14 +383,14 @@ class FileSerializer(BaseFileSerializer):
         min_version='2.0', max_version='2.7',
     )
     target = TargetField(link_type='related', meta={'type': 'get_target_type'})
-    current_user_has_viewed = ser.SerializerMethodField(help_text='Whether the current user has already viewed the file')
 
-    def get_current_user_has_viewed(self, obj):
-        version = obj.versions.order_by('created').last()
-        if version and not self.context['request'].user.is_anonymous:  # This is to ensure compatibility with tests, ugh.
-            return version.seen_by.filter(id=self.context['request'].user.id).exists()
-        else:
-            return True
+    # Assigned via annotation. See api/files/annotations for info
+    show_as_unviewed = ser.BooleanField(
+        read_only=True,
+        required=False,
+        default=False,
+        help_text='Whether to mark the file as unviewed for the current user',
+    )
 
     def get_target_type(self, obj):
         if isinstance(obj, Preprint):

api/files/views.py

@@ -24,6 +24,7 @@
 from api.base import permissions as base_permissions
 from api.nodes.permissions import ContributorOrPublic
 from api.nodes.permissions import ReadOnlyIfRegistration
+from api.files import annotations
 from api.files.permissions import IsPreprintFile
 from api.files.permissions import CheckedOutOrAdmin
 from api.files.permissions import FileMetadataRecordPermission
@@ -110,6 +111,9 @@ def get_object(self):
         # We normally would pass this through `get_file` as an annotation, but the `select_for_update` feature prevents
         # grouping versions in an annotation
         if file.kind == 'file':
+            file.show_as_unviewed = annotations.check_show_as_unviewed(
+                user=self.request.user, osf_file=file,
+            )
             if file.provider == 'osfstorage':
                 file.date_modified = file.versions.aggregate(Max('created'))['created__max']
             else:

api/nodes/views.py

@@ -2,7 +2,7 @@
 
 from distutils.version import StrictVersion
 from django.apps import apps
-from django.db.models import Q, Subquery, F, Max
+from django.db.models import F, Max, Q, Subquery
 from django.utils import timezone
 from django.contrib.contenttypes.models import ContentType
 from rest_framework import generics, permissions as drf_permissions
@@ -65,7 +65,7 @@
 )
 from api.draft_registrations.serializers import DraftRegistrationSerializer, DraftRegistrationDetailSerializer
 from api.files.serializers import FileSerializer, OsfStorageFileSerializer
-from api.files.annotations import DATE_MODIFIED
+from api.files import annotations as file_annotations
 from api.identifiers.serializers import NodeIdentifierSerializer
 from api.identifiers.views import IdentifierList
 from api.institutions.serializers import InstitutionSerializer
@@ -1181,7 +1181,7 @@ def get_queryset(self):
             file_obj = self.get_file_object(resource, path, provider)
 
             if provider == 'osfstorage':
-                queryset = OsfStorageFileNode.objects.filter(id=file_obj.id)
+                queryset = OsfStorageFileNode.objects.filter(id=file_obj.id).annotate
             else:
                 base_class = BaseFileNode.resolve_class(provider, BaseFileNode.FOLDER)
                 queryset = base_class.objects.filter(
@@ -1192,7 +1192,10 @@ def get_queryset(self):
         else:
             queryset = self.get_queryset_from_request()
 
-        return queryset.annotate(date_modified=DATE_MODIFIED)
+        return queryset.annotate(
+            date_modified=file_annotations.DATE_MODIFIED,
+            **file_annotations.make_show_as_unviewed_annotations(self.request.user)
+        )
 
 
 class NodeFileDetail(JSONAPIBaseView, generics.RetrieveAPIView, WaterButlerMixin, NodeMixin):
@@ -1223,6 +1226,9 @@ def get_object(self):
             # We should not have gotten a folder here
             raise NotFound
         if fobj.kind == 'file':
+            fobj.show_as_unviewed = file_annotations.check_show_as_unviewed(
+                user=self.request.user, osf_file=fobj,
+            )
             if fobj.provider == 'osfstorage':
                 fobj.date_modified = fobj.versions.aggregate(Max('created'))['created__max']
             else:

api_tests/files/views/test_file_detail.py

@@ -10,10 +10,11 @@
 from addons.github.models import GithubFileNode
 from addons.osfstorage import settings as osfstorage_settings
 from addons.osfstorage.listeners import checkin_files_task
+from addons.osfstorage.tests.factories import FileVersionFactory
 from api.base.settings.defaults import API_BASE
 from api_tests import utils as api_utils
 from framework.auth.core import Auth
-from osf.models import NodeLog, Session, QuickFilesNode, Node
+from osf.models import NodeLog, Session, QuickFilesNode, Node, FileVersionUserMetadata
 from osf.utils.permissions import WRITE, READ
 from osf.utils.workflows import DefaultStates
 from osf_tests.factories import (
@@ -918,3 +919,62 @@ def test_withdrawn_preprint_files(self, app, file_url, preprint, user, other_use
         # Admin contrib
         res = app.get(file_url, auth=user.auth, expect_errors=True)
         assert res.status_code == 403
+
+@pytest.mark.django_db
+class TestShowAsUnviewed:
+
+    @pytest.fixture
+    def node(self, user):
+        return ProjectFactory(creator=user, is_public=True)
+
+    @pytest.fixture
+    def test_file(self, user, node):
+        test_file = api_utils.create_test_file(node, user, create_guid=False)
+        test_file.add_version(FileVersionFactory())
+        return test_file
+
+    @pytest.fixture
+    def url(self, test_file):
+        return f'/{API_BASE}files/{test_file._id}/'
+
+    def test_show_as_unviewed__previously_seen(self, app, user, test_file, url):
+        FileVersionUserMetadata.objects.create(
+            user=user,
+            file_version=test_file.versions.order_by('created').first()
+        )
+
+        res = app.get(url, auth=user.auth)
+        assert res.json['data']['attributes']['show_as_unviewed']
+
+        FileVersionUserMetadata.objects.create(
+            user=user,
+            file_version=test_file.versions.order_by('-created').first()
+        )
+
+        res = app.get(url, auth=user.auth)
+        assert not res.json['data']['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__not_previously_seen(self, app, user, test_file, url):
+        res = app.get(url, auth=user.auth)
+        assert not res.json['data']['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__different_user(self, app, user, test_file, url):
+        FileVersionUserMetadata.objects.create(
+            user=user,
+            file_version=test_file.versions.order_by('created').first()
+        )
+        file_viewer = AuthUserFactory()
+
+        res = app.get(url, auth=file_viewer.auth)
+        assert not res.json['data']['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__anonymous_user(self, app, test_file, url):
+        res = app.get(url)
+        assert not res.json['data']['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__no_versions(self, app, user, test_file, url):
+        # Most Non-OSFStorage providers don't have versions; make sure this still works
+        test_file.versions.all().delete()
+
+        res = app.get(url, auth=user.auth)
+        assert not res.json['data']['attributes']['show_as_unviewed']

api_tests/nodes/views/test_node_files_list.py

@@ -10,10 +10,12 @@
 
 from addons.github.models import GithubFolder
 from addons.github.tests.factories import GitHubAccountFactory
+from addons.osfstorage.tests.factories import FileVersionFactory
 from api.base.settings.defaults import API_BASE
 from api.base.utils import waterbutler_api_url_for
 from api_tests import utils as api_utils
 from tests.base import ApiTestCase
+from osf.models.files import FileVersionUserMetadata
 from osf_tests.factories import (
     ProjectFactory,
     AuthUserFactory,
@@ -731,3 +733,56 @@ def test_can_view_if_public(self):
     def test_cannot_view_if_private(self):
         res = self.app.get(self.private_url, expect_errors=True)
         assert_equal(res.status_code, 401)
+
+
+class TestShowAsUnviewed(ApiTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.user = AuthUserFactory()
+        self.node = ProjectFactory(is_public=True, creator=self.user)
+        self.test_file = api_utils.create_test_file(self.node, self.user, create_guid=False)
+        self.test_file.add_version(FileVersionFactory())
+        self.url = f'/{API_BASE}nodes/{self.node._id}/files/osfstorage/'
+
+    def test_show_as_unviewed__previously_seen(self):
+        FileVersionUserMetadata.objects.create(
+            user=self.user,
+            file_version=self.test_file.versions.order_by('created').first()
+        )
+
+        res = self.app.get(self.url, auth=self.user.auth)
+        assert res.json['data'][0]['attributes']['show_as_unviewed']
+
+        FileVersionUserMetadata.objects.create(
+            user=self.user,
+            file_version=self.test_file.versions.order_by('-created').first()
+        )
+
+        res = self.app.get(self.url, auth=self.user.auth)
+        assert not res.json['data'][0]['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__not_previously_seen(self):
+        res = self.app.get(self.url, auth=self.user.auth)
+        assert not res.json['data'][0]['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__different_user(self):
+        FileVersionUserMetadata.objects.create(
+            user=self.user,
+            file_version=self.test_file.versions.order_by('created').first()
+        )
+        file_viewer = AuthUserFactory()
+
+        res = self.app.get(self.url, auth=file_viewer.auth)
+        assert not res.json['data'][0]['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__anonymous_user(self):
+        res = self.app.get(self.url)
+        assert not res.json['data'][0]['attributes']['show_as_unviewed']
+
+    def test_show_as_unviewed__no_versions(self):
+        # Most Non-OSFStorage providers don't have versions; make sure this still works
+        self.test_file.versions.all().delete()
+
+        res = self.app.get(self.url, auth=self.user.auth)
+        assert not res.json['data'][0]['attributes']['show_as_unviewed']

tests/test_addons.py

@@ -224,62 +224,6 @@ def test_action_download_mfr_views_non_contrib(self):
         assert_equal(test_file.get_view_count(), 1)
         assert_equal(node.logs.count(), nlogs) # don't log views
 
-    def test_current_user_has_viewed_public(self):
-        node = ProjectFactory(is_public=True)
-        file = create_test_file(node, node.creator, create_guid=False)
-        django_app = JSONAPITestApp()
-
-        file_viewer = AuthUserFactory()
-
-        res = django_app.get(f'/{API_BASE}files/{file._id}/', auth=file_viewer.auth)
-        assert res.status_code == 200
-        assert res.json['data']['attributes']['current_user_has_viewed'] is False
-
-        # This mocks the Waterbutler callback endpoint (`get_auth` function in addons/base/views.py ) which indicates if
-        # a file has been view with the MFR.
-        self.app.get(
-            self.build_url(
-                nid=node._id,
-                provider='osfstorage',
-                path=file.path,
-                version=1
-            ),
-            auth=file_viewer.auth
-        )
-
-        res = django_app.get(f'/{API_BASE}files/{file._id}/', auth=file_viewer.auth)
-
-        version = file.versions.get()
-        assert version.seen_by.exists()
-        assert res.json['data']['attributes']['current_user_has_viewed']
-
-    def test_current_user_has_viewed_private(self):
-        node = ProjectFactory()
-        file = create_test_file(node, node.creator, create_guid=False)
-        django_app = JSONAPITestApp()
-
-        res = django_app.get(f'/{API_BASE}files/{file._id}/', auth=node.creator.auth)
-        assert res.status_code == 200
-        assert res.json['data']['attributes']['current_user_has_viewed'] is False
-
-        # This mocks the Waterbutler callback endpoint (`get_auth` function in addons/base/views.py ) which indicates if
-        # a file has been view with the MFR.
-        self.app.get(
-            self.build_url(
-                nid=node._id,
-                provider='osfstorage',
-                path=file.path,
-                version=1
-            ),
-            auth=node.creator.auth
-        )
-
-        res = django_app.get(f'/{API_BASE}files/{file._id}/', auth=node.creator.auth)
-
-        version = file.versions.get()
-        assert version.seen_by.exists()
-        assert res.json['data']['attributes']['current_user_has_viewed']
-
 
 class TestAddonLogs(OsfTestCase):