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: Error handling on all pages #4841

Merged
merged 5 commits into from
Nov 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion ui/app/routes/allocations/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import { jobCrumbs } from 'nomad-ui/utils/breadcrumb-utils';

export default Route.extend(WithWatchers, {
startWatchers(controller, model) {
controller.set('watcher', this.get('watch').perform(model));
if (model) {
controller.set('watcher', this.get('watch').perform(model));
}
},

// Allocation breadcrumbs extend from job / task group breadcrumbs
Expand Down
13 changes: 8 additions & 5 deletions ui/app/routes/allocations/allocation/task.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,19 @@ export default Route.extend({

model({ name }) {
const allocation = this.modelFor('allocations.allocation');
if (allocation) {
const task = allocation.get('states').findBy('name', name);

if (task) {
return task;
}
// If there is no allocation, then there is no task.
// Let the allocation route handle the 404 error.
if (!allocation) return;

const task = allocation.get('states').findBy('name', name);

if (!task) {
const err = new EmberError(`Task ${name} not found for allocation ${allocation.get('id')}`);
err.code = '404';
this.controllerFor('application').set('error', err);
}

return task;
},
});
2 changes: 1 addition & 1 deletion ui/app/routes/allocations/allocation/task/logs.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ import Route from '@ember/routing/route';
export default Route.extend({
model() {
const task = this._super(...arguments);
return task.get('allocation.node').then(() => task);
return task && task.get('allocation.node').then(() => task);
},
});
6 changes: 4 additions & 2 deletions ui/app/routes/clients/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ export default Route.extend(WithWatchers, {
},

startWatchers(controller, model) {
controller.set('watchModel', this.get('watch').perform(model));
controller.set('watchAllocations', this.get('watchAllocations').perform(model));
if (model) {
controller.set('watchModel', this.get('watch').perform(model));
controller.set('watchAllocations', this.get('watchAllocations').perform(model));
}
},

watch: watchRecord('node'),
Expand Down
6 changes: 4 additions & 2 deletions ui/app/routes/jobs/job/allocations.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import WithWatchers from 'nomad-ui/mixins/with-watchers';
export default Route.extend(WithWatchers, {
model() {
const job = this.modelFor('jobs.job');
return job.get('allocations').then(() => job);
return job && job.get('allocations').then(() => job);
},

startWatchers(controller, model) {
controller.set('watchAllocations', this.get('watchAllocations').perform(model));
if (model) {
controller.set('watchAllocations', this.get('watchAllocations').perform(model));
}
},

watchAllocations: watchRelationship('allocations'),
Expand Down
2 changes: 2 additions & 0 deletions ui/app/routes/jobs/job/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import Route from '@ember/routing/route';
export default Route.extend({
model() {
const job = this.modelFor('jobs.job');
if (!job) return;

return job.fetchRawDefinition().then(definition => ({
job,
definition,
Expand Down
8 changes: 5 additions & 3 deletions ui/app/routes/jobs/job/deployments.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import WithWatchers from 'nomad-ui/mixins/with-watchers';
export default Route.extend(WithWatchers, {
model() {
const job = this.modelFor('jobs.job');
return RSVP.all([job.get('deployments'), job.get('versions')]).then(() => job);
return job && RSVP.all([job.get('deployments'), job.get('versions')]).then(() => job);
},

startWatchers(controller, model) {
controller.set('watchDeployments', this.get('watchDeployments').perform(model));
controller.set('watchVersions', this.get('watchVersions').perform(model));
if (model) {
controller.set('watchDeployments', this.get('watchDeployments').perform(model));
controller.set('watchVersions', this.get('watchVersions').perform(model));
}
},

watchDeployments: watchRelationship('deployments'),
Expand Down
6 changes: 4 additions & 2 deletions ui/app/routes/jobs/job/evaluations.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import WithWatchers from 'nomad-ui/mixins/with-watchers';
export default Route.extend(WithWatchers, {
model() {
const job = this.modelFor('jobs.job');
return job.get('evaluations').then(() => job);
return job && job.get('evaluations').then(() => job);
},

startWatchers(controller, model) {
controller.set('watchEvaluations', this.get('watchEvaluations').perform(model));
if (model) {
controller.set('watchEvaluations', this.get('watchEvaluations').perform(model));
}
},

watchEvaluations: watchRelationship('evaluations'),
Expand Down
44 changes: 31 additions & 13 deletions ui/app/routes/jobs/job/task-group.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import Route from '@ember/routing/route';
import { collect } from '@ember/object/computed';
import EmberError from '@ember/error';
import { resolve } from 'rsvp';
import { watchRecord, watchRelationship } from 'nomad-ui/utils/properties/watch';
import WithWatchers from 'nomad-ui/mixins/with-watchers';
import { qpBuilder } from 'nomad-ui/utils/classes/query-params';
import notifyError from 'nomad-ui/utils/notify-error';

export default Route.extend(WithWatchers, {
breadcrumbs(model) {
Expand All @@ -20,27 +23,42 @@ export default Route.extend(WithWatchers, {
},

model({ name }) {
const job = this.modelFor('jobs.job');

// If there is no job, then there is no task group.
// Let the job route handle the 404.
if (!job) return;

// If the job is a partial (from the list request) it won't have task
// groups. Reload the job to ensure task groups are present.
return this.modelFor('jobs.job')
.reload()
.then(job => {
const reload = job.get('isPartial') ? job.reload() : resolve();
return reload
.then(() => {
const taskGroup = job.get('taskGroups').findBy('name', name);
if (!taskGroup) {
const err = new EmberError(`Task group ${name} for job ${job.get('name')} not found`);
err.code = '404';
throw err;
}

// Refresh job allocations before-hand (so page sort works on load)
return job
.hasMany('allocations')
.reload()
.then(() => {
return job.get('taskGroups').findBy('name', name);
});
});
.then(() => taskGroup);
})
.catch(notifyError(this));
},

startWatchers(controller, model) {
const job = model.get('job');
controller.set('watchers', {
job: this.get('watchJob').perform(job),
summary: this.get('watchSummary').perform(job.get('summary')),
allocations: this.get('watchAllocations').perform(job),
});
if (model) {
const job = model.get('job');
controller.set('watchers', {
job: this.get('watchJob').perform(job),
summary: this.get('watchSummary').perform(job.get('summary')),
allocations: this.get('watchAllocations').perform(job),
});
}
},

watchJob: watchRecord('job'),
Expand Down
6 changes: 4 additions & 2 deletions ui/app/routes/jobs/job/versions.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import WithWatchers from 'nomad-ui/mixins/with-watchers';
export default Route.extend(WithWatchers, {
model() {
const job = this.modelFor('jobs.job');
return job.get('versions').then(() => job);
return job && job.get('versions').then(() => job);
},

startWatchers(controller, model) {
controller.set('watcher', this.get('watchVersions').perform(model));
if (model) {
controller.set('watcher', this.get('watchVersions').perform(model));
}
},

watchVersions: watchRelationship('versions'),
Expand Down
15 changes: 15 additions & 0 deletions ui/tests/acceptance/job-allocations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,18 @@ test('when a search yields no results, the search box remains', function(assert)
assert.ok(Allocations.hasSearchBox, 'Search box is still shown');
});
});

test('when the job for the allocations is not found, an error message is shown, but the URL persists', function(assert) {
Allocations.visit({ id: 'not-a-real-job' });

andThen(() => {
assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
assert.equal(currentURL(), '/jobs/not-a-real-job/allocations', 'The URL persists');
assert.ok(Allocations.error.isPresent, 'Error message is shown');
assert.equal(Allocations.error.title, 'Not Found', 'Error message is for 404');
});
});
15 changes: 15 additions & 0 deletions ui/tests/acceptance/job-definition-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,18 @@ test('when changes are submitted, the site redirects to the job overview page',
assert.equal(currentURL(), `/jobs/${job.id}`, 'Now on the job overview page');
});
});

test('when the job for the definition is not found, an error message is shown, but the URL persists', function(assert) {
Definition.visit({ id: 'not-a-real-job' });

andThen(() => {
assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
assert.equal(currentURL(), '/jobs/not-a-real-job/definition', 'The URL persists');
assert.ok(Definition.error.isPresent, 'Error message is shown');
assert.equal(Definition.error.title, 'Not Found', 'Error message is for 404');
});
});
15 changes: 15 additions & 0 deletions ui/tests/acceptance/job-deployments-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,21 @@ test('when open, a deployment shows a list of all allocations for the deployment
});
});

test('when the job for the deployments is not found, an error message is shown, but the URL persists', function(assert) {
Deployments.visit({ id: 'not-a-real-job' });

andThen(() => {
assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
assert.equal(currentURL(), '/jobs/not-a-real-job/deployments', 'The URL persists');
assert.ok(Deployments.error.isPresent, 'Error message is shown');
assert.equal(Deployments.error.title, 'Not Found', 'Error message is for 404');
});
});

function classForStatus(status) {
const classMap = {
running: 'is-running',
Expand Down
15 changes: 15 additions & 0 deletions ui/tests/acceptance/job-evaluations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,18 @@ test('evaluations table is sortable', function(assert) {
});
});
});

test('when the job for the evaluations is not found, an error message is shown, but the URL persists', function(assert) {
Evaluations.visit({ id: 'not-a-real-job' });

andThen(() => {
assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
assert.equal(currentURL(), '/jobs/not-a-real-job/evaluations', 'The URL persists');
assert.ok(Evaluations.error.isPresent, 'Error message is shown');
assert.equal(Evaluations.error.title, 'Not Found', 'Error message is for 404');
});
});
15 changes: 15 additions & 0 deletions ui/tests/acceptance/job-versions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,18 @@ test('each version mentions the version number, the stability, and the submitted
assert.equal(versionRow.stability, version.stable.toString(), 'Stability');
assert.equal(versionRow.submitTime, formattedSubmitTime, 'Submit time');
});

test('when the job for the versions is not found, an error message is shown, but the URL persists', function(assert) {
Versions.visit({ id: 'not-a-real-job' });

andThen(() => {
assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
assert.equal(currentURL(), '/jobs/not-a-real-job/versions', 'The URL persists');
assert.ok(Versions.error.isPresent, 'Error message is shown');
assert.equal(Versions.error.title, 'Not Found', 'Error message is for 404');
});
});
32 changes: 32 additions & 0 deletions ui/tests/acceptance/task-group-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,35 @@ test('when the allocation has reschedule events, the allocation row is denoted w
assert.ok(rescheduleRow.rescheduled, 'Reschedule row has a reschedule icon');
assert.notOk(normalRow.rescheduled, 'Normal row has no reschedule icon');
});

test('when the job for the task group is not found, an error message is shown, but the URL persists', function(assert) {
TaskGroup.visit({ id: 'not-a-real-job', name: 'not-a-real-task-group' });

andThen(() => {
assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
assert.equal(currentURL(), '/jobs/not-a-real-job/not-a-real-task-group', 'The URL persists');
assert.ok(TaskGroup.error.isPresent, 'Error message is shown');
assert.equal(TaskGroup.error.title, 'Not Found', 'Error message is for 404');
});
});

test('when the task group is not found on the job, an error message is shown, but the URL persists', function(assert) {
TaskGroup.visit({ id: job.id, name: 'not-a-real-task-group' });

andThen(() => {
assert.ok(
server.pretender.handledRequests
.filterBy('status', 200)
.mapBy('url')
.includes(`/v1/job/${job.id}`),
'A request to the job is made and succeeds'
);
assert.equal(currentURL(), `/jobs/${job.id}/not-a-real-task-group`, 'The URL persists');
assert.ok(TaskGroup.error.isPresent, 'Error message is shown');
assert.equal(TaskGroup.error.title, 'Not Found', 'Error message is for 404');
});
});
3 changes: 3 additions & 0 deletions ui/tests/pages/jobs/job/allocations.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from 'ember-cli-page-object';

import allocations from 'nomad-ui/tests/pages/components/allocations';
import error from 'nomad-ui/tests/pages/components/error';

export default create({
visit: visitable('/jobs/:id/allocations'),
Expand Down Expand Up @@ -37,4 +38,6 @@ export default create({
.findBy('id', id)
.sort();
},

error: error(),
});
3 changes: 3 additions & 0 deletions ui/tests/pages/jobs/job/definition.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { create, isPresent, visitable, clickable } from 'ember-cli-page-object';

import jobEditor from 'nomad-ui/tests/pages/components/job-editor';
import error from 'nomad-ui/tests/pages/components/error';

export default create({
visit: visitable('/jobs/:id/definition'),
Expand All @@ -9,4 +10,6 @@ export default create({
editor: jobEditor(),

edit: clickable('[data-test-edit-job]'),

error: error(),
});
3 changes: 3 additions & 0 deletions ui/tests/pages/jobs/job/deployments.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
} from 'ember-cli-page-object';

import allocations from 'nomad-ui/tests/pages/components/allocations';
import error from 'nomad-ui/tests/pages/components/error';

export default create({
visit: visitable('/jobs/:id/deployments'),
Expand Down Expand Up @@ -51,4 +52,6 @@ export default create({
...allocations('[data-test-deployment-allocation]'),
hasAllocations: isPresent('[data-test-deployment-allocations]'),
}),

error: error(),
});
4 changes: 4 additions & 0 deletions ui/tests/pages/jobs/job/evaluations.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { attribute, clickable, create, collection, text, visitable } from 'ember-cli-page-object';

import error from 'nomad-ui/tests/pages/components/error';

export default create({
visit: visitable('/jobs/:id/evaluations'),

Expand All @@ -18,4 +20,6 @@ export default create({
.findBy('id', id)
.sort();
},

error: error(),
});
Loading