From 98284bd5e27dd9ee629b3367873dd5660a8d9703 Mon Sep 17 00:00:00 2001
From: Juan Cruz Viotti <jviotti@openmailbox.org>
Date: Thu, 2 Mar 2017 12:21:48 -0400
Subject: [PATCH] fix(GUI): don't report "invalid archive" errors to TrackJS
 (#1152)

These errors happen when the user selects an archive that contains no
image, or multiple images, and represents a user error, rather than an
unhandled exception.

For this reason, we should not report this to TrackJS.

This means, however, that we need a reliable way to know whether an
error should be reporter or not. This commit implements
`AnalyticsService.shouldReportError()` for this purpose, which will
check an optional `report` property of the error object.

In order to ensure that errors that we don't control are reported, the
function will return `true` if the error object doesn't have that
property.

Change-Type: patch
Changelog-Entry: Don't report "invalid archive" errors to TrackJS.
Fixes: https://github.com/resin-io/etcher/issues/1103
Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>
---
 lib/gui/modules/analytics.js        | 29 +++++++++++-
 lib/image-stream/archive.js         |  1 +
 tests/gui/modules/analytics.spec.js | 73 +++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 1 deletion(-)
 create mode 100644 tests/gui/modules/analytics.spec.js

diff --git a/lib/gui/modules/analytics.js b/lib/gui/modules/analytics.js
index 428933ff..b84eec30 100644
--- a/lib/gui/modules/analytics.js
+++ b/lib/gui/modules/analytics.js
@@ -136,6 +136,29 @@ analytics.service('AnalyticsService', function($log, $window, $mixpanel, Setting
     this.logDebug(message);
   };
 
+  /**
+   * @summary Check whether an error should be reported to TrackJS
+   * @function
+   * @private
+   *
+   * @description
+   * In order to determine whether the error should be reported, we
+   * check a property called `report`. For backwards compatibility, and
+   * to properly handle errors that we don't control, an error without
+   * this property is reported automatically.
+   *
+   * @param {Error} error - error
+   * @returns {Boolean} whether the error should be reported
+   *
+   * @example
+   * if (AnalyticsService.shouldReportError(new Error('foo'))) {
+   *   console.log('We should report this error');
+   * }
+   */
+  this.shouldReportError = (error) => {
+    return !_.has(error, 'report') || Boolean(error.report);
+  };
+
   /**
    * @summary Log an exception
    * @function
@@ -150,7 +173,11 @@ analytics.service('AnalyticsService', function($log, $window, $mixpanel, Setting
    * AnalyticsService.logException(new Error('Something happened'));
    */
   this.logException = (exception) => {
-    if (SettingsModel.get('errorReporting') && isRunningInAsar()) {
+    if (_.every([
+      SettingsModel.get('errorReporting'),
+      isRunningInAsar(),
+      this.shouldReportError(exception)
+    ])) {
       $window.trackJs.track(exception);
     }
 
diff --git a/lib/image-stream/archive.js b/lib/image-stream/archive.js
index 0e18fe20..a09bb431 100644
--- a/lib/image-stream/archive.js
+++ b/lib/image-stream/archive.js
@@ -177,6 +177,7 @@ exports.extractImage = (archive, hooks) => {
     if (imageEntries.length !== 1) {
       const error = new Error('Invalid archive image');
       error.description = 'The archive image should contain one and only one top image file.';
+      error.report = false;
       throw error;
     }
 
diff --git a/tests/gui/modules/analytics.spec.js b/tests/gui/modules/analytics.spec.js
new file mode 100644
index 00000000..f00267d0
--- /dev/null
+++ b/tests/gui/modules/analytics.spec.js
@@ -0,0 +1,73 @@
+'use strict';
+
+const m = require('mochainon');
+const angular = require('angular');
+require('angular-mocks');
+
+describe('Browser: Analytics', function() {
+
+  beforeEach(angular.mock.module(
+    require('../../../lib/gui/modules/analytics')
+  ));
+
+  describe('AnalyticsService', function() {
+
+    let AnalyticsService;
+
+    beforeEach(angular.mock.inject(function(_AnalyticsService_) {
+      AnalyticsService = _AnalyticsService_;
+    }));
+
+    describe('.shouldReportError()', function() {
+
+      it('should return true for a basic error', function() {
+        const error = new Error('foo');
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.true;
+      });
+
+      it('should return true for an error with a report true property', function() {
+        const error = new Error('foo');
+        error.report = true;
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.true;
+      });
+
+      it('should return false for an error with a report false property', function() {
+        const error = new Error('foo');
+        error.report = false;
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.false;
+      });
+
+      it('should return false for an error with a report undefined property', function() {
+        const error = new Error('foo');
+        error.report = undefined;
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.false;
+      });
+
+      it('should return false for an error with a report null property', function() {
+        const error = new Error('foo');
+        error.report = null;
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.false;
+      });
+
+      it('should return false for an error with a report 0 property', function() {
+        const error = new Error('foo');
+        error.report = 0;
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.false;
+      });
+
+      it('should return true for an error with a report 1 property', function() {
+        const error = new Error('foo');
+        error.report = 1;
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.true;
+      });
+
+      it('should cast the report property to boolean', function() {
+        const error = new Error('foo');
+        error.report = '';
+        m.chai.expect(AnalyticsService.shouldReportError(error)).to.be.false;
+      });
+
+    });
+
+  });
+});