ApprovalQueue: use a materialized table #240

Merged
Oleg-Komarov merged 4 commits from approval-queue-fix into main 2024-08-23 15:11:46 +02:00
8 changed files with 242 additions and 59 deletions

View File

@ -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

View File

@ -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),
]

View File

@ -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']),
]

View File

@ -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()

View File

@ -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>

View File

@ -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>

View File

@ -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)

View File

@ -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(
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('-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
)
.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 = {}