From 6c98f1b1a2dc8412daeb423825519ec233ab0543 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 27 Jan 2021 16:39:13 -0800 Subject: [PATCH 1/4] Improve the message-from-adapter-error util Automatically detect ACL errors Provide a generic error message as a fallback --- ui/app/utils/message-from-adapter-error.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ui/app/utils/message-from-adapter-error.js b/ui/app/utils/message-from-adapter-error.js index 2b1ce864bacf..8641576ac2c7 100644 --- a/ui/app/utils/message-from-adapter-error.js +++ b/ui/app/utils/message-from-adapter-error.js @@ -1,6 +1,14 @@ +import { ForbiddenError } from '@ember-data/adapter/error'; + // Returns a single string based on the response the adapter received -export default function messageFromAdapterError(error) { - if (error.errors) { +export default function messageFromAdapterError(error, actionMessage) { + if (error instanceof ForbiddenError) { + return `Your ACL token does not grant permission to ${actionMessage}.`; + } + + if (error.errors?.length) { return error.errors.mapBy('detail').join('\n\n'); } + + return 'Unknown Error'; } From dbf826d7ef1e64431b727c65048b9d95426011d2 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Wed, 27 Jan 2021 16:40:51 -0800 Subject: [PATCH 2/4] Don't use generic ACL error messages When the error is actually a 403, an ACL error is appropriate, but when it isn't, fallback on what the API returns. --- ui/app/components/job-page/parts/latest-deployment.js | 9 +-------- ui/app/components/job-page/parts/title.js | 11 ++--------- ui/app/components/job-page/periodic.js | 5 +++-- ui/app/controllers/allocations/allocation/index.js | 5 +++-- .../controllers/allocations/allocation/task/index.js | 3 ++- 5 files changed, 11 insertions(+), 22 deletions(-) diff --git a/ui/app/components/job-page/parts/latest-deployment.js b/ui/app/components/job-page/parts/latest-deployment.js index dc78a235ed9d..d8cfdd0b6600 100644 --- a/ui/app/components/job-page/parts/latest-deployment.js +++ b/ui/app/components/job-page/parts/latest-deployment.js @@ -1,6 +1,5 @@ import Component from '@ember/component'; import { task } from 'ember-concurrency'; -import { ForbiddenError } from '@ember-data/adapter/error'; import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error'; import { tagName } from '@ember-decorators/component'; import classic from 'ember-classic-decorator'; @@ -18,15 +17,9 @@ export default class LatestDeployment extends Component { try { yield this.get('job.latestDeployment.content').promote(); } catch (err) { - let message = messageFromAdapterError(err); - - if (err instanceof ForbiddenError) { - message = 'Your ACL token does not grant permission to promote deployments.'; - } - this.handleError({ title: 'Could Not Promote Deployment', - description: message, + description: messageFromAdapterError(err, 'promote deployments'), }); } }) diff --git a/ui/app/components/job-page/parts/title.js b/ui/app/components/job-page/parts/title.js index 97dba19276f7..cd713938d171 100644 --- a/ui/app/components/job-page/parts/title.js +++ b/ui/app/components/job-page/parts/title.js @@ -1,6 +1,5 @@ import Component from '@ember/component'; import { task } from 'ember-concurrency'; -import { ForbiddenError } from '@ember-data/adapter/error'; import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error'; import { tagName } from '@ember-decorators/component'; import classic from 'ember-classic-decorator'; @@ -22,7 +21,7 @@ export default class Title extends Component { } catch (err) { this.handleError({ title: 'Could Not Stop Job', - description: 'Your ACL token does not grant permission to stop jobs.', + description: messageFromAdapterError(err, 'stop jobs'), }); } }) @@ -41,15 +40,9 @@ export default class Title extends Component { // Eagerly update the job status to avoid flickering job.set('status', 'running'); } catch (err) { - let message = messageFromAdapterError(err); - - if (err instanceof ForbiddenError) { - message = 'Your ACL token does not grant permission to stop jobs.'; - } - this.handleError({ title: 'Could Not Start Job', - description: message, + description: messageFromAdapterError(err, 'start jobs'), }); } }) diff --git a/ui/app/components/job-page/periodic.js b/ui/app/components/job-page/periodic.js index f3681858119b..622b18e40123 100644 --- a/ui/app/components/job-page/periodic.js +++ b/ui/app/components/job-page/periodic.js @@ -2,6 +2,7 @@ import AbstractJobPage from './abstract'; import { inject as service } from '@ember/service'; import { action } from '@ember/object'; import classic from 'ember-classic-decorator'; +import messageForError from 'nomad-ui/utils/message-from-adapter-error'; @classic export default class Periodic extends AbstractJobPage { @@ -11,10 +12,10 @@ export default class Periodic extends AbstractJobPage { @action forceLaunch() { - this.job.forcePeriodic().catch(() => { + this.job.forcePeriodic().catch(err => { this.set('errorMessage', { title: 'Could Not Force Launch', - description: 'Your ACL token does not grant permission to submit jobs.', + description: messageForError(err, 'submit jobs'), }); }); } diff --git a/ui/app/controllers/allocations/allocation/index.js b/ui/app/controllers/allocations/allocation/index.js index d70f311a7ebb..04771dbb1977 100644 --- a/ui/app/controllers/allocations/allocation/index.js +++ b/ui/app/controllers/allocations/allocation/index.js @@ -9,6 +9,7 @@ import { task } from 'ember-concurrency'; import Sortable from 'nomad-ui/mixins/sortable'; import { lazyClick } from 'nomad-ui/helpers/lazy-click'; import { watchRecord } from 'nomad-ui/utils/properties/watch'; +import messageForError from 'nomad-ui/utils/message-from-adapter-error'; import classic from 'ember-classic-decorator'; @classic @@ -73,7 +74,7 @@ export default class IndexController extends Controller.extend(Sortable) { } catch (err) { this.set('error', { title: 'Could Not Stop Allocation', - description: 'Your ACL token does not grant allocation lifecycle permissions.', + description: messageForError(err, 'manage allocation lifecycle'), }); } }) @@ -85,7 +86,7 @@ export default class IndexController extends Controller.extend(Sortable) { } catch (err) { this.set('error', { title: 'Could Not Restart Allocation', - description: 'Your ACL token does not grant allocation lifecycle permissions.', + description: messageForError(err, 'manage allocation lifecycle'), }); } }) diff --git a/ui/app/controllers/allocations/allocation/task/index.js b/ui/app/controllers/allocations/allocation/task/index.js index 57e661d1f656..73f0f208f4bc 100644 --- a/ui/app/controllers/allocations/allocation/task/index.js +++ b/ui/app/controllers/allocations/allocation/task/index.js @@ -2,6 +2,7 @@ import Controller from '@ember/controller'; import { computed as overridable } from 'ember-overridable-computed'; import { task } from 'ember-concurrency'; import classic from 'ember-classic-decorator'; +import messageForError from 'nomad-ui/utils/message-from-adapter-error'; @classic export default class IndexController extends Controller { @@ -21,7 +22,7 @@ export default class IndexController extends Controller { } catch (err) { this.set('error', { title: 'Could Not Restart Task', - description: 'Your ACL token does not grant allocation lifecycle permissions.', + description: messageForError(err, 'manage allocation lifecycle'), }); } }) From 5dfa55698337cc86850a042b604334dd45e04058 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Jan 2021 12:16:29 -0800 Subject: [PATCH 3/4] Test coverage for the messageFromAdapterError util --- .../utils/message-from-adapter-error-test.js | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 ui/tests/unit/utils/message-from-adapter-error-test.js diff --git a/ui/tests/unit/utils/message-from-adapter-error-test.js b/ui/tests/unit/utils/message-from-adapter-error-test.js new file mode 100644 index 000000000000..7336ee943ceb --- /dev/null +++ b/ui/tests/unit/utils/message-from-adapter-error-test.js @@ -0,0 +1,44 @@ +import { module, test } from 'qunit'; +import { ServerError, ForbiddenError } from '@ember-data/adapter/error'; +import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error'; + +const testCases = [ + { + name: 'Forbidden Error', + in: [new ForbiddenError([], "Can't do that"), 'run tests'], + out: 'Your ACL token does not grant permission to run tests.', + }, + { + name: 'Generic Error', + in: [new ServerError([{ detail: 'DB Max Connections' }], 'Server Error'), 'run tests'], + out: 'DB Max Connections', + }, + { + name: 'Multiple Errors', + in: [ + new ServerError( + [{ detail: 'DB Max Connections' }, { detail: 'Service timeout' }], + 'Server Error' + ), + 'run tests', + ], + out: 'DB Max Connections\n\nService timeout', + }, + { + name: 'Malformed Error (not from Ember Data which should always have an errors list)', + in: [new Error('Go boom'), 'handle custom error messages'], + out: 'Unknown Error', + }, +]; + +module('Unit | Util | messageFromAdapterError', function() { + testCases.forEach(testCase => { + test(testCase.name, function(assert) { + assert.equal( + messageFromAdapterError.apply(null, testCase.in), + testCase.out, + `[${testCase.in.join(', ')}] => ${testCase.out}` + ); + }); + }); +}); From 3aa4cf3f2d58499bd719a0b4e5faaa1b9e6ce80b Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 28 Jan 2021 13:47:22 -0800 Subject: [PATCH 4/4] Add an acceptance test exercising errors from an HTTP response to a notification --- ui/tests/acceptance/task-detail-test.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index 1d548095b31c..8a842c808ebd 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -223,7 +223,7 @@ module('Acceptance | task detail', function(hooks) { ); }); - test('when task restart fails, an error message is shown', async function(assert) { + test('when task restart fails (403), an ACL permissions error message is shown', async function(assert) { server.pretender.put('/v1/client/allocation/:id/restart', () => [403, {}, '']); await Task.restart.idle(); @@ -241,6 +241,22 @@ module('Acceptance | task detail', function(hooks) { assert.notOk(Task.inlineError.isShown, 'Inline error is no longer shown'); }); + test('when task restart fails (500), the error message from the API is piped through to the alert', async function(assert) { + const message = 'A plaintext error message'; + server.pretender.put('/v1/client/allocation/:id/restart', () => [500, {}, message]); + + await Task.restart.idle(); + await Task.restart.confirm(); + + assert.ok(Task.inlineError.isShown); + assert.ok(Task.inlineError.title.includes('Could Not Restart Task')); + assert.equal(Task.inlineError.message, message); + + await Task.inlineError.dismiss(); + + assert.notOk(Task.inlineError.isShown); + }); + test('exec button is present', async function(assert) { assert.ok(Task.execButton.isPresent); });