Bug 1604365 - Remove old preference-experiment action r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D64498

--HG--
extra : moz-landing-system : lando

toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm

@@ -1,95 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-"use strict";
-
-const { PreferenceExperimentAction } = ChromeUtils.import(
-  "resource://normandy/actions/PreferenceExperimentAction.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "ActionSchemas",
-  "resource://normandy/actions/schemas/index.js"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "JsonSchemaValidator",
-  "resource://gre/modules/components-utils/JsonSchemaValidator.jsm"
-);
-
-var EXPORTED_SYMBOLS = ["SinglePreferenceExperimentAction"];
-
-/**
- * The backwards-compatible version of the preference experiment that
- * can only accept a single preference.
- */
-class SinglePreferenceExperimentAction extends PreferenceExperimentAction {
-  get schema() {
-    return ActionSchemas["single-preference-experiment"];
-  }
-
-  async _run(recipe) {
-    const {
-      preferenceBranchType,
-      preferenceName,
-      preferenceType,
-      branches,
-      ...remainingArguments
-    } = recipe.arguments;
-
-    const newArguments = {
-      // The multi-preference-experiment schema requires a string
-      // name/description, which are necessary in the wire format, but
-      // experiment objects can have null for these fields. Add some
-      // filler fields here and remove them after validation.
-      userFacingName: "temp-name",
-      userFacingDescription: "temp-description",
-      ...remainingArguments,
-      branches: branches.map(branch => {
-        const { value, ...branchProps } = branch;
-        return {
-          ...branchProps,
-          preferences: {
-            [preferenceName]: {
-              preferenceBranchType,
-              preferenceType,
-              preferenceValue: value,
-            },
-          },
-        };
-      }),
-    };
-
-    const multiprefSchema = ActionSchemas["multi-preference-experiment"];
-
-    let [
-      valid,
-      validatedArguments,
-    ] = JsonSchemaValidator.validateAndParseParameters(
-      newArguments,
-      multiprefSchema
-    );
-    if (!valid) {
-      throw new Error(
-        `Transformed arguments do not match schema. Original arguments: ${JSON.stringify(
-          recipe.arguments
-        )}, new arguments: ${JSON.stringify(
-          newArguments
-        )}, schema: ${JSON.stringify(multiprefSchema)}`
-      );
-    }
-
-    validatedArguments.userFacingName = null;
-    validatedArguments.userFacingDescription = null;
-
-    recipe.arguments = validatedArguments;
-
-    const newRecipe = {
-      ...recipe,
-      arguments: validatedArguments,
-    };
-
-    return super._run(newRecipe);
-  }
-}

toolkit/components/normandy/lib/ActionsManager.jsm

@@ -23,8 +23,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   PreferenceRolloutAction:
     "resource://normandy/actions/PreferenceRolloutAction.jsm",
   ShowHeartbeatAction: "resource://normandy/actions/ShowHeartbeatAction.jsm",
-  SinglePreferenceExperimentAction:
-    "resource://normandy/actions/SinglePreferenceExperimentAction.jsm",
   Uptake: "resource://normandy/lib/Uptake.jsm",
 });
 
@@ -41,13 +39,10 @@ const actionConstructors = {
   "preference-rollback": PreferenceRollbackAction,
   "preference-rollout": PreferenceRolloutAction,
   "show-heartbeat": ShowHeartbeatAction,
-  "single-preference-experiment": SinglePreferenceExperimentAction,
 };
 
 // Legacy names used by the server and older clients for actions.
-const actionAliases = {
-  "preference-experiment": "single-preference-experiment",
-};
+const actionAliases = {};
 
 /**
  * A class to manage the actions that recipes can use in Normandy.

toolkit/components/normandy/lib/PreferenceExperiments.jsm

@@ -47,6 +47,9 @@
  *   A random ID generated at time of enrollment. It should be included on all
  *   telemetry related to this experiment. It should not be re-used by other
  *   studies, or any other purpose. May be null on old experiments.
+ * @property {string} actionName
+ *   The action who knows about this experiment and is responsible for cleaning
+ *   it up. This should correspond to the `name` of some BaseAction subclass.
  */
 
 /**
@@ -941,5 +944,25 @@ var PreferenceExperiments = {
       }
       storage.saveSoon();
     },
+
+    async migration05RemoveOldAction() {
+      const experiments = await PreferenceExperiments.getAllActive();
+      for (const experiment of experiments) {
+        if (experiment.actionName == "SinglePreferenceExperimentAction") {
+          try {
+            await PreferenceExperiments.stop(experiment.slug, {
+              resetValue: true,
+              reason: "migration-removing-single-pref-action",
+            });
+          } catch (e) {
+            log.error(
+              `Stopping preference experiment ${
+                experiment.slug
+              } during migration failed: ${e}`
+            );
+          }
+        }
+      }
+    },
   },
 };

toolkit/components/normandy/test/NormandyTestUtils.jsm

@@ -104,6 +104,7 @@ const NormandyTestUtils = {
           lastSeen: new Date().toJSON(),
           experimentType: "exp",
           enrollmentId: NormandyUtils.generateUuid(),
+          actionName: "PreferenceExperimentAction",
         },
         attrs,
         {

toolkit/components/normandy/test/browser/browser.ini

@@ -20,7 +20,6 @@ head = head.js
 [browser_actions_PreferenceRolloutAction.js]
 [browser_actions_PreferenceRollbackAction.js]
 [browser_actions_ShowHeartbeatAction.js]
-[browser_actions_SinglePreferenceExperimentAction.js]
 [browser_ActionsManager.js]
 [browser_AddonRollouts.js]
 [browser_AddonStudies.js]

toolkit/components/normandy/test/browser/browser_PreferenceExperiments.js

@@ -181,6 +181,30 @@ const mockV5Data = {
   },
 };
 
+const migrationsInfo = [
+  {
+    migration: PreferenceExperiments.migrations.migration01MoveExperiments,
+    dataBefore: mockV1Data,
+    dataAfter: mockV2Data,
+  },
+  {
+    migration: PreferenceExperiments.migrations.migration02MultiPreference,
+    dataBefore: mockV2Data,
+    dataAfter: mockV3Data,
+  },
+  {
+    migration: PreferenceExperiments.migrations.migration03AddActionName,
+    dataBefore: mockV3Data,
+    dataAfter: mockV4Data,
+  },
+  {
+    migration: PreferenceExperiments.migrations.migration04RenameNameToSlug,
+    dataBefore: mockV4Data,
+    dataAfter: mockV5Data,
+  },
+  // Migration 5 is not a simple data migration. This style of tests does not apply to it.
+];
+
 /**
  * Make a mock `JsonFile` object with a no-op `saveSoon` method and a deep copy
  * of the data passed.
@@ -196,27 +220,30 @@ function makeMockJsonFile(data = {}) {
 
 /** Test that each migration results in the expected data */
 add_task(async function test_migrations() {
-  let mockJsonFile = makeMockJsonFile(mockV1Data);
-  await PreferenceExperiments.migrations.migration01MoveExperiments(
-    mockJsonFile
-  );
-  Assert.deepEqual(mockJsonFile.data, mockV2Data);
+  for (const { migration, dataAfter, dataBefore } of migrationsInfo) {
+    let mockJsonFile = makeMockJsonFile(dataBefore);
+    await migration(mockJsonFile);
+    Assert.deepEqual(
+      mockJsonFile.data,
+      dataAfter,
+      `Migration ${migration.name} should result in the expected data`
+    );
+  }
+});
 
-  mockJsonFile = makeMockJsonFile(mockV2Data);
-  await PreferenceExperiments.migrations.migration02MultiPreference(
-    mockJsonFile
-  );
-  Assert.deepEqual(mockJsonFile.data, mockV3Data);
-
-  mockJsonFile = makeMockJsonFile(mockV3Data);
-  await PreferenceExperiments.migrations.migration03AddActionName(mockJsonFile);
-  Assert.deepEqual(mockJsonFile.data, mockV4Data);
-
-  mockJsonFile = makeMockJsonFile(mockV4Data);
-  await PreferenceExperiments.migrations.migration04RenameNameToSlug(
-    mockJsonFile
-  );
-  Assert.deepEqual(mockJsonFile.data, mockV5Data);
+add_task(async function migrations_are_idempotent() {
+  for (const { migration, dataBefore } of migrationsInfo) {
+    const mockJsonFileOnce = makeMockJsonFile(dataBefore);
+    const mockJsonFileTwice = makeMockJsonFile(dataBefore);
+    await migration(mockJsonFileOnce);
+    await migration(mockJsonFileTwice);
+    await migration(mockJsonFileTwice);
+    Assert.deepEqual(
+      mockJsonFileOnce.data,
+      mockJsonFileTwice.data,
+      "migrating data twice should be idempotent for " + migration.name
+    );
+  }
 });
 
 add_task(async function migration03KeepsActionName() {
@@ -231,26 +258,51 @@ add_task(async function migration03KeepsActionName() {
   Assert.deepEqual(mockJsonFile.data, migratedData);
 });
 
-add_task(async function migrations_are_idempotent() {
-  let dataVersions = [
-    [PreferenceExperiments.migrations.migration01MoveExperiments, mockV1Data],
-    [PreferenceExperiments.migrations.migration02MultiPreference, mockV2Data],
-    [PreferenceExperiments.migrations.migration03AddActionName, mockV3Data],
-    [PreferenceExperiments.migrations.migration04RenameNameToSlug, mockV4Data],
-  ];
-  for (const [migration, mockOldData] of dataVersions) {
-    const mockJsonFileOnce = makeMockJsonFile(mockOldData);
-    const mockJsonFileTwice = makeMockJsonFile(mockOldData);
-    await migration(mockJsonFileOnce);
-    await migration(mockJsonFileTwice);
-    await migration(mockJsonFileTwice);
+// Test that migration 5 works as expected
+decorate_task(
+  PreferenceExperiments.withMockExperiments([
+    NormandyTestUtils.factories.preferenceStudyFactory({
+      actionName: "PreferenceExperimentAction",
+      expired: false,
+    }),
+    NormandyTestUtils.factories.preferenceStudyFactory({
+      actionName: "SinglePreferenceExperimentAction",
+      expired: false,
+    }),
+  ]),
+  async function migration05Works([expKeep, expExpire]) {
+    // pre check
+    const activeSlugsBefore = (await PreferenceExperiments.getAllActive()).map(
+      e => e.slug
+    );
     Assert.deepEqual(
-      mockJsonFileOnce.data,
-      mockJsonFileTwice.data,
-      "migrating data twice should be idempotent for " + migration.name
+      activeSlugsBefore,
+      [expKeep.slug, expExpire.slug],
+      "Both experiments should be present and active before the migration"
+    );
+
+    // run the migration
+    await PreferenceExperiments.migrations.migration05RemoveOldAction();
+
+    // verify behavior
+    const activeSlugsAfter = (await PreferenceExperiments.getAllActive()).map(
+      e => e.slug
+    );
+    Assert.deepEqual(
+      activeSlugsAfter,
+      [expKeep.slug],
+      "The single pref experiment should be ended by the migration"
+    );
+    const allSlugsAfter = (await PreferenceExperiments.getAll()).map(
+      e => e.slug
+    );
+    Assert.deepEqual(
+      allSlugsAfter,
+      [expKeep.slug, expExpire.slug],
+      "Both experiments should still exist after the migration"
     );
   }
-});
+);
 
 // clearAllExperimentStorage
 decorate_task(

toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js

@@ -1,116 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- * http://creativecommons.org/publicdomain/zero/1.0/ */
-"use strict";
-
-ChromeUtils.import(
-  "resource://gre/modules/components-utils/Sampling.jsm",
-  this
-);
-ChromeUtils.import("resource://gre/modules/Services.jsm", this);
-ChromeUtils.import("resource://gre/modules/Preferences.jsm", this);
-ChromeUtils.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
-ChromeUtils.import("resource://normandy/actions/BaseAction.jsm", this);
-ChromeUtils.import("resource://normandy/lib/ClientEnvironment.jsm", this);
-ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this);
-ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this);
-ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
-ChromeUtils.import(
-  "resource://normandy/actions/PreferenceExperimentAction.jsm",
-  this
-);
-ChromeUtils.import(
-  "resource://normandy/actions/SinglePreferenceExperimentAction.jsm",
-  this
-);
-
-function argumentsFactory(args) {
-  return {
-    slug: "test",
-    preferenceName: "fake.preference",
-    preferenceType: "string",
-    preferenceBranchType: "default",
-    branches: [{ slug: "test", value: "foo", ratio: 1 }],
-    isHighPopulation: false,
-    ...args,
-  };
-}
-
-function preferenceExperimentFactory(args) {
-  return recipeFactory({
-    name: "preference-experiment",
-    arguments: argumentsFactory(args),
-  });
-}
-
-decorate_task(
-  withStudiesEnabled,
-  withStub(PreferenceExperimentAction.prototype, "_run"),
-  PreferenceExperiments.withMockExperiments([]),
-  async function enroll_user_if_never_been_in_experiment(runStub) {
-    const action = new SinglePreferenceExperimentAction();
-    const recipe = preferenceExperimentFactory({
-      slug: "test",
-      preferenceName: "fake.preference",
-      preferenceBranchType: "user",
-      branches: [
-        {
-          slug: "branch1",
-          value: "branch1",
-          ratio: 1,
-        },
-        {
-          slug: "branch2",
-          value: "branch2",
-          ratio: 1,
-        },
-      ],
-    });
-    sinon
-      .stub(action, "chooseBranch")
-      .callsFake(async function(slug, branches) {
-        return branches[0];
-      });
-
-    await action.processRecipe(recipe, BaseAction.suitability.FILTER_MATCH);
-    await action.finalize();
-
-    Assert.deepEqual(runStub.args, [
-      [
-        {
-          id: recipe.id,
-          name: "preference-experiment",
-          arguments: {
-            slug: "test",
-            userFacingName: null,
-            userFacingDescription: null,
-            isHighPopulation: false,
-            branches: [
-              {
-                slug: "branch1",
-                ratio: 1,
-                preferences: {
-                  "fake.preference": {
-                    preferenceValue: "branch1",
-                    preferenceType: "string",
-                    preferenceBranchType: "user",
-                  },
-                },
-              },
-              {
-                slug: "branch2",
-                ratio: 1,
-                preferences: {
-                  "fake.preference": {
-                    preferenceValue: "branch2",
-                    preferenceType: "string",
-                    preferenceBranchType: "user",
-                  },
-                },
-              },
-            ],
-          },
-        },
-      ],
-    ]);
-  }
-);