Bug 1515411 - Disable Return to AMO for 65 Beta and Release. r=k88hudson, a=RyanVM
authorAndrei Oprea <andrei.br92@gmail.com>
Wed, 09 Jan 2019 14:48:41 +0000
changeset 509464 4befe1a74031dbeb0f106449047f3f827fe45d7d
parent 509463 8f38ca46fb4495a087734b8269a61a51c284e6da
child 509465 76325782999f2b06123ebbff85df059f3cab0696
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersk88hudson, RyanVM
bugs1515411
milestone65.0
Bug 1515411 - Disable Return to AMO for 65 Beta and Release. r=k88hudson, a=RyanVM This adds an `exclude` field in local providers that allows to exclude specific messages by ID. Test Plan: In a fresh Firefox profile. On OSX make sure this is launched from the Applications folder and not from the installer. > in order to confirm RTAMO is enabled 1. enable `browser.newtabpage.activity-stream.asrouter.devtoolsEnabled` 2. set `browser.newtabpage.activity-stream.asrouter.providers.onboarding` to `{"id":"onboarding","type":"local","localProvider":"OnboardingMessageProvider","enabled":true,"exclude":[]}` 3. navigate to `about:newtab#asrouter` and scroll to the bottom 4. click `Force attribution` 5. navigate to `about:welcome` and confirm that the RTAMO message is shown > in order to confirm RTAMO is disabled 6. set `browser.newtabpage.activity-stream.asrouter.providers.onboarding` to `{"id":"onboarding","type":"local","localProvider":"OnboardingMessageProvider","enabled":true,"exclude":["RETURN_TO_AMO_1"]}` 7. navigate to `about:welcome` and confirm that the FxA accounts page is shown and not RTAMO Differential Revision: https://phabricator.services.mozilla.com/D16053
browser/components/newtab/lib/ASRouter.jsm
browser/components/newtab/lib/ASRouterTargeting.jsm
browser/components/newtab/lib/ActivityStream.jsm
browser/components/newtab/lib/OnboardingMessageProvider.jsm
browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js
browser/components/newtab/test/unit/asrouter/MessageLoaderUtils.test.js
browser/components/newtab/test/unit/content-src/components/ReturnToAMO.test.jsx
browser/components/newtab/test/unit/content-src/components/StartupOverlay.test.jsx
browser/components/newtab/test/unit/unit-entry.js
--- a/browser/components/newtab/lib/ASRouter.jsm
+++ b/browser/components/newtab/lib/ASRouter.jsm
@@ -202,16 +202,20 @@ const MessageLoaderUtils = {
   async loadMessagesForProvider(provider, storage) {
     const loader = this._getMessageLoader(provider);
     let messages = await loader(provider, storage);
     // istanbul ignore if
     if (!messages) {
       messages = [];
       Cu.reportError(new Error(`Tried to load messages for ${provider.id} but the result was not an Array.`));
     }
+    // Filter out messages we temporarily want to exclude
+    if (provider.exclude && provider.exclude.length) {
+      messages = messages.filter(message => !provider.exclude.includes(message.id));
+    }
     const lastUpdated = Date.now();
     return {
       messages: messages.map(msg => ({weight: 100, ...msg, provider: provider.id}))
                         .filter(message => message.weight > 0),
       lastUpdated,
     };
   },
 
--- a/browser/components/newtab/lib/ASRouterTargeting.jsm
+++ b/browser/components/newtab/lib/ASRouterTargeting.jsm
@@ -148,16 +148,34 @@ const QueryCache = {
  */
 function sortMessagesByWeightedRank(messages) {
   return messages
     .map(message => ({message, rank: Math.pow(Math.random(), 1 / message.weight)}))
     .sort((a, b) => b.rank - a.rank)
     .map(({message}) => message);
 }
 
+/**
+ * Messages with targeting should get evaluated first, this way we can have
+ * fallback messages (no targeting at all) that will show up if nothing else
+ * matched
+ */
+function sortMessagesByTargeting(messages) {
+  return messages.sort((a, b) => {
+    if (a.targeting && !b.targeting) {
+      return -1;
+    }
+    if (!a.targeting && b.targeting) {
+      return 1;
+    }
+
+    return 0;
+  });
+}
+
 const TargetingGetters = {
   get locale() {
     return Services.locale.appLocaleAsLangTag;
   },
   get localeLanguageCode() {
     return Services.locale.appLocaleAsLangTag && Services.locale.appLocaleAsLangTag.substr(0, 2);
   },
   get browserSettings() {
@@ -342,17 +360,18 @@ this.ASRouterTargeting = {
    *
    * @param {Array} messages An array of AS router messages
    * @param {obj} impressions An object containing impressions, where keys are message ids
    * @param {trigger} string A trigger expression if a message for that trigger is desired
    * @param {obj|null} context A FilterExpression context. Defaults to TargetingGetters above.
    * @returns {obj} an AS router message
    */
   async findMatchingMessage({messages, trigger, context, onError}) {
-    const sortedMessages = sortMessagesByWeightedRank([...messages]);
+    const weightSortedMessages = sortMessagesByWeightedRank([...messages]);
+    const sortedMessages = sortMessagesByTargeting(weightSortedMessages);
 
     for (const candidate of sortedMessages) {
       if (
         candidate &&
         (trigger ? this.isTriggerMatch(trigger, candidate.trigger) : !candidate.trigger) &&
         // If a trigger expression was passed to this function, the message should match it.
         // Otherwise, we should choose a message with no trigger property (i.e. a message that can show up at any time)
         await this.checkMessageTargeting(candidate, context, onError)
--- a/browser/components/newtab/lib/ActivityStream.jsm
+++ b/browser/components/newtab/lib/ActivityStream.jsm
@@ -202,16 +202,18 @@ const PREFS_CONFIG = new Map([
   }],
   ["asrouter.providers.onboarding", {
     title: "Configuration for onboarding provider",
     value: JSON.stringify({
       id: "onboarding",
       type: "local",
       localProvider: "OnboardingMessageProvider",
       enabled: true,
+      // Block specific messages from this local provider
+      exclude: ["RETURN_TO_AMO_1"],
     }),
   }],
   // See browser/app/profile/firefox.js for other ASR preferences. They must be defined there to enable roll-outs.
 ]);
 
 // Array of each feed's FEEDS_CONFIG factory and values to add to PREFS_CONFIG
 const FEEDS_DATA = [
   {
--- a/browser/components/newtab/lib/OnboardingMessageProvider.jsm
+++ b/browser/components/newtab/lib/OnboardingMessageProvider.jsm
@@ -120,17 +120,16 @@ const ONBOARDING_MESSAGES = async () => 
       },
     },
     targeting: "attributionData.campaign == 'non-fx-button' && attributionData.source == 'addons.mozilla.org'",
     trigger: {id: "showOnboarding"},
   },
   {
     id: "FXA_1",
     template: "fxa_overlay",
-    targeting: "attributionData.campaign != 'non-fx-button' && attributionData.source != 'addons.mozilla.org'",
     trigger: {id: "firstRun"},
   },
   {
     id: "RETURN_TO_AMO_1",
     template: "return_to_amo_overlay",
     content: {
       header: {string_id: "onboarding-welcome-header"},
       title: {string_id: "return-to-amo-sub-header"},
--- a/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js
+++ b/browser/components/newtab/test/unit/asrouter/ASRouterTargeting.test.js
@@ -1,9 +1,10 @@
-import {CachedTargetingGetter} from "lib/ASRouterTargeting.jsm";
+import {ASRouterTargeting, CachedTargetingGetter} from "lib/ASRouterTargeting.jsm";
+import {OnboardingMessageProvider} from "lib/OnboardingMessageProvider.jsm";
 
 // Note that tests for the ASRouterTargeting environment can be found in
 // test/functional/mochitest/browser_asrouter_targeting.js
 
 describe("#CachedTargetingGetter", () => {
   const sixHours = 6 * 60 * 60 * 1000;
   let sandbox;
   let clock;
@@ -45,9 +46,28 @@ describe("#CachedTargetingGetter", () =>
     // workaround
     try {
       await topsitesCache.get();
       assert.isTrue(false);
     } catch (e) {
       assert.calledOnce(global.Cu.reportError);
     }
   });
+  it("should check targeted message before message without targeting", async () => {
+    const messages = (await OnboardingMessageProvider.getUntranslatedMessages());
+    const stub = sandbox.stub(ASRouterTargeting, "checkMessageTargeting").resolves();
+    const context = {attributionData: {campaign: "non-fx-button", source: "addons.mozilla.org"}};
+    await ASRouterTargeting.findMatchingMessage({messages, trigger: {id: "firstRun"}, context});
+
+    assert.calledTwice(stub);
+    assert.equal(stub.firstCall.args[0].id, "RETURN_TO_AMO_1");
+    assert.equal(stub.secondCall.args[0].id, "FXA_1");
+  });
+  it("should return FxA message (is fallback)", async () => {
+    const messages = (await OnboardingMessageProvider.getUntranslatedMessages())
+      .filter(m => m.id !== "RETURN_TO_AMO_1");
+    const context = {attributionData: {campaign: "non-fx-button", source: "addons.mozilla.org"}};
+    const result = await ASRouterTargeting.findMatchingMessage({messages, trigger: {id: "firstRun"}, context});
+
+    assert.isDefined(result);
+    assert.equal(result.id, "FXA_1");
+  });
 });
--- a/browser/components/newtab/test/unit/asrouter/MessageLoaderUtils.test.js
+++ b/browser/components/newtab/test/unit/asrouter/MessageLoaderUtils.test.js
@@ -31,16 +31,24 @@ describe("MessageLoaderUtils", () => {
       const result = await MessageLoaderUtils.loadMessagesForProvider(provider, FAKE_STORAGE);
 
       assert.isArray(result.messages);
       // Does the message have the right properties?
       const [message] = result.messages;
       assert.propertyVal(message, "id", "foo");
       assert.propertyVal(message, "provider", "provider123");
     });
+    it("should filter out local messages listed in the `exclude` field", async () => {
+      const sourceMessage = {id: "foo"};
+      const provider = {id: "provider123", type: "local", messages: [sourceMessage], exclude: ["foo"]};
+
+      const result = await MessageLoaderUtils.loadMessagesForProvider(provider, FAKE_STORAGE);
+
+      assert.lengthOf(result.messages, 0);
+    });
     it("should return messages for remote provider", async () => {
       const sourceMessage = {id: "foo"};
       fetchStub.resolves({ok: true, status: 200, json: () => Promise.resolve({messages: [sourceMessage]}), headers: FAKE_RESPONSE_HEADERS});
       const provider = {id: "provider123", type: "remote", url: "https://foo.com"};
 
       const result = await MessageLoaderUtils.loadMessagesForProvider(provider, FAKE_STORAGE);
       assert.isArray(result.messages);
       // Does the message have the right properties?
new file mode 100644
--- /dev/null
+++ b/browser/components/newtab/test/unit/content-src/components/ReturnToAMO.test.jsx
@@ -0,0 +1,22 @@
+import {mountWithIntl} from "test/unit/utils";
+import React from "react";
+import {ReturnToAMO} from "content-src/asrouter/templates/ReturnToAMO/ReturnToAMO";
+
+describe("<ReturnToAMO>", () => {
+  let dispatch;
+  let onReady;
+  beforeEach(() => {
+    dispatch = sinon.stub();
+    onReady = sinon.stub();
+    const content = {
+      primary_button: {},
+      secondary_button: {},
+    };
+
+    mountWithIntl(<ReturnToAMO onReady={onReady} dispatch={dispatch} content={content} />);
+  });
+
+  it("should call onReady on componentDidMount", () => {
+    assert.calledOnce(onReady);
+  });
+});
--- a/browser/components/newtab/test/unit/content-src/components/StartupOverlay.test.jsx
+++ b/browser/components/newtab/test/unit/content-src/components/StartupOverlay.test.jsx
@@ -3,29 +3,45 @@ import {mountWithIntl} from "test/unit/u
 import React from "react";
 import {_StartupOverlay as StartupOverlay} from "content-src/asrouter/templates/StartupOverlay/StartupOverlay";
 
 describe("<StartupOverlay>", () => {
   let wrapper;
   let dispatch;
   let onReady;
   let onBlock;
+  let sandbox;
   beforeEach(() => {
-    dispatch = sinon.stub();
-    onReady = sinon.stub();
-    onBlock = sinon.stub();
+    sandbox = sinon.sandbox.create();
+    dispatch = sandbox.stub();
+    onReady = sandbox.stub();
+    onBlock = sandbox.stub();
 
     wrapper = mountWithIntl(<StartupOverlay onBlock={onBlock} onReady={onReady} dispatch={dispatch} />);
   });
 
+  afterEach(() => {
+    sandbox.restore();
+  });
+
   it("should not render if state.show is false", () => {
     wrapper.setState({overlayRemoved: true});
     assert.isTrue(wrapper.isEmptyRender());
   });
 
+  it("should call prop.onReady after mount + timeout", async () => {
+    const clock = sandbox.useFakeTimers();
+    wrapper = mountWithIntl(<StartupOverlay onBlock={onBlock} onReady={onReady} dispatch={dispatch} />);
+    wrapper.setState({overlayRemoved: false});
+
+    clock.tick(10);
+
+    assert.calledOnce(onReady);
+  });
+
   it("should emit UserEvent SKIPPED_SIGNIN when you click the skip button", () => {
     let skipButton = wrapper.find(".skip-button");
     assert.ok(skipButton.exists());
     skipButton.simulate("click");
 
     assert.calledOnce(dispatch);
     assert.isUserEventAction(dispatch.firstCall.args[0]);
     assert.calledWith(dispatch, ac.UserEvent({event: at.SKIPPED_SIGNIN, value: {has_flow_params: false}}));
--- a/browser/components/newtab/test/unit/unit-entry.js
+++ b/browser/components/newtab/test/unit/unit-entry.js
@@ -260,16 +260,25 @@ const TEST_GLOBAL = {
         if (name === "attachment") {
           return Promise.resolve([{attachment: {}}]);
         }
         return Promise.resolve([]);
       },
       on() {},
     };
   },
-  Localization: class {},
+  Localization: class {
+    async formatMessages(stringsIds) {
+      return Promise.resolve(stringsIds.map(({id, args}) => ({value: {string_id: id, args}})));
+    }
+  },
+  FxAccountsConfig: {
+    promiseEmailFirstURI(id) {
+      return Promise.resolve(id);
+    },
+  },
 };
 overrider.set(TEST_GLOBAL);
 
 describe("activity-stream", () => {
   after(() => overrider.restore());
   files.forEach(file => req(file));
 });