Bug 1526387 - CFR Addon Recommendations call remote API before clicking "Install" r=k88hudson, a=RyanVM
authorRicky Rosario <rickyrosario@gmail.com>
Fri, 08 Feb 2019 23:59:48 +0000
changeset 509640 be2b97abeeff
parent 509639 8016ccca3704
child 509641 129c513995ea
push id1935
push userryanvm@gmail.com
push dateMon, 11 Feb 2019 16:39:10 +0000
treeherdermozilla-release@be2b97abeeff [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersk88hudson, RyanVM
bugs1526387
milestone65.0.1
Bug 1526387 - CFR Addon Recommendations call remote API before clicking "Install" r=k88hudson, a=RyanVM 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
@@ -324,17 +324,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);
       },
     };
 
@@ -421,107 +422,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"},
           }, {
@@ -465,32 +468,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",
@@ -694,21 +697,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);