ApprovalQueue: use a materialized table #240
@ -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
|
||||||
|
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):
|
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']),
|
||||||
|
]
|
||||||
|
@ -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()
|
||||||
|
@ -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>
|
||||||
|
@ -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>
|
||||||
|
@ -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)
|
||||||
|
@ -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 = {}
|
||||||
|
Loading…
Reference in New Issue
Block a user