From 3fc8f02611fb83e322b9c14315868918e8f2508b Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Wed, 27 Jul 2016 22:33:34 -0400 Subject: [PATCH] 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 --- lib/gui/models/flash-state.js | 81 ++++++++- lib/gui/models/store.js | 15 +- lib/gui/modules/image-writer.js | 15 +- lib/gui/pages/finish/controllers/finish.js | 2 +- lib/gui/pages/main/controllers/main.js | 16 +- lib/gui/pages/main/templates/main.tpl.html | 12 +- tests/gui/models/flash-state.spec.js | 187 +++++++++++++++++++-- 7 files changed, 270 insertions(+), 58 deletions(-) diff --git a/lib/gui/models/flash-state.js b/lib/gui/models/flash-state.js index a883de22..23fcb526 100644 --- a/lib/gui/models/flash-state.js +++ b/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; diff --git a/lib/gui/models/store.js b/lib/gui/models/store.js index 109f40f0..ed89a2ff 100644 --- a/lib/gui/models/store.js +++ b/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}`); } diff --git a/lib/gui/modules/image-writer.js b/lib/gui/modules/image-writer.js index 4701d86a..13b75bb0 100644 --- a/lib/gui/modules/image-writer.js +++ b/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 }); diff --git a/lib/gui/pages/finish/controllers/finish.js b/lib/gui/pages/finish/controllers/finish.js index c64a564f..496220a4 100644 --- a/lib/gui/pages/finish/controllers/finish.js +++ b/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 diff --git a/lib/gui/pages/main/controllers/main.js b/lib/gui/pages/main/controllers/main.js index 1d5cabe9..4255d8b1 100644 --- a/lib/gui/pages/main/controllers/main.js +++ b/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'); diff --git a/lib/gui/pages/main/templates/main.tpl.html b/lib/gui/pages/main/templates/main.tpl.html index 4a0feaba..5333351c 100644 --- a/lib/gui/pages/main/templates/main.tpl.html +++ b/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()"> Finishing... @@ -99,20 +99,20 @@ ng-bind="main.state.getFlashState().percentage + '% Validating...'"> -
+
- + Not enough space on the drive.
Please insert larger one and - + Your removable drive may be corrupted.
Try inserting a different one and - + Oops, seems something went wrong. Click to retry
- diff --git a/tests/gui/models/flash-state.spec.js b/tests/gui/models/flash-state.spec.js index 500322bd..35a23064 100644 --- a/tests/gui/models/flash-state.spec.js +++ b/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; + }); + + }); + }); });