Incorporate Osmose's feedback

* Note: This fix is complicated by the fact that a 'price alert' badge only changes the global badge state. An 'add' product badge changes the tab-specific badge state, which takes precedence over the global badge state.

To avoid event duplication for 'badge_toolbar_button':
  * Add 'hasBadgeTextChanged' function with optional 'tabId' argument, so that the event only fires if the badge text has changed.
    * Previously, we were getting a bunch of redundant `badge_toolbar_button` events for ‘price_alert’ any time the state was changed and we’re on a page with an ‘add’ badge. Since `handlePriceAlerts` is called every time state changes in any way (`store.subscribe(handlePriceAlerts)`), we now check to see if the badge text has changed for a price alert badge on the global browserAction button before recording the 'badge_toolbar_button' with badge type 'price_alert' event.
    * Previously, if tab 1 had badge type 'add' and tab 2 had badge type 'price_alert', and I switch to tab 2 before any calls to 'resend' (#220) are made from tab 1, I would get two extra 'badge_toolbar_button' of type 'add' events. Adding the 'tabId' optional argument fixes this bug.

src/background/extraction.js

@@ -10,7 +10,7 @@
 import config from 'commerce/config';
 import {updateProductWithExtracted} from 'commerce/background/price_updates';
 import {isValidExtractedProduct} from 'commerce/state/products';
-import {recordEvent} from 'commerce/telemetry/extension';
+import {recordEvent, hasBadgeTextChanged} from 'commerce/telemetry/extension';
 
 /**
  * Triggers background tasks when a product is extracted from a webpage. Along
@@ -21,7 +21,7 @@ import {recordEvent} from 'commerce/telemetry/extension';
  * @param {MessageSender} sender
  *  The sender for the content script that extracted this product
  */
-export async function handleExtractedProductData({extractedProduct, sendTelemetry = true}, sender) {
+export async function handleExtractedProductData({extractedProduct}, sender) {
   // Do nothing if the extracted product isn't valid.
   if (!isValidExtractedProduct(extractedProduct)) {
     return;
@@ -51,12 +51,12 @@ export async function handleExtractedProductData({extractedProduct, sendTelemetr
       color: await config.get('badgeDetectBackground'),
       tabId,
     });
-    browser.browserAction.setBadgeText({text: '✚', tabId});
-    if (sendTelemetry) {
+    if (await hasBadgeTextChanged('✚', sender.tab.id)) {
       await recordEvent('badge_toolbar_button', 'toolbar_button', null, {
         badge_type: 'add',
       });
     }
+    browser.browserAction.setBadgeText({text: '✚', tabId});
   }
 
   // Update saved product data if it exists

src/background/price_alerts.js

@@ -22,7 +22,7 @@ import {
 } from 'commerce/state/prices';
 import {getProduct} from 'commerce/state/products';
 import {getVendor} from 'commerce/state/vendors';
-import {recordEvent} from 'commerce/telemetry/extension';
+import {recordEvent, hasBadgeTextChanged} from 'commerce/telemetry/extension';
 
 /**
  * Update the extension UI based on the current state of active price alerts.
@@ -34,10 +34,12 @@ export async function handlePriceAlerts() {
 
   // Show the browser action badge if there are any active alerts.
   if (activeAlerts.length > 0) {
+    if (await hasBadgeTextChanged(`${activeAlerts.length}`)) {
+      await recordEvent('badge_toolbar_button', 'toolbar_button', null, {
+        badge_type: 'price_alert',
+      });
+    }
     browser.browserAction.setBadgeText({text: `${activeAlerts.length}`});
-    await recordEvent('badge_toolbar_button', 'toolbar_button', null, {
-      badge_type: 'price_alert',
-    });
   } else {
     browser.browserAction.setBadgeText({text: null});
   }
@@ -66,7 +68,7 @@ export async function handlePriceAlerts() {
     });
     await recordEvent('send_notification', 'system_notification', null, { // eslint-disable-line no-await-in-loop
       price: alertPrice.amount.getAmount(),
-      price_last_high: highPriceAmount.getAmount(),
+      price_last_high: alert.highPriceAmount,
       price_orig: originalPrice.amount.getAmount(),
       product_key: product.anonId,
     });
@@ -95,12 +97,11 @@ export async function handleNotificationClicked(notificationId) {
     // Record open_external_page event
     const latestPrice = getLatestPriceForProduct(state, product.id);
     const originalPrice = getOldestPriceForProduct(state, product.id);
-    const highPriceAmount = Dinero({amount: alert.highPriceAmount});
     await recordEvent('open_external_page', 'ui_element', null, {
       element: 'system_notification',
       price: latestPrice.amount.getAmount(),
       price_alert: alert.active,
-      price_last_high: highPriceAmount.getAmount(),
+      price_last_high: alert.highPriceAmount,
       price_orig: originalPrice.amount.getAmount(),
       product_key: product.anonId,
     });

src/background/price_updates.js

@@ -91,16 +91,13 @@ export async function updateProductWithExtracted(data) {
   const id = getProductIdFromExtracted(data);
   const product = getProduct(state, id);
   if (product) {
-    const isPriceChange = await store.dispatch(addPriceFromExtracted(data));
-    if (isPriceChange) {
+    const price = await store.dispatch(addPriceFromExtracted(data));
+    if (price) {
       // Record the detect_price_change event
       const previousPrice = getLatestPriceForProduct(state, id);
-      // Need to refetch state, since we just added a price entry for this product
-      const updatedState = store.getState();
-      const latestPrice = getLatestPriceForProduct(updatedState, id);
-      const originalPrice = getOldestPriceForProduct(updatedState, id);
+      const originalPrice = getOldestPriceForProduct(state, id);
       await recordEvent('detect_price_change', 'product_page', null, {
-        price: latestPrice.amount.getAmount(),
+        price: price.amount,
         price_prev: previousPrice.amount.getAmount(),
         price_orig: originalPrice.amount.getAmount(),
         product_key: product.anonId,

src/extraction/index.js

@@ -28,40 +28,72 @@ const EXTRACTION_METHODS = {
   open_graph: extractProductWithOpenGraph,
 };
 
+const IS_BACKGROUND_UPDATE = (function isBackgroundUpdate() {
+  let result = false;
+  try {
+    result = window.top.location.href.startsWith(
+      browser.runtime.getURL('/'), // URL is unique per-install / hard to forge
+    );
+  } catch (err) {
+    // Non-background updates may throw a cross-origin error
+  }
+  return result;
+}());
+
+/**
+ * Helper class to record extraction-related telemetry events.
+ */
+class ExtractionAttempt {
+  constructor() {
+    this.baseExtra = {
+      extraction_id: uuidv4(),
+      is_bg_update: IS_BACKGROUND_UPDATE,
+    };
+  }
+
+  start() {
+    recordEvent('attempt_extraction', 'product_page', null, {
+      ...this.baseExtra,
+    });
+  }
+
+  succeed(methodName) {
+    recordEvent('complete_extraction', 'product_page', null, {
+      ...this.baseExtra,
+      method: methodName,
+    });
+  }
+
+  fail() {
+    recordEvent('complete_extraction', 'product_page', null, {
+      ...this.baseExtra,
+      method: 'none',
+    });
+  }
+}
+
 /**
  * Perform product extraction, trying each method from EXTRACTION_METHODS in
  * order until one of them returns a truthy result.
  * @return {ExtractedProduct|null}
  */
-function extractProduct(isBackgroundUpdate) {
-  const baseExtra = {
-    extraction_id: uuidv4(),
-    is_bg_update: isBackgroundUpdate,
-  };
-  recordEvent('attempt_extraction', 'product_page', null, {
-    ...baseExtra,
-  });
-  for (const [method, extract] of Object.entries(EXTRACTION_METHODS)) {
+function extractProduct() {
+  const attempt = new ExtractionAttempt();
+  attempt.start();
+  for (const [methodName, extract] of Object.entries(EXTRACTION_METHODS)) {
     const extractedProduct = extract(window.document);
     if (extractedProduct) {
-      recordEvent('complete_extraction', 'product_page', null, {
-        ...baseExtra,
-        method,
-      });
+      attempt.succeed(methodName);
       return extractedProduct;
     }
   }
-  recordEvent('complete_extraction', 'product_page', null, {
-    ...baseExtra,
-    method: 'none',
-  });
+  attempt.fail();
   return null;
 }
 
-async function sendProductToBackground(extractedProduct, sendTelemetry) {
+async function sendProductToBackground(extractedProduct) {
   return browser.runtime.sendMessage({
     type: 'extracted-product',
-    sendTelemetry,
     extractedProduct: {
       ...extractedProduct,
       url: document.location.href,
@@ -74,8 +106,8 @@ async function sendProductToBackground(extractedProduct, sendTelemetry) {
  * Checks to see if any product information for the page was found,
  * and if so, sends it to the background script.
  */
-async function attemptExtraction(isBackgroundUpdate) {
-  const extractedProduct = extractProduct(isBackgroundUpdate);
+async function attemptExtraction() {
+  const extractedProduct = extractProduct();
   if (extractedProduct) {
     await sendProductToBackground(extractedProduct);
   }
@@ -87,16 +119,8 @@ async function attemptExtraction(isBackgroundUpdate) {
   // If we're in an iframe, don't bother extracting a product EXCEPT if we were
   // started by the background script for a price check.
   const isInIframe = window !== window.top;
-  let isBackgroundUpdate = false;
-  try {
-    isBackgroundUpdate = window.top.location.href.startsWith(
-      browser.runtime.getURL('/'), // URL is unique per-install / hard to forge
-    );
-  } catch (err) {
-    // Non-background updates may throw a cross-origin error
-  }
 
-  if (isInIframe && !isBackgroundUpdate) {
+  if (isInIframe && !IS_BACKGROUND_UPDATE) {
     return;
   }
 
@@ -111,26 +135,25 @@ async function attemptExtraction(isBackgroundUpdate) {
   const url = new URL(document.location.href);
   const allowList = await config.get('extractionAllowlist');
   const allowAll = allowList.length === 1 && allowList[0] === '*';
-  if (!allowAll && !isBackgroundUpdate && !allowList.includes(url.host)) {
+  if (!allowAll && !IS_BACKGROUND_UPDATE && !allowList.includes(url.host)) {
     return;
   }
 
   // Record visit_supported_site event
-  if (!isBackgroundUpdate) {
+  if (!IS_BACKGROUND_UPDATE) {
     await recordEvent('visit_supported_site', 'supported_site');
   }
 
   // Extract immediately, and again if the readyState changes.
-  let extractedProduct = await attemptExtraction(isBackgroundUpdate);
+  let extractedProduct = await attemptExtraction();
   document.addEventListener('readystatechange', async () => {
-    extractedProduct = await attemptExtraction(isBackgroundUpdate);
+    extractedProduct = await attemptExtraction();
   });
 
   // Messy workaround for bug 1493470: Resend product info to the background
   // script twice in case subframe loads clear the toolbar icon.
-  // TODO(osmose): Remove once Firefox 64 hits the release channel, including second argument
-  // to sendProductToBackground.
-  const resend = () => sendProductToBackground(extractedProduct, false);
+  // TODO(osmose): Remove once Firefox 64 hits the release channel
+  const resend = () => sendProductToBackground(extractedProduct);
   setTimeout(resend, 5000);
   setTimeout(resend, 10000);
 

src/state/prices.js

@@ -229,13 +229,9 @@ export function addPriceFromExtracted(data) {
       // Potentially trigger an alert since there's a new price in town.
       dispatch(triggerPriceAlert(price));
 
-      // If this isn't the first price for this product, record detect_price_change event
-      const prices = getPricesForProduct(state, price.productId);
-      if (prices.length >= 1) {
-        return true;
-      }
+      return price;
     }
-    return false;
+    return null;
   };
 }
 

src/telemetry/extension.js

@@ -206,6 +206,7 @@ async function getActiveTabId() {
 }
 
 export async function getBadgeType() {
+  // browserAction and background scripts have to use activeTabId as a proxy for the tabId
   const activeTabId = await getActiveTabId();
   const badgeText = await browser.browserAction.getBadgeText(
     activeTabId ? {tabId: activeTabId} : {},
@@ -223,6 +224,19 @@ export async function getBadgeType() {
   }
 }
 
+export async function hasBadgeTextChanged(nextText, tabId = null) {
+  // If the nextText contains a digit, we know it's a price alert badge, which only affects
+  // the global badge text.
+  const returnGlobal = (/\d+/.test(nextText));
+  const prevText = await browser.browserAction.getBadgeText(
+    returnGlobal ? {} : {tabId: tabId || await getActiveTabId()},
+  );
+  if (prevText !== nextText) {
+    return true;
+  }
+  return false;
+}
+
 export async function handleWidgetRemoved(widgetId) {
   const addonId = (await browser.management.getSelf()).id;
   // widgetId replaces '@' and '.' in the addonId with _