requirements_dev.txt

@@ -6,6 +6,7 @@ django-debug-toolbar==4.2.0
 factory-boy==3.2.1
 Faker==13.15.1
 filelock==3.7.1
+freezegun==1.5.1
 identify==2.5.2
 mdgen==0.1.10
 nodeenv==1.7.0

reviewers/migrations/0012_approvalqueue.py

@@ -0,0 +1,52 @@
+# Generated by Django 4.2.11 on 2024-08-23 09:42
+
+from django.db import migrations, models
+from django.db.models import Count
+import django.db.models.deletion
+
+
+def populate_approval_queue(apps, schema_editor):
+    ApprovalActivity = apps.get_model('reviewers', 'ApprovalActivity')
+    ApprovalQueue = apps.get_model('reviewers', 'ApprovalQueue')
+    processed_extensions = set()
+    STATUS_CHANGE_TYPES = {"AWR": 0x02, "AWC": 0x01, "APR": 0x00}
+    activity_counts = {}
+    for row in ApprovalActivity.objects.all().values('extension__pk').annotate(count=Count('id')).order_by():
+        activity_counts[row['extension__pk']] = row['count']
+    for activity in ApprovalActivity.objects.order_by('-id').all():
+        extension = activity.extension
+        if extension in processed_extensions:
+            continue
+        if activity.type in STATUS_CHANGE_TYPES:
+            activity_count = activity_counts[extension.pk]
+            timestamp = int(activity.date_created.timestamp())
+            sortkey = (STATUS_CHANGE_TYPES[activity.type] << 32) | timestamp
+            ApprovalQueue.objects.create(
+                activity_count=activity_count,
+                extension=extension,
+                latest_activity=activity,
+                sortkey=sortkey,
+            )
+            processed_extensions.add(extension)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('extensions', '0042_platform_name'),
+        ('reviewers', '0011_alter_approvalactivity_user'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='ApprovalQueue',
+            fields=[
+                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('activity_count', models.PositiveIntegerField()),
+                ('sortkey', models.BigIntegerField()),
+                ('extension', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='extensions.extension')),
+                ('latest_activity', models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='reviewers.approvalactivity')),
+            ],
+        ),
+        migrations.RunPython(populate_approval_queue, migrations.RunPython.noop),
+    ]

reviewers/models.py

@@ -28,6 +28,8 @@ class CannedResponse(CreatedModifiedMixin, models.Model):
 
 
 class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
+    STATUS_CHANGE_TYPES = {"AWR": 0x02, "AWC": 0x01, "APR": 0x00}
+
     class ActivityType(models.TextChoices):
         COMMENT = "COM", _("Comment")
         APPROVED = "APR", _("Approved")
@@ -53,3 +55,37 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
 
     def __str__(self):
         return f"{self.extension.name}: {self.get_type_display()}"
+
+    @property
+    def queue_sortkey(self):
+        """Sorting by moderation status and latest status change timestamp.
+
+        The queue is ordered by status: first "awaiting review', then "awaiting changes", then
+        "approved".
+        Within each group items with most recent status change are sorted to the top.
+
+        Integer timestamp representation takes 4 bytes, the resulting bigint is composed of
+        0x000000SSTTTTTTTT, where SS byte represents the status change type, and TT bytes represent
+        timestamp bytes.
+        """
+        timestamp = int(self.date_created.timestamp())
+        return (self.STATUS_CHANGE_TYPES[self.type] << 32) | timestamp
+
+
+class ApprovalQueue(models.Model):
+    activity_count = models.PositiveIntegerField()
+    extension = models.OneToOneField(
+        'extensions.Extension',
+        on_delete=models.CASCADE,
+    )
+    latest_activity = models.ForeignKey(
+        ApprovalActivity,
+        # we don't delete activity yet, if we start this needs to be updated via a pre_delete signal
+        on_delete=models.PROTECT,
+    )
+    sortkey = models.BigIntegerField()
+
+    class Meta:
+        indexes = [
+            models.Index(fields=['sortkey']),
+        ]

reviewers/signals.py

@@ -4,7 +4,7 @@ from django.db.models.signals import post_save, pre_delete
 from django.dispatch import receiver
 
 from constants.activity import Flag, Verb
-from reviewers.models import ApprovalActivity
+from reviewers.models import ApprovalActivity, ApprovalQueue
 
 
 @receiver(post_save, sender=ApprovalActivity)
@@ -40,6 +40,35 @@ def _create_action_from_review_and_follow(
     )
 
 
+@receiver(post_save, sender=ApprovalActivity)
+def _update_approval_queue(
+    sender: object,
+    instance: ApprovalActivity,
+    created: bool,
+    raw: bool,
+    **kwargs: object,
+) -> None:
+    if raw:
+        return
+    if not created:
+        return
+
+    activity_count = ApprovalActivity.objects.filter(extension=instance.extension).count()
+    if instance.type in ApprovalActivity.STATUS_CHANGE_TYPES:
+        ApprovalQueue.objects.update_or_create(
+            extension=instance.extension,
+            defaults={
+                'activity_count': activity_count,
+                'latest_activity': instance,
+                'sortkey': instance.queue_sortkey,
+            },
+        )
+    else:
+        if item := ApprovalQueue.objects.filter(extension=instance.extension).first():
+            item.activity_count = activity_count
+            item.save(update_fields={'activity_count'})
+
+
 @receiver(pre_delete, sender=ApprovalActivity)
 def _log_deletion(sender: object, instance: ApprovalActivity, **kwargs: object) -> None:
     instance.record_deletion()

reviewers/templates/reviewers/components/review_list_item.html

@@ -22,20 +22,20 @@
   </td>
   <td title="{{ extension.date_created }}">{{ extension.date_created|naturaltime_compact }}</td>
   <td class="ext-review-list-activity" colspan="2">
-    <a href="{{ extension.get_review_url }}#activity-{{ stats.last_activity.id }}">
-      <span>{{ stats.last_activity.date_created|naturaltime_compact }}</span>
+    <a href="{{ extension.get_review_url }}#activity-{{ item.latest_activity.id }}">
+      <span>{{ item.latest_activity.date_created|naturaltime_compact }}</span>
     </a>
     {% include "files/components/scan_details_flag.html" with suspicious_files=extension.suspicious_files %}
   </td>
   <td class="ext-review-list-activity ps-0 text-center">
     <a href="{{ extension.get_review_url }}#activity">
-      <span class="badge">{{ stats.count }}</span>
+      <span class="badge">{{ item.activity_count }}</span>
     </a>
   </td>
   <td class="ext-review-list-status">
     <a href="{{ extension.get_review_url }}" class="text-decoration-none">
-      {% with last_type=stats.last_type_display|default:"Awaiting Review" %}
-      {% include "common/components/status.html" with label=last_type slug=last_type|slugify object=extension classes="d-block" icon=True %}
+      {% with latest_type=item.latest_activity.get_type_display %}
+      {% include "common/components/status.html" with label=latest_type slug=latest_type|slugify object=extension classes="d-block" icon=True %}
       {% endwith %}
     </a>
   </td>

reviewers/templates/reviewers/extensions_review_list.html

@@ -36,9 +36,9 @@
           </tr>
         </thead>
         <tbody>
-          {% for stats in object_list %}
-            {% with extension=stats.extension %}
-            {% include 'reviewers/components/review_list_item.html' with extension=extension stats=stats %}
+          {% for item in object_list %}
+            {% with extension=item.extension %}
+            {% include 'reviewers/components/review_list_item.html' with extension=extension item=item %}
             {% endwith %}
           {% endfor %}
         </tbody>

reviewers/tests/test_views.py

@@ -1,7 +1,11 @@
+from datetime import datetime, timedelta, timezone
+
 from django.test import TestCase
 from django.shortcuts import reverse
+from freezegun import freeze_time
 
 from common.tests.factories.extensions import create_version
+from common.tests.factories.users import UserFactory
 from files.models import File
 from reviewers.models import ApprovalActivity
 
@@ -13,7 +17,7 @@ class CommentsViewTest(TestCase):
         version = create_version(status=File.STATUSES.AWAITING_REVIEW)
         self.default_version = version
         ApprovalActivity(
-            type=ApprovalActivity.ActivityType.COMMENT,
+            type=ApprovalActivity.ActivityType.AWAITING_REVIEW,
             user=version.files.first().user,
             extension=version.extension,
             message='test comment',
@@ -40,3 +44,97 @@ class CommentsViewTest(TestCase):
     # Display only previews and version that are 'pending review'
 
     # Approval requires manager role
+
+
+class QueueOrderTest(TestCase):
+    fixtures = ['licenses']
+
+    def test_order(self):
+        extensions = []
+        user = UserFactory()
+        for i in range(5):
+            version = create_version(status=File.STATUSES.AWAITING_REVIEW)
+            extension = version.extension
+            extensions.append(extension)
+
+        # to control the distance between timestamps
+        starting_datetime = datetime(2020, 12, 31, 23, 2, 3, tzinfo=timezone.utc)
+        delta_s = 0
+        with freeze_time(starting_datetime + timedelta(seconds=delta_s)):
+            ApprovalActivity(
+                type=ApprovalActivity.ActivityType.AWAITING_REVIEW,
+                user=user,
+                extension=extensions[0],
+                message='test comment',
+            ).save()
+
+        delta_s += 1
+        with freeze_time(starting_datetime + timedelta(seconds=delta_s)):
+            ApprovalActivity(
+                type=ApprovalActivity.ActivityType.AWAITING_CHANGES,
+                user=user,
+                extension=extensions[1],
+                message='test comment',
+            ).save()
+
+        delta_s += 1
+        with freeze_time(starting_datetime + timedelta(seconds=delta_s)):
+            ApprovalActivity(
+                type=ApprovalActivity.ActivityType.AWAITING_CHANGES,
+                user=user,
+                extension=extensions[2],
+                message='test comment',
+            ).save()
+
+        apr = None
+        delta_s += 1
+        with freeze_time(starting_datetime + timedelta(seconds=delta_s)):
+            apr = ApprovalActivity(
+                date_created=starting_datetime + timedelta(seconds=delta_s),
+                type=ApprovalActivity.ActivityType.APPROVED,
+                user=user,
+                extension=extensions[3],
+                message='test comment',
+            )
+            apr.save()
+        delta_s += 1
+        with freeze_time(starting_datetime + timedelta(seconds=delta_s)):
+            ApprovalActivity(
+                date_created=starting_datetime + timedelta(seconds=delta_s),
+                type=ApprovalActivity.ActivityType.COMMENT,
+                user=user,
+                extension=extensions[3],
+                message='test comment',
+            ).save()
+
+        delta_s += 1
+        with freeze_time(starting_datetime + timedelta(seconds=delta_s)):
+            ApprovalActivity(
+                date_created=starting_datetime + timedelta(seconds=delta_s),
+                type=ApprovalActivity.ActivityType.COMMENT,
+                user=user,
+                extension=extensions[4],
+                message='test comment',
+            ).save()
+
+        response = self.client.get(reverse('reviewers:approval-queue'))
+        queue = response.context['object_list']
+
+        # extensions[4] doesn't have meaningful activity
+        self.assertEqual(len(queue), 4)
+
+        # ordered by category
+        self.assertEqual(queue[0].extension, extensions[0])
+        # and within category by recent timestamp
+        self.assertEqual(queue[1].extension, extensions[2])
+        self.assertEqual(queue[2].extension, extensions[1])
+        self.assertEqual(queue[3].extension, extensions[3])
+
+        # counts are correct
+        self.assertEqual(queue[0].activity_count, 1)
+        self.assertEqual(queue[1].activity_count, 1)
+        self.assertEqual(queue[2].activity_count, 1)
+        self.assertEqual(queue[3].activity_count, 2)
+
+        # latest_activity doesn't account for non-meaningful activity
+        self.assertEqual(queue[3].latest_activity, apr)

reviewers/views.py

@@ -1,4 +1,3 @@
-from collections import defaultdict
 import logging
 
 from actstream.actions import follow, is_following, unfollow
@@ -11,62 +10,30 @@ import django.forms
 from constants.activity import Flag
 from extensions.models import Extension
 from reviewers.forms import CommentForm
-from reviewers.models import ApprovalActivity
+from reviewers.models import ApprovalActivity, ApprovalQueue
 
 log = logging.getLogger(__name__)
 
-# the ordering of this list determines the order of rows in approval queue listing
-STATUS_CHANGE_TYPES = [
-    ApprovalActivity.ActivityType.AWAITING_REVIEW,
-    ApprovalActivity.ActivityType.AWAITING_CHANGES,
-    ApprovalActivity.ActivityType.APPROVED,
-]
-
 
 class ApprovalQueueView(ListView):
     model = Extension
     paginate_by = 50
 
     def get_queryset(self):
-        qs = ApprovalActivity.objects.prefetch_related(
-            'extension',
-            'extension__authors',
-            'extension__icon',
-            'extension__versions',
-            'extension__versions__files',
-            'extension__versions__files__validation',
-        ).order_by('-id')
-        by_extension = {}
-        by_date_created = []
-        for item in qs:
-            extension = item.extension
-            stats = by_extension.get(extension, None)
-            if not stats:
-                # this check guarantees that we add a record only once per extension,
-                # and iterating over qs we get by_date_created also ordered by item.date_created
-                stats = {
-                    'count': 0,
-                    'extension': extension,
-                    'last_activity': None,
-                    'last_type_display': None,
-                    'type': None,
-                }
-                by_extension[extension] = stats
-                by_date_created.append(stats)
-            stats['count'] += 1
-            if not stats.get('last_activity', None):
-                stats['last_activity'] = item
-            if not stats.get('last_type_display', None) and item.type in STATUS_CHANGE_TYPES:
-                stats['last_type_display'] = item.get_type_display()
-                stats['type'] = item.type
-        groupped_by_type = defaultdict(list)
-        for stats in by_date_created:
-            type = stats['type'] or ApprovalActivity.ActivityType.AWAITING_REVIEW
-            groupped_by_type[type].append(stats)
-        result = []
-        for type in STATUS_CHANGE_TYPES:
-            result.extend(groupped_by_type[type])
-        return result
+        return (
+            ApprovalQueue.objects.select_related(
+                'extension',
+                'latest_activity',
+            )
+            .prefetch_related(
+                'extension__authors',
+                'extension__icon',
+                'extension__versions',
+                'extension__versions__files',
+                'extension__versions__files__validation',
+            )
+            .order_by('-sortkey')
+        )
 
     template_name = 'reviewers/extensions_review_list.html'
 
@@ -102,7 +69,7 @@ class ExtensionsApprovalDetailView(DetailView):
                 activity.user_is_maintainer = True
 
         ctx['review_activity'] = review_activity
-        ctx['status_change_types'] = STATUS_CHANGE_TYPES
+        ctx['status_change_types'] = ApprovalActivity.STATUS_CHANGE_TYPES
 
         if self.request.user.is_authenticated:
             initial = {}