From 103a048e4d4b670ca16d473ed6d6074b17519f94 Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Thu, 15 Jun 2017 12:57:15 -0400 Subject: [PATCH] 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 --- lib/gui/models/settings.js | 25 ++++++++++++++++++----- lib/gui/models/store.js | 33 ++++++++++++++++++------------- tests/gui/models/settings.spec.js | 26 ++++++++++++------------ 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/lib/gui/models/settings.js b/lib/gui/models/settings.js index a5d0d930..d29b621a 100644 --- a/lib/gui/models/settings.js +++ b/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 }); }; diff --git a/lib/gui/models/store.js b/lib/gui/models/store.js index b8e76423..0abed3ca 100644 --- a/lib/gui/models/store.js +++ b/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: { diff --git a/tests/gui/models/settings.spec.js b/tests/gui/models/settings.spec.js index 1eeb4c8a..8b554654 100644 --- a/tests/gui/models/settings.spec.js +++ b/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)); }); });