Bug 1710858 - Deprecate featureConfig.enabled in Experiment/Rollout definition r=k88hudson

Differential Revision: https://phabricator.services.mozilla.com/D115986
2021-05-29 11:56:53 +00:00 · 2021-05-29 11:56:53 +00:00 · edd29bb79a
--- a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage.js
+++ b/browser/components/newtab/test/browser/browser_aboutwelcome_multistage.js
@ -322,9 +322,8 @@ add_task(async function test_multistage_zeroOnboarding_experimentAPI() {
  await setAboutWelcomePref(true);
  await ExperimentAPI.ready();
  let doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig({
-    enabled: false,
    featureId: "aboutwelcome",
-    value: null,
+    value: { enabled: false },
  });

  ExperimentAPI._store._syncToChildren({ flush: true });
@ -363,8 +362,8 @@ add_task(async function test_multistage_aboutwelcome_experimentAPI() {
  await ExperimentAPI.ready();

  let doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig({
-    enabled: true,
    featureId: "aboutwelcome",
+    enabled: true,
    value: {
      id: "my-mochitest-experiment",
      screens: TEST_MULTISTAGE_CONTENT,
@ -508,10 +507,10 @@ add_task(async function test_multistage_aboutwelcome_transitions() {
  await ExperimentAPI.ready();

  let doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig({
-    enabled: true,
    featureId: "aboutwelcome",
    value: {
      id: "my-mochitest-experiment",
+      enabled: true,
      screens: TEST_PROTON_CONTENT,
      isProton: true,
      transitions: true,
@ -568,10 +567,10 @@ add_task(async function test_multistage_aboutwelcome_transitions_off() {
  await ExperimentAPI.ready();

  let doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig({
-    enabled: true,
    featureId: "aboutwelcome",
    value: {
      id: "my-mochitest-experiment",
+      enabled: true,
      screens: TEST_PROTON_CONTENT,
      isProton: true,
      transitions: false,
--- a/browser/components/newtab/test/browser/browser_aboutwelcome_rtamo.js
+++ b/browser/components/newtab/test/browser/browser_aboutwelcome_rtamo.js
@ -209,11 +209,10 @@ add_task(async function test_rtamo_aboutwelcome() {
  );
 });

-add_task(async function test_rtamo_over_experimnts() {
+add_task(async function test_rtamo_over_experiments() {
  let doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig({
    featureId: "aboutwelcome",
-    enabled: true,
-    value: { screens: [] },
+    value: { screens: [], enabled: true },
  });

  let browser = await openRTAMOWelcomePage();
--- a/toolkit/components/nimbus/ExperimentAPI.jsm
+++ b/toolkit/components/nimbus/ExperimentAPI.jsm
@ -314,22 +314,6 @@ class ExperimentFeature {
    this._listenForRemoteDefaults = this._listenForRemoteDefaults.bind(this);
    const variables = this.manifest?.variables || {};

-    // Add special enabled flag
-    if (this.manifest?.enabledFallbackPref) {
-      XPCOMUtils.defineLazyPreferenceGetter(
-        this,
-        "enabled",
-        this.manifest?.enabledFallbackPref,
-        null,
-        () => {
-          ExperimentAPI._store._emitFeatureUpdate(
-            this.featureId,
-            "pref-updated"
-          );
-        }
-      );
-    }
-
    Object.keys(variables).forEach(key => {
      const { type, fallbackPref } = variables[key];
      if (fallbackPref) {
@ -444,12 +428,16 @@ class ExperimentFeature {
      return this.getRemoteConfig().enabled;
    }

-    // Then check the fallback pref, if it is defined
-    if (isBooleanValueDefined(this.enabled)) {
-      return this.enabled;
+    let enabled;
+    try {
+      enabled = this.getVariable("enabled", { sendExposureEvent });
+    } catch (e) {
+      /* This is expected not all features have an enabled flag defined */
+    }
+    if (isBooleanValueDefined(enabled)) {
+      return enabled;
    }

-    // Finally, return options.defaulValue if neither was found
    return defaultValue;
  }

@ -533,7 +521,7 @@ class ExperimentFeature {
    })?.feature?.value?.[variable];

    // Prevent future exposure events if user is enrolled in an experiment
-    if (experimentValue && sendExposureEvent) {
+    if (typeof experimentValue !== "undefined" && sendExposureEvent) {
      this._sendExposureEventOnce = false;
    }

--- a/toolkit/components/nimbus/FeatureManifest.js
+++ b/toolkit/components/nimbus/FeatureManifest.js
@ -56,8 +56,11 @@ const FeatureManifest = {
  aboutwelcome: {
    description: "The about:welcome page",
    isEarlyStartup: true,
-    enabledFallbackPref: "browser.aboutwelcome.enabled",
    variables: {
+      enabled: {
+        type: "boolean",
+        fallbackPref: "browser.aboutwelcome.enabled",
+      },
      screens: {
        type: "json",
        fallbackPref: "browser.aboutwelcome.screens",
@ -105,7 +108,12 @@ const FeatureManifest = {
  upgradeDialog: {
    description: "The dialog shown for major upgrades",
    isEarlyStartup: true,
-    enabledFallbackPref: "browser.startup.upgradeDialog.enabled",
+    variables: {
+      enabled: {
+        type: "boolean",
+        fallbackPref: "browser.startup.upgradeDialog.enabled",
+      },
+    },
  },
  privatebrowsing: {
    description: "about:privatebrowsing",
--- a/toolkit/components/nimbus/schemas/ExperimentFeatureManifest.schema.json
+++ b/toolkit/components/nimbus/schemas/ExperimentFeatureManifest.schema.json
@ -9,9 +9,6 @@
        "description": {
          "type": "string"
        },
-        "enabledFallbackPref": {
-          "type": "string"
-        },
        "isEarlyStartup": {
          "type": "boolean",
          "description": "If the feature values should be cached in prefs for fast early startup."
--- a/toolkit/components/nimbus/schemas/ExperimentFeatureRemote.schema.json
+++ b/toolkit/components/nimbus/schemas/ExperimentFeatureRemote.schema.json
@ -18,28 +18,26 @@
                "type": "string",
                "description": "Configuration identifier that will be included in Telemetry."
              },
-              "description": {
-                "type": "string",
-                "description": "Configuration description."
-              },
              "variables": {
                "type": "object",
-                "description": "Key value pairs that should match the feature manifest definition."
+                "description": "Key value pairs that should match the feature manifest definition.",
+                "properties": {
+                  "enabled": {
+                    "type": "boolean"
+                  }
+                },
+                "required": ["enabled"]
              },
              "targeting": {
                "type": "string",
                "description": "Target the configuration only to specific clients."
              },
-              "enabled": {
-                "type": "boolean",
-                "description": "Is the feature off or on."
-              },
              "description": {
                "type": "string",
                "description": "Explanation for configuration and targeting"
              }
            },
-            "required": ["variables", "enabled", "targeting", "slug"],
+            "required": ["variables", "targeting", "slug"],
            "additionalProperties": false
          }
        }
--- a/toolkit/components/nimbus/schemas/NimbusExperiment.schema.json
+++ b/toolkit/components/nimbus/schemas/NimbusExperiment.schema.json
@ -92,10 +92,6 @@
                    "type": "string",
                    "description": "The identifier for the feature flag"
                  },
-                  "enabled": {
-                    "type": "boolean",
-                    "description": "This can be used to turn the whole feature on/off"
-                  },
                  "value": {
                    "anyOf": [
                      {
@ -109,18 +105,11 @@
                    "description": "Optional extra params for the feature (this should be validated against a schema)"
                  }
                },
-                "required": [
-                  "featureId",
-                  "enabled",
-                  "value"
-                ],
+                "required": ["featureId", "value"],
                "additionalProperties": false
              }
            },
-            "required": [
-              "slug",
-              "ratio"
-            ],
+            "required": ["slug", "ratio"],
            "additionalProperties": false
          },
          "description": "Branch configuration for the experiment"
@ -130,18 +119,12 @@
          "description": "JEXL expression used to filter experiments based on locale, geo, etc."
        },
        "startDate": {
-          "type": [
-            "string",
-            "null"
-          ],
+          "type": ["string", "null"],
          "description": "Actual publish date of the experiment\nNote that this value is expected to be null in Remote Settings.",
          "format": "date-time"
        },
        "endDate": {
-          "type": [
-            "string",
-            "null"
-          ],
+          "type": ["string", "null"],
          "description": "Actual end date of the experiment.\nNote that this value is expected to be null in Remote Settings.",
          "format": "date-time"
        },
@ -154,10 +137,7 @@
          "description": "Duration of enrollment from the start date in days"
        },
        "referenceBranch": {
-          "type": [
-            "string",
-            "null"
-          ],
+          "type": ["string", "null"],
          "description": "The slug of the reference branch"
        },
        "filter_expression": {
--- a/toolkit/components/nimbus/test/NimbusTestUtils.jsm
+++ b/toolkit/components/nimbus/test/NimbusTestUtils.jsm
@ -188,8 +188,7 @@ const ExperimentFakes = {
        slug: "treatment",
        feature: {
          featureId: "test-feature",
-          enabled: true,
-          value: { title: "hello" },
+          value: { title: "hello", enabled: true },
        },
        ...props,
      },
@ -214,15 +213,14 @@ const ExperimentFakes = {
        {
          slug: "control",
          ratio: 1,
-          feature: { featureId: "test-feature", enabled: true, value: null },
+          feature: { featureId: "test-feature", value: { enabled: true } },
        },
        {
          slug: "treatment",
          ratio: 1,
          feature: {
            featureId: "test-feature",
-            enabled: true,
-            value: { title: "hello" },
+            value: { title: "hello", enabled: true },
          },
        },
      ],
--- a/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js
+++ b/toolkit/components/nimbus/test/unit/test_ExperimentAPI_ExperimentFeature.js
@ -27,8 +27,11 @@ function setDefaultBranch(pref, value) {

 const TEST_FALLBACK_PREF = "testprefbranch.config";
 const FAKE_FEATURE_MANIFEST = {
-  enabledFallbackPref: "testprefbranch.enabled",
  variables: {
+    enabled: {
+      type: "boolean",
+      fallbackPref: "testprefbranch.enabled",
+    },
    config: {
      type: "json",
      fallbackPref: TEST_FALLBACK_PREF,
@ -36,8 +39,9 @@ const FAKE_FEATURE_MANIFEST = {
  },
 };
 const FAKE_FEATURE_REMOTE_VALUE = {
-  variables: {},
-  enabled: true,
+  variables: {
+    enabled: true,
+  },
  targeting: "true",
 };

@ -174,7 +178,7 @@ add_task(async function test_ExperimentFeature_isEnabled_default() {
  sandbox.restore();
 });

-add_task(async function test_ExperimentFeature_isEnabled_remote_over_default() {
+add_task(async function test_ExperimentFeature_isEnabled_default_over_remote() {
  const { manager, sandbox } = await setupForExperimentFeature();
  await manager.store.ready();

@ -190,18 +194,25 @@ add_task(async function test_ExperimentFeature_isEnabled_remote_over_default() {

  manager.store.updateRemoteConfigs("foo", {
    ...FAKE_FEATURE_REMOTE_VALUE,
-    enabled: true,
+    variables: { enabled: true },
  });

  await featureInstance.ready();

  Assert.equal(
    featureInstance.isEnabled(),
-    true,
-    "should use the remote value over the default"
+    false,
+    "Should still use userpref over remote"
  );

  Services.prefs.clearUserPref("testprefbranch.enabled");
+
+  Assert.equal(
+    featureInstance.isEnabled(),
+    true,
+    "Should use remote value over default pref"
+  );
+
  sandbox.restore();
 });

@ -233,8 +244,7 @@ add_task(
        slug: "treatment",
        feature: {
          featureId: "foo",
-          enabled: true,
-          value: null,
+          value: { enabled: true },
        },
      },
    });
@ -245,10 +255,9 @@ add_task(
    manager.store.addExperiment(expected);

    const exposureSpy = sandbox.spy(ExperimentAPI, "recordExposureEvent");
-    Services.prefs.setBoolPref("testprefbranch.enabled", false);
    manager.store.updateRemoteConfigs("foo", {
      ...FAKE_FEATURE_REMOTE_VALUE,
-      enabled: false,
+      variables: { enabled: false },
    });

    await featureInstance.ready();
@ -256,16 +265,26 @@ add_task(
    Assert.equal(
      featureInstance.isEnabled(),
      true,
-      "should return the enabled value defined in the experiment, not the default pref or the remote value"
+      "should return the enabled value defined in the experiment not the remote value"
    );

-    Assert.ok(exposureSpy.notCalled, "should emit exposure by default event");
+    Services.prefs.setBoolPref("testprefbranch.enabled", false);
+
+    Assert.equal(
+      featureInstance.isEnabled(),
+      false,
+      "should return the user pref not the experiment value"
+    );
+
+    // Exposure is not triggered if user pref is set
+    Services.prefs.clearUserPref("testprefbranch.enabled");
+
+    Assert.ok(exposureSpy.notCalled, "should not emit exposure by default");

    featureInstance.isEnabled({ sendExposureEvent: true });

    Assert.ok(exposureSpy.calledOnce, "should emit exposure event");

-    Services.prefs.clearUserPref("testprefbranch.enabled");
    sandbox.restore();
  }
 );
@ -277,8 +296,7 @@ add_task(async function test_ExperimentFeature_isEnabled_no_exposure() {
      slug: "treatment",
      feature: {
        featureId: "foo",
-        enabled: false,
-        value: null,
+        value: { enabled: false },
      },
    },
  });
@ -322,8 +340,7 @@ add_task(async function test_record_exposure_event() {
        slug: "treatment",
        feature: {
          featureId: "foo",
-          enabled: false,
-          value: null,
+          value: { enabled: false },
        },
      },
    })
@ -353,8 +370,7 @@ add_task(async function test_record_exposure_event_once() {
        slug: "treatment",
        feature: {
          featureId: "foo",
-          enabled: false,
-          value: null,
+          value: { enabled: false },
        },
      },
    })
@ -383,8 +399,7 @@ add_task(async function test_prevent_double_exposure_getValue() {
        slug: "treatment",
        feature: {
          featureId: "foo",
-          enabled: false,
-          value: null,
+          value: { enabled: false },
        },
      },
    })
@ -416,8 +431,7 @@ add_task(async function test_prevent_double_exposure_isEnabled() {
        slug: "treatment",
        feature: {
          featureId: "foo",
-          enabled: false,
-          value: null,
+          value: { enabled: false },
        },
      },
    })
@ -462,3 +476,50 @@ add_task(async function test_set_remote_before_ready() {

  Assert.ok(feature.getValue().test, "Successfully set");
 });
+
+add_task(async function test_isEnabled_backwards_compatible() {
+  const PREVIOUS_FEATURE_MANIFEST = {
+    variables: {
+      config: {
+        type: "json",
+        fallbackPref: TEST_FALLBACK_PREF,
+      },
+    },
+  };
+  let sandbox = sinon.createSandbox();
+  const manager = ExperimentFakes.manager();
+  sandbox.stub(ExperimentAPI, "_store").get(() => manager.store);
+  const exposureSpy = sandbox.spy(ExperimentAPI, "recordExposureEvent");
+  const feature = new ExperimentFeature("foo", PREVIOUS_FEATURE_MANIFEST);
+
+  await manager.onStartup();
+
+  await ExperimentFakes.remoteDefaultsHelper({
+    feature,
+    store: manager.store,
+    configuration: { variables: {}, enabled: false },
+  });
+
+  Assert.ok(!feature.isEnabled(), "Disabled based on remote configs");
+
+  manager.store.addExperiment(
+    ExperimentFakes.experiment("blah", {
+      featureIds: ["foo"],
+      branch: {
+        slug: "treatment",
+        feature: {
+          featureId: "foo",
+          enabled: true,
+          value: {},
+        },
+      },
+    })
+  );
+
+  Assert.ok(exposureSpy.notCalled, "Not called until now");
+  Assert.ok(
+    feature.isEnabled({ sendExposureEvent: true }),
+    "Enabled based on experiment recipe"
+  );
+  Assert.ok(exposureSpy.calledOnce, "Exposure event sent");
+});
--- a/toolkit/components/nimbus/test/unit/test_ExperimentAPI_NimbusFeatures.js
+++ b/toolkit/components/nimbus/test/unit/test_ExperimentAPI_NimbusFeatures.js
@ -33,15 +33,13 @@ const REMOTE_CONFIGURATION = Object.freeze({
    {
      slug: "non-matching-configuration",
      description: "This configuration does not match because of targeting.",
-      variables: { skipFocus: false, remoteValue: false },
-      enabled: false,
+      variables: { skipFocus: false, remoteValue: false, enabled: false },
      targeting: "false",
    },
    {
      slug: "matching-configuration",
      description: "This configuration will match targeting.",
-      variables: { skipFocus: true, remoteValue: true },
-      enabled: true,
+      variables: { skipFocus: true, remoteValue: true, enabled: true },
      targeting: "true",
    },
  ],
@ -177,7 +175,7 @@ add_task(async function update_remote_defaults_enabled() {
  Assert.equal(
    feature.isEnabled(),
    true,
-    "Feature is enabled by enabledFallbackPref"
+    "Feature is enabled by manifest.variables.enabled"
  );

  manager.store.updateRemoteConfigs(
--- a/toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js
+++ b/toolkit/components/nimbus/test/unit/test_NimbusTestUtils.js
@ -55,8 +55,7 @@ add_task(async function test_enrollWithFeatureConfig() {
  let doExperimentCleanup = await ExperimentFakes.enrollWithFeatureConfig(
    {
      featureId: "enrollWithFeatureConfig",
-      enabled: true,
-      value: null,
+      value: { enabled: true },
    },
    { manager }
  );