From 051f4f53b6684e0f895f43d7743e8d2833c32c46 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Fri, 23 Feb 2024 12:59:54 +0100 Subject: [PATCH] Show Cinder Policy for each Review Action Reason and vice versa in admin Also make cinder policy required only if review action reason is active. --- src/olympia/abuse/admin.py | 21 +++++ src/olympia/abuse/tests/test_admin.py | 77 ++++++++++++++++++- src/olympia/reviewers/admin.py | 25 +++++- ..._alter_reviewactionreason_cinder_policy.py | 24 ++++++ src/olympia/reviewers/models.py | 2 +- src/olympia/reviewers/tests/test_admin.py | 60 ++++++++++++++- 6 files changed, 203 insertions(+), 6 deletions(-) create mode 100644 src/olympia/reviewers/migrations/0035_alter_reviewactionreason_cinder_policy.py diff --git a/src/olympia/abuse/admin.py b/src/olympia/abuse/admin.py index 22b479b45166..7df0b82ed861 100644 --- a/src/olympia/abuse/admin.py +++ b/src/olympia/abuse/admin.py @@ -2,6 +2,8 @@ from django.core.paginator import Paginator from django.db.models import Count from django.template import loader +from django.urls import reverse +from django.utils.html import format_html, format_html_join from olympia.addons.models import Addon, AddonApprovalsCounter from olympia.amo.admin import AMOModelAdmin, DateRangeFilter, FakeChoicesMixin @@ -365,6 +367,7 @@ class CinderPolicyAdmin(AMOModelAdmin): 'uuid', 'parent', 'name', + 'linked_review_reasons', 'text', ) ordering = ('parent__name', 'name') @@ -380,6 +383,24 @@ def has_change_permission(self, request, obj=None): def has_delete_permission(self, request, obj=None): return False + @admin.display(ordering='reviewactionreason__name') + def linked_review_reasons(self, obj): + review_reasons = [ + ( + reverse('admin:reviewers_reviewactionreason_change', args=(reason.pk,)), + reason, + ) + for reason in obj.reviewactionreason_set.all() + ] + + return format_html( + '', + format_html_join('\n', '
  • {}
    • ', review_reasons), + ) + + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('reviewactionreason_set') + admin.site.register(AbuseReport, AbuseReportAdmin) admin.site.register(CinderPolicy, CinderPolicyAdmin) diff --git a/src/olympia/abuse/tests/test_admin.py b/src/olympia/abuse/tests/test_admin.py index a81089421209..aef22ab1ad50 100644 --- a/src/olympia/abuse/tests/test_admin.py +++ b/src/olympia/abuse/tests/test_admin.py @@ -1,3 +1,4 @@ +import uuid from datetime import date, datetime from urllib.parse import parse_qsl, urlparse @@ -7,7 +8,7 @@ from pyquery import PyQuery as pq from olympia import amo -from olympia.abuse.models import AbuseReport +from olympia.abuse.models import AbuseReport, CinderPolicy from olympia.addons.models import AddonApprovalsCounter from olympia.amo.tests import ( TestCase, @@ -18,7 +19,7 @@ user_factory, ) from olympia.ratings.models import Rating -from olympia.reviewers.models import AutoApprovalSummary +from olympia.reviewers.models import AutoApprovalSummary, ReviewActionReason from olympia.versions.models import VersionPreview @@ -573,3 +574,75 @@ def test_detail_rating_report(self): assert not doc('.addon-info-and-previews') assert not doc('.field-addon_name') assert self.report_rating.rating.body in doc('.field-rating').text() + + +class TestCinderPolicyAdmin(TestCase): + @classmethod + def setUpTestData(cls): + cls.user = user_factory(email='someone@mozilla.com') + grant_permission(cls.user, '*:*', 'Admins') + + def setUp(self): + self.client.force_login(self.user) + self.list_url = reverse('admin:abuse_cinderpolicy_changelist') + + def test_list_no_permission(self): + user = user_factory(email='nobody@mozilla.com') + self.client.force_login(user) + response = self.client.get(self.list_url) + assert response.status_code == 403 + + def test_list(self): + foo = CinderPolicy.objects.create(name='Foo') + CinderPolicy.objects.create(name='Bar', parent=foo, uuid=uuid.uuid4()) + zab = CinderPolicy.objects.create(name='Zab', parent=foo, uuid=uuid.uuid4()) + lorem = CinderPolicy.objects.create(name='Lorem', uuid=uuid.uuid4()) + CinderPolicy.objects.create(name='Ipsum', uuid=uuid.uuid4()) + ReviewActionReason.objects.create(name='Attached to Zab', cinder_policy=zab) + ReviewActionReason.objects.create(name='Attached to Lorem', cinder_policy=lorem) + ReviewActionReason.objects.create( + name='Also attached to Lorem', cinder_policy=lorem + ) + + with self.assertNumQueries(7): + # - 2 savepoints (tests) + # - 2 current user & groups + # - 1 count cinder policies + # - 1 cinder policies + # - 1 review action reasons + response = self.client.get(self.list_url) + assert response.status_code == 200 + doc = pq(response.content) + assert len(doc('#result_list tbody tr')) == CinderPolicy.objects.count() + assert doc('#result_list td.field-name').text() == 'Foo Ipsum Lorem Bar Zab' + assert ( + doc('#result_list td.field-linked_review_reasons')[2].text_content() + == 'Also attached to Lorem\nAttached to Lorem' + ) + + def test_list_order_by_reviewreason(self): + foo = CinderPolicy.objects.create(name='Foo') + CinderPolicy.objects.create(name='Bar', parent=foo, uuid=uuid.uuid4()) + zab = CinderPolicy.objects.create(name='Zab', parent=foo, uuid=uuid.uuid4()) + lorem = CinderPolicy.objects.create(name='Lorem', uuid=uuid.uuid4()) + CinderPolicy.objects.create(name='Ipsum', uuid=uuid.uuid4()) + ReviewActionReason.objects.create(name='Attached to Zab', cinder_policy=zab) + ReviewActionReason.objects.create(name='Attached to Lorem', cinder_policy=lorem) + + with self.assertNumQueries(7): + # - 2 savepoints (tests) + # - 2 current user & groups + # - 1 count cinder policies + # - 1 cinder policies + # - 1 review action reasons + # Linked reason is the 4th field, so we have to pass o=4 parameter + # to order on it. + response = self.client.get(self.list_url, {'o': '4'}) + assert response.status_code == 200 + doc = pq(response.content) + assert len(doc('#result_list tbody tr')) == CinderPolicy.objects.count() + assert doc('#result_list td.field-name').text() == 'Foo Ipsum Bar Lorem Zab' + assert ( + doc('#result_list td.field-linked_review_reasons')[4].text_content() + == 'Attached to Zab' + ) diff --git a/src/olympia/reviewers/admin.py b/src/olympia/reviewers/admin.py index 5055c39a70a8..d539ac7c4d2d 100644 --- a/src/olympia/reviewers/admin.py +++ b/src/olympia/reviewers/admin.py @@ -1,12 +1,31 @@ +from django import forms from django.contrib import admin from olympia.amo.admin import AMOModelAdmin +from olympia.zadmin.admin import related_single_content_link from .models import NeedsHumanReview, ReviewActionReason, UsageTier +class ReviewActionReasonAdminForm(forms.ModelForm): + def clean(self): + is_active = self.cleaned_data.get('is_active', False) + if is_active and not self.cleaned_data.get('cinder_policy'): + msg = forms.ValidationError( + self.fields['cinder_policy'].error_messages['required'] + ) + self.add_error('cinder_policy', msg) + return self.cleaned_data + + class ReviewActionReasonAdmin(AMOModelAdmin): - list_display = ('name', 'addon_type', 'is_active') + form = ReviewActionReasonAdminForm + list_display = ( + 'name', + 'linked_cinder_policy', + 'addon_type', + 'is_active', + ) list_filter = ( 'addon_type', 'is_active', @@ -21,6 +40,10 @@ class ReviewActionReasonAdmin(AMOModelAdmin): ) raw_id_fields = ('cinder_policy',) view_on_site = False + list_select_related = ('cinder_policy', 'cinder_policy__parent') + + def linked_cinder_policy(self, obj): + return related_single_content_link(obj, 'cinder_policy') admin.site.register(ReviewActionReason, ReviewActionReasonAdmin) diff --git a/src/olympia/reviewers/migrations/0035_alter_reviewactionreason_cinder_policy.py b/src/olympia/reviewers/migrations/0035_alter_reviewactionreason_cinder_policy.py new file mode 100644 index 000000000000..8b68e332b539 --- /dev/null +++ b/src/olympia/reviewers/migrations/0035_alter_reviewactionreason_cinder_policy.py @@ -0,0 +1,24 @@ +# Generated by Django 4.2.9 on 2024-02-23 11:48 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + dependencies = [ + ("abuse", "0025_cinderjob_resolvable_in_reviewer_tools_and_more"), + ("reviewers", "0034_create-specific-cinder-switch"), + ] + + operations = [ + migrations.AlterField( + model_name="reviewactionreason", + name="cinder_policy", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="abuse.cinderpolicy", + ), + ), + ] diff --git a/src/olympia/reviewers/models.py b/src/olympia/reviewers/models.py index 773e82e5e32a..600a7d46eec9 100644 --- a/src/olympia/reviewers/models.py +++ b/src/olympia/reviewers/models.py @@ -697,7 +697,7 @@ class ReviewActionReason(ModelBase): default=amo.ADDON_ANY, ) cinder_policy = models.ForeignKey( - CinderPolicy, null=True, on_delete=models.SET_NULL + CinderPolicy, blank=True, null=True, on_delete=models.SET_NULL ) def labelled_name(self): diff --git a/src/olympia/reviewers/tests/test_admin.py b/src/olympia/reviewers/tests/test_admin.py index ec29b6152894..e6d5128300e4 100644 --- a/src/olympia/reviewers/tests/test_admin.py +++ b/src/olympia/reviewers/tests/test_admin.py @@ -1,3 +1,5 @@ +import uuid + from django.conf import settings from django.contrib import admin from django.contrib.messages.storage import default_storage as default_messages_storage @@ -7,9 +9,16 @@ from pyquery import PyQuery as pq from olympia import core -from olympia.amo.tests import TestCase, addon_factory, user_factory, version_factory +from olympia.abuse.models import CinderPolicy +from olympia.amo.tests import ( + TestCase, + addon_factory, + grant_permission, + user_factory, + version_factory, +) from olympia.reviewers.admin import NeedsHumanReviewAdmin -from olympia.reviewers.models import NeedsHumanReview +from olympia.reviewers.models import NeedsHumanReview, ReviewActionReason class TestNeedsHumanReviewAdmin(TestCase): @@ -148,3 +157,50 @@ def test_activate_selected_action(self): v2.reload() assert nhr2.is_active assert v2.due_date + + +class TestReviewActionReasonAdmin(TestCase): + @classmethod + def setUpTestData(cls): + cls.user = user_factory(email='someone@mozilla.com') + grant_permission(cls.user, '*:*', 'Admins') + + def setUp(self): + self.client.force_login(self.user) + self.list_url = reverse('admin:reviewers_reviewactionreason_changelist') + + def test_list_no_permission(self): + user = user_factory(email='nobody@mozilla.com') + self.client.force_login(user) + response = self.client.get(self.list_url) + assert response.status_code == 403 + + def test_list(self): + foo = CinderPolicy.objects.create(name='Foo') + CinderPolicy.objects.create(name='Bar', parent=foo, uuid=uuid.uuid4()) + zab = CinderPolicy.objects.create(name='Zab', parent=foo, uuid=uuid.uuid4()) + lorem = CinderPolicy.objects.create(name='Lorem', uuid=uuid.uuid4()) + CinderPolicy.objects.create(name='Ipsum', uuid=uuid.uuid4()) + ReviewActionReason.objects.create(name='Attached to Zab', cinder_policy=zab) + ReviewActionReason.objects.create(name='Attached to Lorem', cinder_policy=lorem) + ReviewActionReason.objects.create( + name='Also attached to Lorem', cinder_policy=lorem + ) + + with self.assertNumQueries(6): + # - 2 savepoints (tests) + # - 2 current user & groups + # - 1 count review action reasons + # - 1 review action reasons (+ cinder policies and parents in one query) + response = self.client.get(self.list_url) + assert response.status_code == 200 + doc = pq(response.content) + assert len(doc('#result_list tbody tr')) == ReviewActionReason.objects.count() + assert ( + doc('#result_list th.field-name').text() + == 'Also attached to Lorem Attached to Lorem Attached to Zab' + ) + assert ( + doc('#result_list td.field-linked_cinder_policy')[2].text_content() + == 'Foo, specifically Zab' + )