Show "pending" activity in devhub for all versions and not just the latest one

src/olympia/activity/models.py

@@ -3,7 +3,7 @@
 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 @@
 
 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 @@ def test_rejected_version_still_valid(self):
         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 @@ def test_queries(self):
             '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 @@ def test_queries(self):
             #   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.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 @@ def check_permissions(self, request):
     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 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