allow stage pref rollouts to be updated fixes #2404 (#2405)

* allow stage pref rollouts to be updated fixes #2404 * switched to update_or_create
mozilla · Mar 19, 2020 · 629993f · 629993f
1 parent bba8abd
commit 629993f
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 14 deletions.
diff --git a/app/experimenter/experiments/serializers/design.py b/app/experimenter/experiments/serializers/design.py
@@ -321,14 +321,18 @@ def update(self, instance, validated_data):
             existing_preference_ids = set(
                 instance.preferences.all().values_list("id", flat=True)
             )
+
+            submitted_preference_ids = []
             for preference_data in preferences_data:
                 preference_data["experiment"] = instance
-                RolloutPreference.objects.create(**preference_data)
+                pref_id = preference_data.pop("id", None)
+                if pref_id:
+                    submitted_preference_ids.append(pref_id)
+                RolloutPreference.objects.update_or_create(
+                    id=pref_id, defaults=preference_data
+                )
 
-            submitted_preference_ids = set(
-                [p.get("id") for p in preferences_data if p.get("id")]
-            )
-            removed_ids = existing_preference_ids - submitted_preference_ids
+            removed_ids = existing_preference_ids - set(submitted_preference_ids)
 
             if removed_ids:
                 RolloutPreference.objects.filter(id__in=removed_ids).delete()

diff --git a/app/experimenter/experiments/tests/serializers/test_design.py b/app/experimenter/experiments/tests/serializers/test_design.py
@@ -1251,24 +1251,36 @@ def test_pref_name_uniqueness_constraint(self):
 
     def test_save_pref_rollout_with_existing_prefs(self):
         experiment = ExperimentFactory.create(type=Experiment.TYPE_ROLLOUT)
-        preference = RolloutPreference(
+        preference1 = RolloutPreference(
             pref_type=Experiment.PREF_TYPE_INT,
             pref_name="browser.pref",
             pref_value="4",
             experiment=experiment,
         )
-        preference.save()
+        preference1.save()
+        original_pref1_id = experiment.preferences.first().id
 
-        original_pref_id = experiment.preferences.first().id
+        RolloutPreference(
+            pref_type=Experiment.PREF_TYPE_INT,
+            pref_name="browser.pref2",
+            pref_value="4",
+            experiment=experiment,
+        ).save()
 
         data = {
             "rollout_type": Experiment.TYPE_PREF,
             "preferences": [
                 {
-                    "pref_name": "browser.pref2",
+                    "id": original_pref1_id,
+                    "pref_name": "change.original.pref",
+                    "pref_type": Experiment.PREF_TYPE_STR,
+                    "pref_value": "change original pref",
+                },
+                {
+                    "pref_name": "browser.pref3",
                     "pref_type": Experiment.PREF_TYPE_STR,
                     "pref_value": "A STRING!",
-                }
+                },
             ],
         }
 
@@ -1277,20 +1289,24 @@ def test_save_pref_rollout_with_existing_prefs(self):
         )
 
         self.assertTrue(serializer.is_valid())
-
         experiment = serializer.save()
 
         self.assertEqual(experiment.rollout_type, data["rollout_type"])
 
-        self.assertEqual(experiment.preferences.count(), 1)
+        self.assertEqual(experiment.preferences.count(), 2)
         data_pref1 = data["preferences"][0]
-        pref1 = experiment.preferences.first()
+        data_pref2 = data["preferences"][1]
+        pref1 = experiment.preferences.get(id=original_pref1_id)
+        pref2 = experiment.preferences.exclude(id=original_pref1_id)[0]
 
-        self.assertNotEqual(original_pref_id, pref1.id)
         self.assertEqual(pref1.pref_name, data_pref1["pref_name"])
         self.assertEqual(pref1.pref_type, data_pref1["pref_type"])
         self.assertEqual(pref1.pref_value, data_pref1["pref_value"])
 
+        self.assertEqual(pref2.pref_name, data_pref2["pref_name"])
+        self.assertEqual(pref2.pref_type, data_pref2["pref_type"])
+        self.assertEqual(pref2.pref_value, data_pref2["pref_value"])
+
     def test_serializer_outputs_empty_pref_when_no_prefs(self):
         experiment = ExperimentFactory.create(type=Experiment.TYPE_ROLLOUT)