refactor: move flash results state to Redux store (#529)

This is a rather big PR that moves the flash results information to the
Redux store, which simplifies and improves a lot of things as throughly
described in the commits that introduced Redux.

Here's a summary of the changes:

- Add a `flashResults` property to the store.

- Validate the contents of `flashResults`, handling certain edge cases
that make the modal incoherent.

- Split `ImageWriterService.setFlashing()` to
`ImageWriterService.setFlashingFlag()` and
`ImageWriterService.unsetFlashingFlag()`.

- Require the flash results to be passed to
`ImageWriterService.unsetFlashingFlag()`.

- Stop resolving the flash results from `ImageWriterService.flash()`.

- Implement `ImageWriterService.getFlashResults()`.

- Make the `RESET_FLASH_STATE` action reset the flash results.

- Access the source checksum from the store in the "finish" page,
  instead of requiring the controller to pass it as a state parameter.

- Implement `.wasLastFlashSuccessful()` function in the main controller
to replace the `.success` property.

- Completely remove the `.success` property in the main controller.

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

lib/gui/app.js

@@ -148,7 +148,6 @@ app.controller('AppController', function(
   this.writer = ImageWriterService;
   this.settings = SettingsModel.data;
   this.tooltipModal = TooltipModalService;
-  this.success = true;
 
   this.handleError = (error) => {
     OSDialogService.showError(error);
@@ -249,10 +248,14 @@ app.controller('AppController', function(
       preserveImage: true
     });
 
-    this.success = true;
+    this.writer.resetState();
     AnalyticsService.logEvent('Restart after failure');
   };
 
+  this.wasLastFlashSuccessful = () => {
+    return _.get(this.writer.getFlashResults(), 'passedValidation', true);
+  };
+
   this.flash = (image, drive) => {
 
     if (this.writer.isFlashing()) {
@@ -268,22 +271,17 @@ app.controller('AppController', function(
       device: drive.device
     });
 
-    return this.writer.flash(image, drive).then((results) => {
+    return this.writer.flash(image, drive).then(() => {
+      const results = ImageWriterService.getFlashResults();
 
       if (results.cancelled) {
         return;
       }
 
-      // TODO: Find a better way to manage flash/check
+      if (results.passedValidation) {
-      // success/error state than a global boolean flag.
-      this.success = results.passedValidation;
-
-      if (this.success) {
         OSNotificationService.send('Success!', 'Your flash is complete');
         AnalyticsService.logEvent('Done');
-        $state.go('success', {
+        $state.go('success');
-          checksum: results.sourceChecksum
-        });
       } else {
         OSNotificationService.send('Oops!', 'Looks like your flash has failed');
         AnalyticsService.logEvent('Validation error');

lib/gui/models/store.js

@@ -30,6 +30,7 @@ const DEFAULT_STATE = Immutable.fromJS({
   availableDrives: [],
   selection: {},
   isFlashing: false,
+  flashResults: {},
   flashState: {
     percentage: 0,
     speed: 0
@@ -139,17 +140,58 @@ const storeReducer = (state, action) => {
     }
 
     case ACTIONS.RESET_FLASH_STATE: {
-      return state.set('flashState', DEFAULT_STATE.get('flashState'));
+      return state
+        .set('flashState', DEFAULT_STATE.get('flashState'))
+        .set('flashResults', DEFAULT_STATE.get('flashResults'));
     }
 
     case ACTIONS.SET_FLASHING_FLAG: {
-      return state.set('isFlashing', true);
+      return state
+        .set('isFlashing', true)
+        .set('flashResults', DEFAULT_STATE.get('flashResults'));
     }
 
     case ACTIONS.UNSET_FLASHING_FLAG: {
-      return storeReducer(state.set('isFlashing', false), {
+      if (!action.data) {
-        type: ACTIONS.RESET_FLASH_STATE
+        throw new Error('Missing results');
-      });
+      }
+
+      if (_.isUndefined(action.data.passedValidation) || _.isNull(action.data.passedValidation)) {
+        throw new Error('Missing results passedValidation');
+      }
+
+      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}`);
+      }
+
+      if (action.data.cancelled && action.data.sourceChecksum) {
+        throw new Error('The sourceChecksum value can\'t exist if the flashing was cancelled');
+      }
+
+      if (action.data.cancelled && action.data.passedValidation) {
+        throw new Error('The passedValidation value can\'t be true if the flashing was cancelled');
+      }
+
+      if (action.data.passedValidation && !action.data.sourceChecksum) {
+        throw new Error('Missing results sourceChecksum');
+      }
+
+      if (action.data.passedValidation && !_.isString(action.data.sourceChecksum)) {
+        throw new Error(`Invalid results sourceChecksum: ${action.data.sourceChecksum}`);
+      }
+
+      return state
+        .set('isFlashing', false)
+        .set('flashResults', Immutable.fromJS(action.data))
+        .set('flashState', DEFAULT_STATE.get('flashState'));
     }
 
     case ACTIONS.SELECT_DRIVE: {

lib/gui/modules/image-writer.js

@@ -21,6 +21,7 @@
  */
 
 const angular = require('angular');
+const _ = require('lodash');
 const Store = require('../models/store');
 const childWriter = require('../../src/child-writer');
 
@@ -81,28 +82,49 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel)
   };
 
   /**
-   * @summary Set the flashing status
+   * @summary Set the flashing flag
    * @function
    * @private
    *
    * @description
    * This function is extracted for testing purposes.
    *
-   * @param {Boolean} status - flashing status
+   * The flag is used to signify that we're going to
+   * start a flash process.
    *
    * @example
-   * ImageWriterService.setFlashing(true);
+   * ImageWriterService.setFlashingFlag();
    */
-  this.setFlashing = (status) => {
+  this.setFlashingFlag = () => {
-    if (Boolean(status)) {
+    Store.dispatch({
-      Store.dispatch({
+      type: Store.Actions.SET_FLASHING_FLAG
-        type: Store.Actions.SET_FLASHING_FLAG
+    });
-      });
+  };
-    } else {
+
-      Store.dispatch({
+  /**
-        type: Store.Actions.UNSET_FLASHING_FLAG
+   * @summary Unset the flashing flag
-      });
+   * @function
-    }
+   * @private
+   *
+   * @description
+   * This function is extracted for testing purposes.
+   *
+   * The flag is used to signify that the write process ended.
+   *
+   * @param {Object} results - flash results
+   *
+   * @example
+   * ImageWriterService.unsetFlashingFlag({
+   *   passedValidation: true,
+   *   cancelled: false,
+   *   sourceChecksum: 'a1b45d'
+   * });
+   */
+  this.unsetFlashingFlag = (results) => {
+    Store.dispatch({
+      type: Store.Actions.UNSET_FLASHING_FLAG,
+      data: results
+    });
   };
 
   /**
@@ -137,6 +159,20 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel)
     });
   };
 
+  /**
+   * @summary Get the flash results
+   * @function
+   * @public
+   *
+   * @returns {Object} flash results
+   *
+   * @example
+   * const results = ImageWriterService.getFlashResults();
+   */
+  this.getFlashResults = () => {
+    return Store.getState().toJS().flashResults;
+  };
+
   /**
    * @summary Perform write operation
    * @function
@@ -178,8 +214,6 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel)
    *
    * @param {String} image - image path
    * @param {Object} drive - drive
-   *
-   * @fulfil {Object} flash results
    * @returns {Promise}
    *
    * @example
@@ -194,10 +228,24 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel)
       return $q.reject(new Error('There is already a flash in progress'));
     }
 
-    this.setFlashing(true);
+    this.setFlashingFlag();
 
-    return this.performWrite(image, drive, this.setProgressState).finally(() => {
+    return this.performWrite(image, drive, this.setProgressState).then((results) => {
-      this.setFlashing(false);
+
+      // TODO: Make sure `etcher-image-write` always
+      // sends a `cancelled` property.
+      _.defaults(results, {
+        cancelled: false
+      });
+
+      this.unsetFlashingFlag(results);
+    }).catch((error) => {
+      this.unsetFlashingFlag({
+        cancelled: false,
+        passedValidation: false
+      });
+
+      return $q.reject(error);
     });
   };
 

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

@@ -16,7 +16,7 @@
 
 'use strict';
 
-module.exports = function($state, $stateParams, SelectionStateModel, AnalyticsService, SettingsModel) {
+module.exports = function($state, ImageWriterService, SelectionStateModel, AnalyticsService, SettingsModel) {
 
   /**
    * @summary Settings data
@@ -26,11 +26,11 @@ module.exports = function($state, $stateParams, SelectionStateModel, AnalyticsSe
   this.settings = SettingsModel.data;
 
   /**
-   * @summary Route params
+   * @summary Source checksum
-   * @type Object
+   * @type String
    * @public
    */
-  this.params = $stateParams;
+  this.checksum = ImageWriterService.getFlashResults().sourceChecksum;
 
   /**
    * @summary Restart the flashing process

lib/gui/pages/finish/finish.js

@@ -31,6 +31,7 @@ const MODULE_NAME = 'Etcher.Pages.Finish';
 const FinishPage = angular.module(MODULE_NAME, [
   require('angular-ui-router'),
   require('../../modules/analytics'),
+  require('../../modules/image-writer'),
   require('../../models/selection-state'),
   require('../../models/settings')
 ]);
@@ -40,7 +41,7 @@ FinishPage.controller('FinishController', require('./controllers/finish'));
 FinishPage.config(($stateProvider) => {
   $stateProvider
     .state('success', {
-      url: '/success/:checksum',
+      url: '/success',
       controller: 'FinishController as finish',
       templateUrl: './pages/finish/templates/success.tpl.html'
     });

lib/gui/pages/finish/templates/success.tpl.html

@@ -28,7 +28,7 @@
         </div>
       </div>
 
-      <span class="label label-big label-default">CRC32 CHECKSUM : <b class="monospace">{{ ::finish.params.checksum }}</b></span>
+      <span class="label label-big label-default">CRC32 CHECKSUM : <b class="monospace">{{ ::finish.checksum }}</b></span>
     </div>
   </div>
 </div>

lib/gui/partials/main.html

@@ -84,7 +84,7 @@
           percentage="app.writer.state.percentage"
           striped="{{ app.writer.state.type == 'check' }}"
           ng-attr-active="{{ app.writer.isFlashing() }}"
-          ng-show="app.success"
+          ng-show="app.wasLastFlashSuccessful()"
           ng-click="app.flash(app.selection.getImagePath(), app.selection.getDrive())"
           ng-disabled="!app.selection.hasImage() || !app.selection.hasDrive()">
             <span ng-show="app.writer.state.percentage == 100 && app.writer.isFlashing()">Finishing...</span>
@@ -96,7 +96,7 @@
               ng-bind="app.writer.state.percentage + '% Validating...'"></span>
         </progress-button>
 
-        <div class="alert-ribbon alert-warning" ng-class="{ 'alert-ribbon--open': !app.success }">
+        <div class="alert-ribbon alert-warning" ng-class="{ 'alert-ribbon--open': !app.wasLastFlashSuccessful() }">
           <span class="glyphicon glyphicon-warning-sign"></span>
           <span ng-show="app.settings.validateWriteOnSuccess">
             Your removable drive did not pass validation check.<br>Please insert another one and <button class="btn btn-link" ng-click="app.restartAfterFailure()">try again</button>
@@ -106,7 +106,7 @@
           </span>
         </div>
 
-        <button class="btn btn-warning btn-brick" ng-hide="app.success" ng-click="app.restartAfterFailure()">
+        <button class="btn btn-warning btn-brick" ng-hide="app.wasLastFlashSuccessful()" ng-click="app.restartAfterFailure()">
           <span class="glyphicon glyphicon-repeat"></span> Retry
         </button>
 

tests/gui/modules/image-writer.spec.js

@@ -37,7 +37,7 @@ describe('Browser: ImageWriter', function() {
 
     describe('.resetState()', function() {
 
-      it('should be able to reset the state', function() {
+      it('should be able to reset the progress state', function() {
         ImageWriterService.state = {
           percentage: 50,
           speed: 3
@@ -52,6 +52,17 @@ describe('Browser: ImageWriter', function() {
         });
       });
 
+      it('should be able to reset the progress state', function() {
+        ImageWriterService.unsetFlashingFlag({
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
+
+        ImageWriterService.resetState();
+        m.chai.expect(ImageWriterService.getFlashResults()).to.deep.equal({});
+      });
+
     });
 
     describe('.isFlashing()', function() {
@@ -61,7 +72,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should return true if flashing', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(ImageWriterService.isFlashing()).to.be.true;
       });
 
@@ -70,7 +81,12 @@ describe('Browser: ImageWriter', function() {
     describe('.setProgressState()', function() {
 
       it('should not allow setting the state if flashing is false', function() {
-        ImageWriterService.setFlashing(false);
+        ImageWriterService.unsetFlashingFlag({
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
+
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -82,7 +98,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if type is missing', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             percentage: 50,
@@ -93,7 +109,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if type is not a string', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 1234,
@@ -105,7 +121,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if percentage is missing', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -116,7 +132,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if percentage is not a number', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -128,7 +144,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if eta is missing', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -139,7 +155,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should not throw if eta is equal to zero', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -151,7 +167,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if eta is not a number', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -163,7 +179,7 @@ describe('Browser: ImageWriter', function() {
       });
 
       it('should throw if speed is missing', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
         m.chai.expect(function() {
           ImageWriterService.setProgressState({
             type: 'write',
@@ -175,28 +191,120 @@ describe('Browser: ImageWriter', function() {
 
     });
 
-    describe('.setFlashing()', function() {
+    describe('.getFlashResults()', function() {
 
-      it('should be able to set flashing to true', function() {
+      it('should get the flash results', function() {
-        ImageWriterService.setFlashing(true);
+        ImageWriterService.setFlashingFlag();
-        m.chai.expect(ImageWriterService.isFlashing()).to.be.true;
+
+        const expectedResults = {
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        };
+
+        ImageWriterService.unsetFlashingFlag(expectedResults);
+        const results = ImageWriterService.getFlashResults();
+        m.chai.expect(results).to.deep.equal(expectedResults);
+      });
+
+    });
+
+    describe('.unsetFlashingFlag()', function() {
+
+      it('should throw if no flashing results', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag();
+        }).to.throw('Missing results');
+      });
+
+      it('should throw if no passedValidation', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            cancelled: false,
+            sourceChecksum: '1234'
+          });
+        }).to.throw('Missing results passedValidation');
+      });
+
+      it('should throw if passedValidation is not boolean', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: 'true',
+            cancelled: false,
+            sourceChecksum: '1234'
+          });
+        }).to.throw('Invalid results passedValidation: true');
+      });
+
+      it('should throw if no cancelled', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            sourceChecksum: '1234'
+          });
+        }).to.throw('Missing results cancelled');
+      });
+
+      it('should throw if cancelled is not boolean', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: 'false',
+            sourceChecksum: '1234'
+          });
+        }).to.throw('Invalid results cancelled: false');
+      });
+
+      it('should throw if passedValidation is true and sourceChecksum does not exist', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: false
+          });
+        }).to.throw('Missing results sourceChecksum');
+      });
+
+      it('should throw if passedValidation is true and sourceChecksum is not a string', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: false,
+            sourceChecksum: 12345
+          });
+        }).to.throw('Invalid results sourceChecksum: 12345');
+      });
+
+      it('should throw if cancelled is true and sourceChecksum exists', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: false,
+            cancelled: true,
+            sourceChecksum: '1234'
+          });
+        }).to.throw('The sourceChecksum value can\'t exist if the flashing was cancelled');
+      });
+
+      it('should throw if cancelled is true and passedValidation is true', function() {
+        m.chai.expect(function() {
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: true
+          });
+        }).to.throw('The passedValidation value can\'t be true if the flashing was cancelled');
       });
 
       it('should be able to set flashing to false', function() {
-        ImageWriterService.setFlashing(false);
+        ImageWriterService.unsetFlashingFlag({
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
+
         m.chai.expect(ImageWriterService.isFlashing()).to.be.false;
       });
 
-      it('should cast to boolean by default', function() {
+      it('should reset the flashing state', function() {
-        ImageWriterService.setFlashing('hello');
+        ImageWriterService.setFlashingFlag();
-        m.chai.expect(ImageWriterService.isFlashing()).to.be.true;
-
-        ImageWriterService.setFlashing('');
-        m.chai.expect(ImageWriterService.isFlashing()).to.be.false;
-      });
-
-      it('should reset the flashing state if set to false', function() {
-        ImageWriterService.setFlashing(true);
 
         ImageWriterService.setProgressState({
           type: 'write',
@@ -212,7 +320,11 @@ describe('Browser: ImageWriter', function() {
           speed: 0
         });
 
-        ImageWriterService.setFlashing(false);
+        ImageWriterService.unsetFlashingFlag({
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        });
 
         $timeout.flush();
 
@@ -224,13 +336,40 @@ describe('Browser: ImageWriter', function() {
 
     });
 
+    describe('.setFlashingFlag()', function() {
+
+      it('should be able to set flashing to true', function() {
+        ImageWriterService.setFlashingFlag();
+        m.chai.expect(ImageWriterService.isFlashing()).to.be.true;
+      });
+
+      it('should reset the flash results', function() {
+        const expectedResults = {
+          passedValidation: true,
+          cancelled: false,
+          sourceChecksum: '1234'
+        };
+
+        ImageWriterService.unsetFlashingFlag(expectedResults);
+        const results = ImageWriterService.getFlashResults();
+        m.chai.expect(results).to.deep.equal(expectedResults);
+        ImageWriterService.setFlashingFlag();
+        m.chai.expect(ImageWriterService.getFlashResults()).to.deep.equal({});
+      });
+
+    });
+
     describe('.flash()', function() {
 
       describe('given a succesful write', function() {
 
         beforeEach(function() {
           this.performWriteStub = m.sinon.stub(ImageWriterService, 'performWrite');
-          this.performWriteStub.returns($q.resolve());
+          this.performWriteStub.returns($q.resolve({
+            passedValidation: true,
+            cancelled: false,
+            sourceChecksum: '1234'
+          }));
         });
 
         afterEach(function() {
@@ -238,14 +377,24 @@ describe('Browser: ImageWriter', function() {
         });
 
         it('should set flashing to false when done', function() {
-          ImageWriterService.setFlashing(false);
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: false,
+            sourceChecksum: '1234'
+          });
+
           ImageWriterService.flash('foo.img', '/dev/disk2');
           $rootScope.$apply();
           m.chai.expect(ImageWriterService.isFlashing()).to.be.false;
         });
 
         it('should prevent writing more than once', function() {
-          ImageWriterService.setFlashing(false);
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: false,
+            sourceChecksum: '1234'
+          });
+
           ImageWriterService.flash('foo.img', '/dev/disk2');
           ImageWriterService.flash('foo.img', '/dev/disk2');
           $rootScope.$apply();
@@ -286,7 +435,11 @@ describe('Browser: ImageWriter', function() {
         });
 
         it('should be rejected with the error', function() {
-          ImageWriterService.setFlashing(false);
+          ImageWriterService.unsetFlashingFlag({
+            passedValidation: true,
+            cancelled: false,
+            sourceChecksum: '1234'
+          });
 
           let rejection;
           ImageWriterService.flash('foo.img', '/dev/disk2').catch(function(error) {