Improve performance of `CinderJob.resolvable_in_reviewer_tools()` / `CinderJob.for_addon()` (#21882)

* Improve performance of CinderJob.resolvable_in_reviewer_tools() / CinderJob.for_addon()

Add denormalized fields to help figure out whether a CinderJob is for a particular
add-on and whether it's meant for AMO reviewers, which greatly simplifies the
corresponding queries, improving their performance.

- `target_addon` is set at reporting time and never changes after
- `resolvable_in_reviewer_tools` is also set at reporting time but can be
   flipped when processing escalations from Cinder.
This commit is contained in:
Mathieu Pillard 2024-02-20 13:14:29 +01:00 коммит произвёл GitHub
Родитель 16d545453f
Коммит 73500c8fa8
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: B5690EEEBB952194
10 изменённых файлов: 357 добавлений и 51 удалений

Просмотреть файл

@ -0,0 +1,30 @@
from django.core.management.base import BaseCommand
from olympia.abuse.models import CinderJob
class Command(BaseCommand):
help = (
'One-off command to fill CinderJob.target_addon and '
'CinderJob.resolvable_in_reviewer_tools fields'
)
def handle(self, *args, **options):
jobs = CinderJob.objects.all()
for job in jobs:
if abuse_report := job.initial_abuse_report:
job.target_addon = (
# If the abuse report is against a guid, set target_addon
# on the job accordingly.
abuse_report.guid and abuse_report.target
)
job.resolvable_in_reviewer_tools = (
# If the abuse report was meant to be handled by reviewers
# from the start or it's been escalated, set
# resolvable_in_reviewer_tools accordingly.
abuse_report.is_handled_by_reviewers
or job.decision_action
== CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON
)
job.save()

Просмотреть файл

@ -0,0 +1,29 @@
# Generated by Django 4.2.9 on 2024-02-19 10:53
from django.db import migrations, models
import django.db.models.deletion
class Migration(migrations.Migration):
dependencies = [
("addons", "0049_clear_bad_url_data"),
("abuse", "0024_cinderpolicy_parent_alter_cinderpolicy_uuid"),
]
operations = [
migrations.AddField(
model_name="cinderjob",
name="resolvable_in_reviewer_tools",
field=models.BooleanField(default=None, null=True),
),
migrations.AddField(
model_name="cinderjob",
name="target_addon",
field=models.ForeignKey(
blank=True,
null=True,
on_delete=django.db.models.deletion.SET_NULL,
to="addons.addon",
),
),
]

Просмотреть файл

@ -3,7 +3,6 @@ from itertools import chain
from django.core.exceptions import ImproperlyConfigured
from django.db import models
from django.db.models import Q
from django.db.transaction import atomic
from olympia import amo
@ -41,14 +40,7 @@ from .utils import (
class CinderJobQuerySet(BaseQuerySet):
def for_addon(self, addon):
return (
self.filter(
Q(abusereport__guid=addon.addonguid_guid)
| Q(appealed_jobs__abusereport__guid=addon.addonguid_guid)
)
.order_by('-created')
.distinct()
)
return self.filter(target_addon=addon).order_by('-pk')
def unresolved(self):
return self.filter(
@ -56,23 +48,7 @@ class CinderJobQuerySet(BaseQuerySet):
)
def resolvable_in_reviewer_tools(self):
# Note this isn't quite the same as AbuseReport.is_handled_by_reviewers
# - it doesn't verify the guids are valid add-ons,
# - and includes escalations too.
def get_filter_fields(prefix):
filter_fields = (
('__reason__in', AbuseReport.REASONS.REVIEWER_HANDLED.values),
('__location__in', AbuseReport.LOCATION.REVIEWER_HANDLED.values),
)
return {prefix + key: tuple(val) for key, val in filter_fields}
return self.exclude(
abusereport__guid=None, appealed_jobs__abusereport__guid=None
).filter(
Q(**get_filter_fields('abusereport'))
| Q(**get_filter_fields('appealed_jobs__abusereport'))
| Q(decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON)
)
return self.filter(resolvable_in_reviewer_tools=True)
class CinderJobManager(ManagerBase):
@ -146,11 +122,17 @@ class CinderJob(ModelBase):
# appeal for multiple previous decisions (jobs).
related_name='appealed_jobs',
)
target_addon = models.ForeignKey(
to=Addon, blank=True, null=True, on_delete=models.deletion.SET_NULL
)
resolvable_in_reviewer_tools = models.BooleanField(default=None, null=True)
objects = CinderJobManager()
@property
def target(self):
if self.target_addon_id:
return self.target_addon
# this works because all abuse reports for a single job and all appeals
# for a single job are for the same target.
if initial_abuse_report := self.initial_abuse_report:
@ -309,8 +291,13 @@ class CinderJob(ModelBase):
reporter_entity = cls.get_cinder_reporter(abuse_report)
entity_helper = cls.get_entity_helper(abuse_report)
job_id = entity_helper.report(report=report_entity, reporter=reporter_entity)
target = abuse_report.target
defaults = {
'target_addon': target if isinstance(target, Addon) else None,
'resolvable_in_reviewer_tools': abuse_report.is_handled_by_reviewers,
}
with atomic():
cinder_job, _ = cls.objects.get_or_create(job_id=job_id)
cinder_job, _ = cls.objects.get_or_create(job_id=job_id, defaults=defaults)
abuse_report.update(cinder_job=cinder_job)
# Additional context can take a while, so it is reported outside the
# atomic() block so that the transaction can be committed quickly,
@ -339,6 +326,8 @@ class CinderJob(ModelBase):
decision_notes=decision_notes[
: self._meta.get_field('decision_notes').max_length
],
resolvable_in_reviewer_tools=self.resolvable_in_reviewer_tools
or decision_action == self.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
)
policies = CinderPolicy.objects.filter(
uuid__in=policy_ids
@ -390,7 +379,13 @@ class CinderJob(ModelBase):
appealer=appealer_entity,
)
with atomic():
appeal_job, _ = self.__class__.objects.get_or_create(job_id=appeal_id)
appeal_job, _ = self.__class__.objects.get_or_create(
job_id=appeal_id,
defaults={
'target_addon': self.target_addon,
'resolvable_in_reviewer_tools': self.resolvable_in_reviewer_tools,
},
)
self.update(appeal_job=appeal_job)
if is_reporter:
abuse_report.update(
@ -428,7 +423,7 @@ class CinderJob(ModelBase):
action_helper.notify_reporters()
if not getattr(log_entry.log, 'hide_developer', False):
action_helper.notify_owners(policy_text=log_entry.details.get('comments'))
if (report := self.initial_abuse_report) and report.is_handled_by_reviewers:
if self.resolvable_in_reviewer_tools:
entity_helper.close_job(job_id=self.job_id)

Просмотреть файл

@ -0,0 +1,106 @@
from django.core.management import call_command
from olympia.abuse.models import AbuseReport, CinderJob
from olympia.amo.tests import TestCase, addon_factory, user_factory
class TestFillCinderJob(TestCase):
def test_fill_somehow_no_abuse_reports(self):
job = CinderJob.objects.create(job_id='job123')
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert not job.target_addon
assert not job.resolvable_in_reviewer_tools
def test_fill_addon(self):
addon_factory() # Extra add-on, shouldn't matter.
addon = addon_factory()
job = CinderJob.objects.create(job_id='job123')
report = AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert job.target_addon == report.target == addon
assert not job.resolvable_in_reviewer_tools
def test_fill_appealed_job(self):
addon_factory() # Extra add-on, shouldn't matter.
addon = addon_factory()
job = CinderJob.objects.create(
job_id='job123', appeal_job=CinderJob.objects.create(job_id='appeal123')
)
report = AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert job.target_addon == report.target == addon
assert not job.resolvable_in_reviewer_tools
job.appeal_job.reload()
assert job.appeal_job.target_addon == report.target == addon
assert not job.appeal_job.resolvable_in_reviewer_tools
def test_fill_non_addon(self):
user = user_factory()
job = CinderJob.objects.create(job_id='job123')
AbuseReport.objects.create(user=user, cinder_job=job)
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert job.target_addon is None
assert not job.resolvable_in_reviewer_tools
def test_fill_resolvable_in_reviewer_tools(self):
addon_factory() # Extra add-on, shouldn't matter.
addon = addon_factory()
job = CinderJob.objects.create(job_id='job123')
report = AbuseReport.objects.create(
guid=addon.guid,
cinder_job=job,
location=AbuseReport.LOCATION.BOTH,
reason=AbuseReport.REASONS.POLICY_VIOLATION,
)
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert job.target_addon == report.target == addon
assert job.resolvable_in_reviewer_tools
def test_fill_not_resolvable_in_reviewer_tools(self):
addon_factory() # Extra add-on, shouldn't matter.
addon = addon_factory()
job = CinderJob.objects.create(job_id='job123')
# Location makes it not resolvable in reviewer tools unless escalated
# even though the reason is policy violation.
report = AbuseReport.objects.create(
guid=addon.guid,
cinder_job=job,
location=AbuseReport.LOCATION.AMO,
reason=AbuseReport.REASONS.POLICY_VIOLATION,
)
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert job.target_addon == report.target == addon
assert not job.resolvable_in_reviewer_tools
def test_fill_escalated_addon(self):
addon_factory() # Extra add-on, shouldn't matter.
addon = addon_factory()
job = CinderJob.objects.create(
job_id='job123',
decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
)
report = AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
call_command('fill_cinderjobs_denormalized_fields')
job.reload()
assert job.target_addon == report.target == addon
assert job.resolvable_in_reviewer_tools

Просмотреть файл

@ -362,25 +362,20 @@ class TestAbuseManager(TestCase):
class TestCinderJobManager(TestCase):
def test_for_addon_finds_by_guid(self):
def test_for_addon(self):
addon = addon_factory()
job = CinderJob.objects.create()
AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
AbuseReport.objects.create(guid=addon.guid, cinder_job=job)
assert list(CinderJob.objects.for_addon(addon)) == [job]
def test_for_addon_finds_by_original_guid(self):
addon = addon_factory(guid='foo@bar')
addon.update(guid='guid-reused-by-pk-42')
job = CinderJob.objects.create()
AbuseReport.objects.create(guid='foo@bar', cinder_job=job)
assert list(CinderJob.objects.for_addon(addon)) == []
job.update(target_addon=addon)
assert list(CinderJob.objects.for_addon(addon)) == [job]
def test_for_addon_appealed(self):
addon = addon_factory()
appeal_job = CinderJob.objects.create(job_id='appeal')
appeal_job = CinderJob.objects.create(job_id='appeal', target_addon=addon)
original_job = CinderJob.objects.create(
job_id='original', appeal_job=appeal_job
job_id='original',
appeal_job=appeal_job,
target_addon=addon,
)
AbuseReport.objects.create(guid=addon.guid, cinder_job=original_job)
assert list(CinderJob.objects.for_addon(addon)) == [original_job, appeal_job]
@ -419,15 +414,21 @@ class TestCinderJobManager(TestCase):
cinder_job=CinderJob.objects.create(job_id=3),
)
qs = CinderJob.objects.resolvable_in_reviewer_tools()
assert list(qs) == []
job.update(resolvable_in_reviewer_tools=True)
qs = CinderJob.objects.resolvable_in_reviewer_tools()
assert list(qs) == [job]
appeal_job = CinderJob.objects.create(job_id=4)
appeal_job = CinderJob.objects.create(
job_id=4, resolvable_in_reviewer_tools=True
)
job.update(appeal_job=appeal_job)
qs = CinderJob.objects.resolvable_in_reviewer_tools()
assert list(qs) == [job, appeal_job]
not_policy_report.cinder_job.update(
decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON
decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
resolvable_in_reviewer_tools=True,
)
qs = CinderJob.objects.resolvable_in_reviewer_tools()
assert list(qs) == [not_policy_report.cinder_job, job, appeal_job]
@ -458,6 +459,11 @@ class TestCinderJob(TestCase):
appeal_appeal_job.target == appeal_job.target == cinder_job.target == addon
)
def test_target_addon(self):
addon = addon_factory()
cinder_job = CinderJob.objects.create(job_id='1234', target_addon=addon)
assert cinder_job.target == addon
def test_initial_abuse_report(self):
cinder_job = CinderJob.objects.create(job_id='1234')
assert cinder_job.initial_abuse_report is None
@ -578,6 +584,8 @@ class TestCinderJob(TestCase):
cinder_job = CinderJob.objects.get()
assert cinder_job.job_id == '1234-xyz'
assert cinder_job.abusereport_set.get() == abuse_report
assert cinder_job.target_addon == abuse_report.target
assert not cinder_job.resolvable_in_reviewer_tools
# And check if we get back the same job_id for a subsequent report we update
@ -588,6 +596,41 @@ class TestCinderJob(TestCase):
cinder_job.reload()
assert CinderJob.objects.count() == 1
assert list(cinder_job.abusereport_set.all()) == [abuse_report, another_report]
assert cinder_job.target_addon == abuse_report.target
assert not cinder_job.resolvable_in_reviewer_tools
def test_report_resolvable_in_reviewer_tools(self):
abuse_report = AbuseReport.objects.create(
guid=addon_factory().guid,
reason=AbuseReport.REASONS.POLICY_VIOLATION,
location=AbuseReport.LOCATION.ADDON,
)
responses.add(
responses.POST,
f'{settings.CINDER_SERVER_URL}create_report',
json={'job_id': '1234-xyz'},
status=201,
)
CinderJob.report(abuse_report)
cinder_job = CinderJob.objects.get()
assert cinder_job.job_id == '1234-xyz'
assert cinder_job.abusereport_set.get() == abuse_report
assert cinder_job.target_addon == abuse_report.target
assert cinder_job.resolvable_in_reviewer_tools
# And check if we get back the same job_id for a subsequent report we update
another_report = AbuseReport.objects.create(
guid=addon_factory().guid, reason=AbuseReport.REASONS.ILLEGAL
)
CinderJob.report(another_report)
cinder_job.reload()
assert CinderJob.objects.count() == 1
assert list(cinder_job.abusereport_set.all()) == [abuse_report, another_report]
assert cinder_job.target_addon == abuse_report.target
assert cinder_job.resolvable_in_reviewer_tools
def test_get_action_helper(self):
DECISION_ACTIONS = CinderJob.DECISION_ACTIONS
@ -699,15 +742,38 @@ class TestCinderJob(TestCase):
assert notify_mock.call_count == 1
assert list(cinder_job.policies.all()) == [policy]
def test_process_decision_escalate_addon(self):
addon = addon_factory()
cinder_job = CinderJob.objects.create(job_id='1234', target_addon=addon)
assert not cinder_job.resolvable_in_reviewer_tools
new_date = datetime(2024, 1, 1)
cinder_job.process_decision(
decision_id='12345',
decision_date=new_date,
decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
decision_notes='blah',
policy_ids=[],
)
assert cinder_job.decision_id == '12345'
assert cinder_job.decision_date == new_date
assert (
cinder_job.decision_action == CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON
)
assert cinder_job.decision_notes == 'blah'
assert cinder_job.resolvable_in_reviewer_tools
assert cinder_job.target_addon == addon
def test_appeal_as_target(self):
addon = addon_factory()
abuse_report = AbuseReport.objects.create(
guid=addon_factory().guid,
guid=addon.guid,
reason=AbuseReport.REASONS.ILLEGAL,
reporter=user_factory(),
cinder_job=CinderJob.objects.create(
decision_id='4815162342-lost',
decision_date=self.days_ago(179),
decision_action=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
target_addon=addon,
),
)
assert not abuse_report.reporter_appeal_date
@ -728,13 +794,15 @@ class TestCinderJob(TestCase):
abuse_report.cinder_job.reload()
assert abuse_report.cinder_job.appeal_job_id
assert abuse_report.cinder_job.appeal_job.job_id == '2432615184-tsol'
assert abuse_report.cinder_job.appeal_job.target_addon == addon
abuse_report.reload()
assert not abuse_report.reporter_appeal_date
assert not abuse_report.appellant_job
def test_appeal_as_reporter(self):
addon = addon_factory()
abuse_report = AbuseReport.objects.create(
guid=addon_factory().guid,
guid=addon.guid,
reason=AbuseReport.REASONS.ILLEGAL,
reporter=user_factory(),
)
@ -743,6 +811,7 @@ class TestCinderJob(TestCase):
decision_id='4815162342-lost',
decision_date=self.days_ago(179),
decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE,
target_addon=addon,
)
)
assert not abuse_report.reporter_appeal_date
@ -763,6 +832,57 @@ class TestCinderJob(TestCase):
abuse_report.cinder_job.reload()
assert abuse_report.cinder_job.appeal_job
assert abuse_report.cinder_job.appeal_job.job_id == '2432615184-tsol'
assert abuse_report.cinder_job.appeal_job.target_addon == addon
abuse_report.reload()
assert abuse_report.appellant_job.job_id == '2432615184-tsol'
assert abuse_report.reporter_appeal_date
def test_appeal_as_reporter_already_appealed(self):
addon = addon_factory()
abuse_report = AbuseReport.objects.create(
guid=addon.guid,
reason=AbuseReport.REASONS.ILLEGAL,
reporter=user_factory(),
)
abuse_report.update(
cinder_job=CinderJob.objects.create(
decision_id='4815162342-lost',
decision_date=self.days_ago(179),
decision_action=CinderJob.DECISION_ACTIONS.AMO_APPROVE,
target_addon=addon,
)
)
# Pretend there was already an appeal job from a different reporter.
# Make that resolvable in reviewer tools as if it had been escalated,
# to ensure the get_or_create() call that we make can't trigger an
# IntegrityError because of the additional parameters (job_id must
# be the only field we use to retrieve the job).
abuse_report.cinder_job.update(
appeal_job=CinderJob.objects.create(
job_id='2432615184-tsol',
target_addon=addon,
resolvable_in_reviewer_tools=True,
)
)
assert not abuse_report.reporter_appeal_date
responses.add(
responses.POST,
f'{settings.CINDER_SERVER_URL}appeal',
json={'external_id': '2432615184-tsol'},
status=201,
)
abuse_report.cinder_job.appeal(
abuse_report=abuse_report,
appeal_text='appeal text',
user=abuse_report.reporter,
is_reporter=True,
)
abuse_report.cinder_job.reload()
assert abuse_report.cinder_job.appeal_job
assert abuse_report.cinder_job.appeal_job.job_id == '2432615184-tsol'
assert abuse_report.cinder_job.appeal_job.target_addon == addon
abuse_report.reload()
assert abuse_report.appellant_job.job_id == '2432615184-tsol'
assert abuse_report.reporter_appeal_date

Просмотреть файл

@ -16,6 +16,7 @@ class Migration(migrations.Migration):
dependencies = [
('addons', '0035_auto_20210812_1726'),
('waffle', '0001_initial'),
]
operations = [migrations.RunPython(create_waffle_switch)]

Просмотреть файл

@ -30,6 +30,7 @@ class Migration(migrations.Migration):
dependencies = [
('addons', '0038_auto_20220530_1639'),
('blocklist', '0027_auto_20220530_1639'),
]
operations = [

Просмотреть файл

@ -12,6 +12,7 @@ class Migration(migrations.Migration):
dependencies = [
('blocklist', '0023_auto_20201103_1108'),
('waffle', '0001_initial'),
]
operations = [

Просмотреть файл

@ -797,7 +797,11 @@ class TestReviewForm(TestCase):
'location': AbuseReport.LOCATION.ADDON,
'reason': AbuseReport.REASONS.POLICY_VIOLATION,
}
cinder_job_2_reports = CinderJob.objects.create(job_id='2 reports')
cinder_job_2_reports = CinderJob.objects.create(
job_id='2 reports',
resolvable_in_reviewer_tools=True,
target_addon=self.addon,
)
AbuseReport.objects.create(
**abuse_kw, cinder_job=cinder_job_2_reports, message='aaa'
)
@ -807,6 +811,8 @@ class TestReviewForm(TestCase):
cinder_job_appealed = CinderJob.objects.create(
job_id='appealed',
decision_action=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
resolvable_in_reviewer_tools=True,
target_addon=self.addon,
)
AbuseReport.objects.create(
**abuse_kw,
@ -814,11 +820,17 @@ class TestReviewForm(TestCase):
message='ccc',
addon_version='1.2',
)
cinder_job_appeal = CinderJob.objects.create(job_id='appeal')
cinder_job_appeal = CinderJob.objects.create(
job_id='appeal',
resolvable_in_reviewer_tools=True,
target_addon=self.addon,
)
cinder_job_appealed.update(appeal_job=cinder_job_appeal)
cinder_job_escalated = CinderJob.objects.create(
job_id='escalated',
decision_action=CinderJob.DECISION_ACTIONS.AMO_ESCALATE_ADDON,
resolvable_in_reviewer_tools=True,
target_addon=self.addon,
)
AbuseReport.objects.create(
**{**abuse_kw, 'location': AbuseReport.LOCATION.AMO},
@ -830,7 +842,11 @@ class TestReviewForm(TestCase):
AbuseReport.objects.create(
**{**abuse_kw, 'location': AbuseReport.LOCATION.AMO},
message='eee',
cinder_job=CinderJob.objects.create(job_id='not reviewer handled'),
cinder_job=CinderJob.objects.create(
job_id='not reviewer handled',
resolvable_in_reviewer_tools=False,
target_addon=self.addon,
),
)
AbuseReport.objects.create(
**{**abuse_kw},
@ -838,6 +854,7 @@ class TestReviewForm(TestCase):
cinder_job=CinderJob.objects.create(
job_id='already resovled',
decision_action=CinderJob.DECISION_ACTIONS.AMO_DISABLE_ADDON,
resolvable_in_reviewer_tools=True,
),
)

Просмотреть файл

@ -5536,7 +5536,9 @@ class TestReview(ReviewBase):
guid=self.addon.guid,
location=AbuseReport.LOCATION.ADDON,
reason=AbuseReport.REASONS.POLICY_VIOLATION,
cinder_job=CinderJob.objects.create(job_id='999'),
cinder_job=CinderJob.objects.create(
job_id='999', target_addon=self.addon, resolvable_in_reviewer_tools=True
),
message='Its baaaad',
)
response = self.client.get(self.url)
@ -5564,7 +5566,9 @@ class TestReview(ReviewBase):
cinder_policy=CinderPolicy.objects.create(),
)
self.addon.update(status=amo.STATUS_APPROVED)
cinder_job = CinderJob.objects.create(job_id='123')
cinder_job = CinderJob.objects.create(
job_id='123', target_addon=self.addon, resolvable_in_reviewer_tools=True
)
AbuseReport.objects.create(
guid=self.addon.guid,
location=AbuseReport.LOCATION.BOTH,
@ -5611,7 +5615,9 @@ class TestReview(ReviewBase):
canned_response='reason',
cinder_policy=CinderPolicy.objects.create(),
)
cinder_job = CinderJob.objects.create(job_id='123')
cinder_job = CinderJob.objects.create(
job_id='123', target_addon=self.addon, resolvable_in_reviewer_tools=True
)
AbuseReport.objects.create(
guid=self.addon.guid,
location=AbuseReport.LOCATION.BOTH,