Skip to content

Commit

Permalink
Amend notes for review actions with the policy information (#21844)
Browse files Browse the repository at this point in the history
* Amend notes for review actions with the policy information

This is to align the reviewer response to the DSA email template.
The reject boilerplate is no longer needed as the policies listed will
specify the violations.
  • Loading branch information
diox committed Feb 13, 2024
1 parent d6c0327 commit d330ed5
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 25 deletions.
11 changes: 11 additions & 0 deletions src/olympia/abuse/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -874,3 +874,14 @@ class CinderPolicy(ModelBase):

def __str__(self):
return self.name

def full_text(self, canned_response_text=None):
if not canned_response_text:
canned_response_text = self.text
parts = []
if self.parent:
parts.append(f'{self.parent.name}, specifically ')
parts.append(self.name)
if canned_response_text:
parts.append(f': {canned_response_text}')
return ''.join(parts)
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
{{ manual_policy_text }}
{% else %}
{% for policy in policies %}
- {% if policy.parent %}{{ policy.parent.name }}, specifically {% endif %}{{ policy.name }}{% if policy.text %}: {{ policy.text }}{% endif %}
{# Policies text may contain HTML entities, this is a text email so we consider that safe #}
- {{ policy.full_text|safe }}
{% endfor %}
{% endif %}
{% endif %}
26 changes: 26 additions & 0 deletions src/olympia/abuse/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1264,6 +1264,32 @@ def test_create_cinder_policy_with_duplicate_uuid(self):
uuid=existing_policy.uuid,
)

def test_full_text(self):
parent_policy = CinderPolicy.objects.create(
name='Parent Policy',
text='Parent Policy Description',
uuid='parent-uuid',
)
child_policy = CinderPolicy.objects.create(
name='Child Policy',
text='Child Policy Description',
uuid='child-uuid',
parent=parent_policy,
)
assert parent_policy.full_text() == 'Parent Policy: Parent Policy Description'
assert (
parent_policy.full_text('Some Canned Response')
== 'Parent Policy: Some Canned Response'
)
assert (
child_policy.full_text()
== 'Parent Policy, specifically Child Policy: Child Policy Description'
)
assert (
child_policy.full_text('Some Canned Response')
== 'Parent Policy, specifically Child Policy: Some Canned Response'
)

def test_without_parents_if_their_children_are_present(self):
parent_policy = CinderPolicy.objects.create(
name='Parent of Policy 1',
Expand Down
14 changes: 12 additions & 2 deletions src/olympia/abuse/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,14 @@ def setUp(self):
)
self.cinder_job.policies.add(
CinderPolicy.objects.create(
uuid='1234', name='Bad policy', text='This is bad thing'
uuid='1234',
name='Bad policy',
text='This is bad thing',
parent=CinderPolicy.objects.create(
uuid='p4r3nt',
name='Parent Policy',
text='Parent policy text',
),
)
)
self.abuse_report_no_auth = AbuseReport.objects.create(
Expand Down Expand Up @@ -155,7 +162,10 @@ def _test_owner_takedown_email(self, subject, snippet):
)
in mail_item.body
)
assert 'Bad policy: This is bad thing' in mail_item.body
assert (
'\n - Parent Policy, specifically Bad policy: This is bad thing\n'
in mail_item.body
)

def _test_owner_affirmation_email(self, subject):
mail_item = mail.outbox[0]
Expand Down
7 changes: 6 additions & 1 deletion src/olympia/reviewers/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,12 @@ def create_option(self, *args, **kwargs):
# label_from_instance() on ReasonsChoiceField returns the full object,
# not a label, this is what makes this work.
obj = option['label']
option['attrs']['data-value'] = obj.canned_response
canned_response = (
obj.cinder_policy.full_text(obj.canned_response)
if obj.cinder_policy
else obj.canned_response
)
option['attrs']['data-value'] = f'- {canned_response}\n'
option['label'] = str(obj)
return option

Expand Down
52 changes: 40 additions & 12 deletions src/olympia/reviewers/tests/test_forms.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import uuid
from datetime import datetime

from django.utils.encoding import force_str
Expand All @@ -6,7 +7,7 @@
from waffle.testutils import override_switch

from olympia import amo
from olympia.abuse.models import AbuseReport, CinderJob
from olympia.abuse.models import AbuseReport, CinderJob, CinderPolicy
from olympia.addons.models import Addon
from olympia.amo.tests import (
TestCase,
Expand Down Expand Up @@ -213,23 +214,56 @@ def test_reasons(self):
is_active=True,
canned_response='Canned response for C',
)
self.reason_d = ReviewActionReason.objects.create(
name='d reason',
is_active=True,
canned_response='Canned response for D',
cinder_policy=CinderPolicy.objects.create(
uuid=uuid.uuid4(), name='Lone Policy', text='Lone Policy Description'
),
)
self.reason_e = ReviewActionReason.objects.create(
name='e reason',
is_active=True,
canned_response='Canned response for E',
cinder_policy=CinderPolicy.objects.create(
uuid=uuid.uuid4(),
name='Nested Policy',
text='Nested Policy Description',
parent=CinderPolicy.objects.create(
uuid=uuid.uuid4(),
name='Parent Policy',
text='Parent Policy Description',
),
),
)
self.empty_reason = ReviewActionReason.objects.create(
name='d reason',
is_active=True,
canned_response='',
)
form = self.get_form()
choices = form.fields['reasons'].choices
assert len(choices) == 2 # Only active reasons
assert len(choices) == 4 # Only active reasons
# Reasons are displayed in alphabetical order.
assert list(choices.queryset)[0] == self.reason_a
assert list(choices.queryset)[1] == self.reason_c
assert list(choices.queryset)[2] == self.reason_d
assert list(choices.queryset)[3] == self.reason_e

# Assert that the canned_response is written to data-value of the
# checkboxes.
doc = pq(str(form['reasons']))
assert doc('input')[0].attrib.get('data-value') == self.reason_a.canned_response
assert doc('input')[1].attrib.get('data-value') == self.reason_c.canned_response
assert doc('input')[0].attrib.get('data-value') == '- Canned response for A\n'
assert doc('input')[1].attrib.get('data-value') == '- Canned response for C\n'
assert (
doc('input')[2].attrib.get('data-value')
== '- Lone Policy: Canned response for D\n'
)
assert (
doc('input')[3].attrib.get('data-value')
== '- Parent Policy, specifically Nested Policy: Canned response for E\n'
)

def test_reasons_by_type(self):
self.reason_all = ReviewActionReason.objects.create(
Expand Down Expand Up @@ -322,14 +356,8 @@ def test_boilerplate(self):
doc('input')[0].attrib.get('data-value')
== 'Thank you for your contribution.'
)
assert doc('input')[1].attrib.get('data-value') == (
"This add-on didn't pass review because of the following "
'problems:\n\n1) '
)
assert doc('input')[2].attrib.get('data-value') == (
"This add-on didn't pass review because of the following "
'problems:\n\n1) '
)
assert doc('input')[1].attrib.get('data-value') is None
assert doc('input')[2].attrib.get('data-value') is None
assert doc('input')[3].attrib.get('data-value') is None
assert doc('input')[4].attrib.get('data-value') is None

Expand Down
9 changes: 1 addition & 8 deletions src/olympia/reviewers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,11 +563,6 @@ def get_actions(self):
can_approve_multiple = False

# Definitions for all actions.
boilerplate_for_approve = 'Thank you for your contribution.'
boilerplate_for_reject = (
"This add-on didn't pass review because of the following problems:\n\n1) "
)

actions['public'] = {
'method': self.handler.approve_latest_version,
'minimal': False,
Expand All @@ -587,7 +582,7 @@ def get_actions(self):
'allows_reasons': not is_static_theme,
'resolves_abuse_reports': True,
'requires_reasons': False,
'boilerplate_text': boilerplate_for_approve,
'boilerplate_text': 'Thank you for your contribution.',
}
actions['reject'] = {
'method': self.handler.reject_latest_version,
Expand All @@ -610,7 +605,6 @@ def get_actions(self):
'allows_reasons': True,
'resolves_abuse_reports': True,
'requires_reasons': not is_static_theme,
'boilerplate_text': boilerplate_for_reject,
}
actions['approve_content'] = {
'method': self.handler.approve_content,
Expand Down Expand Up @@ -679,7 +673,6 @@ def get_actions(self):
'allows_reasons': True,
'resolves_abuse_reports': True,
'requires_reasons': not is_static_theme,
'boilerplate_text': boilerplate_for_reject,
}
actions['unreject_latest_version'] = {
'method': self.handler.unreject_latest_version,
Expand Down

0 comments on commit d330ed5

Please sign in to comment.