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 <jviottidc@gmail.com>

* 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 <jviottidc@gmail.com>

* 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 <jviottidc@gmail.com>

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) => {

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

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;
     }
 

lib/gui/components/update-notifier/templates/update-notifier-modal.tpl.html

@@ -13,7 +13,7 @@
 
   <div class="checkbox text-right">
     <label>
-      <input type="checkbox" ng-model="modal.settings.sleepUpdateCheck">
+      <input type="checkbox" ng-model="modal.settings.get('sleepUpdateCheck')">
       <span>Remind me again in 7 days</span>
     </label>
   </div>

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();
+  };
 
 });
 

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
 });

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

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);

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

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

@@ -2,7 +2,7 @@
   <div class="col-xs">
     <div class="box text-center">
       <h3><span class="tick tick--success" class="space-right-tiny"></span> Flash Complete!</h3>
-      <p class="soft space-vertical-small" ng-show="finish.settings.unmountOnSuccess">Safely ejected and ready for use</p>
+      <p class="soft space-vertical-small" ng-show="finish.settings.get('unmountOnSuccess')">Safely ejected and ready for use</p>
 
       <div class="row center-xs space-vertical-medium">
         <div class="col-xs-4 space-medium">

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;
 
 };

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

@@ -3,21 +3,30 @@
 
   <div class="checkbox">
     <label>
-      <input type="checkbox" ng-model="settings.storage.errorReporting">
+      <input type="checkbox"
+        ng-model="settings.currentData.errorReporting"
+        ng-change="settings.model.set('errorReporting', settings.currentData.errorReporting)">
+
       <span>Report errors</span>
     </label>
   </div>
 
   <div class="checkbox">
     <label>
-      <input type="checkbox" ng-model="settings.storage.unmountOnSuccess">
+      <input type="checkbox"
+        ng-model="settings.currentData.unmountOnSuccess"
+        ng-change="settings.model.set('unmountOnSuccess', settings.currentData.unmountOnSuccess)">
+
       <span>Auto-unmount on success</span>
     </label>
   </div>
 
   <div class="checkbox">
     <label>
-      <input type="checkbox" ng-model="settings.storage.validateWriteOnSuccess">
+      <input type="checkbox"
+        ng-model="settings.currentData.validateWriteOnSuccess"
+        ng-change="settings.model.set('validateWriteOnSuccess', settings.currentData.validateWriteOnSuccess)">
+
       <span>Validate write on success</span>
     </label>
   </div>

lib/gui/partials/main.html

@@ -98,10 +98,10 @@
 
         <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">
+          <span ng-show="app.settings.get('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>
           </span>
-          <span ng-hide="app.settings.validateWriteOnSuccess">
+          <span ng-hide="app.settings.get('validateWriteOnSuccess')">
             Oops, seems something went wrong. Click <button class="btn btn-link" ng-click="app.restartAfterFailure()">here</button> to retry
           </span>
         </div>

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",

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;
           });
 
         });

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));
+        });
+      });
+
+    });
+
+  });
+});