Merge pull request #3634 from mozilla/tag-metrics-with-correct-service-in-sync-optional-flow;=shane-tomlinson

Set `service` tag for metrics events based on fxa_status response.

packages/fxa-content-server/app/scripts/lib/metrics.js

@@ -584,6 +584,17 @@ _.extend(Metrics.prototype, Backbone.Events, {
     return id;
   },
 
+  /**
+   * Set the `service` parameter to use for all future metrics.
+   * This is useful in cases where don't learn the appropriate
+   * service value until after the app has been initialized.
+   *
+   * @param {String} [service] The service identifier
+   */
+  setService(service) {
+    this._service = service || NOT_REPORTED_VALUE;
+  },
+
   /**
    * Set the view name prefix for metrics that contain a viewName.
    * This is used to differentiate between flows when the same

packages/fxa-content-server/app/scripts/models/auth_brokers/fx-sync.js

@@ -67,6 +67,7 @@ export default BaseAuthenticationBroker.extend({
         // the service in the query parameter currently overrides the status message
         // this is due to backwards compatibility
         this.relier.set('service', response.clientId);
+        this._metrics.setService(response.clientId);
       }
     }
     const additionalEngineIds =

packages/fxa-content-server/app/tests/spec/lib/metrics.js

@@ -704,6 +704,15 @@ describe('lib/metrics', () => {
     });
   });
 
+  describe('setService', function() {
+    it('sets the service identifier', function() {
+      metrics.setService('00112233445566');
+      const { service } = metrics.getFilteredData();
+
+      assert.equal(service, '00112233445566');
+    });
+  });
+
   describe('isCollectionEnabled', () => {
     it('reports that collection is enabled if `isSampledUser===true`', () => {
       assert.isTrue(metrics.isCollectionEnabled());

packages/fxa-content-server/app/tests/spec/models/auth_brokers/fx-sync.js

@@ -84,33 +84,59 @@ describe('models/auth_brokers/fx-sync', () => {
     });
   });
 
-  describe('syncOptional capability', () => {
-    it('sets `syncOptional` to false if no multiService capability', () => {
-      relier.set('service', 'foo');
-      broker.onFxaStatus({ capabilities: {} });
+  describe('fxa_status response handling', () => {
+    describe('syncOptional capability', () => {
+      it('sets `syncOptional` to false if no multiService capability', () => {
+        relier.set('service', 'foo');
+        broker.onFxaStatus({ capabilities: {} });
 
-      assert.isFalse(broker.hasCapability('syncOptional'));
+        assert.isFalse(broker.hasCapability('syncOptional'));
+      });
+
+      it('sets `syncOptional` to false if multiService: false', () => {
+        relier.set('service', 'foo');
+        broker.onFxaStatus({ capabilities: { multiService: false } });
+
+        assert.isFalse(broker.hasCapability('syncOptional'));
+      });
+
+      it('sets `syncOptional` to false if service===sync', () => {
+        relier.set('service', 'sync');
+        broker.onFxaStatus({ capabilities: { multiService: true } });
+
+        assert.isFalse(broker.hasCapability('syncOptional'));
+      });
+
+      it('sets `syncOptional` to true if multiService and not sync', () => {
+        relier.set('service', 'foo');
+        broker.onFxaStatus({ capabilities: { multiService: true } });
+
+        assert.isTrue(broker.hasCapability('syncOptional'));
+      });
     });
 
-    it('sets `syncOptional` to false if multiService: false', () => {
-      relier.set('service', 'foo');
-      broker.onFxaStatus({ capabilities: { multiService: false } });
+    describe('clientId', () => {
+      it('sets `service` based on `clientId` when not otherwise known', () => {
+        broker.onFxaStatus({
+          capabilities: { multiService: true },
+          clientId: 'fx-desktop',
+        });
 
-      assert.isFalse(broker.hasCapability('syncOptional'));
-    });
+        assert.equal(relier.get('service'), 'fx-desktop');
+        assert.equal(metrics.getFilteredData().service, 'fx-desktop');
+      });
 
-    it('sets `syncOptional` to false if service===sync', () => {
-      relier.set('service', 'sync');
-      broker.onFxaStatus({ capabilities: { multiService: true } });
+      it('does not override an explicit `service` value obtained from query parameters', () => {
+        relier.set('service', 'sync');
+        metrics.setService('sync');
+        broker.onFxaStatus({
+          capabilities: { multiService: true },
+          clientId: 'fx-desktop',
+        });
 
-      assert.isFalse(broker.hasCapability('syncOptional'));
-    });
-
-    it('sets `syncOptional` to true if multiService and not sync', () => {
-      relier.set('service', 'foo');
-      broker.onFxaStatus({ capabilities: { multiService: true } });
-
-      assert.isTrue(broker.hasCapability('syncOptional'));
+        assert.equal(relier.get('service'), 'sync');
+        assert.equal(metrics.getFilteredData().service, 'sync');
+      });
     });
   });