refactor(GUI): replace SET_SETTING with an atomic SET_SETTINGS action (#1515)

This commit is the first on a series of commit to incrementally
implement support for configuration files (so we avoid a huge PR like we
have at the moment).

Once of the first things we can do is replace the `SET_SETTING` redux
action with an atomic `SET_SETTINGS` action that sets all the settings
for the application at once.

The purpose of this change is that later the `SET_SETTINGS` action can
be modified to stringify all the settings and store them in a
configuration file, without having to deal with merges, conflicts, etc
(since the client application if forced to resolve those problems before
calling the `SET_SETTINGS` action.)

The behaviour of the code remains almost the same, with the exception
that the user can now set settings that we don't know about, so the user
can switch between Etcher versions without getting weird errors if one
of the configuration keys he has doesn't exist in the other version.

See: https://github.com/resin-io/etcher/pull/1382
Change-Type: patch
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

lib/gui/models/settings.js

@@ -20,7 +20,9 @@
  * @module Etcher.Models.Settings
  */
 
+const _ = require('lodash');
 const Store = require('./store');
+const errors = require('../../shared/errors');
 
 /**
  * @summary Set a setting value
@@ -34,12 +36,25 @@ const Store = require('./store');
  * settings.set('unmountOnSuccess', true);
  */
 exports.set = (key, value) => {
+  if (_.isNil(key)) {
+    throw errors.createError({
+      title: 'Missing setting key'
+    });
+  }
+
+  if (!_.isString(key)) {
+    throw errors.createError({
+      title: `Invalid setting key: ${key}`
+    });
+  }
+
+  const newSettings = _.assign(exports.getAll(), {
+    [key]: value
+  });
+
   Store.dispatch({
-    type: Store.Actions.SET_SETTING,
-    data: {
-      key,
-      value
-    }
+    type: Store.Actions.SET_SETTINGS,
+    data: newSettings
   });
 };
 

lib/gui/models/store.js

@@ -79,7 +79,7 @@ const ACTIONS = _.fromPairs(_.map([
   'SELECT_IMAGE',
   'REMOVE_DRIVE',
   'REMOVE_IMAGE',
-  'SET_SETTING'
+  'SET_SETTINGS'
 ], (message) => {
   return [ message, message ];
 }));
@@ -452,35 +452,40 @@ const storeReducer = (state = DEFAULT_STATE, action) => {
       return state.deleteIn([ 'selection', 'image' ]);
     }
 
-    case ACTIONS.SET_SETTING: {
-      const key = action.data.key;
-      const value = action.data.value;
-
-      if (!key) {
+    case ACTIONS.SET_SETTINGS: {
+      if (!action.data) {
         throw errors.createError({
-          title: 'Missing setting key'
+          title: 'Missing settings'
         });
       }
 
-      if (!_.isString(key)) {
+      if (!_.isPlainObject(action.data)) {
         throw errors.createError({
-          title: `Invalid setting key: ${key}`
+          title: `Invalid settings: ${action.data}`
         });
       }
 
-      if (!DEFAULT_STATE.get('settings').has(key)) {
+      const invalidKey = _.find(_.keys(action.data), (key) => {
+        return !_.isString(key);
+      });
+
+      if (!_.isNil(invalidKey)) {
         throw errors.createError({
-          title: `Unsupported setting: ${key}`
+          title: `Invalid setting key: ${invalidKey}`
         });
       }
 
-      if (_.isObject(value)) {
+      const invalidPair = _.find(_.toPairs(action.data), (pair) => {
+        return _.isObject(_.last(pair));
+      });
+
+      if (!_.isNil(invalidPair)) {
         throw errors.createError({
-          title: `Invalid setting value: ${value}`
+          title: `Invalid setting value: ${_.last(invalidPair)} for ${_.first(invalidPair)}`
         });
       }
 
-      return state.setIn([ 'settings', key ], value);
+      return state.setIn([ 'settings' ], Immutable.fromJS(action.data));
     }
 
     default: {

tests/gui/models/settings.spec.js

@@ -9,20 +9,20 @@ describe('Browser: settings', function() {
 
   describe('settings', function() {
 
-    const SUPPORTED_KEYS = _.keys(Store.Defaults.get('settings').toJS());
+    const DEFAULT_KEYS = _.keys(Store.Defaults.get('settings').toJS());
 
     beforeEach(function() {
       this.settings = settings.getAll();
     });
 
     afterEach(function() {
-      _.each(SUPPORTED_KEYS, (supportedKey) => {
+      _.each(DEFAULT_KEYS, (supportedKey) => {
         settings.set(supportedKey, this.settings[supportedKey]);
       });
     });
 
     it('should be able to set and read values', function() {
-      const keyUnderTest = _.first(SUPPORTED_KEYS);
+      const keyUnderTest = _.sample(DEFAULT_KEYS);
       const originalValue = settings.get(keyUnderTest);
 
       settings.set(keyUnderTest, !originalValue);
@@ -33,10 +33,10 @@ describe('Browser: settings', function() {
 
     describe('.set()', function() {
 
-      it('should throw if the key is not supported', function() {
-        m.chai.expect(function() {
-          settings.set('foobar', true);
-        }).to.throw('Unsupported setting: foobar');
+      it('should set an unknown key', function() {
+        m.chai.expect(settings.get('foobar')).to.be.undefined;
+        settings.set('foobar', true);
+        m.chai.expect(settings.get('foobar')).to.be.true;
       });
 
       it('should throw if no key', function() {
@@ -52,23 +52,23 @@ describe('Browser: settings', function() {
       });
 
       it('should throw if setting an object', function() {
-        const keyUnderTest = _.first(SUPPORTED_KEYS);
+        const keyUnderTest = _.sample(DEFAULT_KEYS);
         m.chai.expect(function() {
           settings.set(keyUnderTest, {
             setting: 1
           });
-        }).to.throw('Invalid setting value: [object Object]');
+        }).to.throw(`Invalid setting value: [object Object] for ${keyUnderTest}`);
       });
 
       it('should throw if setting an array', function() {
-        const keyUnderTest = _.first(SUPPORTED_KEYS);
+        const keyUnderTest = _.sample(DEFAULT_KEYS);
         m.chai.expect(function() {
           settings.set(keyUnderTest, [ 1, 2, 3 ]);
-        }).to.throw('Invalid setting value: 1,2,3');
+        }).to.throw(`Invalid setting value: 1,2,3 for ${keyUnderTest}`);
       });
 
       it('should set the key to undefined if no value', function() {
-        const keyUnderTest = _.first(SUPPORTED_KEYS);
+        const keyUnderTest = _.sample(DEFAULT_KEYS);
         settings.set(keyUnderTest);
         m.chai.expect(settings.get(keyUnderTest)).to.be.undefined;
       });
@@ -80,7 +80,7 @@ describe('Browser: settings', function() {
       it('should be able to read all values', function() {
         const allValues = settings.getAll();
 
-        _.each(SUPPORTED_KEYS, function(supportedKey) {
+        _.each(DEFAULT_KEYS, function(supportedKey) {
           m.chai.expect(allValues[supportedKey]).to.equal(settings.get(supportedKey));
         });
       });