ApprovalQueue: use a materialized table #240
@ -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
|
||||
|
52
reviewers/migrations/0012_approvalqueue.py
Normal file
52
reviewers/migrations/0012_approvalqueue.py
Normal 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),
|
||||
]
|
@ -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']),
|
||||
]
|
||||
|
@ -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()
|
||||
|
@ -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>
|
||||
|
@ -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>
|
||||
|
@ -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)
|
||||
|
@ -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 = {}
|
||||
|
Loading…
Reference in New Issue
Block a user