Bug 1526387 - CFR Addon Recommendations call remote API before clicking "Install" r=k88hudson
authorRicky Rosario <rickyrosario@gmail.com>
Fri, 08 Feb 2019 23:59:48 +0000
changeset 458335 25cbdc59246f
parent 458334 fb6650393dd1
child 458336 5656f8b5c547
push id77808
push userrrosario@mozilla.com
push dateSat, 09 Feb 2019 03:00:52 +0000
treeherderautoland@25cbdc59246f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersk88hudson
bugs1526387
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1526387 - CFR Addon Recommendations call remote API before clicking "Install" r=k88hudson MozReview-Commit-ID: EgXnUTzfPf3 Differential Revision: https://phabricator.services.mozilla.com/D19229
browser/components/newtab/lib/CFRPageActions.jsm
browser/components/newtab/test/browser/browser_asrouter_cfr.js
browser/components/newtab/test/unit/asrouter/CFRPageActions.test.js
--- a/browser/components/newtab/lib/CFRPageActions.jsm
+++ b/browser/components/newtab/lib/CFRPageActions.jsm
@@ -320,17 +320,18 @@ class PageAction {
     for (let button of secondary) {
       let label = await this.getStrings(button.label);
       secondaryBtnStrings.push({label, attributes: label.attributes});
     }
 
     const mainAction = {
       label: primaryBtnStrings,
       accessKey: primaryBtnStrings.attributes.accesskey,
-      callback: () => {
+      callback: async () => {
+        primary.action.data.url = await CFRPageActions._fetchLatestAddonVersion(content.addon.id); // eslint-disable-line no-use-before-define
         this._blockMessage(id);
         this.dispatchUserAction(primary.action);
         this.hide();
         this._sendTelemetry({message_id: id, bucket_id: content.bucket_id, event: "INSTALL"});
         RecommendationMap.delete(browser);
       },
     };
 
@@ -417,107 +418,68 @@ const CFRPageActions = {
     } else {
       // There's no recommendation specified for this browser, so hide the page action
       pageAction.hide();
     }
   },
 
   /**
    * Fetch the URL to the latest add-on xpi so the recommendation can download it.
-   * @param addon          The add-on provided by the CFRMessageProvider
-   * @return               A string for the URL that was fetched
+   * @param id          The add-on ID
+   * @return            A string for the URL that was fetched
    */
-  async _fetchLatestAddonVersion({id}) {
+  async _fetchLatestAddonVersion(id) {
     let url = null;
     try {
-      const response = await fetch(`${ADDONS_API_URL}/${id}`);
+      const response = await fetch(`${ADDONS_API_URL}/${id}/`, {credentials: "omit"});
       if (response.status !== 204 && response.ok) {
         const json = await response.json();
         url = json.current_version.files[0].url;
       }
     } catch (e) {
       Cu.reportError("Failed to get the latest add-on version for this recommendation");
     }
     return url;
   },
 
-  async _maybeAddAddonInstallURL(recommendation) {
-    const {content, template} = recommendation;
-    // If this is CFR is not for an add-on, return the original recommendation
-    if (template !== "cfr_doorhanger") {
-      return recommendation;
-    }
-
-    const url = await this._fetchLatestAddonVersion(content.addon);
-    // If we failed to get a url to the latest xpi, return false so we know not to show
-    // a recommendation
-    if (!url) {
-      return false;
-    }
-
-    // Update the action's data with the url to the latest xpi, leave the rest
-    // of the recommendation properties intact
-    return {
-      ...recommendation,
-      content: {
-        ...content,
-        buttons: {
-          ...content.buttons,
-          primary: {
-            ...content.buttons.primary,
-            action: {...content.buttons.primary.action, data: {url}},
-          },
-        },
-      },
-    };
-  },
-
   /**
    * Force a recommendation to be shown. Should only happen via the Admin page.
    * @param browser                 The browser for the recommendation
-   * @param originalRecommendation  The recommendation to show
+   * @param recommendation  The recommendation to show
    * @param dispatchToASRouter      A function to dispatch resulting actions to
    * @return                        Did adding the recommendation succeed?
    */
-  async forceRecommendation(browser, originalRecommendation, dispatchToASRouter) {
+  async forceRecommendation(browser, recommendation, dispatchToASRouter) {
     // If we are forcing via the Admin page, the browser comes in a different format
     const win = browser.browser.ownerGlobal;
-    const recommendation = await this._maybeAddAddonInstallURL(originalRecommendation);
-    if (!recommendation) {
-      return false;
-    }
     const {id, content} = recommendation;
     RecommendationMap.set(browser.browser, {id, content});
     if (!PageActionMap.has(win)) {
       PageActionMap.set(win, new PageAction(win, dispatchToASRouter));
     }
     await PageActionMap.get(win).show(recommendation, true);
     return true;
   },
 
   /**
    * Add a recommendation specific to the given browser and host.
    * @param browser                 The browser for the recommendation
    * @param host                    The host for the recommendation
-   * @param originalRecommendation  The recommendation to show
+   * @param recommendation  The recommendation to show
    * @param dispatchToASRouter      A function to dispatch resulting actions to
    * @return                        Did adding the recommendation succeed?
    */
-  async addRecommendation(browser, host, originalRecommendation, dispatchToASRouter) {
+  async addRecommendation(browser, host, recommendation, dispatchToASRouter) {
     const win = browser.ownerGlobal;
     if (PrivateBrowsingUtils.isWindowPrivate(win)) {
       return false;
     }
     if (browser !== win.gBrowser.selectedBrowser || !isHostMatch(browser, host)) {
       return false;
     }
-    const recommendation = await this._maybeAddAddonInstallURL(originalRecommendation);
-    if (!recommendation) {
-      return false;
-    }
     const {id, content} = recommendation;
     RecommendationMap.set(browser, {id, host, content});
     if (!PageActionMap.has(win)) {
       PageActionMap.set(win, new PageAction(win, dispatchToASRouter));
     }
     await PageActionMap.get(win).show(recommendation, true);
     return true;
   },
--- a/browser/components/newtab/test/browser/browser_asrouter_cfr.js
+++ b/browser/components/newtab/test/browser/browser_asrouter_cfr.js
@@ -13,31 +13,32 @@ function trigger_cfr_panel(browser, trig
       content: {
         notification_text: "Mochitest",
         heading_text: "Mochitest",
         info_icon: {
           label: {attributes: {tooltiptext: "Why am I seeing this"}},
           sumo_path: "extensionrecommendations",
         },
         addon: {
+          id: "addon-id",
           title: "Addon name",
           icon: "foo",
           author: "Author name",
           amo_url: "https://example.com",
         },
         text: "Mochitest",
         buttons: {
           primary: {
             label: {
               value: "OK",
               attributes: {accesskey: "O"},
             },
             action: {
               type: action.type,
-              data: {url: action.url},
+              data: {},
             },
           },
           secondary: [{
             label: {
               value: "Cancel",
               attributes: {accesskey: "C"},
             },
           }, {
@@ -56,22 +57,22 @@ function trigger_cfr_panel(browser, trig
     },
     // Use the real AS dispatch method to trigger real notifications
     ASRouter.dispatch
   );
 }
 
 add_task(async function setup() {
   // Store it in order to restore to the original value
-  const {_maybeAddAddonInstallURL} = CFRPageActions;
+  const {_fetchLatestAddonVersion} = CFRPageActions;
   // Prevent fetching the real addon url and making a network request
-  CFRPageActions._maybeAddAddonInstallURL = x => x;
+  CFRPageActions._fetchLatestAddonVersion = x => "http://example.com";
 
   registerCleanupFunction(() => {
-    CFRPageActions._maybeAddAddonInstallURL = _maybeAddAddonInstallURL;
+    CFRPageActions._fetchLatestAddonVersion = _fetchLatestAddonVersion;
   });
 });
 
 add_task(async function test_cfr_notification_show() {
   // addRecommendation checks that scheme starts with http and host matches
   let browser = gBrowser.selectedBrowser;
   await BrowserTestUtils.loadURI(browser, "http://example.com/");
   await BrowserTestUtils.browserLoaded(browser, false, "http://example.com/");
@@ -99,17 +100,17 @@ add_task(async function test_cfr_notific
 });
 
 add_task(async function test_cfr_addon_install() {
   // addRecommendation checks that scheme starts with http and host matches
   const browser = gBrowser.selectedBrowser;
   await BrowserTestUtils.loadURI(browser, "http://example.com/");
   await BrowserTestUtils.browserLoaded(browser, false, "http://example.com/");
 
-  const response = await trigger_cfr_panel(browser, "example.com", {type: "INSTALL_ADDON_FROM_URL", url: "http://example.com"});
+  const response = await trigger_cfr_panel(browser, "example.com", {type: "INSTALL_ADDON_FROM_URL"});
   Assert.ok(response, "Should return true if addRecommendation checks were successful");
 
   const showPanel = BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshown");
   // Open the panel
   document.getElementById("contextual-feature-recommendation").click();
   await showPanel;
 
   Assert.ok(document.getElementById("contextual-feature-recommendation-notification").hidden === false,
--- a/browser/components/newtab/test/unit/asrouter/CFRPageActions.test.js
+++ b/browser/components/newtab/test/unit/asrouter/CFRPageActions.test.js
@@ -51,17 +51,20 @@ describe("CFRPageActions", () => {
           rating: 4.2,
           users: 1234,
           amo_url: "a_path_to_amo",
         },
         text: "Here is the recommendation text body",
         buttons: {
           primary: {
             label: {string_id: "primary_button_id"},
-            action: {id: "primary_action"},
+            action: {
+              id: "primary_action",
+              data: {},
+            },
           },
           secondary: [{
             label: {string_id: "secondary_button_id"},
             action: {id: "secondary_action"},
           }, {
             label: {string_id: "secondary_button_id_2"},
             action: {id: "secondary_action"},
           }, {
@@ -463,32 +466,32 @@ describe("CFRPageActions", () => {
             source: "CFR",
             message_id: fakeRecommendation.id,
             bucket_id: fakeRecommendation.content.bucket_id,
             event: "CLICK_DOORHANGER",
           },
         });
       });
       it("should set the main action correctly", async () => {
+        sinon.stub(CFRPageActions, "_fetchLatestAddonVersion").resolves("latest-addon.xpi");
         await pageAction._handleClick();
         const mainAction = global.PopupNotifications.show.firstCall.args[4]; // eslint-disable-line prefer-destructuring
-
         assert.deepEqual(mainAction.label, {value: "Primary Button", attributes: {accesskey: "p"}});
         sandbox.spy(pageAction, "hide");
-        mainAction.callback();
+        await mainAction.callback();
         assert.calledOnce(pageAction.hide);
         // Should block the message
         assert.calledWith(dispatchStub, {
           type: "BLOCK_MESSAGE_BY_ID",
           data: {id: fakeRecommendation.id},
         });
         // Should trigger the action
         assert.calledWith(
           dispatchStub,
-          {type: "USER_ACTION", data: {id: "primary_action"}},
+          {type: "USER_ACTION", data: {id: "primary_action", data: {url: "latest-addon.xpi"}}},
           {browser: fakeBrowser}
         );
         // Should send telemetry
         assert.calledWith(dispatchStub, {
           type: "DOORHANGER_TELEMETRY",
           data: {
             action: "cfr_user_event",
             source: "CFR",
@@ -692,21 +695,19 @@ describe("CFRPageActions", () => {
         assert.isFalse(CFRPageActions.PageActionMap.has(win));
         await CFRPageActions.addRecommendation(fakeBrowser, fakeHost, fakeRecommendation, dispatchStub);
         const pageAction = CFRPageActions.PageActionMap.get(win);
         assert.equal(win, pageAction.window);
         assert.equal(dispatchStub, pageAction._dispatchToASRouter);
         assert.calledOnce(PageAction.prototype.show);
       });
       it("should add the right url if we fetched and addon install URL", async () => {
-        sinon.stub(CFRPageActions, "_fetchLatestAddonVersion").returns(Promise.resolve("latest-addon.xpi"));
         fakeRecommendation.template = "cfr_doorhanger";
         await CFRPageActions.addRecommendation(fakeBrowser, fakeHost, fakeRecommendation, dispatchStub);
         const recommendation = CFRPageActions.RecommendationMap.get(fakeBrowser);
-        assert.equal(recommendation.content.buttons.primary.action.data.url, "latest-addon.xpi");
 
         // sanity check - just go through some of the rest of the attributes to make sure they were untouched
         assert.equal(recommendation.id, fakeRecommendation.id);
         assert.equal(recommendation.content.heading_text, fakeRecommendation.content.heading_text);
         assert.equal(recommendation.content.addon, fakeRecommendation.content.addon);
         assert.equal(recommendation.content.text, fakeRecommendation.content.text);
         assert.equal(recommendation.content.buttons.secondary, fakeRecommendation.content.buttons.secondary);
         assert.equal(recommendation.content.buttons.primary.action.id, fakeRecommendation.content.buttons.primary.action.id);