refactor(GUI): increase encapsulation of flash results structure (#608)

Currently, the client application knows too much about how the flash
results are stored in the internal state, and relies on its structure to
perform its logic.

This PR introduces several getters to `FlashStateModel` and makes
`FlashStateModel.getFlashResults()` private, ensuring clients don't
depend on the flash results object.

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

lib/gui/models/flash-state.js

@@ -145,7 +145,7 @@ FlashState.service('FlashStateModel', function() {
   /**
    * @summary Get the flash results
    * @function
-   * @public
+   * @private
    *
    * @returns {Object} flash results
    *
@@ -170,6 +170,85 @@ FlashState.service('FlashStateModel', function() {
     return Store.getState().get('flashState').toJS();
   };
 
+  /**
+   * @summary Determine if the last flash was successful
+   * @function
+   * @public
+   *
+   * @description
+   * This function returns true if the flash was cancelled, given
+   * a cancellation is a user request, and doesn't represent an
+   * actual flash error.
+   *
+   * This function returns true if there was no last flash.
+   *
+   * @returns {Boolean} whether the last flash was successful
+   *
+   * @example
+   * if (FlashStateModel.wasLastFlashSuccessful()) {
+   *   console.log('The last flash was successful');
+   * }
+   */
+  this.wasLastFlashSuccessful = () => {
+    return _.some([
+      this.wasLastFlashCancelled(),
+      _.get(this.getFlashResults(), 'passedValidation', true)
+    ]);
+  };
+
+  /**
+   * @summary Determine if the last flash was cancelled
+   * @function
+   * @public
+   *
+   * @description
+   * This function returns false if there was no last flash.
+   *
+   * @returns {Boolean} whether the last flash was cancelled
+   *
+   * @example
+   * if (FlashStateModel.wasLastFlashCancelled()) {
+   *   console.log('The last flash was cancelled');
+   * }
+   */
+  this.wasLastFlashCancelled = () => {
+    return _.get(this.getFlashResults(), 'cancelled', false);
+  };
+
+  /**
+   * @summary Get last flash source checksum
+   * @function
+   * @public
+   *
+   * @description
+   * This function returns undefined if there was no last flash.
+   *
+   * @returns {(String|Undefined)} the last flash source checksum
+   *
+   * @example
+   * const checksum = FlashStateModel.getLastFlashSourceChecksum();
+   */
+  this.getLastFlashSourceChecksum = () => {
+    return this.getFlashResults().sourceChecksum;
+  };
+
+  /**
+   * @summary Get last flash error code
+   * @function
+   * @public
+   *
+   * @description
+   * This function returns undefined if there was no last flash.
+   *
+   * @returns {(String|Undefined)} the last flash error code
+   *
+   * @example
+   * const errorCode = FlashStateModel.getLastFlashErrorCode();
+   */
+  this.getLastFlashErrorCode = () => {
+    return this.getFlashResults().errorCode;
+  };
+
 });
 
 module.exports = MODULE_NAME;

lib/gui/models/store.js

@@ -170,18 +170,15 @@ const storeReducer = (state, action) => {
         throw new Error('Missing results');
       }
 
-      if (_.isUndefined(action.data.passedValidation) || _.isNull(action.data.passedValidation)) {
-        throw new Error('Missing results passedValidation');
-      }
+      _.defaults(action.data, {
+        passedValidation: false,
+        cancelled: false
+      });
 
       if (!_.isBoolean(action.data.passedValidation)) {
         throw new Error(`Invalid results passedValidation: ${action.data.passedValidation}`);
       }
 
-      if (_.isUndefined(action.data.cancelled) || _.isNull(action.data.cancelled)) {
-        throw new Error('Missing results cancelled');
-      }
-
       if (!_.isBoolean(action.data.cancelled)) {
         throw new Error(`Invalid results cancelled: ${action.data.cancelled}`);
       }
@@ -202,6 +199,10 @@ const storeReducer = (state, action) => {
         throw new Error(`Invalid results sourceChecksum: ${action.data.sourceChecksum}`);
       }
 
+      if (action.data.passedValidation && action.data.errorCode) {
+        throw new Error('The errorCode value can\'t be set if the flashing passed validation');
+      }
+
       if (action.data.errorCode && !_.isString(action.data.errorCode)) {
         throw new Error(`Invalid results errorCode: ${action.data.errorCode}`);
       }

lib/gui/modules/image-writer.js

@@ -21,7 +21,6 @@
  */
 
 const angular = require('angular');
-const _ = require('lodash');
 const childWriter = require('../../src/child-writer');
 
 const MODULE_NAME = 'Etcher.Modules.ImageWriter';
@@ -102,20 +101,8 @@ imageWriter.service('ImageWriterService', function($q, $rootScope, SettingsModel
         FlashStateModel.setProgressState(state);
       });
 
-    }).then((results) => {
-
-      // TODO: Make sure `etcher-image-write` always
-      // sends a `cancelled` and `passedValidation` property.
-      _.defaults(results, {
-        cancelled: false,
-        passedValidation: false
-      });
-
-      FlashStateModel.unsetFlashingFlag(results);
-    }).catch((error) => {
+    }).then(FlashStateModel.unsetFlashingFlag).catch((error) => {
       FlashStateModel.unsetFlashingFlag({
-        cancelled: false,
-        passedValidation: false,
         errorCode: error.code
       });
 

lib/gui/pages/finish/controllers/finish.js

@@ -30,7 +30,7 @@ module.exports = function($state, FlashStateModel, SelectionStateModel, Analytic
    * @type String
    * @public
    */
-  this.checksum = FlashStateModel.getFlashResults().sourceChecksum;
+  this.checksum = FlashStateModel.getLastFlashSourceChecksum();
 
   /**
    * @summary Restart the flashing process

lib/gui/pages/main/controllers/main.js

@@ -168,16 +168,6 @@ module.exports = function(
     AnalyticsService.logEvent('Restart after failure');
   };
 
-  this.wasLastFlashSuccessful = () => {
-    const flashResults = FlashStateModel.getFlashResults();
-
-    if (_.get(flashResults, 'cancelled', false)) {
-      return true;
-    }
-
-    return _.get(flashResults, 'passedValidation', true);
-  };
-
   this.flash = (image, drive) => {
 
     if (FlashStateModel.isFlashing()) {
@@ -194,13 +184,11 @@ module.exports = function(
     });
 
     return ImageWriterService.flash(image, drive).then(() => {
-      const results = FlashStateModel.getFlashResults();
-
-      if (results.cancelled) {
+      if (FlashStateModel.wasLastFlashCancelled()) {
         return;
       }
 
-      if (results.passedValidation) {
+      if (FlashStateModel.wasLastFlashSuccessful()) {
         OSNotificationService.send('Success!', 'Your flash is complete');
         AnalyticsService.logEvent('Done');
         $state.go('success');

lib/gui/pages/main/templates/main.tpl.html

@@ -87,7 +87,7 @@
           percentage="main.state.getFlashState().percentage"
           striped="{{ main.state.getFlashState().type == 'check' }}"
           ng-attr-active="{{ main.state.isFlashing() }}"
-          ng-show="main.wasLastFlashSuccessful()"
+          ng-show="main.state.wasLastFlashSuccessful()"
           ng-click="main.flash(main.selection.getImagePath(), main.selection.getDrive())"
           ng-disabled="!main.selection.hasImage() || !main.selection.hasDrive()">
             <span ng-show="main.state.getFlashState().percentage == 100 && main.state.isFlashing()">Finishing...</span>
@@ -99,20 +99,20 @@
               ng-bind="main.state.getFlashState().percentage + '% Validating...'"></span>
         </progress-button>
 
-        <div class="alert-ribbon alert-warning" ng-class="{ 'alert-ribbon--open': !main.wasLastFlashSuccessful() }">
+        <div class="alert-ribbon alert-warning" ng-class="{ 'alert-ribbon--open': !main.state.wasLastFlashSuccessful() }">
           <span class="glyphicon glyphicon-warning-sign"></span>
-          <span ng-show="main.state.getFlashResults().errorCode === 'ENOSPC'">
+          <span ng-show="main.state.getLastFlashErrorCode() === 'ENOSPC'">
             Not enough space on the drive.<br>Please insert larger one and <button class="btn btn-link" ng-click="main.restartAfterFailure()">try again</button>
           </span>
-          <span ng-show="main.state.getFlashResults().errorCode !== 'ENOSPC' && main.settings.get('validateWriteOnSuccess')">
+          <span ng-show="main.state.getLastFlashErrorCode() !== 'ENOSPC' && main.settings.get('validateWriteOnSuccess')">
             Your removable drive may be corrupted.<br>Try inserting a different one and <button class="btn btn-link" ng-click="main.restartAfterFailure()">press "retry"</button>
           </span>
-          <span ng-show="main.state.getFlashResults().errorCode !== 'ENOSPC' && !main.settings.get('validateWriteOnSuccess')">
+          <span ng-show="main.state.getLastFlashErrorCode() !== 'ENOSPC' && !main.settings.get('validateWriteOnSuccess')">
             Oops, seems something went wrong. Click <button class="btn btn-link" ng-click="main.restartAfterFailure()">here</button> to retry
           </span>
         </div>
 
-        <button class="btn btn-warning btn-brick" ng-hide="main.wasLastFlashSuccessful()" ng-click="main.restartAfterFailure()">
+        <button class="btn btn-warning btn-brick" ng-hide="main.state.wasLastFlashSuccessful()" ng-click="main.restartAfterFailure()">
           <span class="glyphicon glyphicon-repeat"></span> Retry
         </button>
 

tests/gui/models/flash-state.spec.js

@@ -256,7 +256,7 @@ describe('Browser: FlashStateModel', function() {
       it('should throw if errorCode is defined but it is not a number', function() {
         m.chai.expect(function() {
           FlashStateModel.unsetFlashingFlag({
-            passedValidation: true,
+            passedValidation: false,
             cancelled: false,
             sourceChecksum: '1234',
             errorCode: 123
@@ -264,13 +264,19 @@ describe('Browser: FlashStateModel', function() {
         }).to.throw('Invalid results errorCode: 123');
       });
 
-      it('should throw if no passedValidation', function() {
-        m.chai.expect(function() {
-          FlashStateModel.unsetFlashingFlag({
-            cancelled: false,
-            sourceChecksum: '1234'
-          });
-        }).to.throw('Missing results passedValidation');
+      it('should default passedValidation to false', function() {
+        FlashStateModel.unsetFlashingFlag({
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
+
+        const flashResults = FlashStateModel.getFlashResults();
+
+        m.chai.expect(flashResults).to.deep.equal({
+          passedValidation: false,
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
       });
 
       it('should throw if passedValidation is not boolean', function() {
@@ -283,13 +289,19 @@ describe('Browser: FlashStateModel', function() {
         }).to.throw('Invalid results passedValidation: true');
       });
 
-      it('should throw if no cancelled', function() {
-        m.chai.expect(function() {
-          FlashStateModel.unsetFlashingFlag({
-            passedValidation: true,
-            sourceChecksum: '1234'
-          });
-        }).to.throw('Missing results cancelled');
+      it('should default cancelled to false', function() {
+        FlashStateModel.unsetFlashingFlag({
+          passedValidation: true,
+          sourceChecksum: '1234'
+        });
+
+        const flashResults = FlashStateModel.getFlashResults();
+
+        m.chai.expect(flashResults).to.deep.equal({
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
       });
 
       it('should throw if cancelled is not boolean', function() {
@@ -331,6 +343,17 @@ describe('Browser: FlashStateModel', function() {
         }).to.throw('The sourceChecksum value can\'t exist if the flashing was cancelled');
       });
 
+      it('should throw if passedValidation is true and errorCode is set', function() {
+        m.chai.expect(function() {
+          FlashStateModel.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: false,
+            sourceChecksum: '1234',
+            errorCode: 'ENOSPC'
+          });
+        }).to.throw('The errorCode value can\'t be set if the flashing passed validation');
+      });
+
       it('should throw if cancelled is true and passedValidation is true', function() {
         m.chai.expect(function() {
           FlashStateModel.unsetFlashingFlag({
@@ -402,6 +425,140 @@ describe('Browser: FlashStateModel', function() {
 
     });
 
+    describe('.wasLastFlashSuccessful()', function() {
+
+      it('should return true given a pristine state', function() {
+        FlashStateModel.resetState();
+        m.chai.expect(FlashStateModel.wasLastFlashSuccessful()).to.be.true;
+      });
+
+      it('should return false if !cancelled && !passedValidation', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: false
+        });
+
+        m.chai.expect(FlashStateModel.wasLastFlashSuccessful()).to.be.false;
+      });
+
+      it('should return true if !cancelled && passedValidation', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: true
+        });
+
+        m.chai.expect(FlashStateModel.wasLastFlashSuccessful()).to.be.true;
+      });
+
+      it('should return true if cancelled && !passedValidation', function() {
+        FlashStateModel.unsetFlashingFlag({
+          cancelled: true,
+          passedValidation: false
+        });
+
+        m.chai.expect(FlashStateModel.wasLastFlashSuccessful()).to.be.true;
+      });
+
+    });
+
+    describe('.wasLastFlashCancelled()', function() {
+
+      it('should return false given a pristine state', function() {
+        FlashStateModel.resetState();
+        m.chai.expect(FlashStateModel.wasLastFlashCancelled()).to.be.false;
+      });
+
+      it('should return false if !cancelled && !passedValidation', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: false
+        });
+
+        m.chai.expect(FlashStateModel.wasLastFlashCancelled()).to.be.false;
+      });
+
+      it('should return false if !cancelled && passedValidation', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: true
+        });
+
+        m.chai.expect(FlashStateModel.wasLastFlashCancelled()).to.be.false;
+      });
+
+      it('should return true if cancelled && !passedValidation', function() {
+        FlashStateModel.unsetFlashingFlag({
+          cancelled: true,
+          passedValidation: false
+        });
+
+        m.chai.expect(FlashStateModel.wasLastFlashCancelled()).to.be.true;
+      });
+
+    });
+
+    describe('.getLastFlashSourceChecksum()', function() {
+
+      it('should return undefined given a pristine state', function() {
+        FlashStateModel.resetState();
+        m.chai.expect(FlashStateModel.getLastFlashSourceChecksum()).to.be.undefined;
+      });
+
+      it('should return the last flash source checksum', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: true
+        });
+
+        m.chai.expect(FlashStateModel.getLastFlashSourceChecksum()).to.equal('1234');
+      });
+
+      it('should return undefined if the last flash was cancelled', function() {
+        FlashStateModel.unsetFlashingFlag({
+          cancelled: true,
+          passedValidation: false
+        });
+
+        m.chai.expect(FlashStateModel.getLastFlashSourceChecksum()).to.be.undefined;
+      });
+
+    });
+
+    describe('.getLastFlashErrorCode()', function() {
+
+      it('should return undefined given a pristine state', function() {
+        FlashStateModel.resetState();
+        m.chai.expect(FlashStateModel.getLastFlashErrorCode()).to.be.undefined;
+      });
+
+      it('should return the last flash error code', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: false,
+          errorCode: 'ENOSPC'
+        });
+
+        m.chai.expect(FlashStateModel.getLastFlashErrorCode()).to.equal('ENOSPC');
+      });
+
+      it('should return undefined if the last flash did not report an error code', function() {
+        FlashStateModel.unsetFlashingFlag({
+          sourceChecksum: '1234',
+          cancelled: false,
+          passedValidation: true
+        });
+
+        m.chai.expect(FlashStateModel.getLastFlashErrorCode()).to.be.undefined;
+      });
+
+    });
+
   });
 
 });