From 1a49b36a14c353670569ba45fa79ab8809e79648 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Fri, 24 Jun 2016 16:06:27 -0400 Subject: [PATCH] refactor: store settings in redux store (#530) * refactor: getter/setter interface for SettingsModel This PR introduces a getter/setter interface for `SettingsModel`, which replaces the old way of managing setting values by simply assigning properties to an object. This is the first step towards moving the settings functionality to the Redux store. Signed-off-by: Juan Cruz Viotti * refactor: store settings in redux store The state data structure now contains a property called `settings`, which is a map containing all setting values. The list of supported settings can be calculated by retrieving the keys from the `settings` object, which means that if we support a setting, we must include a default. Signed-off-by: Juan Cruz Viotti * feat: store settings in localStorage This functionality was deleted by acb0de2 when moving the settings object to the redux store, promising that the feature will be added back in a future commit. Signed-off-by: Juan Cruz Viotti --- lib/gui/app.js | 2 +- .../controllers/update-notifier.js | 6 +- .../services/update-notifier.js | 6 +- .../templates/update-notifier-modal.tpl.html | 2 +- lib/gui/models/settings.js | 63 ++++++++--- lib/gui/models/store.js | 96 ++++++++++++++++- lib/gui/modules/analytics.js | 6 +- lib/gui/modules/image-writer.js | 5 +- lib/gui/pages/finish/controllers/finish.js | 4 +- .../pages/finish/templates/success.tpl.html | 2 +- .../pages/settings/controllers/settings.js | 11 +- .../settings/templates/settings.tpl.html | 15 ++- lib/gui/partials/main.html | 4 +- package.json | 2 +- tests/gui/components/update-notifier.spec.js | 14 +-- tests/gui/models/settings.spec.js | 101 ++++++++++++++++++ 16 files changed, 293 insertions(+), 46 deletions(-) create mode 100644 tests/gui/models/settings.spec.js diff --git a/lib/gui/app.js b/lib/gui/app.js index 67623c66..16f0b4c8 100644 --- a/lib/gui/app.js +++ b/lib/gui/app.js @@ -146,7 +146,7 @@ app.controller('AppController', function( this.selection = SelectionStateModel; this.drives = DrivesModel; this.writer = ImageWriterService; - this.settings = SettingsModel.data; + this.settings = SettingsModel; this.tooltipModal = TooltipModalService; this.handleError = (error) => { diff --git a/lib/gui/components/update-notifier/controllers/update-notifier.js b/lib/gui/components/update-notifier/controllers/update-notifier.js index 29f0d256..468cc1ae 100644 --- a/lib/gui/components/update-notifier/controllers/update-notifier.js +++ b/lib/gui/components/update-notifier/controllers/update-notifier.js @@ -23,14 +23,14 @@ module.exports = function($uibModalInstance, SettingsModel) { // If the controller is instantiated, means the modal was shown. // Compare that to `UpdateNotifierService.notify()`, which could // have been called, but the modal could have failed to be shown. - SettingsModel.data.lastUpdateNotify = Date.now(); + SettingsModel.set('lastUpdateNotify', Date.now()); /** - * @summary Settings data + * @summary Settings model * @type Object * @public */ - this.settings = SettingsModel.data; + this.settings = SettingsModel; /** * @summary Close the modal diff --git a/lib/gui/components/update-notifier/services/update-notifier.js b/lib/gui/components/update-notifier/services/update-notifier.js index efa4acbf..1ed87a42 100644 --- a/lib/gui/components/update-notifier/services/update-notifier.js +++ b/lib/gui/components/update-notifier/services/update-notifier.js @@ -54,14 +54,14 @@ module.exports = function($uibModal, UPDATE_NOTIFIER_SLEEP_TIME, ManifestBindSer * } */ this.shouldCheckForUpdates = () => { - const lastUpdateNotify = SettingsModel.data.lastUpdateNotify; + const lastUpdateNotify = SettingsModel.get('lastUpdateNotify'); - if (!SettingsModel.data.sleepUpdateCheck || !lastUpdateNotify) { + if (!SettingsModel.get('sleepUpdateCheck') || !lastUpdateNotify) { return true; } if (lastUpdateNotify - Date.now() > UPDATE_NOTIFIER_SLEEP_TIME) { - SettingsModel.data.sleepUpdateCheck = false; + SettingsModel.set('sleepUpdateCheck', false); return true; } diff --git a/lib/gui/components/update-notifier/templates/update-notifier-modal.tpl.html b/lib/gui/components/update-notifier/templates/update-notifier-modal.tpl.html index c89eb4f1..683f713c 100644 --- a/lib/gui/components/update-notifier/templates/update-notifier-modal.tpl.html +++ b/lib/gui/components/update-notifier/templates/update-notifier-modal.tpl.html @@ -13,7 +13,7 @@
diff --git a/lib/gui/models/settings.js b/lib/gui/models/settings.js index f698af91..c4c9be02 100644 --- a/lib/gui/models/settings.js +++ b/lib/gui/models/settings.js @@ -21,25 +21,62 @@ */ const angular = require('angular'); -require('ngstorage'); +const Store = require('./store'); const MODULE_NAME = 'Etcher.Models.Settings'; -const SettingsModel = angular.module(MODULE_NAME, [ - 'ngStorage' -]); +const SettingsModel = angular.module(MODULE_NAME, []); -SettingsModel.service('SettingsModel', function($localStorage) { +SettingsModel.service('SettingsModel', function() { /** - * @summary Settings data - * @type Object + * @summary Set a setting value + * @function * @public + * + * @param {String} key - setting key + * @param {*} value - setting value + * + * @example + * SettingsModel.set('unmountOnSuccess', true); */ - this.data = $localStorage.$default({ - errorReporting: true, - unmountOnSuccess: true, - validateWriteOnSuccess: true, - sleepUpdateCheck: false - }); + this.set = (key, value) => { + Store.dispatch({ + type: Store.Actions.SET_SETTING, + data: { + key, + value + } + }); + }; + + /** + * @summary Get a setting value + * @function + * @public + * + * @param {String} key - setting key + * @returns {*} setting value + * + * @example + * const value = SettingsModel.get('unmountOnSuccess'); + */ + this.get = (key) => { + return this.getAll()[key]; + }; + + /** + * @summary Get all setting values + * @function + * @public + * + * @returns {Object} all setting values + * + * @example + * const allSettings = SettingsModel.getAll(); + * console.log(allSettings.unmountOnSuccess); + */ + this.getAll = () => { + return Store.getState().get('settings').toJS(); + }; }); diff --git a/lib/gui/models/store.js b/lib/gui/models/store.js index e9a1bf0e..5855fd5b 100644 --- a/lib/gui/models/store.js +++ b/lib/gui/models/store.js @@ -19,6 +19,7 @@ const Immutable = require('immutable'); const _ = require('lodash'); const redux = require('redux'); +const persistState = require('redux-localstorage'); /** * @summary Application default state @@ -34,9 +35,24 @@ const DEFAULT_STATE = Immutable.fromJS({ flashState: { percentage: 0, speed: 0 + }, + settings: { + errorReporting: true, + unmountOnSuccess: true, + validateWriteOnSuccess: true, + sleepUpdateCheck: false, + lastUpdateNotify: null } }); +/** + * @summary State path to be persisted + * @type String + * @constant + * @private + */ +const PERSISTED_PATH = 'settings'; + /** * @summary Application supported action messages * @type Object @@ -51,7 +67,8 @@ const ACTIONS = _.fromPairs(_.map([ 'SELECT_DRIVE', 'SELECT_IMAGE', 'REMOVE_DRIVE', - 'REMOVE_IMAGE' + 'REMOVE_IMAGE', + 'SET_SETTING' ], (message) => { return [ message, message ]; })); @@ -263,6 +280,29 @@ const storeReducer = (state, action) => { return state.deleteIn([ 'selection', 'image' ]); } + case ACTIONS.SET_SETTING: { + const key = action.data.key; + const value = action.data.value; + + if (!key) { + throw new Error('Missing setting key'); + } + + if (!_.isString(key)) { + throw new Error(`Invalid setting key: ${key}`); + } + + if (!DEFAULT_STATE.get('settings').has(key)) { + throw new Error(`Unsupported setting: ${key}`); + } + + if (_.isObject(value)) { + throw new Error(`Invalid setting value: ${value}`); + } + + return state.setIn([ 'settings', key ], value); + } + default: { return state; } @@ -270,6 +310,56 @@ const storeReducer = (state, action) => { } }; -module.exports = _.merge(redux.createStore(storeReducer), { - Actions: ACTIONS +module.exports = _.merge(redux.createStore( + storeReducer, + DEFAULT_STATE, + redux.compose(persistState(PERSISTED_PATH, { + + // The following options are set for the sole + // purpose of dealing correctly with ImmutableJS + // collections. + // See: https://github.com/elgerlambert/redux-localstorage#immutable-data + + slicer: (key) => { + return (state) => { + return state.get(key); + }; + }, + + serialize: (collection) => { + return JSON.stringify(collection.toJS()); + }, + + deserialize: (data) => { + return Immutable.fromJS(JSON.parse(data)); + }, + + merge: (state, subset) => { + + // In the first run, there will be no information + // to deserialize. In this case, we avoid merging, + // otherwise we will be basically erasing the property + // we aim the keep serialising the in future. + if (!subset) { + return; + } + + // Blindly setting the state to the deserialised subset + // means that a user could manually edit `localStorage` + // and extend the application state settings with + // unsupported properties, since it can bypass validation. + // + // The alternative, which would be dispatching each + // deserialised settins through the appropriate action + // is not very elegant, nor performant, so we decide + // to intentionally ignore this little flaw since + // adding extra properties makes no damage at all. + return state.set(PERSISTED_PATH, subset); + + } + + })) +), { + Actions: ACTIONS, + Defaults: DEFAULT_STATE }); diff --git a/lib/gui/modules/analytics.js b/lib/gui/modules/analytics.js index b2878078..085a7ddf 100644 --- a/lib/gui/modules/analytics.js +++ b/lib/gui/modules/analytics.js @@ -78,7 +78,7 @@ analytics.config(($provide) => { return (exception, cause) => { const SettingsModel = $injector.get('SettingsModel'); - if (SettingsModel.data.errorReporting) { + if (SettingsModel.get('errorReporting')) { $window.trackJs.track(exception); } @@ -96,7 +96,7 @@ analytics.config(($provide) => { const SettingsModel = $injector.get('SettingsModel'); - if (SettingsModel.data.errorReporting) { + if (SettingsModel.get('errorReporting')) { $window.trackJs.console.debug(message); } @@ -144,7 +144,7 @@ analytics.service('AnalyticsService', function($log, $mixpanel, SettingsModel) { */ this.logEvent = (message, data) => { - if (SettingsModel.data.errorReporting) { + if (SettingsModel.get('errorReporting')) { // Clone data before passing it to `mixpanel.track` // since this function mutates the object adding diff --git a/lib/gui/modules/image-writer.js b/lib/gui/modules/image-writer.js index 992c4b5b..c4657d3d 100644 --- a/lib/gui/modules/image-writer.js +++ b/lib/gui/modules/image-writer.js @@ -197,7 +197,10 @@ imageWriter.service('ImageWriterService', function($q, $timeout, SettingsModel) */ this.performWrite = (image, drive, onProgress) => { return $q((resolve, reject) => { - const child = childWriter.write(image, drive, SettingsModel.data); + const child = childWriter.write(image, drive, { + validateWriteOnSuccess: SettingsModel.get('validateWriteOnSuccess'), + unmountOnSuccess: SettingsModel.get('unmountOnSuccess') + }); child.on('error', reject); child.on('done', resolve); child.on('progress', onProgress); diff --git a/lib/gui/pages/finish/controllers/finish.js b/lib/gui/pages/finish/controllers/finish.js index 43500c06..24094ee9 100644 --- a/lib/gui/pages/finish/controllers/finish.js +++ b/lib/gui/pages/finish/controllers/finish.js @@ -19,11 +19,11 @@ module.exports = function($state, ImageWriterService, SelectionStateModel, AnalyticsService, SettingsModel) { /** - * @summary Settings data + * @summary Settings model * @type Object * @public */ - this.settings = SettingsModel.data; + this.settings = SettingsModel; /** * @summary Source checksum diff --git a/lib/gui/pages/finish/templates/success.tpl.html b/lib/gui/pages/finish/templates/success.tpl.html index b10ce993..c78dc05c 100644 --- a/lib/gui/pages/finish/templates/success.tpl.html +++ b/lib/gui/pages/finish/templates/success.tpl.html @@ -2,7 +2,7 @@

Flash Complete!

-

Safely ejected and ready for use

+

Safely ejected and ready for use

diff --git a/lib/gui/pages/settings/controllers/settings.js b/lib/gui/pages/settings/controllers/settings.js index 2a66af75..b96b9b05 100644 --- a/lib/gui/pages/settings/controllers/settings.js +++ b/lib/gui/pages/settings/controllers/settings.js @@ -19,10 +19,17 @@ module.exports = function(SettingsModel) { /** - * @summary Settings data + * @summary Current settings value * @type Object * @public */ - this.storage = SettingsModel.data; + this.currentData = SettingsModel.getAll(); + + /** + * @summary Settings model + * @type Object + * @public + */ + this.model = SettingsModel; }; diff --git a/lib/gui/pages/settings/templates/settings.tpl.html b/lib/gui/pages/settings/templates/settings.tpl.html index b0469e42..79f0d71d 100644 --- a/lib/gui/pages/settings/templates/settings.tpl.html +++ b/lib/gui/pages/settings/templates/settings.tpl.html @@ -3,21 +3,30 @@
diff --git a/lib/gui/partials/main.html b/lib/gui/partials/main.html index ad7db37d..12544267 100644 --- a/lib/gui/partials/main.html +++ b/lib/gui/partials/main.html @@ -98,10 +98,10 @@
- + Your removable drive did not pass validation check.
Please insert another one and - + Oops, seems something went wrong. Click to retry
diff --git a/package.json b/package.json index 5bf28e40..d63ec932 100644 --- a/package.json +++ b/package.json @@ -72,8 +72,8 @@ "immutable": "^3.8.1", "is-elevated": "^1.0.0", "lodash": "^4.5.1", - "ngstorage": "^0.3.10", "redux": "^3.5.2", + "redux-localstorage": "^0.4.1", "resin-cli-errors": "^1.2.0", "resin-cli-form": "^1.4.1", "resin-cli-visuals": "^1.2.8", diff --git a/tests/gui/components/update-notifier.spec.js b/tests/gui/components/update-notifier.spec.js index 8d6fe3ae..9050404d 100644 --- a/tests/gui/components/update-notifier.spec.js +++ b/tests/gui/components/update-notifier.spec.js @@ -28,7 +28,7 @@ describe('Browser: UpdateNotifier', function() { describe('given the `sleepUpdateCheck` is disabled', function() { beforeEach(function() { - SettingsModel.data.sleepUpdateCheck = false; + SettingsModel.set('sleepUpdateCheck', false); }); it('should return true', function() { @@ -41,13 +41,13 @@ describe('Browser: UpdateNotifier', function() { describe('given the `sleepUpdateCheck` is enabled', function() { beforeEach(function() { - SettingsModel.data.sleepUpdateCheck = true; + SettingsModel.set('sleepUpdateCheck', true); }); describe('given the `lastUpdateNotify` was never updated', function() { beforeEach(function() { - SettingsModel.data.lastUpdateNotify = undefined; + SettingsModel.set('lastUpdateNotify', undefined); }); it('should return true', function() { @@ -60,7 +60,7 @@ describe('Browser: UpdateNotifier', function() { describe('given the `lastUpdateNotify` was very recently updated', function() { beforeEach(function() { - SettingsModel.data.lastUpdateNotify = Date.now() + 1000; + SettingsModel.set('lastUpdateNotify', Date.now() + 1000); }); it('should return false', function() { @@ -73,7 +73,7 @@ describe('Browser: UpdateNotifier', function() { describe('given the `lastUpdateNotify` was updated long ago', function() { beforeEach(function() { - SettingsModel.data.lastUpdateNotify = Date.now() + UPDATE_NOTIFIER_SLEEP_TIME + 1000; + SettingsModel.set('lastUpdateNotify', Date.now() + UPDATE_NOTIFIER_SLEEP_TIME + 1000); }); it('should return true', function() { @@ -82,9 +82,9 @@ describe('Browser: UpdateNotifier', function() { }); it('should unset the `sleepUpdateCheck` setting', function() { - m.chai.expect(SettingsModel.data.sleepUpdateCheck).to.be.true; + m.chai.expect(SettingsModel.get('sleepUpdateCheck')).to.be.true; UpdateNotifierService.shouldCheckForUpdates(); - m.chai.expect(SettingsModel.data.sleepUpdateCheck).to.be.false; + m.chai.expect(SettingsModel.get('sleepUpdateCheck')).to.be.false; }); }); diff --git a/tests/gui/models/settings.spec.js b/tests/gui/models/settings.spec.js new file mode 100644 index 00000000..c85476b7 --- /dev/null +++ b/tests/gui/models/settings.spec.js @@ -0,0 +1,101 @@ +'use strict'; + +const m = require('mochainon'); +const _ = require('lodash'); +const angular = require('angular'); +require('angular-mocks'); +const Store = require('../../../lib/gui/models/store'); + +describe('Browser: SettingsModel', function() { + + beforeEach(angular.mock.module( + require('../../../lib/gui/models/settings') + )); + + describe('SettingsModel', function() { + + const SUPPORTED_KEYS = _.keys(Store.Defaults.get('settings').toJS()); + let SettingsModel; + + beforeEach(angular.mock.inject(function(_SettingsModel_) { + SettingsModel = _SettingsModel_; + })); + + beforeEach(function() { + this.settings = SettingsModel.getAll(); + }); + + afterEach(function() { + _.each(SUPPORTED_KEYS, (supportedKey) => { + SettingsModel.set(supportedKey, this.settings[supportedKey]); + }); + }); + + it('should be able to set and read values', function() { + const keyUnderTest = _.first(SUPPORTED_KEYS); + const originalValue = SettingsModel.get(keyUnderTest); + + SettingsModel.set(keyUnderTest, !originalValue); + m.chai.expect(SettingsModel.get(keyUnderTest)).to.equal(!originalValue); + SettingsModel.set(keyUnderTest, originalValue); + m.chai.expect(SettingsModel.get(keyUnderTest)).to.equal(originalValue); + }); + + describe('.set()', function() { + + it('should throw if the key is not supported', function() { + m.chai.expect(function() { + SettingsModel.set('foobar', true); + }).to.throw('Unsupported setting: foobar'); + }); + + it('should throw if no key', function() { + m.chai.expect(function() { + SettingsModel.set(null, true); + }).to.throw('Missing setting key'); + }); + + it('should throw if key is not a string', function() { + m.chai.expect(function() { + SettingsModel.set(1234, true); + }).to.throw('Invalid setting key: 1234'); + }); + + it('should throw if setting an object', function() { + const keyUnderTest = _.first(SUPPORTED_KEYS); + m.chai.expect(function() { + SettingsModel.set(keyUnderTest, { + x: 1 + }); + }).to.throw('Invalid setting value: [object Object]'); + }); + + it('should throw if setting an array', function() { + const keyUnderTest = _.first(SUPPORTED_KEYS); + m.chai.expect(function() { + SettingsModel.set(keyUnderTest, [ 1, 2, 3 ]); + }).to.throw('Invalid setting value: 1,2,3'); + }); + + it('should set the key to undefined if no value', function() { + const keyUnderTest = _.first(SUPPORTED_KEYS); + SettingsModel.set(keyUnderTest); + m.chai.expect(SettingsModel.get(keyUnderTest)).to.be.undefined; + }); + + }); + + describe('.getAll()', function() { + + it('should be able to read all values', function() { + const allValues = SettingsModel.getAll(); + + _.each(SUPPORTED_KEYS, function(supportedKey) { + m.chai.expect(allValues[supportedKey]).to.equal(SettingsModel.get(supportedKey)); + }); + }); + + }); + + }); +});