From 39101254269a11f290478509e6e40a01f1b49aeb Mon Sep 17 00:00:00 2001
From: Jared Kerim <jkerim@mozilla.com>
Date: Wed, 30 May 2018 15:45:16 -0400
Subject: [PATCH] Allow in review experiments to be edited fixes #416

---
 Makefile                                      |  2 +-
 app/experimenter/experiments/models.py        | 94 +++++++++----------
 .../experiments/tests/test_models.py          | 27 ++++--
 .../templates/experiments/detail.html         | 14 +--
 4 files changed, 74 insertions(+), 63 deletions(-)

diff --git a/Makefile b/Makefile
index 5b1f2dd886..bc6a88f3e5 100644
--- a/Makefile
+++ b/Makefile
@@ -22,7 +22,7 @@ lint: compose_build
 black: compose_build
 	docker-compose run app black -l 79 .
 
-check: compose_build lint black test
+check: compose_build black lint test
 	echo "Success"
 
 makemigrations: compose_build
diff --git a/app/experimenter/experiments/models.py b/app/experimenter/experiments/models.py
index 3f899fdc7d..24cafd2866 100644
--- a/app/experimenter/experiments/models.py
+++ b/app/experimenter/experiments/models.py
@@ -121,8 +121,8 @@ def is_in_review(self):
         return self.status == self.STATUS_REVIEW
 
     @property
-    def is_readonly(self):
-        return not self.is_draft
+    def is_editable(self):
+        return self.is_draft or self.is_in_review
 
     @property
     def is_begun(self):
@@ -136,6 +136,51 @@ def _transition_date(self, start_state, end_state):
         if change.count() == 1:
             return change.get().changed_on
 
+    @property
+    def is_high_risk(self):
+        return True in self._risk_questions
+
+    @property
+    def completed_overview(self):
+        return self.pk is not None
+
+    @property
+    def completed_variants(self):
+        return self.variants.exists()
+
+    @property
+    def completed_objectives(self):
+        return (
+            self.objectives != self.OBJECTIVES_DEFAULT
+            and self.analysis != self.ANALYSIS_DEFAULT
+        )
+
+    @property
+    def _risk_questions(self):
+        return (
+            self.risk_partner_related,
+            self.risk_brand,
+            self.risk_fast_shipped,
+            self.risk_confidential,
+            self.risk_release_population,
+        )
+
+    @property
+    def completed_risks(self):
+        return (
+            None not in self._risk_questions
+            and self.testing != self.TESTING_DEFAULT
+        )
+
+    @property
+    def is_ready_for_review(self):
+        return (
+            self.completed_overview
+            and self.completed_variants
+            and self.completed_objectives
+            and self.completed_risks
+        )
+
     @property
     def population(self):
         return "{percent:g}% of {channel} Firefox {version}".format(
@@ -185,51 +230,6 @@ def test_tube_url(self):
             "https://firefox-test-tube.herokuapp.com/experiments/{slug}/"
         ).format(slug=self.slug)
 
-    @property
-    def _risk_questions(self):
-        return (
-            self.risk_partner_related,
-            self.risk_brand,
-            self.risk_fast_shipped,
-            self.risk_confidential,
-            self.risk_release_population,
-        )
-
-    @property
-    def is_high_risk(self):
-        return True in self._risk_questions
-
-    @property
-    def completed_overview(self):
-        return self.pk is not None
-
-    @property
-    def completed_variants(self):
-        return self.variants.exists()
-
-    @property
-    def completed_objectives(self):
-        return (
-            self.objectives != self.OBJECTIVES_DEFAULT
-            and self.analysis != self.ANALYSIS_DEFAULT
-        )
-
-    @property
-    def completed_risks(self):
-        return (
-            None not in self._risk_questions
-            and self.testing != self.TESTING_DEFAULT
-        )
-
-    @property
-    def is_ready_for_review(self):
-        return (
-            self.completed_overview
-            and self.completed_variants
-            and self.completed_objectives
-            and self.completed_risks
-        )
-
 
 class ExperimentVariant(models.Model):
     experiment = models.ForeignKey(
diff --git a/app/experimenter/experiments/tests/test_models.py b/app/experimenter/experiments/tests/test_models.py
index b3125591d0..3e9ea251ae 100644
--- a/app/experimenter/experiments/tests/test_models.py
+++ b/app/experimenter/experiments/tests/test_models.py
@@ -89,15 +89,26 @@ def test_variant_property_returns_experiment_variant(self):
         )
         self.assertEqual(experiment.variant, variant)
 
-    def test_experiment_with_created_status_is_not_readonly(self):
-        experiment = ExperimentFactory.create_with_variants()
-        self.assertFalse(experiment.is_readonly)
+    def test_experiment_is_editable_when_is_draft(self):
+        experiment = ExperimentFactory.create_with_status(
+            Experiment.STATUS_DRAFT
+        )
+        self.assertTrue(experiment.is_editable)
 
-    def test_experiment_with_any_status_after_created_is_readonly(self):
-        experiment = ExperimentFactory.create_with_variants()
-        experiment.status = experiment.STATUS_REVIEW
-        experiment.save()
-        self.assertTrue(experiment.is_readonly)
+    def test_experiment_is_editable_when_is_in_review(self):
+        experiment = ExperimentFactory.create_with_status(
+            Experiment.STATUS_REVIEW
+        )
+        self.assertTrue(experiment.is_editable)
+
+    def test_experient_is_not_editable_after_review(self):
+        all_statuses = set([status[0] for status in Experiment.STATUS_CHOICES])
+        editable_statuses = set(
+            [Experiment.STATUS_DRAFT, Experiment.STATUS_REVIEW]
+        )
+        for status in all_statuses - editable_statuses:
+            experiment = ExperimentFactory.create_with_status(status)
+            self.assertFalse(experiment.is_editable)
 
     def test_experiment_is_not_begun(self):
         statuses = (
diff --git a/app/experimenter/templates/experiments/detail.html b/app/experimenter/templates/experiments/detail.html
index 96d2e1e06a..3964e06f53 100644
--- a/app/experimenter/templates/experiments/detail.html
+++ b/app/experimenter/templates/experiments/detail.html
@@ -27,7 +27,7 @@ <h4 class="col-md-8 md-mt-0">
           Overview
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-overview-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>
@@ -64,7 +64,7 @@ <h4 class="col-md-8">
           Population
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-overview-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>
@@ -86,7 +86,7 @@ <h4 class="col-md-8">
           Firefox Pref &amp; Branches
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-variants-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>
@@ -118,7 +118,7 @@ <h4 class="col-md-8">
           Objectives
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-objectives-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>
@@ -143,7 +143,7 @@ <h4 class="col-md-8">
           Analysis
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-objectives-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>
@@ -169,7 +169,7 @@ <h4 class="col-md-8">
           Risk Assessment
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-risks-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>
@@ -252,7 +252,7 @@ <h4 class="col-md-8">
           Testing Plan
         </h4>
 
-        {% if experiment.is_draft %}
+        {% if experiment.is_editable %}
           <div class="col-md-4 text-right">
             <a class="noanchorstyle" href="{% url "experiments-risks-update" slug=experiment.slug %}"><h5><span class="glyphicon glyphicon-pencil"></span> Edit</h5></a>
           </div>