Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record counts for review queues, including detail per promoted class #22906

Merged
merged 12 commits into from
Dec 9, 2024
15 changes: 15 additions & 0 deletions src/olympia/amo/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,21 @@ def only_translations(self):
qs = qs.transform(transformer.get_trans)
return qs

def optimized_count(self):
"""
Slightly optimized count() for cases where there is a DISTINCT in the
queryset.

When a count() call is made on a queryset that has a distinct, that
causes django to run the full SELECT (including all fields, distinct,
ordering etc) in a subquery and then COUNT() on the result of that
subquery, which is costly/innefficient. That's tracked in
https://code.djangoproject.com/ticket/30685.
We can't easily fix the fact that there is a subquery, but we can
avoid selecting all fields and ordering in that subquery needlessly.
"""
return self.values('pk').order_by().count()


class RawQuerySet(models.query.RawQuerySet):
"""A RawQuerySet with __len__."""
Expand Down
13 changes: 7 additions & 6 deletions src/olympia/lib/settings_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1500,17 +1500,18 @@ def read_only_mode(env):
# List all jobs that should be callable with cron here.
# syntax is: job_and_method_name: full.package.path
CRON_JOBS = {
'update_addon_average_daily_users': 'olympia.addons.cron',
'update_addon_weekly_downloads': 'olympia.addons.cron',
'addon_last_updated': 'olympia.addons.cron',
'update_addon_hotness': 'olympia.addons.cron',
'gc': 'olympia.amo.cron',
'write_sitemaps': 'olympia.amo.cron',
'process_blocklistsubmissions': 'olympia.blocklist.cron',
'upload_mlbf_to_remote_settings': 'olympia.blocklist.cron',
'record_reviewer_queues_counts': 'olympia.reviewers.cron',
'sync_suppressed_emails_cron': 'olympia.users.cron',
'update_addon_average_daily_users': 'olympia.addons.cron',
'update_addon_hotness': 'olympia.addons.cron',
'update_addon_weekly_downloads': 'olympia.addons.cron',
'update_blog_posts': 'olympia.devhub.cron',
'update_user_ratings': 'olympia.users.cron',
'sync_suppressed_emails_cron': 'olympia.users.cron',
'upload_mlbf_to_remote_settings': 'olympia.blocklist.cron',
'write_sitemaps': 'olympia.amo.cron',
}

RECOMMENDATION_ENGINE_URL = env(
Expand Down
4 changes: 2 additions & 2 deletions src/olympia/ratings/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import olympia.core.logger
from olympia import activity, amo, core
from olympia.amo.fields import PositiveAutoField
from olympia.amo.models import ManagerBase, ModelBase
from olympia.amo.models import BaseQuerySet, ManagerBase, ModelBase
from olympia.amo.templatetags import jinja_helpers
from olympia.amo.utils import send_mail_jinja
from olympia.translations.templatetags.jinja_helpers import truncate
Expand All @@ -19,7 +19,7 @@
log = olympia.core.logger.getLogger('z.ratings')


class RatingQuerySet(models.QuerySet):
class RatingQuerySet(BaseQuerySet):
"""
A queryset modified for soft deletion.
"""
Expand Down
30 changes: 30 additions & 0 deletions src/olympia/reviewers/cron.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from datetime import date

from olympia.constants.promoted import NOT_PROMOTED, PROMOTED_GROUPS
from olympia.reviewers.models import QueueCount
from olympia.reviewers.utils import PendingManualApprovalQueueTable
from olympia.reviewers.views import reviewer_tables_registry


def record_reviewer_queues_counts():
today = date.today()
# Grab a queryset for each reviewer queue.
querysets = {
queue.name: queue.get_queryset(None)
for queue in reviewer_tables_registry.values()
}
# Also drill down manual review queue by promoted class (there is no real
# queue for each, but we still want that data).
for group in PROMOTED_GROUPS:
if group != NOT_PROMOTED:
querysets[f'{PendingManualApprovalQueueTable.name}/{group.api_name}'] = (
PendingManualApprovalQueueTable.get_queryset(
None
).filter(promotedaddon__group_id=group.id)
)

# Execute a count for each queryset and record a QueueCount instance for it
for key, qs in querysets.items():
QueueCount.objects.get_or_create(
name=key, date=today, defaults={'value': qs.optimized_count()}
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Generated by Django 4.2.16 on 2024-12-05 12:00

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('reviewers', '0038_auto_20240909_1647'),
]

operations = [
migrations.AlterField(
model_name='autoapprovalsummary',
name='verdict',
field=models.PositiveSmallIntegerField(choices=[(0, 'Would *not* have been auto-approved (dry-run mode was in effect)'), (1, 'Would have been auto-approved (dry-run mode was in effect)'), (2, 'Was auto-approved'), (3, 'Was *not* auto-approved')], default=3),
),
migrations.AlterField(
model_name='needshumanreview',
name='reason',
field=models.SmallIntegerField(choices=[(0, 'Unknown'), (1, 'Hit scanner rule'), (2, 'Was added to a promoted group'), (3, 'Over growth threshold for usage tier'), (4, 'Previous version in channel had needs human review set'), (5, 'Sources provided while pending rejection'), (6, 'Developer replied'), (7, 'Manually set as needing human review by a reviewer'), (8, 'Auto-approved but still had an approval delay set in the past'), (9, 'Over abuse reports threshold for usage tier'), (10, 'Escalated for an abuse report, via cinder'), (11, 'Reported for abuse within the add-on'), (12, "Appeal of a reviewer's decision about a policy violation"), (13, 'Has auto-approval disabled'), (14, 'Belongs to a promoted group')], default=0, editable=False),
),
migrations.AlterField(
model_name='usagetier',
name='growth_threshold_before_flagging',
field=models.IntegerField(blank=True, default=None, help_text='Percentage point increase in growth over the average in that tier before we start automatically flagging the add-on for human review. For example, if set to 50 and the average hotness in that tier is 0.01, add-ons over 0.51 hotness get flagged.', null=True),
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Generated by Django 4.2.16 on 2024-12-05 12:01

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('reviewers', '0039_alter_autoapprovalsummary_verdict_and_more'),
]

operations = [
migrations.CreateModel(
name='QueueCount',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('date', models.DateField(auto_now_add=True)),
('name', models.CharField(max_length=255)),
('value', models.IntegerField()),
],
),
migrations.AddConstraint(
model_name='queuecount',
constraint=models.UniqueConstraint(fields=('name', 'date'), name='queue_count_unique_name_date'),
),
]
16 changes: 16 additions & 0 deletions src/olympia/reviewers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -951,3 +951,19 @@ def update_due_date_for_needs_human_review_change(
):
if update_fields is None or 'is_active' in update_fields:
instance.version.reset_due_date()


class QueueCount(models.Model):
date = models.DateField(auto_now_add=True)
name = models.CharField(max_length=255)
value = models.IntegerField()

class Meta:
constraints = [
models.UniqueConstraint(
fields=('name', 'date'), name='queue_count_unique_name_date'
),
]

def __str__(self):
return f'{self.name} for {self.date}: {self.value}'
6 changes: 3 additions & 3 deletions src/olympia/reviewers/templates/reviewers/queue.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
{% endif %}
{% endblock %}

{% block bodyclass %}{{ super() }} {{ " content-review" if tab == "content_review" else "" }}{% endblock %}
{% block bodyclass %}{{ super() }} {{ " content-review" if tab == "queue_content_review" else "" }}{% endblock %}

{% block content %}

Expand All @@ -24,7 +24,7 @@
{{ page|paginator }}
</div>

{% if tab == 'moderated' %}
{% if tab == 'queue_moderated' %}
<div id="reviews-flagged">
<form method="post" class="item">
<div class="review-saved">
Expand Down Expand Up @@ -77,7 +77,7 @@ <h3>
{% endif %}
</form>
</div>
{% elif tab == 'held_decisions' %}
{% elif tab == 'queue_decisions' %}
<table id="held-decision-queue" class="data-grid">
<thead>
<tr class="listing-header">
Expand Down
22 changes: 3 additions & 19 deletions src/olympia/reviewers/templatetags/jinja_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,9 @@ def queue_tabnav(context, reviewer_tables_registry):
request = context['request']
tabnav = []

for queue in (
'extension',
'mad',
'theme',
'moderated',
'content_review',
'pending_rejection',
'held_decisions',
):
if acl.action_allowed_for(
request.user, reviewer_tables_registry[queue].permission
):
tabnav.append(
(
queue,
reviewer_tables_registry[queue].urlname,
reviewer_tables_registry[queue].title,
)
)
for tab, queue in reviewer_tables_registry.items():
if acl.action_allowed_for(request.user, queue.permission):
tabnav.append((tab, queue.name, queue.title))

return tabnav

Expand Down
147 changes: 147 additions & 0 deletions src/olympia/reviewers/tests/test_cron.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
from datetime import date

from django.conf import settings

from freezegun import freeze_time

from olympia import amo
from olympia.amo.tests import TestCase, addon_factory, user_factory
from olympia.constants.promoted import NOTABLE, PROMOTED_GROUPS, RECOMMENDED
from olympia.reviewers.cron import record_reviewer_queues_counts
from olympia.reviewers.models import NeedsHumanReview, QueueCount
from olympia.reviewers.views import reviewer_tables_registry


class TestQueueCount(TestCase):
def setUp(self):
user_factory(pk=settings.TASK_USER_ID)

def _test_expected_count(self, date):
# We are recording every queue, plus drilling down in every promoted
# group, minus the special not promoted group.
expected_count = len(reviewer_tables_registry) + len(PROMOTED_GROUPS) - 1
assert QueueCount.objects.filter(date=date).count() == expected_count

def test_empty(self):
with freeze_time('2024-12-03'):
expected_date = date.today()
record_reviewer_queues_counts()

self._test_expected_count(expected_date)

for metric in QueueCount.objects.all():
assert metric.date == expected_date
assert metric.name
assert metric.value == 0

def test_basic(self):
addon_factory()
addon_factory(
needshumanreview_kw={'reason': NeedsHumanReview.REASONS.UNKNOWN},
)
addon_factory(
file_kw={'status': amo.STATUS_AWAITING_REVIEW},
needshumanreview_kw={
'reason': NeedsHumanReview.REASONS.AUTO_APPROVAL_DISABLED
},
)
self.addon_recommended_1 = addon_factory(
file_kw={'status': amo.STATUS_AWAITING_REVIEW},
promoted=RECOMMENDED,
needshumanreview_kw={
'reason': NeedsHumanReview.REASONS.BELONGS_TO_PROMOTED_GROUP
},
)
addon_factory(
file_kw={'status': amo.STATUS_AWAITING_REVIEW},
promoted=RECOMMENDED,
needshumanreview_kw={
'reason': NeedsHumanReview.REASONS.BELONGS_TO_PROMOTED_GROUP
},
)
addon_factory(
file_kw={'status': amo.STATUS_AWAITING_REVIEW},
promoted=NOTABLE,
needshumanreview_kw={
'reason': NeedsHumanReview.REASONS.BELONGS_TO_PROMOTED_GROUP
},
)

with freeze_time('2024-12-03'):
expected_date = date.today()
record_reviewer_queues_counts()

self._test_expected_count(expected_date)

metric = QueueCount.objects.get(name='queue_extension')
assert metric.date == expected_date
assert metric.value == 5

metric = QueueCount.objects.get(name='queue_extension/recommended')
assert metric.date == expected_date
assert metric.value == 2

metric = QueueCount.objects.get(name='queue_extension/notable')
assert metric.date == expected_date
assert metric.value == 1

def test_twice_same_date_doesnt_override(self):
self.test_basic()
self.test_basic()

def test_twice_different_day(self):
self.test_basic()
previous_date = QueueCount.objects.latest('pk').date

self.addon_recommended_1.current_version.file.update(status=amo.STATUS_APPROVED)
self.addon_recommended_1.current_version.needshumanreview_set.all()[0].update(
is_active=False
)

with freeze_time('2024-12-04'):
expected_date = date.today()
record_reviewer_queues_counts()

# Previous date records are not affected.
self._test_expected_count(previous_date)
assert (
QueueCount.objects.get(date=previous_date, name='queue_extension').value
== 5
)
assert (
QueueCount.objects.get(
date=previous_date, name='queue_extension/recommended'
).value
== 2
)
assert (
QueueCount.objects.get(
date=previous_date, name='queue_extension/notable'
).value
== 1
)

# New date records
self._test_expected_count(expected_date)

# One fewer add-on in the queue.
assert (
QueueCount.objects.get(date=expected_date, name='queue_extension').value
== 4
)

# One fewer add-on in the queue that was recommended.
assert (
QueueCount.objects.get(
date=expected_date, name='queue_extension/recommended'
).value
== 1
)

# No changes to notable.
assert (
QueueCount.objects.get(
date=expected_date, name='queue_extension/notable'
).value
== 1
)
Loading
Loading