Allow policies to selected directly for reviewer tools actions

Author: eviljeff

src/olympia/reviewers/forms.py

@@ -1,4 +1,6 @@
+import html
 import zipfile
+from collections import defaultdict
 from datetime import datetime, timedelta
 
 from django import forms
@@ -15,11 +17,13 @@
 from django.utils.translation import gettext
 
 import markupsafe
+import waffle
 
 import olympia.core.logger
 from olympia import amo, ratings
 from olympia.abuse.models import CinderJob, CinderPolicy, ContentDecision
 from olympia.access import acl
+from olympia.addons.models import Addon
 from olympia.amo.forms import AMOModelForm
 from olympia.constants.abuse import DECISION_ACTIONS
 from olympia.constants.reviewers import REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT
@@ -104,22 +108,10 @@ class NumberInput(widgets.Input):
     input_type = 'number'
 
 
-class VersionsChoiceField(ModelMultipleChoiceField):
-    """
-    Widget to use together with VersionsChoiceWidget to display the list of
-    versions used by review page for some actions.
-    """
-
-    def label_from_instance(self, obj):
-        """Return the object instead of transforming into a label at this stage
-        so that it's available in the widget."""
-        return obj
-
-
 class VersionsChoiceWidget(forms.SelectMultiple):
     """
-    Widget to use together with VersionsChoiceField to display the list of
-    versions used by review page for some actions.
+    Widget to use together with WidgetRenderedModelMultipleChoiceField to display the
+    list of versions used by review page for some actions.
     """
 
     actions_filters = {
@@ -308,6 +300,37 @@ def create_option(
         )
 
 
+class CinderPolicyWidget(forms.CheckboxSelectMultiple):
+    """
+    Widget to use together with a WidgetRenderedModelMultipleChoiceField to display
+    select elements with additional attribute to allow toggling.
+    """
+
+    def create_option(
+        self, name, value, label, selected, index, subindex=None, attrs=None
+    ):
+        obj = label
+        label = str(obj)
+        attrs = attrs or {}
+        actions_on_policy = {
+            DECISION_ACTIONS.for_api_value(action).value
+            for action in obj.enforcement_actions
+            if DECISION_ACTIONS.has_api_value(action)
+        }
+        actions = (
+            reviewer_action
+            for reviewer_action, defn in self.helper_actions.items()
+            # show this policy for this action if there any common enforcement actions
+            if (ha_ea := defn.get('enforcement_actions', ()))
+            and actions_on_policy.intersection(ha_ea)
+        )
+        attrs['class'] = 'data-toggle'
+        attrs['data-value'] = ' '.join(actions)
+        return super().create_option(
+            name, value, label, selected, index, subindex, attrs
+        )
+
+
 class ActionChoiceWidget(forms.RadioSelect):
     """
     Widget to add boilerplate_text to action options.
@@ -383,7 +406,7 @@ class ReviewForm(forms.Form):
         required=True, widget=forms.Textarea(), label='Comments:'
     )
     action = forms.ChoiceField(required=True, widget=ActionChoiceWidget)
-    versions = VersionsChoiceField(
+    versions = WidgetRenderedModelMultipleChoiceField(
         # The <select> is displayed/hidden dynamically depending on the action
         # so it needs the data-toggle class (data-value attribute is set later
         # during __init__). VersionsChoiceWidget takes care of adding that to
@@ -444,13 +467,13 @@ class ReviewForm(forms.Form):
         queryset=CinderJob.objects.none(),
         widget=CinderJobsWidget(attrs={'class': 'data-toggle-hide'}),
     )
-
-    cinder_policies = forms.ModelMultipleChoiceField(
+    # queryset and widget are set later in __init__
+    cinder_policies = WidgetRenderedModelMultipleChoiceField(
         # queryset is set later in __init__
         queryset=CinderPolicy.objects.none(),
         required=True,
         label='Choose one or more policies:',
-        widget=widgets.CheckboxSelectMultiple,
+        widget=CinderPolicyWidget(),
     )
     appeal_action = forms.MultipleChoiceField(
         required=False,
@@ -470,8 +493,11 @@ def is_valid(self):
                 self.fields['versions'].required = True
             if not action.get('requires_reasons', False):
                 self.fields['reasons'].required = False
-            if not action.get('requires_policies'):
+            if not action.get('enforcement_actions'):
                 self.fields['cinder_policies'].required = False
+            else:
+                # we no longer strictly require comments with cinder policies
+                self.fields['comments'].required = False
             if self.data.get('cinder_jobs_to_resolve'):
                 # if a cinder job is being resolved we need a review reason
                 if action.get('requires_reasons_for_cinder_jobs'):
@@ -508,8 +534,9 @@ def clean(self):
         if self.cleaned_data.get('cinder_jobs_to_resolve') and self.cleaned_data.get(
             'cinder_policies'
         ):
-            actions = self.helper.handler.get_decision_actions_from_policies(
-                self.cleaned_data.get('cinder_policies')
+            actions = CinderPolicy.get_decision_actions_from_policies(
+                self.cleaned_data.get('cinder_policies'),
+                for_entity=Addon,
             )
             if len(actions) == 0:
                 self.add_error(
@@ -561,6 +588,23 @@ def clean(self):
                         ),
                     )
 
+        # Add in any dynamic placeholder values for the cinder policies
+        # see jinja_helpers.render_text_with_input_fields for name format
+        if policies := {
+            str(p.id): p for p in self.cleaned_data.get('cinder_policies', [])
+        }:
+            policy_values = defaultdict(dict)
+            for key, value in self.data.items():
+                if (
+                    key.startswith('policy-value-')
+                    and (split_1 := key.split('policy-value-', 1))
+                    and (id_and_placeholder := html.unescape(split_1[1]))
+                    and len(split_2 := id_and_placeholder.split('-', 1)) == 2
+                    and (policy := policies.get(split_2[0]))
+                ):
+                    policy_values[policy.uuid][split_2[1]] = value
+            self.cleaned_data['policy_values'] = policy_values
+
         return self.cleaned_data
 
     def clean_delayed_rejection_date(self):
@@ -579,6 +623,9 @@ def clean_version_pk(self):
     def __init__(self, *args, **kw):
         self.helper = kw.pop('helper')
         super().__init__(*args, **kw)
+        if waffle.switch_is_active('policy_selection_rather_than_reasons'):
+            # When we're using policies reviewers shouldn't need to write as much
+            self.fields['comments'].widget = forms.Textarea(attrs={'rows': 2})
         if any(action.get('delayable') for action in self.helper.actions.values()):
             # Default delayed rejection date should be
             # REVIEWER_DELAYED_REJECTION_PERIOD_DAYS_DEFAULT days in the
@@ -646,15 +693,19 @@ def __init__(self, *args, **kw):
         ]
 
         # Set the queryset for reasons based on the add-on type.
-        self.fields['reasons'].queryset = ReviewActionReason.objects.filter(
-            is_active=True,
-            addon_type__in=[
-                amo.ADDON_ANY,
-                amo.ADDON_STATICTHEME
-                if self.helper.addon.type == amo.ADDON_STATICTHEME
-                else amo.ADDON_EXTENSION,
-            ],
-        ).exclude(canned_response='')
+        self.fields['reasons'].queryset = (
+            ReviewActionReason.objects.filter(
+                is_active=True,
+                addon_type__in=[
+                    amo.ADDON_ANY,
+                    amo.ADDON_STATICTHEME
+                    if self.helper.addon.type == amo.ADDON_STATICTHEME
+                    else amo.ADDON_EXTENSION,
+                ],
+            )
+            .exclude(canned_response='')
+            .select_related('cinder_policy__parent')
+        )
 
         # Add actions from the helper into the action widget so we can access
         # them in create_option.
@@ -668,7 +719,10 @@ def __init__(self, *args, **kw):
         # Set the queryset for policies to show as options
         self.fields['cinder_policies'].queryset = CinderPolicy.objects.filter(
             expose_in_reviewer_tools=True
-        )
+        ).select_related('parent')
+
+        # Pass on the reviewer tools actions so we can set the show/hide on policies
+        self.fields['cinder_policies'].widget.helper_actions = self.helper.actions
 
     @property
     def unreviewed_files(self):

src/olympia/reviewers/migrations/0043_auto_20250508_1731.py

@@ -0,0 +1,16 @@
+# Generated by Django 4.2.20 on 2025-05-08 17:31
+
+from django.db import migrations
+
+from olympia.core.db.migrations import CreateWaffleSwitch
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('reviewers', '0042_alter_needshumanreview_reason'),
+    ]
+
+    operations = [
+        CreateWaffleSwitch('policy_selection_rather_than_reasons')
+    ]

src/olympia/reviewers/templates/reviewers/review.html

@@ -192,10 +192,20 @@ <h3 id="history">
         </div>
 
         <div class="review-actions-comments-reasons">
-          <div class="review-actions-comments data-toggle review-comments" data-value="{{ actions_comments|join(' ') }}">
-            <label for="id_comments">{{ form.comments.label }}</label>
-            {{ form.comments }}
-            {{ form.comments.errors }}
+
+          <div class="review-actions-comments">
+            <div class="policy-texts data-toggle" data-value="{{ actions_policies|join(' ') }}">
+              {% for policy in cinder_policies %}
+                <div id="policy-text-{{ policy.id }}" hidden>{{ policy|render_text_with_input_fields }}</div>
+              {% endfor %}
+            </div>
+            <div class="review-comments data-toggle" data-value="{{ actions_comments|join(' ') }}">
+              <details{% if not waffle.switch('policy_selection_rather_than_reasons') %} open{% endif %}>
+                <summary><label for="id_comments">{{ form.comments.label }}</label></summary>
+                {{ form.comments }}
+              </details>
+              {{ form.comments.errors }}
+            </div>
           </div>
           <div class="data-toggle review-actions-reasons"
             data-value="{{ actions_reasons|join(' ') }}">

src/olympia/reviewers/templatetags/jinja_helpers.py

@@ -1,5 +1,8 @@
 import datetime
 
+from django.utils.html import escape
+from django.utils.safestring import mark_safe
+
 import jinja2
 from django_jinja import library
 
@@ -70,3 +73,14 @@ def format_score(value):
 @library.filter
 def to_dom_id(string):
     return string.replace('.', '_')
+
+
+@library.filter
+def render_text_with_input_fields(policy):
+    class WrapKey(dict):
+        def __missing__(self, key):
+            # django.utils.html.escape doesn't escape quotes
+            key = key.replace('"', '"')
+            return f'<input placeholder="{key}" name="policy-value-{policy.id}-{key}">'
+
+    return mark_safe(escape(policy.text).format_map(WrapKey({})))