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 factory-boy==3.2.1
Faker==13.15.1 Faker==13.15.1
filelock==3.7.1 filelock==3.7.1
freezegun==1.5.1
identify==2.5.2 identify==2.5.2
mdgen==0.1.10 mdgen==0.1.10
nodeenv==1.7.0 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): class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
STATUS_CHANGE_TYPES = {"AWR": 0x02, "AWC": 0x01, "APR": 0x00}
class ActivityType(models.TextChoices): class ActivityType(models.TextChoices):
COMMENT = "COM", _("Comment") COMMENT = "COM", _("Comment")
APPROVED = "APR", _("Approved") APPROVED = "APR", _("Approved")
@ -53,3 +55,37 @@ class ApprovalActivity(CreatedModifiedMixin, RecordDeletionMixin, models.Model):
def __str__(self): def __str__(self):
return f"{self.extension.name}: {self.get_type_display()}" 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 django.dispatch import receiver
from constants.activity import Flag, Verb from constants.activity import Flag, Verb
from reviewers.models import ApprovalActivity from reviewers.models import ApprovalActivity, ApprovalQueue
@receiver(post_save, sender=ApprovalActivity) @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) @receiver(pre_delete, sender=ApprovalActivity)
def _log_deletion(sender: object, instance: ApprovalActivity, **kwargs: object) -> None: def _log_deletion(sender: object, instance: ApprovalActivity, **kwargs: object) -> None:
instance.record_deletion() instance.record_deletion()

View File

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

View File

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

View File

@ -1,7 +1,11 @@
from datetime import datetime, timedelta, timezone
from django.test import TestCase from django.test import TestCase
from django.shortcuts import reverse from django.shortcuts import reverse
from freezegun import freeze_time
from common.tests.factories.extensions import create_version from common.tests.factories.extensions import create_version
from common.tests.factories.users import UserFactory
from files.models import File from files.models import File
from reviewers.models import ApprovalActivity from reviewers.models import ApprovalActivity
@ -13,7 +17,7 @@ class CommentsViewTest(TestCase):
version = create_version(status=File.STATUSES.AWAITING_REVIEW) version = create_version(status=File.STATUSES.AWAITING_REVIEW)
self.default_version = version self.default_version = version
ApprovalActivity( ApprovalActivity(
type=ApprovalActivity.ActivityType.COMMENT, type=ApprovalActivity.ActivityType.AWAITING_REVIEW,
user=version.files.first().user, user=version.files.first().user,
extension=version.extension, extension=version.extension,
message='test comment', message='test comment',
@ -40,3 +44,97 @@ class CommentsViewTest(TestCase):
# Display only previews and version that are 'pending review' # Display only previews and version that are 'pending review'
# Approval requires manager role # 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 import logging
from actstream.actions import follow, is_following, unfollow from actstream.actions import follow, is_following, unfollow
@ -11,62 +10,30 @@ import django.forms
from constants.activity import Flag from constants.activity import Flag
from extensions.models import Extension from extensions.models import Extension
from reviewers.forms import CommentForm from reviewers.forms import CommentForm
from reviewers.models import ApprovalActivity from reviewers.models import ApprovalActivity, ApprovalQueue
log = logging.getLogger(__name__) 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): class ApprovalQueueView(ListView):
model = Extension model = Extension
paginate_by = 50 paginate_by = 50
def get_queryset(self): def get_queryset(self):
qs = ApprovalActivity.objects.prefetch_related( return (
'extension', ApprovalQueue.objects.select_related(
'extension__authors', 'extension',
'extension__icon', 'latest_activity',
'extension__versions', )
'extension__versions__files', .prefetch_related(
'extension__versions__files__validation', 'extension__authors',
).order_by('-id') 'extension__icon',
by_extension = {} 'extension__versions',
by_date_created = [] 'extension__versions__files',
for item in qs: 'extension__versions__files__validation',
extension = item.extension )
stats = by_extension.get(extension, None) .order_by('-sortkey')
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
template_name = 'reviewers/extensions_review_list.html' template_name = 'reviewers/extensions_review_list.html'
@ -102,7 +69,7 @@ class ExtensionsApprovalDetailView(DetailView):
activity.user_is_maintainer = True activity.user_is_maintainer = True
ctx['review_activity'] = review_activity 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: if self.request.user.is_authenticated:
initial = {} initial = {}