From eb2fd6b1f5dfb138bd6ee79b30036e35c4797742 Mon Sep 17 00:00:00 2001
From: Shane Tomlinson <stomlinson@mozilla.com>
Date: Tue, 10 Jan 2017 11:51:33 +0000
Subject: [PATCH] fix(metrics): POST metrics if *any* field has changed since
 the last send.

This fixes a problem where clicks on the app store marketing buttons were not
recorded if metrics were already flushed but no new events were logged.

The only two fields that do *not* cause metrics to be flushed are
`duration` and `flushTime` since these will be different on every send.

fixes #4479
---
 app/scripts/lib/metrics.js    | 35 +++++++++++++++++++++----
 app/tests/spec/lib/metrics.js | 48 +++++++++++++++++++++++------------
 2 files changed, 62 insertions(+), 21 deletions(-)

diff --git a/app/scripts/lib/metrics.js b/app/scripts/lib/metrics.js
index cad9b324a5..47611c392c 100644
--- a/app/scripts/lib/metrics.js
+++ b/app/scripts/lib/metrics.js
@@ -167,10 +167,12 @@ define(function (require, exports, module) {
 
       var filteredData = this.getFilteredData();
 
-      if (! this._isFlushRequired(filteredData)) {
+      if (! this._isFlushRequired(filteredData, this._lastFlushedData)) {
         return p();
       }
 
+      this._lastFlushedData = filteredData;
+
       this._speedTrap.events.clear();
       this._speedTrap.timers.clear();
 
@@ -185,9 +187,30 @@ define(function (require, exports, module) {
         .then(sent => sent || send());
     },
 
-    _isFlushRequired (data) {
-      return data.events.length !== 0 ||
-        Object.keys(data.timers).length !== 0;
+    /**
+     * Check if a flush is required for the given `data`. A flush is
+     * required if any data has changed since the last flush.
+     *
+     * @param {Object} data - potential data to flush
+     * @param {Object} lastFlushedData - last data that was flushed.
+     * @returns {Boolean}
+     * @private
+     */
+    _isFlushRequired (data, lastFlushedData) {
+      if (! lastFlushedData) {
+        return true;
+      }
+      // Only check fields that are in the new payload. `data` could be
+      // a subset of `_lastFlushedData`, in which case no flush should occur.
+      return _.any(data, (value, key) => {
+        // these keys are distinct every flush attempt, ignore.
+        if (key === 'duration' || key === 'flushTime') {
+          return false;
+        }
+
+        // _.isEqual does a deep comparision of objects and arrays.
+        return ! _.isEqual(lastFlushedData[key], value);
+      });
     },
 
     _clearInactivityFlushTimeout () {
@@ -245,7 +268,9 @@ define(function (require, exports, module) {
         utm_term: this._utmTerm, //eslint-disable-line camelcase
       });
 
-      return allData;
+      // Create a deep copy of the data so that any modifications to contained
+      // objects or arrays do not affect the returned copy of the data.
+      return JSON.parse(JSON.stringify(allData));
     },
 
     /**
diff --git a/app/tests/spec/lib/metrics.js b/app/tests/spec/lib/metrics.js
index ebd1c3b59d..34a2e721e0 100644
--- a/app/tests/spec/lib/metrics.js
+++ b/app/tests/spec/lib/metrics.js
@@ -19,6 +19,9 @@ define(function (require, exports, module) {
   const TestHelpers = require('../../lib/helpers');
   const WindowMock = require('../../mocks/window');
 
+  const MARKETING_CAMPAIGN = 'campaign1';
+  const MARKETING_CAMPAIGN_URL = 'https://accounts.firefox.com';
+
   describe('lib/metrics', function () {
     var metrics;
     var windowMock;
@@ -468,17 +471,33 @@ define(function (require, exports, module) {
         });
       });
 
-      describe('flush, no events or timers', function () {
+      describe('flush, no data has changed, flush again', () => {
         beforeEach(function () {
-          sandbox.stub(environment, 'hasSendBeacon', function () {
-            return true;
-          });
-          sandbox.stub(windowMock.navigator, 'sendBeacon', function () {});
-          return metrics.flush();
+          sandbox.stub(environment, 'hasSendBeacon', () => true);
+          sandbox.stub(windowMock.navigator, 'sendBeacon', () => true);
+          return metrics.flush()
+            .then(() => metrics.flush());
+        });
+
+        it('sends data once', function () {
+          assert.equal(windowMock.navigator.sendBeacon.callCount, 1);
+        });
+      });
+
+      describe('flush, data has changed, flush again', () => {
+        beforeEach(function () {
+          sandbox.stub(environment, 'hasSendBeacon', () => true);
+          sandbox.stub(windowMock.navigator, 'sendBeacon', () => true);
+          metrics.logMarketingImpression(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL);
+          return metrics.flush()
+            .then(() => {
+              metrics.logMarketingClick(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL);
+              return metrics.flush();
+            });
         });
 
-        it('does not send data', function () {
-          assert.equal(windowMock.navigator.sendBeacon.callCount, 0);
+        it('sends data both times', function () {
+          assert.equal(windowMock.navigator.sendBeacon.callCount, 2);
         });
       });
 
@@ -599,14 +618,11 @@ define(function (require, exports, module) {
 
     describe('logMarketingImpression & logMarketingClick', function () {
       it('logs the marketing impression and click', function () {
-        var campaign = 'campaign1';
-        var url = 'https://accounts.firefox.com';
-
-        assert.isUndefined(metrics.getMarketingImpression(campaign, url));
-        metrics.logMarketingImpression(campaign, url);
-        assert.isFalse(metrics.getMarketingImpression(campaign, url).clicked);
-        metrics.logMarketingClick(campaign, url);
-        assert.isTrue(metrics.getMarketingImpression(campaign, url).clicked);
+        assert.isUndefined(metrics.getMarketingImpression(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL));
+        metrics.logMarketingImpression(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL);
+        assert.isFalse(metrics.getMarketingImpression(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL).clicked);
+        metrics.logMarketingClick(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL);
+        assert.isTrue(metrics.getMarketingImpression(MARKETING_CAMPAIGN, MARKETING_CAMPAIGN_URL).clicked);
       });
     });