Show "pending" activity in devhub for all versions and not just the latest one (#22681)

src/olympia/activity/models.py

@@ -3,7 +3,7 @@ import os
 import uuid
 from collections import defaultdict
 from copy import copy
-from datetime import datetime
+from datetime import date, datetime
 from inspect import isclass
 
 from django.apps import apps
@@ -41,6 +41,18 @@ MAX_TOKEN_USE_COUNT = 100
 
 GENERIC_USER_NAME = gettext('Add-ons Review Team')
 
+# Activity ids that are not considered as needing a reply from developers, so
+# they are never considered "pending".
+NOT_PENDING_IDS = (
+    amo.LOG.DEVELOPER_REPLY_VERSION.id,
+    amo.LOG.APPROVE_VERSION.id,
+    amo.LOG.REJECT_VERSION.id,
+    amo.LOG.PRELIMINARY_VERSION.id,
+    amo.LOG.PRELIMINARY_ADDON_MIGRATED.id,
+    amo.LOG.NOTES_FOR_REVIEWERS_CHANGED.id,
+    amo.LOG.SOURCE_CODE_UPLOADED.id,
+)
+
 
 def attachment_upload_path(instance, filename):
     ext = os.path.splitext(filename)[1]
@@ -313,6 +325,38 @@ class ActivityLogQuerySet(BaseQuerySet):
     def default_transformer(self, logs):
         ActivityLog.arguments_builder(logs)
 
+    def pending_for_developer(self, for_version=None):
+        """Return ActivityLog that are considered "pending" for developers.
+
+        An Activity will be considered "pending" if it's a review queue
+        activity not hidden to developers that is more recent that the latest
+        activity created by a developer/reviewer. Said differently: if a
+        developer doesn't do something after a reviewer action, that reviewer
+        action will be considered pending."""
+        if for_version is None:
+            for_version = models.OuterRef('versionlog__version_id')
+        latest_reply_date = models.functions.Coalesce(
+            models.Subquery(
+                self.filter(
+                    action__in=NOT_PENDING_IDS,
+                    versionlog__version=for_version,
+                )
+                .values('created')
+                .order_by('-created')[:1]
+            ),
+            date.min,
+        )
+        return (
+            # The subquery needs to run on the already filterd activities we
+            # care about (it uses `self`, i.e. the current state of the
+            # queryset), so we need to filter by action first, then trigger the
+            # subquery, then filter by the result, we can't group all of that
+            # in a single filter() call.
+            self.filter(action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER)
+            .annotate(latest_reply_date=latest_reply_date)
+            .filter(created__gt=models.F('latest_reply_date'))
+        )
+
 
 class ActivityLogManager(ManagerBase):
     _queryset_class = ActivityLogQuerySet

src/olympia/activity/tests/test_models.py

@@ -99,6 +99,129 @@ class TestActivityLogToken(TestCase):
         assert self.token.is_valid()
 
 
+class TestActivityLogManager(TestCase):
+    def test_pending_for_developer(self):
+        to_create = (
+            # Tests with Developer_Reply
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.DEVELOPER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                1,
+            ),
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.DEVELOPER_REPLY_VERSION,
+                0,
+            ),
+            # Tests with Approval
+            (
+                amo.LOG.APPROVE_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                2,
+            ),
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.APPROVE_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                1,
+            ),
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.APPROVE_VERSION,
+                0,
+            ),
+            # Tests with Rejection
+            (
+                amo.LOG.REJECT_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                2,
+            ),
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REJECT_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                1,
+            ),
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REJECT_VERSION,
+                0,
+            ),
+            # Test with no approve or reject
+            (
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                amo.LOG.REVIEWER_REPLY_VERSION,
+                3,
+            ),
+        )
+
+        user = user_factory()
+        addon = addon_factory()
+        expected = []
+        for action1, action2, action3, count in to_create:
+            version = version_factory(addon=addon)
+            logs = (
+                ActivityLog.objects.create(action1, addon, version, user=user),
+                ActivityLog.objects.create(action2, addon, version, user=user),
+                ActivityLog.objects.create(action3, addon, version, user=user),
+            )
+            logs[-3].update(created=self.days_ago(2))
+            logs[-2].update(created=self.days_ago(1))
+            logs[-1].update(created=self.days_ago(0))
+            if count:
+                expected.extend(logs[-count:])
+        results = list(ActivityLog.objects.for_addons(addon).pending_for_developer())
+        assert len(results) == len(expected)
+        assert set(results) == set(expected)
+
+    def test_with_reply_going_to_multiple_versions_with_developer_reply(self):
+        user = user_factory()
+        addon = addon_factory()
+        v1 = addon.current_version
+        v2 = version_factory(addon=addon)
+        # Make a reviewer reply on both versions
+        grouped_reviewer_reply = ActivityLog.objects.create(
+            amo.LOG.REVIEWER_REPLY_VERSION,
+            addon,
+            v1,
+            v2,
+            user=user,
+        )
+        grouped_reviewer_reply.update(created=self.days_ago(42))
+        # Make the developer reply only on one of the versions
+        developer_reply_on_v1 = ActivityLog.objects.create(
+            amo.LOG.DEVELOPER_REPLY_VERSION,
+            addon,
+            v1,
+            user=user,
+        )
+        developer_reply_on_v1.update(created=self.days_ago(41))
+
+        # Extra data that shouldn't be relevant
+        version_factory(addon=addon)
+        extra_addon = addon_factory()
+        ActivityLog.objects.create(
+            amo.LOG.REVIEWER_REPLY_VERSION,
+            extra_addon,
+            extra_addon.current_version,
+            user=user,
+        )
+        results = list(
+            ActivityLog.objects.for_versions(
+                addon.versions.all()
+            ).pending_for_developer()
+        )
+        assert len(results) == 1
+        assert results[0] == grouped_reviewer_reply
+
+
 class TestActivityLog(TestCase):
     fixtures = ['base/addon_3615']
 

src/olympia/activity/tests/test_views.py

@@ -302,7 +302,7 @@ class TestReviewNotesViewSetList(ReviewNotesViewSetDetailMixin, TestCase):
             'fiiiine', amo.LOG.REVIEWER_REPLY_VERSION, self.days_ago(0)
         )
         self._login_developer()
-        with self.assertNumQueries(17):
+        with self.assertNumQueries(16):
             # - 2 savepoints because of tests
             # - 2 user and groups
             # - 2 addon and its translations
@@ -315,8 +315,7 @@ class TestReviewNotesViewSetList(ReviewNotesViewSetDetailMixin, TestCase):
             #   enough yet to pass that to the activity log queryset, it's
             #   difficult since it's not a FK)
             # - 2 version and its translations (same issue)
-            # - 2 for highlighting (repeats the query to fetch the activity log
-            #   per version)
+            # - 1 for highlighting "pending" activities
             response = self.client.get(self.url)
             assert response.status_code == 200
 

src/olympia/activity/utils.py

@@ -394,28 +394,6 @@ def send_activity_mail(
         )
 
 
-NOT_PENDING_IDS = (
-    amo.LOG.DEVELOPER_REPLY_VERSION.id,
-    amo.LOG.APPROVE_VERSION.id,
-    amo.LOG.REJECT_VERSION.id,
-    amo.LOG.PRELIMINARY_VERSION.id,
-    amo.LOG.PRELIMINARY_ADDON_MIGRATED.id,
-    amo.LOG.NOTES_FOR_REVIEWERS_CHANGED.id,
-    amo.LOG.SOURCE_CODE_UPLOADED.id,
-)
-
-
-def filter_queryset_to_pending_replies(queryset, log_type_ids=NOT_PENDING_IDS):
-    latest_reply_date = (
-        queryset.filter(action__in=log_type_ids)
-        .values_list('created', flat=True)
-        .first()
-    )
-    if not latest_reply_date:
-        return queryset
-    return queryset.filter(created__gt=latest_reply_date)
-
-
 def bounce_mail(message, reason):
     recipient_header = (
         None

src/olympia/activity/views.py

@@ -27,7 +27,6 @@ from olympia.activity.serializers import (
 from olympia.activity.tasks import process_email
 from olympia.activity.utils import (
     action_from_user,
-    filter_queryset_to_pending_replies,
     log_and_notify,
 )
 from olympia.addons.views import AddonChildMixin
@@ -96,9 +95,7 @@ class VersionReviewNotesViewSet(
     def get_serializer_context(self):
         ctx = super().get_serializer_context()
         ctx['to_highlight'] = list(
-            filter_queryset_to_pending_replies(self.get_queryset()).values_list(
-                'pk', flat=True
-            )
+            self.get_queryset().pending_for_developer().values_list('pk', flat=True)
         )
         return ctx
 

src/olympia/devhub/templates/devhub/versions/list.html

@@ -62,12 +62,10 @@
           <a href="#" class="review-history-show" data-div="#{{ version.id }}-review-history"
             id="review-history-show-{{ version.id }}" data-version="{{ version.id }}">{{ _('Review History') }}</a>
           <a href="#" class="review-history-hide hidden">{{ _('Close Review History') }}</a>
-          {% if latest_version_in_channel_including_disabled == version %}
-              {% set pending_count = pending_activity_log_count_for_developer(version) %}
-              {% if pending_count > 0 %}
-                  <b class="review-history-pending-count">{{ pending_count }}</b>
-              {% endif %}
-          {% endif %}
+            {% set pending_count = pending_activity_log_count_for_developer(version) %}
+            {% if pending_count > 0 %}
+                <b class="review-history-pending-count">{{ pending_count }}</b>
+            {% endif %}
       </div>
     </td>
     <td class="file-validation">

src/olympia/devhub/templatetags/jinja_helpers.py

@@ -6,7 +6,6 @@ from django_jinja import library
 from olympia import amo
 from olympia.access import acl
 from olympia.activity.models import ActivityLog
-from olympia.activity.utils import filter_queryset_to_pending_replies
 from olympia.amo.templatetags.jinja_helpers import format_date, new_context, page_title
 from olympia.files.models import File
 
@@ -88,10 +87,12 @@ def summarize_validation(validation):
 
 @library.global_function
 def pending_activity_log_count_for_developer(version):
-    alog = ActivityLog.objects.for_versions(version).filter(
-        action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER
-    )
-    return filter_queryset_to_pending_replies(alog).count()
+    # unread_count is an annotation set by version_list() view to do this once
+    # for all versions in the list. We use it if it's present, otherwise fall
+    # back to the per-version computation.
+    if hasattr(version, 'unread_count'):
+        return version.unread_count
+    return ActivityLog.objects.for_versions(version).pending_for_developer().count()
 
 
 @library.global_function

src/olympia/devhub/tests/test_views_versions.py

@@ -282,7 +282,6 @@ class TestVersion(TestCase):
         version_two = version_factory(
             addon=self.addon,
             license=version.license,
-            version='1.2.3',
             file_kw={'status': status},
         )
         return version_two, version_two.file
@@ -658,8 +657,14 @@ class TestVersion(TestCase):
         )
 
     def test_pending_activity_count(self):
+        v1 = self.addon.current_version
+        v1.update(created=self.days_ago(1))
         v2, _ = self._extra_version_and_file(amo.STATUS_AWAITING_REVIEW)
+        v3, _ = self._extra_version_and_file(amo.STATUS_APPROVED)
         # Add some activity log messages
+        ActivityLog.objects.create(
+            amo.LOG.REVIEWER_REPLY_VERSION, v1.addon, v1, user=self.user
+        )
         ActivityLog.objects.create(
             amo.LOG.REVIEWER_REPLY_VERSION, v2.addon, v2, user=self.user
         )
@@ -667,17 +672,66 @@ class TestVersion(TestCase):
             amo.LOG.REVIEWER_REPLY_VERSION, v2.addon, v2, user=self.user
         )
 
-        response = self.client.get(self.url)
+        with self.assertNumQueries(43):
+            # FIXME: lots of optimizations left to do. That count shouldn't go
+            # higher.
+            # 1. SAVEPOINT
+            # 2. the add-on
+            # 3. translations for that add-on (default transformer)
+            # 4. categories for that add-on (default transformer)
+            # 5. current version for that add-on (default transformer)
+            # 6. translations for the current version (default transformer)
+            # 7. applications versions for the current version (default transformer)
+            # 8. users for that add-on (default transformer)
+            # 9. previews for that add-on (default transformer)
+            # 10. current user
+            # 11. groups for that user
+            # 12. check on user being an author
+            # 13. count versions for the add-on for pagination
+            # 14. RELEASE SAVEPOINT
+            # 15. Add-ons for that user
+            # 16. Latest version in listed channel
+            # 17. Translations for that version
+            # 18. Latest version in unlisted channel
+            # 19. Latest public version in listed channel
+            # 20. Translations for that version
+            # 21. check on user being an author (dupe)
+            # 22. site notice
+            # 23. suppressed email waffle switch check
+            # 24. 8 latest add-ons from that user for the menu
+            # 25. translations for those add-ons
+            # 26. authors for those add-ons
+            # 27. count of pending activities on latest version
+            # 28. file validation for that latest version
+            # 29. is add-on promoted (for deletion warning)
+            # 30. check on user being an author (dupe)
+            # 31. versions being displayed w/ pending activities count and file attached
+            # 32. translations for those versions
+            # 33. applications versions for those versions
+            # 34. file validation
+            # 35. file validation (other version)
+            # 36. check on user being an author (dupe)
+            # 37. latest non-disabled version
+            # 38. translations for that version
+            # 39. are there versions in unlisted channel
+            # 40. versions in unlisted channel
+            # 41. translations for those versions
+            # 42. latest non-disabled version in unlisted channel
+            # 43. check on user being an author (dupe)
+            response = self.client.get(self.url)
         assert response.status_code == 200
         doc = pq(response.content)
 
-        # Two versions...
-        assert doc('.review-history-show').length == 2
-        # ...but only one counter, for the latest version
+        # Three versions...
+        assert doc('.review-history-show').length == 3
+        # ...2 have pending activities
         pending_activity_count = doc('.review-history-pending-count')
-        assert pending_activity_count.length == 1
-        # There are two activity logs pending
-        assert pending_activity_count.text() == '2'
+        assert pending_activity_count.length == 2
+        # There are two activity logs pending on v2, one on v1.
+        pending_activity_count_for_v2 = pending_activity_count[0]
+        assert pending_activity_count_for_v2.text_content() == '2'
+        pending_activity_count_for_v1 = pending_activity_count[1]
+        assert pending_activity_count_for_v1.text_content() == '1'
 
     def test_channel_tag(self):
         self.addon.current_version.update(created=self.days_ago(1))

src/olympia/devhub/views.py

@@ -10,7 +10,7 @@ from django.conf import settings
 from django.core.exceptions import PermissionDenied
 from django.core.files.storage import default_storage as storage
 from django.db import transaction
-from django.db.models import Count
+from django.db.models import Count, F, Func, OuterRef, Subquery
 from django.http import JsonResponse
 from django.shortcuts import get_object_or_404, redirect
 from django.template import loader
@@ -1267,7 +1267,30 @@ def check_validation_override(request, form, addon, version):
 
 @dev_required
 def version_list(request, addon_id, addon):
-    qs = addon.versions.order_by('-created')
+    unread_count = (
+        (
+            ActivityLog.objects.all()
+            # There are 2 subquery: the one in pending_for_developer() to
+            # determine the date that determines whether an activity is pending
+            # or not, and then that queryset which is applied for each version.
+            # That means the version filtering needs to be applied twice: for
+            # both the date threshold (inner subquery, so the version id to
+            # refer to is the parent of the parent) and the unread count itself
+            # ("regular" subquery so the version id to refer to is just the
+            # parent).
+            .pending_for_developer(for_version=OuterRef(OuterRef('id')))
+            # pending_for_developer() evaluates the queryset it's called from
+            # so we have to apply our second filter w/ OuterRef *after* calling
+            # it, otherwise OuterRef would point to the wrong parent.
+            .filter(versionlog__version=OuterRef('id'))
+            .values('id')
+        )
+        .annotate(count=Func(F('id'), function='COUNT'))
+        .values('count')
+    )
+    qs = addon.versions.annotate(unread_count=Subquery(unread_count)).order_by(
+        '-created'
+    )
     versions = amo_utils.paginate(request, qs)
     is_admin = acl.action_allowed_for(request.user, amo.permissions.REVIEWS_ADMIN)