From 5379ca241a98e56b2bd87492e80cf67a0d83207e Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 10:04:33 -0700 Subject: [PATCH 1/8] Handle errors in the application route This is the only way to preserve the URL in all cases. --- ui/app/controllers/application.js | 35 +++++++++++++++++++++++++++++++ ui/app/routes/application.js | 10 +++++++++ ui/app/templates/application.hbs | 19 ++++++++++++++++- 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 ui/app/controllers/application.js diff --git a/ui/app/controllers/application.js b/ui/app/controllers/application.js new file mode 100644 index 000000000..0516a7090 --- /dev/null +++ b/ui/app/controllers/application.js @@ -0,0 +1,35 @@ +import Ember from 'ember'; + +const { Controller, computed } = Ember; + +export default Controller.extend({ + error: null, + + errorStr: computed('error', function() { + return this.get('error').toString(); + }), + + errorCodes: computed('error', function() { + const error = this.get('error'); + const codes = [error.code]; + + if (error.errors) { + error.errors.forEach(err => { + codes.push(err.status); + }); + } + + return codes + .compact() + .uniq() + .map(code => '' + code); + }), + + is404: computed('errorCodes.[]', function() { + return this.get('errorCodes').includes('404'); + }), + + is500: computed('errorCodes.[]', function() { + return this.get('errorCodes').includes('500'); + }), +}); diff --git a/ui/app/routes/application.js b/ui/app/routes/application.js index 7197f1819..1b6fe6335 100644 --- a/ui/app/routes/application.js +++ b/ui/app/routes/application.js @@ -3,9 +3,19 @@ import Ember from 'ember'; const { Route } = Ember; export default Route.extend({ + resetController(controller, isExiting) { + if (isExiting) { + controller.set('error', null); + } + }, + actions: { didTransition() { window.scrollTo(0, 0); }, + + error(error) { + this.controllerFor('application').set('error', error); + }, }, }); diff --git a/ui/app/templates/application.hbs b/ui/app/templates/application.hbs index dceb76dfa..0dfba7e6e 100644 --- a/ui/app/templates/application.hbs +++ b/ui/app/templates/application.hbs @@ -1,2 +1,19 @@ {{partial "svg-patterns"}} -{{outlet}} +{{#unless error}} + {{outlet}} +{{else}} +
+
+ {{#if is500}} +

Server Error

+

A server error prevented data from being sent to the client.

+ {{else if is404}} +

Not Found

+

What you're looking for couldn't be found. It either doesn't exist or you are not authorized to see it.

+ {{/if}} + {{#if (eq config.environment "development")}} + 
{{errorStr}}
+ {{/if}} +
+
+{{/unless}} From cb5d2204324aed2b65a8c10a02fd0fdda04fa433 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 10:05:18 -0700 Subject: [PATCH 2/8] Style error pages --- ui/app/styles/components.scss | 1 + ui/app/styles/components/error-container.scss | 22 +++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 ui/app/styles/components/error-container.scss diff --git a/ui/app/styles/components.scss b/ui/app/styles/components.scss index 50f6ef1c8..2a5e6cb78 100644 --- a/ui/app/styles/components.scss +++ b/ui/app/styles/components.scss @@ -2,6 +2,7 @@ @import "./components/boxed-section"; @import "./components/breadcrumbs"; @import "./components/empty-message"; +@import "./components/error-container"; @import "./components/gutter"; @import "./components/inline-definitions"; @import "./components/job-diff"; diff --git a/ui/app/styles/components/error-container.scss b/ui/app/styles/components/error-container.scss new file mode 100644 index 000000000..298d7fe52 --- /dev/null +++ b/ui/app/styles/components/error-container.scss @@ -0,0 +1,22 @@ +.error-container { + width: 100%; + height: 100%; + padding-top: 25vh; + display: flex; + justify-content: center; + background: $grey-lighter; + + .error-message { + max-width: 600px; + + .title, + .subtitle { + text-align: center; + } + } + + .error-stack-trace { + border: 1px solid $grey-light; + border-radius: $radius; + } +} From 1e817cb35bfb3ba61f1c8a0dd8086031f92dca9e Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 10:06:13 -0700 Subject: [PATCH 3/8] Handle 404s on jobs --- ui/app/routes/jobs/job.js | 4 +++- ui/app/utils/notify-error.js | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 ui/app/utils/notify-error.js diff --git a/ui/app/routes/jobs/job.js b/ui/app/routes/jobs/job.js index abe106775..5ae53a122 100644 --- a/ui/app/routes/jobs/job.js +++ b/ui/app/routes/jobs/job.js @@ -1,4 +1,5 @@ import Ember from 'ember'; +import notifyError from 'nomad-ui/utils/notify-error'; const { Route, inject } = Ember; @@ -10,6 +11,7 @@ export default Route.extend({ .find('job', job_id) .then(job => { return job.get('allocations').then(() => job); - }); + }) + .catch(notifyError(this)); }, }); diff --git a/ui/app/utils/notify-error.js b/ui/app/utils/notify-error.js new file mode 100644 index 000000000..3ca5f5987 --- /dev/null +++ b/ui/app/utils/notify-error.js @@ -0,0 +1,7 @@ +// An error handler to provide to a promise catch to set an error +// on the application controller. +export default function notifyError(route) { + return error => { + route.controllerFor('application').set('error', error); + }; +} From 1e9b6077300c856af5bfe7bdbb03e1d83c767a0a Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 10:06:24 -0700 Subject: [PATCH 4/8] Handle 404s on nodes --- ui/app/routes/nodes/node.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ui/app/routes/nodes/node.js b/ui/app/routes/nodes/node.js index 4e5511637..9571d975a 100644 --- a/ui/app/routes/nodes/node.js +++ b/ui/app/routes/nodes/node.js @@ -1,14 +1,19 @@ import Ember from 'ember'; +import notifyError from 'nomad-ui/utils/notify-error'; const { Route, inject } = Ember; export default Route.extend({ store: inject.service(), + model() { + return this._super(...arguments).catch(notifyError(this)); + }, + afterModel(model) { - if (model.get('isPartial')) { + if (model && model.get('isPartial')) { return model.reload().then(node => node.get('allocations')); } - return model.get('allocations'); + return model && model.get('allocations'); }, }); From ae6658dd941c4a32a0af8a6cb830c19dc822f4f2 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 10:47:02 -0700 Subject: [PATCH 5/8] Handle 404s for agents --- ui/app/routes/servers/server.js | 10 ++++++++++ ui/app/serializers/agent.js | 11 +++++++++++ 2 files changed, 21 insertions(+) create mode 100644 ui/app/routes/servers/server.js diff --git a/ui/app/routes/servers/server.js b/ui/app/routes/servers/server.js new file mode 100644 index 000000000..c4c67569f --- /dev/null +++ b/ui/app/routes/servers/server.js @@ -0,0 +1,10 @@ +import Ember from 'ember'; +import notifyError from 'nomad-ui/utils/notify-error'; + +const { Route } = Ember; + +export default Route.extend({ + model() { + return this._super(...arguments).catch(notifyError(this)); + }, +}); diff --git a/ui/app/serializers/agent.js b/ui/app/serializers/agent.js index e3cb04997..f54db7276 100644 --- a/ui/app/serializers/agent.js +++ b/ui/app/serializers/agent.js @@ -1,4 +1,5 @@ import ApplicationSerializer from './application'; +import { AdapterError } from 'ember-data/adapters/errors'; export default ApplicationSerializer.extend({ attrs: { @@ -8,6 +9,16 @@ export default ApplicationSerializer.extend({ }, normalize(typeHash, hash) { + if (!hash) { + // It's unusual to throw an adapter error from a serializer, + // but there is no single server end point so the serializer + // acts like the API in this case. + const error = new AdapterError([{ status: '404' }]); + + error.message = 'Requested Agent was not found in set of available Agents'; + throw error; + } + hash.ID = hash.Name; hash.Datacenter = hash.Tags && hash.Tags.dc; hash.Region = hash.Tags && hash.Tags.region; From ddf9dae545efda4f3f03d2d717cc44647073228d Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 10:59:59 -0700 Subject: [PATCH 6/8] Handle allocation 404s --- ui/app/mixins/with-model-error-handling.js | 10 ++++++++++ ui/app/routes/allocations/allocation.js | 6 ++++++ ui/app/routes/servers/server.js | 8 ++------ 3 files changed, 18 insertions(+), 6 deletions(-) create mode 100644 ui/app/mixins/with-model-error-handling.js create mode 100644 ui/app/routes/allocations/allocation.js diff --git a/ui/app/mixins/with-model-error-handling.js b/ui/app/mixins/with-model-error-handling.js new file mode 100644 index 000000000..585c61595 --- /dev/null +++ b/ui/app/mixins/with-model-error-handling.js @@ -0,0 +1,10 @@ +import Ember from 'ember'; +import notifyError from 'nomad-ui/utils/notify-error'; + +const { Mixin } = Ember; + +export default Mixin.create({ + model() { + return this._super(...arguments).catch(notifyError(this)); + }, +}); diff --git a/ui/app/routes/allocations/allocation.js b/ui/app/routes/allocations/allocation.js new file mode 100644 index 000000000..60eff663b --- /dev/null +++ b/ui/app/routes/allocations/allocation.js @@ -0,0 +1,6 @@ +import Ember from 'ember'; +import WithModelErrorHandling from 'nomad-ui/mixins/with-model-error-handling'; + +const { Route } = Ember; + +export default Route.extend(WithModelErrorHandling); diff --git a/ui/app/routes/servers/server.js b/ui/app/routes/servers/server.js index c4c67569f..60eff663b 100644 --- a/ui/app/routes/servers/server.js +++ b/ui/app/routes/servers/server.js @@ -1,10 +1,6 @@ import Ember from 'ember'; -import notifyError from 'nomad-ui/utils/notify-error'; +import WithModelErrorHandling from 'nomad-ui/mixins/with-model-error-handling'; const { Route } = Ember; -export default Route.extend({ - model() { - return this._super(...arguments).catch(notifyError(this)); - }, -}); +export default Route.extend(WithModelErrorHandling); From 1b82586fb6c4ead5ca9fe53edb5210e5639735ff Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 17:05:41 -0700 Subject: [PATCH 7/8] Test coverage for 404s on resources --- ui/tests/acceptance/allocation-detail-test.js | 21 +++++++++++++++++++ ui/tests/acceptance/client-detail-test.js | 21 +++++++++++++++++++ ui/tests/acceptance/job-detail-test.js | 21 +++++++++++++++++++ ui/tests/acceptance/server-detail-test.js | 18 +++++++++++++++- 4 files changed, 80 insertions(+), 1 deletion(-) diff --git a/ui/tests/acceptance/allocation-detail-test.js b/ui/tests/acceptance/allocation-detail-test.js index 51997cef6..a310b8a86 100644 --- a/ui/tests/acceptance/allocation-detail-test.js +++ b/ui/tests/acceptance/allocation-detail-test.js @@ -183,3 +183,24 @@ test('each recent event should list the time, type, and description of the event 'Event message' ); }); + +test('when the allocation is not found, an error message is shown, but the URL persists', function( + assert +) { + visit('/allocations/not-a-real-allocation'); + + andThen(() => { + assert.equal( + server.pretender.handledRequests.findBy('status', 404).url, + '/v1/allocation/not-a-real-allocation', + 'A request to the non-existent allocation is made' + ); + assert.equal(currentURL(), '/allocations/not-a-real-allocation', 'The URL persists'); + assert.ok(find('.error-message'), 'Error message is shown'); + assert.equal( + find('.error-message .title').textContent, + 'Not Found', + 'Error message is for 404' + ); + }); +}); diff --git a/ui/tests/acceptance/client-detail-test.js b/ui/tests/acceptance/client-detail-test.js index 78a8e66f4..01dd007d4 100644 --- a/ui/tests/acceptance/client-detail-test.js +++ b/ui/tests/acceptance/client-detail-test.js @@ -181,3 +181,24 @@ test('each allocation should link to the job the allocation belongs to', functio test('/nodes/:id should list all attributes for the node', function(assert) { assert.ok(find('.attributes-table'), 'Attributes table is on the page'); }); + +test('when the node is not found, an error message is shown, but the URL persists', function( + assert +) { + visit('/nodes/not-a-real-node'); + + andThen(() => { + assert.equal( + server.pretender.handledRequests.findBy('status', 404).url, + '/v1/node/not-a-real-node', + 'A request to the non-existent node is made' + ); + assert.equal(currentURL(), '/nodes/not-a-real-node', 'The URL persists'); + assert.ok(find('.error-message'), 'Error message is shown'); + assert.equal( + find('.error-message .title').textContent, + 'Not Found', + 'Error message is for 404' + ); + }); +}); diff --git a/ui/tests/acceptance/job-detail-test.js b/ui/tests/acceptance/job-detail-test.js index ba73c7d10..0c7ed178c 100644 --- a/ui/tests/acceptance/job-detail-test.js +++ b/ui/tests/acceptance/job-detail-test.js @@ -260,3 +260,24 @@ test('the active deployment section can be expanded to show task groups and allo ); }); }); + +test('when the job is not found, an error message is shown, but the URL persists', function( + assert +) { + visit('/jobs/not-a-real-job'); + + andThen(() => { + assert.equal( + server.pretender.handledRequests.findBy('status', 404).url, + '/v1/job/not-a-real-job', + 'A request to the non-existent job is made' + ); + assert.equal(currentURL(), '/jobs/not-a-real-job', 'The URL persists'); + assert.ok(find('.error-message'), 'Error message is shown'); + assert.equal( + find('.error-message .title').textContent, + 'Not Found', + 'Error message is for 404' + ); + }); +}); diff --git a/ui/tests/acceptance/server-detail-test.js b/ui/tests/acceptance/server-detail-test.js index 4b4af6673..2815df672 100644 --- a/ui/tests/acceptance/server-detail-test.js +++ b/ui/tests/acceptance/server-detail-test.js @@ -1,5 +1,5 @@ import Ember from 'ember'; -import { findAll, currentURL, visit } from 'ember-native-dom-helpers'; +import { find, findAll, currentURL, visit } from 'ember-native-dom-helpers'; import { test } from 'qunit'; import moduleForAcceptance from 'nomad-ui/tests/helpers/module-for-acceptance'; @@ -42,3 +42,19 @@ test('the active server should be denoted in the table', function(assert) { 'Active server matches current route' ); }); + +test('when the server is not found, an error message is shown, but the URL persists', function( + assert +) { + visit('/servers/not-a-real-server'); + + andThen(() => { + assert.equal(currentURL(), '/servers/not-a-real-server', 'The URL persists'); + assert.ok(find('.error-message'), 'Error message is shown'); + assert.equal( + find('.error-message .title').textContent, + 'Not Found', + 'Error message is for 404' + ); + }); +}); From 810c3d5467cb232099a7f1f7e539f8632e359802 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Sep 2017 17:18:28 -0700 Subject: [PATCH 8/8] Simple catch-all route for 404s on pages --- ui/app/router.js | 2 ++ ui/app/routes/not-found.js | 11 +++++++++++ 2 files changed, 13 insertions(+) create mode 100644 ui/app/routes/not-found.js diff --git a/ui/app/router.js b/ui/app/router.js index ee03341de..a63b46fc4 100644 --- a/ui/app/router.js +++ b/ui/app/router.js @@ -35,6 +35,8 @@ Router.map(function() { if (config.environment === 'development') { this.route('freestyle'); } + + this.route('not-found', { path: '/*' }); }); export default Router; diff --git a/ui/app/routes/not-found.js b/ui/app/routes/not-found.js new file mode 100644 index 000000000..1974ee9ba --- /dev/null +++ b/ui/app/routes/not-found.js @@ -0,0 +1,11 @@ +import Ember from 'ember'; + +const { Route, Error: EmberError } = Ember; + +export default Route.extend({ + model() { + const err = new EmberError('Page not found'); + err.code = '404'; + this.controllerFor('application').set('error', err); + }, +});