Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Region Switcher #4572

Merged
merged 32 commits into from
Aug 13, 2018
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ccae1fb
Bare minimum Mirage support for regions
DingoEatingFuzz Aug 2, 2018
dc1a031
Add three-way region property (query param, service, localStorage)
DingoEatingFuzz Aug 2, 2018
27308a8
Styles for the region switcher
DingoEatingFuzz Aug 3, 2018
200b3c3
Add the region qp to every api request
DingoEatingFuzz Aug 4, 2018
2e26a61
Reset the system service when unloading the store
DingoEatingFuzz Aug 4, 2018
840069d
Harden up the system service for the event of store unloading
DingoEatingFuzz Aug 4, 2018
8d36ac8
Fetch regions and namespaces in the application route
DingoEatingFuzz Aug 4, 2018
8c3e5b2
Only show the region switcher when there are multiple regions
DingoEatingFuzz Aug 4, 2018
47210b3
Add region switcher to the global header
DingoEatingFuzz Aug 6, 2018
3f26214
Move the region switcher out of the secondary nav and into the gutter…
DingoEatingFuzz Aug 6, 2018
610db7c
Make the dropdown ever so slightly off-white
DingoEatingFuzz Aug 6, 2018
95fcbda
Line breadcrumbs up flush with section content
DingoEatingFuzz Aug 6, 2018
d77504c
Never show the menu divider for the first menu item
DingoEatingFuzz Aug 6, 2018
134ab34
Remove the gutter menu from the allocations page
DingoEatingFuzz Aug 6, 2018
7e6f02d
Account for the service:system dependency due to region
DingoEatingFuzz Aug 7, 2018
4715696
Clear up the data flow for namespaces
DingoEatingFuzz Aug 7, 2018
a5da73d
Repeat the new namespace pattern for region
DingoEatingFuzz Aug 8, 2018
61bdc3b
Sidestep a transpilation bug.
DingoEatingFuzz Aug 8, 2018
4473bb9
The application route doesn't need to fetch namespaces
DingoEatingFuzz Aug 8, 2018
c5439df
Add the region qp to all requests made through the token service
DingoEatingFuzz Aug 8, 2018
d690709
Get the server's region (aka default region) from the API
DingoEatingFuzz Aug 9, 2018
ae0bf90
Only deal with the region param (in the app and in api calls) when ne…
DingoEatingFuzz Aug 9, 2018
fe315fe
Address an issue with certain dependent keys
DingoEatingFuzz Aug 9, 2018
822d998
Handle errors when getting regions or the default regions
DingoEatingFuzz Aug 9, 2018
b767a87
Update tests to handle region switching
DingoEatingFuzz Aug 9, 2018
362aff0
Simplify the control flow around changing namespaces and regions
DingoEatingFuzz Aug 10, 2018
c55df8f
Specify the request type for token self
DingoEatingFuzz Aug 10, 2018
dbc18c9
Properly model regions in Mirage
DingoEatingFuzz Aug 10, 2018
ae464a0
Unit test coverage for adding the region param to requests
DingoEatingFuzz Aug 10, 2018
95e8259
Acceptance tests for the region switcher
DingoEatingFuzz Aug 10, 2018
3192267
Use the model hook and setupController hook instead of afterModel
DingoEatingFuzz Aug 13, 2018
a61ad7a
List the new region mirage env var in the environment file
DingoEatingFuzz Aug 13, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ui/app/adapters/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const namespace = 'v1';
export default RESTAdapter.extend({
namespace,

system: service(),
token: service(),

headers: computed('token.secret', function() {
Expand All @@ -35,6 +36,17 @@ export default RESTAdapter.extend({
});
},

ajaxOptions(url, type, options = {}) {
options.data || (options.data = {});
if (this.get('system.shouldIncludeRegion')) {
const region = this.get('system.activeRegion');
if (region) {
options.data.region = region;
}
}
return this._super(url, type, options);
},

// In order to remove stale records from the store, findHasMany has to unload
// all records related to the request in question.
findHasMany(store, snapshot, link, relationship) {
Expand Down
2 changes: 1 addition & 1 deletion ui/app/adapters/token.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export default ApplicationAdapter.extend({
namespace: namespace + '/acl',

findSelf() {
return this.ajax(`${this.buildURL()}/token/self`).then(token => {
return this.ajax(`${this.buildURL()}/token/self`, 'GET').then(token => {
const store = this.get('store');
store.pushPayload('token', {
tokens: [token],
Expand Down
2 changes: 2 additions & 0 deletions ui/app/components/global-header.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import Component from '@ember/component';

export default Component.extend({
'data-test-global-header': true,

onHamburgerClick() {},
});
21 changes: 21 additions & 0 deletions ui/app/components/region-switcher.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import Component from '@ember/component';
import { computed } from '@ember/object';
import { inject as service } from '@ember/service';

export default Component.extend({
system: service(),
router: service(),
store: service(),

sortedRegions: computed('system.regions', function() {
return this.get('system.regions')
.toArray()
.sort();
}),

gotoRegion(region) {
this.get('router').transitionTo('jobs', {
queryParams: { region },
});
},
});
7 changes: 7 additions & 0 deletions ui/app/controllers/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ import codesForError from '../utils/codes-for-error';

export default Controller.extend({
config: service(),
system: service(),

queryParams: {
region: 'region',
},

region: null,

error: null,

Expand Down
24 changes: 0 additions & 24 deletions ui/app/controllers/jobs.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { inject as service } from '@ember/service';
import Controller from '@ember/controller';
import { observer } from '@ember/object';
import { run } from '@ember/runloop';

export default Controller.extend({
system: service(),
Expand All @@ -13,26 +11,4 @@ export default Controller.extend({
isForbidden: false,

jobNamespace: 'default',

// The namespace query param should act as an alias to the system active namespace.
// But query param defaults can't be CPs: https://github.com/emberjs/ember.js/issues/9819
syncNamespaceService: forwardNamespace('jobNamespace', 'system.activeNamespace'),
syncNamespaceParam: forwardNamespace('system.activeNamespace', 'jobNamespace'),
});

function forwardNamespace(source, destination) {
return observer(source, `${source}.id`, function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't make it all the way through the branch, but I just wanted to throw out a 👍 for the afterSetup route hook approach you originally took to replace this observer:

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one was that? I have already suppressed all memories of this code 😅

const newNamespace = this.get(`${source}.id`) || this.get(source);
const currentNamespace = this.get(`${destination}.id`) || this.get(destination);
const bothAreDefault =
(currentNamespace == undefined || currentNamespace === 'default') &&
(newNamespace == undefined || newNamespace === 'default');

if (currentNamespace !== newNamespace && !bothAreDefault) {
this.set(destination, newNamespace);
run.next(() => {
this.send('refreshRoute');
});
}
});
}
26 changes: 11 additions & 15 deletions ui/app/controllers/jobs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,17 @@ export default Controller.extend(Sortable, Searchable, {
Filtered jobs are those that match the selected namespace and aren't children
of periodic or parameterized jobs.
*/
filteredJobs: computed(
'model.[]',
'model.@each.parent',
'system.activeNamespace',
'system.namespaces.length',
function() {
const hasNamespaces = this.get('system.namespaces.length');
const activeNamespace = this.get('system.activeNamespace.id') || 'default';

return this.get('model')
.compact()
.filter(job => !hasNamespaces || job.get('namespace.id') === activeNamespace)
.filter(job => !job.get('parent.content'));
}
),
filteredJobs: computed('model.[]', 'model.@each.parent', function() {
// Namespace related properties are ommitted from the dependent keys
// due to a prop invalidation bug caused by region switching.
const hasNamespaces = this.get('system.namespaces.length');
const activeNamespace = this.get('system.activeNamespace.id') || 'default';

return this.get('model')
.compact()
.filter(job => !hasNamespaces || job.get('namespace.id') === activeNamespace)
.filter(job => !job.get('parent.content'));
}),

listToSort: alias('filteredJobs'),
listToSearch: alias('listSorted'),
Expand Down
2 changes: 2 additions & 0 deletions ui/app/controllers/settings/tokens.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getOwner } from '@ember/application';

export default Controller.extend({
token: service(),
system: service(),
store: service(),

secret: reads('token.secret'),
Expand Down Expand Up @@ -43,6 +44,7 @@ export default Controller.extend({

// Clear out all data to ensure only data the new token is privileged to
// see is shown
this.get('system').reset();
this.resetStore();

// Immediately refetch the token now that the store is empty
Expand Down
52 changes: 52 additions & 0 deletions ui/app/routes/application.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,68 @@
import { inject as service } from '@ember/service';
import { next } from '@ember/runloop';
import Route from '@ember/routing/route';
import { AbortError } from 'ember-data/adapters/errors';
import RSVP from 'rsvp';

export default Route.extend({
config: service(),
system: service(),
store: service(),

queryParams: {
region: {
refreshModel: true,
},
},

resetController(controller, isExiting) {
if (isExiting) {
controller.set('error', null);
}
},

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 defaultRegion = this.get('system.defaultRegion.region');
const currentRegion = this.get('system.activeRegion') || defaultRegion;

// 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;
}
);
},

// setupController doesn't refire when the model hook refires as part of
// a query param change
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😳

Is that because the model hook is missing from this route? My understanding of the rule was that setupController fires on entering the route, and when model returns a truthy value.

If you had a trivial model hook, maybe that would cause setupController to fire?

model(params) {
  return params.region;
},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Innnnterresting. I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works 🙌

afterModel(model, transition) {
const queryParam = transition.queryParams.region;
const controller = this.controllerFor('application');

// 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);
});
}

return this._super(...arguments);
},

actions: {
didTransition() {
if (!this.get('config.isTest')) {
Expand Down
36 changes: 13 additions & 23 deletions ui/app/routes/jobs.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { inject as service } from '@ember/service';
import Route from '@ember/routing/route';
import { run } from '@ember/runloop';
import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state';
import notifyForbidden from 'nomad-ui/utils/notify-forbidden';

Expand All @@ -15,34 +14,25 @@ export default Route.extend(WithForbiddenState, {
},
],

beforeModel() {
return this.get('system.namespaces');
},

model() {
return this.get('store')
.findAll('job', { reload: true })
.catch(notifyForbidden(this));
queryParams: {
jobNamespace: {
refreshModel: true,
},
},

syncToController(controller) {
const namespace = this.get('system.activeNamespace.id');
beforeModel(transition) {
return this.get('system.namespaces').then(namespaces => {
const queryParam = transition.queryParams.namespace;
this.set('system.activeNamespace', queryParam || 'default');

// The run next is necessary to let the controller figure
// itself out before updating QPs.
// See: https://github.com/emberjs/ember.js/issues/5465
run.next(() => {
if (namespace && namespace !== 'default') {
controller.set('jobNamespace', namespace);
} else {
controller.set('jobNamespace', 'default');
}
return namespaces;
});
},

setupController(controller) {
this.syncToController(controller);
return this._super(...arguments);
model() {
return this.get('store')
.findAll('job', { reload: true })
.catch(notifyForbidden(this));
},

actions: {
Expand Down
Loading