From 619376c3b85a4a82e3c795e590afa7f663a0e8eb Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 24 Aug 2018 13:17:45 -0700 Subject: [PATCH 1/5] Add Start Job action on the job overview page for when a job is dead --- ui/app/components/job-page/parts/title.js | 31 +++++++++++++++++++ .../components/job-page/parts/title.hbs | 8 +++++ 2 files changed, 39 insertions(+) diff --git a/ui/app/components/job-page/parts/title.js b/ui/app/components/job-page/parts/title.js index fb8e3232c00..a7f079705b7 100644 --- a/ui/app/components/job-page/parts/title.js +++ b/ui/app/components/job-page/parts/title.js @@ -1,4 +1,5 @@ import Component from '@ember/component'; +import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error'; export default Component.extend({ tagName: '', @@ -19,5 +20,35 @@ export default Component.extend({ }); }); }, + + startJob() { + const job = this.get('job'); + job + .fetchRawDefinition() + .then(definition => { + // A stopped job will have this Stop = true metadata + // If Stop is true when submitted to the cluster, the job + // won't transition from the Dead to Running state. + delete definition.Stop; + job.set('_newDefinition', JSON.stringify(definition)); + }) + .then(() => { + return job.parse(); + }) + .then(() => { + return job.update(); + }) + .catch(err => { + let message = messageFromAdapterError(err); + if (!message || message === 'Forbidden') { + message = 'Your ACL token does not grant permission to stop jobs.'; + } + + this.get('handleError')({ + title: 'Could Not Start Job', + description: message, + }); + }); + }, }, }); diff --git a/ui/app/templates/components/job-page/parts/title.hbs b/ui/app/templates/components/job-page/parts/title.hbs index 445640d760a..6bbd9f9b4b9 100644 --- a/ui/app/templates/components/job-page/parts/title.hbs +++ b/ui/app/templates/components/job-page/parts/title.hbs @@ -10,5 +10,13 @@ confirmText="Yes, Stop" confirmationMessage="Are you sure you want to stop this job?" onConfirm=(action "stopJob")}} + {{else}} + {{two-step-button + data-test-start + idleText="Start" + cancelText="Cancel" + confirmText="Yes, Start" + confirmationMessage="Are you sure you want to start this job?" + onConfirm=(action "startJob")}} {{/if}} From 1b481a3b72f6b1b6873d0185bd735d9530a583ae Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 24 Aug 2018 13:18:10 -0700 Subject: [PATCH 2/5] Test coverage for the Start Job behavior --- ui/tests/integration/job-page/helpers.js | 24 +++++++- .../integration/job-page/periodic-test.js | 57 ++++++++++++++++++- ui/tests/integration/job-page/service-test.js | 42 +++++++++++++- 3 files changed, 117 insertions(+), 6 deletions(-) diff --git a/ui/tests/integration/job-page/helpers.js b/ui/tests/integration/job-page/helpers.js index ef64e86af45..21deb51b17e 100644 --- a/ui/tests/integration/job-page/helpers.js +++ b/ui/tests/integration/job-page/helpers.js @@ -19,11 +19,31 @@ export function stopJob() { }); } -export function expectStopError(assert) { +export function startJob() { + click('[data-test-start] [data-test-idle-button]'); + return wait().then(() => { + click('[data-test-start] [data-test-confirm-button]'); + return wait(); + }); +} + +export function expectStartRequest(assert, server, job) { + const expectedURL = jobURL(job); + const request = server.pretender.handledRequests + .filterBy('method', 'POST') + .find(req => req.url === expectedURL); + + const requestPayload = JSON.parse(request.requestBody).Job; + + assert.ok(request, 'POST URL was made correctly'); + assert.ok(requestPayload.Stop == null, 'The Stop signal is not sent in the POST request'); +} + +export function expectError(assert, title) { return () => { assert.equal( find('[data-test-job-error-title]').textContent, - 'Could Not Stop Job', + title, 'Appropriate error is shown' ); assert.ok( diff --git a/ui/tests/integration/job-page/periodic-test.js b/ui/tests/integration/job-page/periodic-test.js index ba93d1a51ad..0908447f9d0 100644 --- a/ui/tests/integration/job-page/periodic-test.js +++ b/ui/tests/integration/job-page/periodic-test.js @@ -4,7 +4,14 @@ import { click, find, findAll } from 'ember-native-dom-helpers'; import wait from 'ember-test-helpers/wait'; import hbs from 'htmlbars-inline-precompile'; import { startMirage } from 'nomad-ui/initializers/ember-cli-mirage'; -import { jobURL, stopJob, expectStopError, expectDeleteRequest } from './helpers'; +import { + jobURL, + stopJob, + startJob, + expectError, + expectDeleteRequest, + expectStartRequest, +} from './helpers'; moduleForComponent('job-page/periodic', 'Integration | Component | job-page/periodic', { integration: true, @@ -167,5 +174,51 @@ test('Stopping a job without proper permissions shows an error message', functio return wait(); }) .then(stopJob) - .then(expectStopError(assert)); + .then(expectError(assert, 'Could Not Stop Job')); +}); + +test('Starting a job sends a post request for the job using the current definition', function(assert) { + let job; + + const mirageJob = this.server.create('job', 'periodic', { + childrenCount: 0, + createAllocations: false, + status: 'dead', + }); + this.store.findAll('job'); + + return wait() + .then(() => { + job = this.store.peekAll('job').findBy('plainId', mirageJob.id); + + this.setProperties(commonProperties(job)); + this.render(commonTemplate); + + return wait(); + }) + .then(startJob) + .then(() => expectStartRequest(assert, this.server, job)); +}); + +test('Starting a job without proper permissions shows an error message', function(assert) { + this.server.pretender.post('/v1/job/:id', () => [403, {}, null]); + + const mirageJob = this.server.create('job', 'periodic', { + childrenCount: 0, + createAllocations: false, + status: 'dead', + }); + this.store.findAll('job'); + + return wait() + .then(() => { + const job = this.store.peekAll('job').findBy('plainId', mirageJob.id); + + this.setProperties(commonProperties(job)); + this.render(commonTemplate); + + return wait(); + }) + .then(startJob) + .then(expectError(assert, 'Could Not Start Job')); }); diff --git a/ui/tests/integration/job-page/service-test.js b/ui/tests/integration/job-page/service-test.js index aef698a149c..ff29b3da996 100644 --- a/ui/tests/integration/job-page/service-test.js +++ b/ui/tests/integration/job-page/service-test.js @@ -4,7 +4,7 @@ import { test, moduleForComponent } from 'ember-qunit'; import wait from 'ember-test-helpers/wait'; import hbs from 'htmlbars-inline-precompile'; import { startMirage } from 'nomad-ui/initializers/ember-cli-mirage'; -import { stopJob, expectStopError, expectDeleteRequest } from './helpers'; +import { startJob, stopJob, expectError, expectDeleteRequest, expectStartRequest } from './helpers'; import Job from 'nomad-ui/tests/pages/jobs/detail'; moduleForComponent('job-page/service', 'Integration | Component | job-page/service', { @@ -88,7 +88,45 @@ test('Stopping a job without proper permissions shows an error message', functio return wait(); }) .then(stopJob) - .then(expectStopError(assert)); + .then(expectError(assert, 'Could Not Stop Job')); +}); + +test('Starting a job sends a post request for the job using the current definition', function(assert) { + let job; + + const mirageJob = makeMirageJob(this.server, { status: 'dead' }); + this.store.findAll('job'); + + return wait() + .then(() => { + job = this.store.peekAll('job').findBy('plainId', mirageJob.id); + + this.setProperties(commonProperties(job)); + this.render(commonTemplate); + + return wait(); + }) + .then(startJob) + .then(() => expectStartRequest(assert, this.server, job)); +}); + +test('Starting a job without proper permissions shows an error message', function(assert) { + this.server.pretender.post('/v1/job/:id', () => [403, {}, null]); + + const mirageJob = makeMirageJob(this.server, { status: 'dead' }); + this.store.findAll('job'); + + return wait() + .then(() => { + const job = this.store.peekAll('job').findBy('plainId', mirageJob.id); + + this.setProperties(commonProperties(job)); + this.render(commonTemplate); + + return wait(); + }) + .then(startJob) + .then(expectError(assert, 'Could Not Start Job')); }); test('Recent allocations shows allocations in the job context', function(assert) { From f09e9a41bc2baa102bbb227f22b5eaf7822cf1b4 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 24 Aug 2018 13:20:29 -0700 Subject: [PATCH 3/5] Switch stop/run job actions to EC tasks --- ui/app/components/job-page/parts/title.js | 76 +++++++++---------- .../components/job-page/parts/title.hbs | 4 +- 2 files changed, 36 insertions(+), 44 deletions(-) diff --git a/ui/app/components/job-page/parts/title.js b/ui/app/components/job-page/parts/title.js index a7f079705b7..d6b4dcbf50a 100644 --- a/ui/app/components/job-page/parts/title.js +++ b/ui/app/components/job-page/parts/title.js @@ -1,4 +1,5 @@ import Component from '@ember/component'; +import { task } from 'ember-concurrency'; import messageFromAdapterError from 'nomad-ui/utils/message-from-adapter-error'; export default Component.extend({ @@ -9,46 +10,37 @@ export default Component.extend({ handleError() {}, - actions: { - stopJob() { - this.get('job') - .stop() - .catch(() => { - this.get('handleError')({ - title: 'Could Not Stop Job', - description: 'Your ACL token does not grant permission to stop jobs.', - }); - }); - }, - - startJob() { - const job = this.get('job'); - job - .fetchRawDefinition() - .then(definition => { - // A stopped job will have this Stop = true metadata - // If Stop is true when submitted to the cluster, the job - // won't transition from the Dead to Running state. - delete definition.Stop; - job.set('_newDefinition', JSON.stringify(definition)); - }) - .then(() => { - return job.parse(); - }) - .then(() => { - return job.update(); - }) - .catch(err => { - let message = messageFromAdapterError(err); - if (!message || message === 'Forbidden') { - message = 'Your ACL token does not grant permission to stop jobs.'; - } - - this.get('handleError')({ - title: 'Could Not Start Job', - description: message, - }); - }); - }, - }, + stopJob: task(function*() { + try { + yield this.get('job').stop(); + } catch (err) { + this.get('handleError')({ + title: 'Could Not Stop Job', + description: 'Your ACL token does not grant permission to stop jobs.', + }); + } + }), + + startJob: task(function*() { + const job = this.get('job'); + const definition = yield job.fetchRawDefinition(); + + delete definition.Stop; + job.set('_newDefinition', JSON.stringify(definition)); + + try { + yield job.parse(); + yield job.update(); + } catch (err) { + let message = messageFromAdapterError(err); + if (!message || message === 'Forbidden') { + message = 'Your ACL token does not grant permission to stop jobs.'; + } + + this.get('handleError')({ + title: 'Could Not Start Job', + description: message, + }); + } + }), }); diff --git a/ui/app/templates/components/job-page/parts/title.hbs b/ui/app/templates/components/job-page/parts/title.hbs index 6bbd9f9b4b9..a7cf0187624 100644 --- a/ui/app/templates/components/job-page/parts/title.hbs +++ b/ui/app/templates/components/job-page/parts/title.hbs @@ -9,7 +9,7 @@ cancelText="Cancel" confirmText="Yes, Stop" confirmationMessage="Are you sure you want to stop this job?" - onConfirm=(action "stopJob")}} + onConfirm=(perform stopJob)}} {{else}} {{two-step-button data-test-start @@ -17,6 +17,6 @@ cancelText="Cancel" confirmText="Yes, Start" confirmationMessage="Are you sure you want to start this job?" - onConfirm=(action "startJob")}} + onConfirm=(perform startJob)}} {{/if}} From f79f0370968dd4e16c5742caba52904335f7c305 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 24 Aug 2018 13:41:14 -0700 Subject: [PATCH 4/5] Add a confirmation loading state to the two-step-button component --- ui/app/components/two-step-button.js | 1 + .../freestyle/sg-two-step-button.hbs | 18 +++++++++++++ .../templates/components/two-step-button.hbs | 25 ++++++++++++------ ui/tests/integration/two-step-button-test.js | 26 +++++++++++++++++++ 4 files changed, 62 insertions(+), 8 deletions(-) diff --git a/ui/app/components/two-step-button.js b/ui/app/components/two-step-button.js index 7016e96ec24..43152684117 100644 --- a/ui/app/components/two-step-button.js +++ b/ui/app/components/two-step-button.js @@ -8,6 +8,7 @@ export default Component.extend({ cancelText: '', confirmText: '', confirmationMessage: '', + awaitingConfirmation: false, onConfirm() {}, onCancel() {}, diff --git a/ui/app/templates/components/freestyle/sg-two-step-button.hbs b/ui/app/templates/components/freestyle/sg-two-step-button.hbs index f39096b0e1e..74f43919fbd 100644 --- a/ui/app/templates/components/freestyle/sg-two-step-button.hbs +++ b/ui/app/templates/components/freestyle/sg-two-step-button.hbs @@ -20,3 +20,21 @@ {{/freestyle-usage}} + +{{#freestyle-usage "two-step-button-loading" title="Two Step Button Loading State"}} +
+

+ This is a page title + {{two-step-button + idleText="Scary Action" + cancelText="Nvm" + confirmText="Yep" + confirmationMessage="Wait, really? Like...seriously?" + awaitingConfirmation=true + state="prompt"}} +

+
+{{/freestyle-usage}} +{{#freestyle-annotation}} + Note: the state property is internal state and only used here to bypass the idle state for demonstration purposes. +{{/freestyle-annotation}} diff --git a/ui/app/templates/components/two-step-button.hbs b/ui/app/templates/components/two-step-button.hbs index e9fe906b59e..cffa9dd7b10 100644 --- a/ui/app/templates/components/two-step-button.hbs +++ b/ui/app/templates/components/two-step-button.hbs @@ -4,16 +4,25 @@ {{else if isPendingConfirmation}} {{confirmationMessage}} - - {{/if}} diff --git a/ui/tests/integration/two-step-button-test.js b/ui/tests/integration/two-step-button-test.js index 41eff1e6e4a..12d23c1d738 100644 --- a/ui/tests/integration/two-step-button-test.js +++ b/ui/tests/integration/two-step-button-test.js @@ -13,6 +13,7 @@ const commonProperties = () => ({ cancelText: 'Cancel Action', confirmText: 'Confirm Action', confirmationMessage: 'Are you certain', + awaitingConfirmation: false, onConfirm: sinon.spy(), onCancel: sinon.spy(), }); @@ -23,6 +24,7 @@ const commonTemplate = hbs` cancelText=cancelText confirmText=confirmText confirmationMessage=confirmationMessage + awaitingConfirmation=awaitingConfirmation onConfirm=onConfirm onCancel=onCancel}} `; @@ -109,3 +111,27 @@ test('confirming the promptForConfirmation state calls the onConfirm hook and re }); }); }); + +test('when awaitingConfirmation is true, the cancel and submit buttons are disabled and the submit button is loading', function(assert) { + const props = commonProperties(); + props.awaitingConfirmation = true; + this.setProperties(props); + this.render(commonTemplate); + + click('[data-test-idle-button]'); + + return wait().then(() => { + assert.ok( + find('[data-test-cancel-button]').hasAttribute('disabled'), + 'The cancel button is disabled' + ); + assert.ok( + find('[data-test-confirm-button]').hasAttribute('disabled'), + 'The confirm button is disabled' + ); + assert.ok( + find('[data-test-confirm-button]').classList.contains('is-loading'), + 'The confirm button is in a loading state' + ); + }); +}); From f4ceb2264c272daa46128d6bf757fe6efecd44ca Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 24 Aug 2018 14:02:13 -0700 Subject: [PATCH 5/5] Fix the flickering issue with start/stop job When the server doesn't respond immediately, there is a visible window of time between the action being submitted and the blocking query coming back with the new job status. --- ui/app/components/job-page/parts/title.js | 7 ++++++- ui/app/components/two-step-button.js | 6 ++++++ ui/app/templates/components/job-page/parts/title.hbs | 2 ++ ui/app/templates/components/two-step-button.hbs | 5 +---- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/ui/app/components/job-page/parts/title.js b/ui/app/components/job-page/parts/title.js index d6b4dcbf50a..ba328462726 100644 --- a/ui/app/components/job-page/parts/title.js +++ b/ui/app/components/job-page/parts/title.js @@ -12,7 +12,10 @@ export default Component.extend({ stopJob: task(function*() { try { - yield this.get('job').stop(); + const job = this.get('job'); + yield job.stop(); + // Eagerly update the job status to avoid flickering + this.job.set('status', 'dead'); } catch (err) { this.get('handleError')({ title: 'Could Not Stop Job', @@ -31,6 +34,8 @@ export default Component.extend({ try { yield job.parse(); yield job.update(); + // Eagerly update the job status to avoid flickering + job.set('status', 'running'); } catch (err) { let message = messageFromAdapterError(err); if (!message || message === 'Forbidden') { diff --git a/ui/app/components/two-step-button.js b/ui/app/components/two-step-button.js index 43152684117..fe40c16f244 100644 --- a/ui/app/components/two-step-button.js +++ b/ui/app/components/two-step-button.js @@ -1,5 +1,6 @@ import Component from '@ember/component'; import { equal } from '@ember/object/computed'; +import RSVP from 'rsvp'; export default Component.extend({ classNames: ['two-step-button'], @@ -23,5 +24,10 @@ export default Component.extend({ promptForConfirmation() { this.set('state', 'prompt'); }, + confirm() { + RSVP.resolve(this.get('onConfirm')()).then(() => { + this.send('setToIdle'); + }); + }, }, }); diff --git a/ui/app/templates/components/job-page/parts/title.hbs b/ui/app/templates/components/job-page/parts/title.hbs index a7cf0187624..131b06e5627 100644 --- a/ui/app/templates/components/job-page/parts/title.hbs +++ b/ui/app/templates/components/job-page/parts/title.hbs @@ -9,6 +9,7 @@ cancelText="Cancel" confirmText="Yes, Stop" confirmationMessage="Are you sure you want to stop this job?" + awaitingConfirmation=stopJob.isRunning onConfirm=(perform stopJob)}} {{else}} {{two-step-button @@ -17,6 +18,7 @@ cancelText="Cancel" confirmText="Yes, Start" confirmationMessage="Are you sure you want to start this job?" + awaitingConfirmation=startJob.isRunning onConfirm=(perform startJob)}} {{/if}} diff --git a/ui/app/templates/components/two-step-button.hbs b/ui/app/templates/components/two-step-button.hbs index cffa9dd7b10..1b95184fea5 100644 --- a/ui/app/templates/components/two-step-button.hbs +++ b/ui/app/templates/components/two-step-button.hbs @@ -19,10 +19,7 @@ data-test-confirm-button class="button is-danger is-small {{if awaitingConfirmation "is-loading"}}" disabled={{awaitingConfirmation}} - onclick={{action (queue - (action "setToIdle") - (action onConfirm) - )}}> + onclick={{action "confirm"}}> {{confirmText}} {{/if}}