From 4e6f0a980cc7157f1de136fa9ecc81847655d02c Mon Sep 17 00:00:00 2001 From: Joe Fleming Date: Wed, 27 Aug 2014 17:28:11 -0700 Subject: [PATCH 1/4] state isn't in urlParam, reuse existing state --- src/kibana/components/state_management/state.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kibana/components/state_management/state.js b/src/kibana/components/state_management/state.js index 8c0f34c11890..7630bd1af4a9 100644 --- a/src/kibana/components/state_management/state.js +++ b/src/kibana/components/state_management/state.js @@ -26,7 +26,7 @@ define(function (require) { */ State.prototype.fetch = function () { var search = $location.search(); - var stash = rison.decode(search[this._urlParam] || '()'); + var stash = (search[this._urlParam]) ? rison.decode(search[this._urlParam]) : this.toObject(); _.defaults(stash, this._defaults); // apply diff to state from stash, this is side effecting so // it will change state in place. @@ -42,7 +42,7 @@ define(function (require) { */ State.prototype.save = function () { var search = $location.search(); - var stash = rison.decode(search[this._urlParam] || '()'); + var stash = (search[this._urlParam]) ? rison.decode(search[this._urlParam]) : this.toObject(); var state = this.toObject(); _.defaults(state, this._defaults); // apply diff to stash from state, this is side effecting so From 699e729fee355c7949e17f72b065763132b7c1fb Mon Sep 17 00:00:00 2001 From: Joe Fleming Date: Thu, 28 Aug 2014 16:22:49 -0700 Subject: [PATCH 2/4] refactor how state reads from URL, override method in global state to persist from memory --- .../state_management/global_state.js | 10 +++++++- .../components/state_management/state.js | 24 ++++++++++++------- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/kibana/components/state_management/global_state.js b/src/kibana/components/state_management/global_state.js index 6a3a2bc429fe..191fe2930d85 100644 --- a/src/kibana/components/state_management/global_state.js +++ b/src/kibana/components/state_management/global_state.js @@ -6,7 +6,7 @@ define(function (require) { var module = require('modules').get('kibana/global_state'); - module.service('globalState', function (Private, $rootScope) { + module.service('globalState', function (Private, $rootScope, $location) { var State = Private(require('components/state_management/state')); _.inherits(GlobalState, State); @@ -18,6 +18,14 @@ define(function (require) { return qs.replaceParamInUrl(url, this._urlParam, this.toRISON()); }; + GlobalState.prototype._readFromURL = function (method) { + var search = $location.search(); + if (method === 'fetch') { + return (search[this._urlParam]) ? rison.decode(search[this._urlParam]) : this.toObject(); + } + return rison.decode(search[this._urlParam] || '()'); + }; + return new GlobalState(); }); }); diff --git a/src/kibana/components/state_management/state.js b/src/kibana/components/state_management/state.js index 7630bd1af4a9..84871eb200e6 100644 --- a/src/kibana/components/state_management/state.js +++ b/src/kibana/components/state_management/state.js @@ -20,17 +20,22 @@ define(function (require) { this.fetch(); } + State.prototype._readFromURL = function (method) { + var search = $location.search(); + return rison.decode(search[this._urlParam] || '()'); + }; + /** * Fetches the state from the url * @returns {void} */ State.prototype.fetch = function () { - var search = $location.search(); - var stash = (search[this._urlParam]) ? rison.decode(search[this._urlParam]) : this.toObject(); + var stash = this._readFromURL('fetch'); + _.defaults(stash, this._defaults); - // apply diff to state from stash, this is side effecting so - // it will change state in place. + // apply diff to state from stash, will change state in place via side effect var diffResults = applyDiff(this, stash); + if (diffResults.keys.length) { this.emit('fetch_with_changes', diffResults.keys); } @@ -41,16 +46,19 @@ define(function (require) { * @returns {void} */ State.prototype.save = function () { - var search = $location.search(); - var stash = (search[this._urlParam]) ? rison.decode(search[this._urlParam]) : this.toObject(); + var stash = this._readFromURL('save'); var state = this.toObject(); + _.defaults(state, this._defaults); - // apply diff to stash from state, this is side effecting so - // it will change stash in place. + // apply diff to state from stash, will change state in place via side effect var diffResults = applyDiff(stash, state); + if (diffResults.keys.length) { this.emit('save_with_changes', diffResults.keys); } + + // persist the state in the URL + var search = $location.search(); search[this._urlParam] = this.toRISON(); $location.search(search); }; From dddadb8757ce6832402f19ca5d6e717cb5153696 Mon Sep 17 00:00:00 2001 From: Joe Fleming Date: Thu, 28 Aug 2014 16:23:40 -0700 Subject: [PATCH 3/4] rename base object test, reorg state test, add test for fetch clobbering state when missing from URL --- test/unit/specs/factories/base_object.js | 62 +++++++++--------- test/unit/specs/state_management/state.js | 76 +++++++++++++++-------- 2 files changed, 79 insertions(+), 59 deletions(-) diff --git a/test/unit/specs/factories/base_object.js b/test/unit/specs/factories/base_object.js index 09158d4d4e8b..29b3a3b03024 100644 --- a/test/unit/specs/factories/base_object.js +++ b/test/unit/specs/factories/base_object.js @@ -7,42 +7,40 @@ define(function (require) { // Load kibana require('index'); - describe('State Management', function () { - describe('BaseObject', function () { - var $rootScope; - var BaseObject; + describe('Base Object', function () { + var $rootScope; + var BaseObject; - beforeEach(function () { - module('kibana'); + beforeEach(function () { + module('kibana'); - inject(function (_$rootScope_, Private) { - $rootScope = _$rootScope_; - BaseObject = Private(require('factories/base_object')); - }); + inject(function (_$rootScope_, Private) { + $rootScope = _$rootScope_; + BaseObject = Private(require('factories/base_object')); }); - - it('should take an inital set of values', function () { - var baseObject = new BaseObject({ message: 'test' }); - expect(baseObject).to.have.property('message', 'test'); - }); - - it('should serialize _attributes to RISON', function () { - var baseObject = new BaseObject(); - baseObject.message = 'Testing... 1234'; - var rison = baseObject.toRISON(); - expect(rison).to.equal('(message:\'Testing... 1234\')'); - }); - - it('should serialize _attributes for JSON', function () { - var baseObject = new BaseObject(); - baseObject.message = 'Testing... 1234'; - baseObject._private = 'foo'; - baseObject.$private = 'stuff'; - var json = JSON.stringify(baseObject); - expect(json).to.equal('{"message":"Testing... 1234"}'); - }); - }); + + it('should take an inital set of values', function () { + var baseObject = new BaseObject({ message: 'test' }); + expect(baseObject).to.have.property('message', 'test'); + }); + + it('should serialize _attributes to RISON', function () { + var baseObject = new BaseObject(); + baseObject.message = 'Testing... 1234'; + var rison = baseObject.toRISON(); + expect(rison).to.equal('(message:\'Testing... 1234\')'); + }); + + it('should serialize _attributes for JSON', function () { + var baseObject = new BaseObject(); + baseObject.message = 'Testing... 1234'; + baseObject._private = 'foo'; + baseObject.$private = 'stuff'; + var json = JSON.stringify(baseObject); + expect(json).to.equal('{"message":"Testing... 1234"}'); + }); + }); }); diff --git a/test/unit/specs/state_management/state.js b/test/unit/specs/state_management/state.js index 4298ff154adc..a6ada2d84d98 100644 --- a/test/unit/specs/state_management/state.js +++ b/test/unit/specs/state_management/state.js @@ -1,5 +1,4 @@ define(function (require) { - var angular = require('angular'); var _ = require('lodash'); var sinon = require('sinon/sinon'); require('services/private'); @@ -8,26 +7,41 @@ define(function (require) { require('index'); describe('State Management', function () { - describe('State', function () { + var $rootScope, $location, State, Events; - var $rootScope, $location, State, Events; + beforeEach(function () { + module('kibana'); - beforeEach(function () { - module('kibana'); + inject(function (_$rootScope_, _$location_, Private) { + $location = _$location_; + $rootScope = _$rootScope_; + State = Private(require('components/state_management/state')); + Events = Private(require('factories/events')); + }); + }); - inject(function (_$rootScope_, _$location_, Private) { - $location = _$location_; - $rootScope = _$rootScope_; - State = Private(require('components/state_management/state')); - Events = Private(require('factories/events')); - }); + describe('Provider', function () { + it('should reset the state to the defaults', function () { + var state = new State('_s', { message: ['test'] }); + state.reset(); + var search = $location.search(); + expect(search).to.have.property('_s'); + expect(search._s).to.equal('(message:!(test))'); + expect(state.message).to.eql(['test']); + }); + + it('should apply the defaults upon initialization', function () { + var state = new State('_s', { message: 'test' }); + expect(state).to.have.property('message', 'test'); }); it('should inherit from Events', function () { var state = new State(); expect(state).to.be.an(Events); }); + }); + describe('Search', function () { it('should save to $location.search()', function () { var state = new State('_s', { test: 'foo' }); state.save(); @@ -47,8 +61,9 @@ define(function (require) { var search = $location.search(); $rootScope.$apply(); }); + }); - + describe('Fetch', function () { it('should emit an event if changes are fetched', function (done) { var state = new State(); state.on('fetch_with_changes', function (keys) { @@ -91,20 +106,6 @@ define(function (require) { expect(state).to.have.property('message', 'test'); }); - it('should reset the state to the defaults', function () { - var state = new State('_s', { message: ['test'] }); - state.reset(); - var search = $location.search(); - expect(search).to.have.property('_s'); - expect(search._s).to.equal('(message:!(test))'); - expect(state.message).to.eql(['test']); - }); - - it('should apply the defaults upon initialization', function () { - var state = new State('_s', { message: 'test' }); - expect(state).to.have.property('message', 'test'); - }); - it('should call fetch when $routeUpdate is fired on $rootScope', function () { var state = new State(); var spy = sinon.spy(state, 'fetch'); @@ -112,7 +113,28 @@ define(function (require) { sinon.assert.calledOnce(spy); }); + it('should clear state when missing form URL', function () { + var stateObj; + var state = new State(); + + // set satte via URL + $location.search({ _s: '(foo:(bar:baz))' }); + state.fetch(); + stateObj = state.toObject(); + expect(stateObj).to.eql({ foo: { bar: 'baz' } }); + + // ensure changing URL changes state + $location.search({ _s: '(one:two)' }); + state.fetch(); + stateObj = state.toObject(); + expect(stateObj).to.eql({ one: 'two' }); + + // remove search, state should be empty + $location.search({}); + state.fetch(); + stateObj = state.toObject(); + expect(stateObj).to.eql({}); + }); }); }); - }); From 2b479b72c5707ed4c17d7073366c50ba9b84cb53 Mon Sep 17 00:00:00 2001 From: Joe Fleming Date: Thu, 28 Aug 2014 16:24:05 -0700 Subject: [PATCH 4/4] add global state test to ensure state is persisted from memory when missing from URL --- test/unit/index.html | 1 + .../specs/state_management/global_state.js | 37 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 test/unit/specs/state_management/global_state.js diff --git a/test/unit/index.html b/test/unit/index.html index f74d632cbf45..b6c293282c00 100644 --- a/test/unit/index.html +++ b/test/unit/index.html @@ -81,6 +81,7 @@ 'specs/courier/search_source/_get_normalized_sort', 'specs/factories/base_object', 'specs/state_management/state', + 'specs/state_management/global_state', 'specs/utils/diff_object', 'specs/utils/diff_time_picker_vals', 'specs/factories/events', diff --git a/test/unit/specs/state_management/global_state.js b/test/unit/specs/state_management/global_state.js new file mode 100644 index 000000000000..601a4178c696 --- /dev/null +++ b/test/unit/specs/state_management/global_state.js @@ -0,0 +1,37 @@ +define(function (require) { + var sinon = require('sinon/sinon'); + require('components/state_management/global_state'); + + // Load kibana + require('index'); + + describe('State Management', function () { + var $rootScope, $location, state; + + beforeEach(function () { + module('kibana'); + + inject(function (_$location_, globalState) { + $location = _$location_; + state = globalState; + }); + }); + + describe('Global State', function () { + it('should use previous state when not in URL', function () { + // set satte via URL + $location.search({ _g: '(foo:(bar:baz))' }); + state.fetch(); + expect(state.toObject()).to.eql({ foo: { bar: 'baz' } }); + + $location.search({ _g: '(fizz:buzz)' }); + state.fetch(); + expect(state.toObject()).to.eql({ fizz: 'buzz' }); + + $location.search({}); + state.fetch(); + expect(state.toObject()).to.eql({ fizz: 'buzz' }); + }); + }); + }); +});