From daad143fbe188e9b2af790d7ffde1d4c462961bd Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 23 Aug 2024 13:15:50 +0200 Subject: [PATCH 1/4] ApprovalQueue: use a materialized table see #238 This change improves the listing performance: old code had to process all ApprovalActivity to compute an extension's moderation status and position in the queue. Now we maintain a sortkey, a reference to the latest "meaningful" activity object, and a total comment count. These fields are updated in a post_save signal. "Meaningful" activity means moderation status changes: approved, awaiting changes, awaiting review. "Non-meaningful" activity shouldn't affect queue position anymore and extensions without "meaningful" activity should not appear in the queue, but their respective detail pages should still be reachable via a direct link. This UX may still need improvement, and #210 may be relevant here. --- reviewers/migrations/0012_approvalqueue.py | 52 +++++++++++++++ reviewers/models.py | 26 ++++++++ reviewers/signals.py | 27 +++++++- .../components/review_list_item.html | 10 +-- .../reviewers/extensions_review_list.html | 6 +- reviewers/tests/test_views.py | 2 +- reviewers/views.py | 65 +++++-------------- 7 files changed, 129 insertions(+), 59 deletions(-) create mode 100644 reviewers/migrations/0012_approvalqueue.py diff --git a/reviewers/migrations/0012_approvalqueue.py b/reviewers/migrations/0012_approvalqueue.py new file mode 100644 index 00000000..bacf49ca --- /dev/null +++ b/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), + ] diff --git a/reviewers/models.py b/reviewers/models.py index b4f3dd7b..0f3efc94 100644 --- a/reviewers/models.py +++ b/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,27 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model): def __str__(self): return f"{self.extension.name}: {self.get_type_display()}" + + @property + def queue_sortkey(self): + 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']), + ] diff --git a/reviewers/signals.py b/reviewers/signals.py index 2311e524..4b13c815 100644 --- a/reviewers/signals.py +++ b/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,31 @@ 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 + if instance.type not in ApprovalActivity.STATUS_CHANGE_TYPES: + return + activity_count = ApprovalActivity.objects.filter(extension=instance.extension).count() + ApprovalQueue.objects.update_or_create( + extension=instance.extension, + defaults={ + 'activity_count': activity_count, + 'latest_activity': instance, + 'sortkey': instance.queue_sortkey, + }, + ) + + @receiver(pre_delete, sender=ApprovalActivity) def _log_deletion(sender: object, instance: ApprovalActivity, **kwargs: object) -> None: instance.record_deletion() diff --git a/reviewers/templates/reviewers/components/review_list_item.html b/reviewers/templates/reviewers/components/review_list_item.html index 5b30c9df..0a2998dd 100644 --- a/reviewers/templates/reviewers/components/review_list_item.html +++ b/reviewers/templates/reviewers/components/review_list_item.html @@ -22,20 +22,20 @@ {{ extension.date_created|naturaltime_compact }} - - {{ stats.last_activity.date_created|naturaltime_compact }} + + {{ item.latest_activity.date_created|naturaltime_compact }} {% include "files/components/scan_details_flag.html" with suspicious_files=extension.suspicious_files %} - {{ stats.count }} + {{ item.activity_count }} - {% 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 %} diff --git a/reviewers/templates/reviewers/extensions_review_list.html b/reviewers/templates/reviewers/extensions_review_list.html index 13b6a66f..6dd75034 100644 --- a/reviewers/templates/reviewers/extensions_review_list.html +++ b/reviewers/templates/reviewers/extensions_review_list.html @@ -36,9 +36,9 @@ - {% 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 %} diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index d2028a1d..0bc391bc 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -13,7 +13,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', diff --git a/reviewers/views.py b/reviewers/views.py index a119e363..b11a79a8 100644 --- a/reviewers/views.py +++ b/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 = {} -- 2.30.2 From 9362feb2ddecf268bd4a7c6b2daf956e9bbfc965 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 23 Aug 2024 14:03:50 +0200 Subject: [PATCH 2/4] fix signal logic --- reviewers/signals.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/reviewers/signals.py b/reviewers/signals.py index 4b13c815..9ed8ec91 100644 --- a/reviewers/signals.py +++ b/reviewers/signals.py @@ -52,17 +52,21 @@ def _update_approval_queue( return if not created: return - if instance.type not in ApprovalActivity.STATUS_CHANGE_TYPES: - return + activity_count = ApprovalActivity.objects.filter(extension=instance.extension).count() - ApprovalQueue.objects.update_or_create( - extension=instance.extension, - defaults={ - 'activity_count': activity_count, - 'latest_activity': instance, - 'sortkey': instance.queue_sortkey, - }, - ) + 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) -- 2.30.2 From f7d26d7298a23dbc12c7cf23c7e64e64e73b0466 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 23 Aug 2024 14:15:10 +0200 Subject: [PATCH 3/4] sortkey doc --- reviewers/models.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/reviewers/models.py b/reviewers/models.py index 0f3efc94..29c008e5 100644 --- a/reviewers/models.py +++ b/reviewers/models.py @@ -58,6 +58,16 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model): @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 -- 2.30.2 From c0b3fb3d02349482c43d267e44caad57abc41786 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 23 Aug 2024 15:09:49 +0200 Subject: [PATCH 4/4] basic queue ordering test --- requirements_dev.txt | 1 + reviewers/tests/test_views.py | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/requirements_dev.txt b/requirements_dev.txt index e589676c..47bb70ca 100644 --- a/requirements_dev.txt +++ b/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 diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index 0bc391bc..fa5f4a70 100644 --- a/reviewers/tests/test_views.py +++ b/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 @@ -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) -- 2.30.2