Add permissions for QA and Relman signoff fixes #1171

app/experimenter/experiments/apps.py

@@ -8,4 +8,29 @@ class ExperimentsConfig(AppConfig):
     name = "experimenter.experiments"
 
     def ready(self):
+
+        # create user groups and add permissions to them
+        from django.contrib.auth.models import Group, Permission
+        from django.contrib.contenttypes.models import ContentType
+        from experimenter.experiments.models import Experiment
+
         markus.configure(settings.MARKUS_BACKEND)
+
+        relman_signoff_group, created = Group.objects.get_or_create(
+            name="Relman Signoff"
+        )
+        qa_signoff_group, created = Group.objects.get_or_create(
+            name="QA Signoff"
+        )
+
+        if created:
+            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)

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,35 @@ 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_auto_20190425_1912.py

@@ -0,0 +1,23 @@
+# Generated by Django 2.1.7 on 2019-04-25 19:12
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("experiments", "0048_experiment_risk_external_team_impact")
+    ]
+
+    operations = [
+        migrations.AlterModelOptions(
+            name="experiment",
+            options={
+                "permissions": (
+                    ("can_check_QA_signoff", "Can Check QA signoff box"),
+                ),
+                "verbose_name": "Experiment",
+                "verbose_name_plural": "Experiments",
+            },
+        )
+    ]

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

@@ -0,0 +1,25 @@
+# Generated by Django 2.1.7 on 2019-04-29 17:11
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("experiments", "0049_auto_20190425_1912")]
+
+    operations = [
+        migrations.AlterModelOptions(
+            name="experiment",
+            options={
+                "permissions": (
+                    ("can_check_QA_signoff", "Can check QA signoff box"),
+                    (
+                        "can_check_relman_signoff",
+                        "Can check relman signoff box",
+                    ),
+                ),
+                "verbose_name": "Experiment",
+                "verbose_name_plural": "Experiments",
+            },
+        )
+    ]

app/experimenter/experiments/models.py

@@ -215,6 +215,10 @@ class Experiment(ExperimentConstants, models.Model):
     class Meta:
         verbose_name = "Experiment"
         verbose_name_plural = "Experiments"
+        permissions = (
+            ("can_check_QA_signoff", "Can check QA signoff box"),
+            ("can_check_relman_signoff", "Can check relman signoff box"),
+        )
 
     def get_absolute_url(self):
         return reverse("experiments-detail", kwargs={"slug": self.slug})

app/experimenter/experiments/tests/test_forms.py

@@ -10,6 +10,10 @@
 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 django.contrib.auth.models import User
+
 
 from experimenter.experiments.forms import (
     BugzillaURLField,
@@ -30,7 +34,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 +1146,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 +1222,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 +1233,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 +1252,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 +1336,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">