From 362aff0fab943c3edf6d7df8dba53919fa0096c6 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 9 Aug 2018 18:12:26 -0700 Subject: [PATCH] Simplify the control flow around changing namespaces and regions The UI will no longer try to redirect to the appropriate namespace or region if one is found in localStorage. Instead, it will assume that the lack of query param means the default namespaces or region is desired. --- ui/app/components/region-switcher.js | 9 +--- ui/app/routes/application.js | 48 ++++++++----------- ui/app/routes/jobs.js | 27 +---------- ui/app/services/system.js | 5 +- .../templates/components/region-switcher.hbs | 1 + 5 files changed, 26 insertions(+), 64 deletions(-) diff --git a/ui/app/components/region-switcher.js b/ui/app/components/region-switcher.js index af42ed9a9..41574a908 100644 --- a/ui/app/components/region-switcher.js +++ b/ui/app/components/region-switcher.js @@ -1,5 +1,4 @@ import Component from '@ember/component'; -import { next } from '@ember/runloop'; import { computed } from '@ember/object'; import { inject as service } from '@ember/service'; @@ -15,12 +14,8 @@ export default Component.extend({ }), gotoRegion(region) { - this.get('system').reset(); - this.get('store').unloadAll(); - next(() => { - this.get('router').transitionTo('jobs', { - queryParams: { region }, - }); + this.get('router').transitionTo('jobs', { + queryParams: { region }, }); }, }); diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index a6afcc386..6119eb4ce 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -7,6 +7,7 @@ import RSVP from 'rsvp'; export default Route.extend({ config: service(), system: service(), + store: service(), queryParams: { region: { @@ -20,37 +21,26 @@ export default Route.extend({ } }, - afterSetup(fn) { - this._afterSetups || (this._afterSetups = []); - this._afterSetups.push(fn); - }, - beforeModel(transition) { return RSVP.all([this.get('system.regions'), this.get('system.defaultRegion')]).then( promises => { if (!this.get('system.shouldShowRegions')) return promises; const queryParam = transition.queryParams.region; - const activeRegion = this.get('system.activeRegion'); const defaultRegion = this.get('system.defaultRegion.region'); + const currentRegion = this.get('system.activeRegion') || defaultRegion; - if (!queryParam && activeRegion !== defaultRegion) { - // No query param: use what is in local storage, fallback to defaultRegion - this.afterSetup(controller => { - controller.set('region', activeRegion); - }); - } else if (queryParam && queryParam !== activeRegion) { - // Query param: use the query param, set that value in local storage - this.set('system.activeRegion', queryParam); - if (queryParam === defaultRegion) { - // Query param === default: don't use the query param, set that value in local storage, - // and clear the controller value. - this.afterSetup(controller => { - controller.set('region', null); - }); - } + // Only reset the store if the region actually changed + if ( + (queryParam && queryParam !== currentRegion) || + (!queryParam && currentRegion !== defaultRegion) + ) { + this.get('system').reset(); + this.get('store').unloadAll(); } + this.set('system.activeRegion', queryParam || defaultRegion); + return promises; } ); @@ -58,14 +48,18 @@ export default Route.extend({ // setupController doesn't refire when the model hook refires as part of // a query param change - afterModel() { + afterModel(model, transition) { + const queryParam = transition.queryParams.region; const controller = this.controllerFor('application'); - next(() => { - (this._afterSetups || []).forEach(fn => { - fn(controller); + + // The default region shouldn't show up as a query param since + // it's superfluous. + if (queryParam === this.get('system.defaultRegion.region')) { + next(() => { + controller.set('region', null); }); - this._afterSetups = []; - }); + } + return this._super(...arguments); }, diff --git a/ui/app/routes/jobs.js b/ui/app/routes/jobs.js index a221735b6..495649106 100644 --- a/ui/app/routes/jobs.js +++ b/ui/app/routes/jobs.js @@ -1,5 +1,4 @@ import { inject as service } from '@ember/service'; -import { next } from '@ember/runloop'; import Route from '@ember/routing/route'; import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state'; import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; @@ -21,39 +20,15 @@ export default Route.extend(WithForbiddenState, { }, }, - afterSetup(fn) { - this._afterSetups || (this._afterSetups = []); - this._afterSetups.push(fn); - }, - beforeModel(transition) { return this.get('system.namespaces').then(namespaces => { const queryParam = transition.queryParams.namespace; - const activeNamespace = this.get('system.activeNamespace.id'); - - if (!queryParam && activeNamespace !== 'default') { - this.afterSetup(controller => { - controller.set('jobNamespace', activeNamespace); - }); - } else if (queryParam && queryParam !== activeNamespace) { - this.set('system.activeNamespace', queryParam); - } + this.set('system.activeNamespace', queryParam || 'default'); return namespaces; }); }, - afterModel() { - const controller = this.controllerFor('jobs'); - next(() => { - (this._afterSetups || []).forEach(fn => { - fn(controller); - }); - this._afterSetups = []; - }); - return this._super(...arguments); - }, - model() { return this.get('store') .findAll('job', { reload: true }) diff --git a/ui/app/services/system.js b/ui/app/services/system.js index da7aa10ca..7d6d21792 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -58,9 +58,6 @@ export default Service.extend({ return region; } - // If the region in localStorage is no longer in the cluster, it needs to - // be cleared from localStorage - // this.set('activeRegion', null); return null; }, set(key, value) { @@ -114,7 +111,7 @@ export default Service.extend({ return namespace; } - // If the namespace is localStorage is no longer in the cluster, it needs to + // If the namespace in localStorage is no longer in the cluster, it needs to // be cleared from localStorage this.set('activeNamespace', null); return this.get('namespaces').findBy('id', 'default'); diff --git a/ui/app/templates/components/region-switcher.hbs b/ui/app/templates/components/region-switcher.hbs index 36e9ad1e2..df2da1870 100644 --- a/ui/app/templates/components/region-switcher.hbs +++ b/ui/app/templates/components/region-switcher.hbs @@ -1,6 +1,7 @@ {{#if system.shouldShowRegions}} {{#power-select data-test-region-switcher + tagName="div" triggerClass=decoration options=sortedRegions selected=system.activeRegion