Skip to content

Commit

Permalink
[Config] Acknowledge when Advanced Settings respond (elastic#16485)
Browse files Browse the repository at this point in the history
This adds a return value to the `Promise` returned by `config.set`, which allows callers to `await` it  and verify that their setting was applied without checking the settings (which is a race condition).
  • Loading branch information
pickypg committed Feb 5, 2018
1 parent f5b3b46 commit 1b4fab3
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 7 deletions.
31 changes: 30 additions & 1 deletion src/ui/public/config/__tests__/config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import expect from 'expect.js';
import ngMock from 'ng_mock';

import { Notifier } from 'ui/notify';

describe('config component', function () {
let config;
let $scope;
Expand Down Expand Up @@ -67,7 +69,7 @@ describe('config component', function () {
expect($scope).to.have.property('dateFormat', dateFormat);
});

it('alows overriding the property name', function () {
it('allows overriding the property name', function () {
const dateFormat = config.get('dateFormat');
config.bindToScope($scope, 'dateFormat', 'defaultDateFormat');
expect($scope).to.not.have.property('dateFormat');
Expand All @@ -88,4 +90,31 @@ describe('config component', function () {

});

describe('#_change', () => {

it('returns true for success', async () => {
// immediately resolve to avoid timing issues
const delayedUpdate = () => Promise.resolve();

expect(await config._change('expect_true', 'value', { _delayedUpdate: delayedUpdate })).to.be(true);
// setting to the same should set it to true as well
expect(await config._change('expect_true', 'value')).to.be(true);

config.remove('expect_true');
});

it('returns false for failure', async () => {
const message = 'TEST - _change - EXPECTED';
// immediately resolve to avoid timing issues
const delayedUpdate = () => Promise.reject(new Error(message));

expect(await config._change('expected_false', 'value', { _delayedUpdate: delayedUpdate })).to.be(false);

// cleanup the notification so that the test harness does not complain
const notif = Notifier.prototype._notifs.find(notif => notif.content.indexOf(message) !== -1);
notif.clear();
});

});

});
14 changes: 8 additions & 6 deletions src/ui/public/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ module.service(`config`, function (Private, $rootScope, chrome, uiSettings) {

config.getAll = () => cloneDeep(settings);
config.get = (key, defaultValue) => getCurrentValue(key, defaultValue);
config.set = (key, val) => change(key, isPlainObject(val) ? angular.toJson(val) : val);
config.remove = key => change(key, null);
config.set = (key, val) => config._change(key, isPlainObject(val) ? angular.toJson(val) : val);
config.remove = key => config._change(key, null);
config.isDeclared = key => key in settings;
config.isDefault = key => !config.isDeclared(key) || nullOrEmpty(settings[key].userValue);
config.isCustom = key => config.isDeclared(key) && !('value' in settings[key]);
Expand Down Expand Up @@ -57,26 +57,28 @@ any custom setting configuration watchers for "${key}" may fix this issue.`);
return scope.$on(`change:config`, update);
}

function change(key, value) {
config._change = (key, value, { _delayedUpdate = delayedUpdate } = { }) => {
const declared = config.isDeclared(key);
const oldVal = declared ? settings[key].userValue : undefined;
const newVal = key in defaults && defaults[key].defaultValue === value ? null : value;
const unchanged = oldVal === newVal;
if (unchanged) {
return Promise.resolve();
return Promise.resolve(true);
}
const initialVal = declared ? config.get(key) : undefined;
localUpdate(key, newVal, initialVal);

return delayedUpdate(key, newVal)
return _delayedUpdate(key, newVal)
.then(updatedSettings => {
settings = mergeSettings(defaults, updatedSettings);
return true;
})
.catch(reason => {
localUpdate(key, initialVal, config.get(key));
notify.error(reason);
return false;
});
}
};

function localUpdate(key, newVal, oldVal) {
patch(key, newVal);
Expand Down

0 comments on commit 1b4fab3

Please sign in to comment.