fix(medic#8778): skip over invalid telemetry values

Author: garethbowen

webapp/src/ts/modules/reports/reports-add.component.ts

@@ -204,23 +204,21 @@ export class ReportsAddComponent implements OnInit, OnDestroy, AfterViewInit {
       const form = await this.formService.render(formContext);
       this.form = form;
       this.globalActions.setLoadingContent(false);
-      if (!model.doc || !model.doc._id) {
-        return;
+      if (model?.doc?._id) {
+        await this.ngZone.runOutsideAngular(() => this.renderAttachmentPreviews(model));
       }
-
-      await this.ngZone.runOutsideAngular(() => this.renderAttachmentPreviews(model));
-
-      this.telemetryData.postRender = Date.now();
-      this.telemetryData.action = model.doc ? 'edit' : 'add';
-      this.telemetryData.form = model.formInternalId;
-      this.telemetryService.record(
-        `enketo:reports:${this.telemetryData.form}:${this.telemetryData.action}:render`,
-        this.telemetryData.postRender - this.telemetryData.preRender
-      );
     } catch (err) {
       this.setError(err);
       console.error('Error loading form.', err);
     }
+
+    this.telemetryData.postRender = Date.now();
+    this.telemetryData.action = model.doc ? 'edit' : 'add';
+    this.telemetryData.form = model.formInternalId;
+    this.telemetryService.record(
+      `enketo:reports:${this.telemetryData.form}:${this.telemetryData.action}:render`,
+      this.telemetryData.postRender - this.telemetryData.preRender
+    );
   }
 
   private setError(err) {

webapp/src/ts/services/telemetry.service.ts

@@ -145,16 +145,35 @@ export class TelemetryService {
     };
   }
 
+  /**
+   * Emit the value of the doc.
+   * Skip over values that aren't numeric because they will cause an Error in the "_stats" reduce function.
+   * Exposed for unit testing.
+   * @param doc The db doc to map.
+   * @param emit A function called with the key and value to map the doc to.
+   */
+  _aggregateMap(doc, emit) {
+    const val = doc.value;
+    if (typeof val === 'number' && !Number.isNaN(val)) {
+      emit(doc.key, val);
+    }
+  }
+
+  private aggregateMapReduce(db) {
+    return db.query(
+      {
+        map: this._aggregateMap,
+        reduce: '_stats',
+      },
+      { group: true }
+    );
+  }
+
   private async aggregate(db, dbName) {
     const [ metadata, dbInfo, reduceResult ] = await Promise.all([
       this.generateMetadataSection(dbName),
-      this.dbService
-        .get()
-        .info(),
-      db.query(
-        { reduce: '_stats', map: (doc, emit) => emit(doc.key, doc.value) },
-        { group: true },
-      )
+      this.dbService.get().info(),
+      this.aggregateMapReduce(db),
     ]);
 
     const aggregateDoc = {
@@ -287,6 +306,10 @@ export class TelemetryService {
     if (value === undefined) {
       value = 1;
     }
+    if (typeof value !== 'number' || Number.isNaN(value)) {
+      console.error(new Error(`Invalid telemetry value "${value}" for key "${key}"`));
+      return;
+    }
 
     try {
       const today = this.getToday();

webapp/tests/karma/ts/modules/reports/reports-add.component.spec.ts

@@ -34,6 +34,7 @@ describe('Reports Add Component', () => {
   let geoHandle;
   let formService;
   let router;
+  let telemetryService;
   let route;
 
   beforeEach(waitForAsync(() => {
@@ -54,6 +55,7 @@ describe('Reports Add Component', () => {
       params: new Subject(),
     };
     router = { navigate: sinon.stub() };
+    telemetryService = { record: sinon.stub() };
 
     const mockedSelectors = [
       { selector: Selectors.getLoadingContent, value: false },
@@ -85,7 +87,7 @@ describe('Reports Add Component', () => {
           { provide: FormService, useValue: formService },
           { provide: ActivatedRoute, useValue: route },
           { provide: Router, useValue: router },
-          { provide: TelemetryService, useValue: { record: sinon.stub() }},
+          { provide: TelemetryService, useValue: telemetryService },
         ],
       })
       .compileComponents()
@@ -201,6 +203,9 @@ describe('Reports Add Component', () => {
         component.ngAfterViewInit();
         tick();
 
+        expect(telemetryService.record.callCount).to.equal(1);
+        expect(telemetryService.record.args[0][0]).to.equal('enketo:reports:my_form:add:render');
+        expect(telemetryService.record.args[0][1]).to.not.be.undefined;
         expect(geolocationService.init.calledOnce).to.be.true;
         expect(openReportContentStub.calledOnce).to.be.true;
         expect(openReportContentStub.args[0]).to.deep.equal([{ formInternalId: 'my_form' }]);

webapp/tests/karma/ts/services/telemetry.service.spec.ts

@@ -167,6 +167,12 @@ describe('TelemetryService', () => {
       expect(telemetryDb.destroy.calledOnce).to.be.true;
     });
 
+    it('should log an error and abort if value is non-numeric', async () => {
+      await service.record('test', 'bad-value');
+      expect(consoleErrorSpy.called).to.be.true;
+      expect(telemetryDb.post.called).to.be.false;
+    });
+
     const setupDbMocks = () => {
       telemetryDb.query.resolves({
         rows: [
@@ -423,4 +429,53 @@ describe('TelemetryService', () => {
       expect(telemetryDb.close.notCalled).to.be.true;
     });
   });
+
+  describe('aggregateMap', () => {
+
+    describe('should emit numerical value: ', () => {
+
+      for (const val of [
+        45,
+        4.5,
+        -20,
+        0
+      ]) {
+        it(JSON.stringify(val), () => {
+          const doc = {
+            key: 'mykey',
+            value: val
+          };
+          const emit = sinon.stub();
+          service._aggregateMap(doc, emit);
+          expect(emit.callCount).to.equal(1);
+          expect(emit.args[0][0]).to.equal('mykey');
+          expect(emit.args[0][1]).to.equal(val);
+        });
+      }
+
+    });
+
+    describe('should NOT emit non-numerical value: ', () => {
+
+      for (const val of [
+        'astring',
+        undefined,
+        null,
+        { an: 'object' },
+        Number.NaN
+      ]) {
+        it('' + val, () => {
+          const doc = {
+            key: 'mykey',
+            value: val
+          };
+          const emit = sinon.stub();
+          service._aggregateMap(doc, emit);
+          expect(emit.called).to.be.false;
+        });
+      }
+
+    });
+
+  });
 });