Add permissions for QA and Relman signoff fixes #1171

app/experimenter/experiments/apps.py

@@ -8,4 +8,5 @@ class ExperimentsConfig(AppConfig):
     name = "experimenter.experiments"
 
     def ready(self):
+
         markus.configure(settings.MARKUS_BACKEND)

app/experimenter/experiments/forms.py

@@ -3,6 +3,7 @@
 from django import forms
 from django.conf import settings
 from django.contrib.auth import get_user_model
+from django.contrib import messages
 from django.db import transaction
 from django.forms import BaseInlineFormSet
 from django.forms import inlineformset_factory
@@ -916,6 +917,37 @@ def save(self, *args, **kwargs):
 
         return experiment
 
+    def clean(self):
+
+        super(ExperimentReviewForm, self).clean()
+
+        permissions = [
+            (
+                "review_qa",
+                "experiments.can_check_QA_signoff",
+                "You don't have permission to edit QA signoff fields",
+            ),
+            (
+                "review_relman",
+                "experiments.can_check_relman_signoff",
+                "You don't have permission to edit Release "
+                "Management signoff fields",
+            ),
+        ]
+
+        # user cannot check or uncheck QA and relman
+        # sign off boxes without permission
+        for field_name, permission_name, error_message in permissions:
+            if (
+                field_name in self.changed_data
+                and not self.request.user.has_perm(permission_name)
+            ):
+                self.changed_data.remove(field_name)
+                self.cleaned_data.pop(field_name)
+                messages.warning(self.request, error_message)
+
+        return self.cleaned_data
+
 
 class ExperimentStatusForm(
     ExperimentConstants, ChangeLogMixin, forms.ModelForm

app/experimenter/experiments/migrations/0049_add_permissions.py

@@ -0,0 +1,45 @@
+# Generated by Django 2.1.7 on 2019-04-25 19:12
+
+from django.db import migrations
+
+
+def create_permissions(apps, schema_editor):
+    Permission = apps.get_model("auth", "Permission")
+    ContentType = apps.get_model("contenttypes", "ContentType")
+    Experiment = apps.get_model("experiments", "Experiment")
+
+    content_type = ContentType.objects.get_for_model(Experiment)
+
+    qa_permission = Permission.objects.create(
+        codename="can_check_QA_signoff",
+        name="Can check QA signoff box",
+        content_type=content_type,
+    )
+
+    relman_permission = Permission.objects.create(
+        codename="can_check_relman_signoff",
+        name="Can check relman signoff box",
+        content_type=content_type,
+    )
+
+
+def remove_permissions(apps, schema_editor):
+    Permission = apps.get_model("auth", "Permission")
+
+    relman_signoff = Permission.objects.get(
+        codename="can_check_relman_signoff"
+    )
+    relman_signoff.delete()
+
+    qa_signoff = Permission.objects.get(codename="can_check_QA_signoff")
+    qa_signoff.delete()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("experiments", "0048_experiment_risk_external_team_impact"),
+        ("auth", "0009_alter_user_last_name_max_length"),
+    ]
+
+    operations = [migrations.RunPython(create_permissions, remove_permissions)]

app/experimenter/experiments/migrations/0050_create_group_add_permissions.py

@@ -0,0 +1,45 @@
+from django.db import migrations
+
+
+def add_groups(apps, schema_editor):
+
+    Group = apps.get_model("auth", "Group")
+    Permission = apps.get_model("auth", "Permission")
+    ContentType = apps.get_model("contenttypes", "ContentType")
+    Experiment = apps.get_model("experiments", "Experiment")
+
+    relman_signoff_group = Group.objects.create(name="Relman Signoff")
+    qa_signoff_group = Group.objects.create(name="QA Signoff")
+
+    content_type = ContentType.objects.get_for_model(Experiment)
+    relman_permission = Permission.objects.get(
+        content_type=content_type, codename="can_check_relman_signoff"
+    )
+    qa_permission = Permission.objects.get(
+        content_type=content_type, codename="can_check_QA_signoff"
+    )
+
+    relman_signoff_group.permissions.add(relman_permission)
+    qa_signoff_group.permissions.add(qa_permission)
+
+
+def remove_groups(apps, schema_editor):
+    Group = apps.get_model("auth", "Group")
+
+    relman_signoff_group = Group.objects.get(name="Relman Signoff")
+    relman_signoff_group.delete()
+
+    qa_signoff_group = Group.objects.get(name="QA Signoff")
+    qa_signoff_group.delete()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("experiments", "0049_add_permissions"),
+        ("contenttypes", "0002_remove_content_type_name"),
+        ("experiments", "0048_experiment_risk_external_team_impact"),
+        ("auth", "0009_alter_user_last_name_max_length"),
+    ]
+
+    operations = [migrations.RunPython(add_groups, remove_groups)]

app/experimenter/experiments/tests/test_forms.py

@@ -10,6 +10,9 @@
 from django.utils.text import slugify
 from faker import Factory as FakerFactory
 from parameterized import parameterized_class
+from django.contrib.auth.models import Permission
+from django.contrib.contenttypes.models import ContentType
+
 
 from experimenter.experiments.forms import (
     BugzillaURLField,
@@ -30,7 +33,10 @@
 )
 from experimenter.experiments.models import Experiment, ExperimentVariant
 from experimenter.base.tests.factories import CountryFactory, LocaleFactory
-from experimenter.experiments.tests.factories import ExperimentFactory
+from experimenter.experiments.tests.factories import (
+    ExperimentFactory,
+    UserFactory,
+)
 from experimenter.experiments.tests.mixins import (
     MockBugzillaMixin,
     MockTasksMixin,
@@ -1139,6 +1145,16 @@ class TestExperimentReviewForm(
 ):
 
     def test_form_saves_reviews(self):
+        user = UserFactory.create()
+        content_type = ContentType.objects.get_for_model(Experiment)
+        experiment_model_permissions = Permission.objects.filter(
+            content_type=content_type, codename__startswith="can_check"
+        )
+        for permission in experiment_model_permissions:
+            user.user_permissions.add(permission)
+
+        self.request.user = user
+
         experiment = ExperimentFactory.create_with_status(
             Experiment.STATUS_REVIEW
         )
@@ -1205,7 +1221,7 @@ def test_added_reviews_property(self):
             Experiment.STATUS_REVIEW
         )
 
-        data = {"review_relman": True, "review_science": True}
+        data = {"review_bugzilla": True, "review_science": True}
 
         form = ExperimentReviewForm(
             request=self.request, data=data, instance=experiment
@@ -1216,15 +1232,15 @@ def test_added_reviews_property(self):
 
         self.assertEqual(len(form.added_reviews), 2)
         self.assertEqual(len(form.removed_reviews), 0)
-        self.assertIn(form.fields["review_relman"].label, form.added_reviews)
+        self.assertIn(form.fields["review_bugzilla"].label, form.added_reviews)
         self.assertIn(form.fields["review_science"].label, form.added_reviews)
 
     def test_removed_reviews_property(self):
         experiment = ExperimentFactory.create_with_status(
-            Experiment.STATUS_REVIEW, review_relman=True, review_science=True
+            Experiment.STATUS_REVIEW, review_bugzilla=True, review_science=True
         )
 
-        data = {"review_relman": False, "review_science": False}
+        data = {"review_bugzilla": False, "review_science": False}
 
         form = ExperimentReviewForm(
             request=self.request, data=data, instance=experiment
@@ -1235,7 +1251,9 @@ def test_removed_reviews_property(self):
 
         self.assertEqual(len(form.added_reviews), 0)
         self.assertEqual(len(form.removed_reviews), 2)
-        self.assertIn(form.fields["review_relman"].label, form.removed_reviews)
+        self.assertIn(
+            form.fields["review_bugzilla"].label, form.removed_reviews
+        )
         self.assertIn(
             form.fields["review_science"].label, form.removed_reviews
         )
@@ -1317,6 +1335,86 @@ def test_optional_reviews_when_a_risk_option_is_true(self):
         self.assertNotIn(form["review_vp"], form.optional_reviews)
         self.assertIn(form["review_vp"], form.required_reviews)
 
+    def test_cannot_check_review_relman_without_permissions(self):
+        user_1 = UserFactory.create()
+        user_2 = UserFactory.create()
+
+        content_type = ContentType.objects.get_for_model(Experiment)
+        permission = Permission.objects.get(
+            content_type=content_type, codename="can_check_relman_signoff"
+        )
+        user_1.user_permissions.add(permission)
+        self.assertTrue(
+            user_1.has_perm("experiments.can_check_relman_signoff")
+        )
+        self.assertFalse(
+            user_2.has_perm("experiments.can_check_relman_signoff")
+        )
+
+        experiment = ExperimentFactory.create_with_status(
+            Experiment.STATUS_REVIEW
+        )
+
+        self.request.user = user_2
+        form = ExperimentReviewForm(
+            request=self.request,
+            data={"review_relman": True},
+            instance=experiment,
+        )
+
+        self.assertTrue(form.is_valid())
+        experiment = form.save()
+        self.assertFalse(experiment.review_relman)
+
+        self.request.user = user_1
+
+        form = ExperimentReviewForm(
+            request=self.request,
+            data={"review_relman": True},
+            instance=experiment,
+        )
+
+        self.assertTrue(form.is_valid())
+        experiment = form.save()
+
+        self.assertTrue(experiment.review_relman)
+
+    def test_cannot_check_review_qa_without_permissions(self):
+        user_1 = UserFactory.create()
+        user_2 = UserFactory.create()
+
+        content_type = ContentType.objects.get_for_model(Experiment)
+        permission = Permission.objects.get(
+            content_type=content_type, codename="can_check_QA_signoff"
+        )
+        user_1.user_permissions.add(permission)
+        self.assertTrue(user_1.has_perm("experiments.can_check_QA_signoff"))
+        self.assertFalse(user_2.has_perm("experiments.can_check_QA_signoff"))
+
+        experiment = ExperimentFactory.create_with_status(
+            Experiment.STATUS_REVIEW
+        )
+
+        self.request.user = user_2
+        form = ExperimentReviewForm(
+            request=self.request, data={"review_qa": True}, instance=experiment
+        )
+
+        self.assertTrue(form.is_valid())
+        experiment = form.save()
+        self.assertFalse(experiment.review_qa)
+
+        self.request.user = user_1
+
+        form = ExperimentReviewForm(
+            request=self.request, data={"review_qa": True}, instance=experiment
+        )
+
+        self.assertTrue(form.is_valid())
+        experiment = form.save()
+
+        self.assertTrue(experiment.review_qa)
+
 
 class TestExperimentStatusForm(
     MockBugzillaMixin, MockRequestMixin, MockTasksMixin, TestCase

app/experimenter/experiments/tests/test_views.py

@@ -1125,8 +1125,6 @@ def test_view_updates_reviews_and_redirects(self):
             "review_qa_requested": True,
             "review_intent_to_ship": True,
             "review_bugzilla": True,
-            "review_qa": True,
-            "review_relman": True,
             "review_advisory": True,
             "review_legal": True,
             "review_ux": True,
@@ -1153,8 +1151,6 @@ def test_view_updates_reviews_and_redirects(self):
         experiment = Experiment.objects.get()
 
         self.assertTrue(experiment.review_science)
-        self.assertTrue(experiment.review_relman)
-        self.assertTrue(experiment.review_qa)
         self.assertTrue(experiment.review_legal)
         self.assertTrue(experiment.review_ux)
         self.assertTrue(experiment.review_security)

app/experimenter/templates/base.html

@@ -99,6 +99,24 @@ <h3 class="pt-2">
       </div>
     {% endif %}
 
+    {% if messages %}
+    <div class="alert-danger">
+      <div class="container">
+        <div class="row">
+            <div class="col pt-3 pb-1">
+              {% for message in messages %}
+                <p>
+                  <span class="fas fa-info-circle"></span>
+                  {{ message }}
+                </p>
+              {% endfor %}
+            </div>
+        </div>
+      </div>
+
+    </div>
+    {% endif %}
+
     <div class="alert-info">
       <div class="container">
         <div class="row">