From 65b39de564fffd1fe0f2d00970584067df440983 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Thu, 8 Apr 2021 15:50:51 -0500 Subject: [PATCH 01/15] Add button to revert to an older version This is a start, much to be determined/done. --- ui/app/adapters/job-version.js | 21 ++++++++++++++++ ui/app/components/job-version.js | 15 ++++++++++++ ui/app/models/job-version.js | 4 +++ ui/app/templates/components/job-version.hbs | 16 ++++++++---- ui/mirage/config.js | 5 ++++ ui/tests/acceptance/job-versions-test.js | 27 +++++++++++++++++++++ ui/tests/pages/jobs/job/versions.js | 8 ++++++ 7 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 ui/app/adapters/job-version.js diff --git a/ui/app/adapters/job-version.js b/ui/app/adapters/job-version.js new file mode 100644 index 000000000000..c51763c187db --- /dev/null +++ b/ui/app/adapters/job-version.js @@ -0,0 +1,21 @@ +import ApplicationAdapter from './application'; +import addToPath from 'nomad-ui/utils/add-to-path'; + +export default class JobVersionAdapter extends ApplicationAdapter { + + revertTo(jobVersion) { + const jobAdapter = this.store.adapterFor('job'); + + const url = addToPath(jobAdapter.urlForFindRecord(jobVersion.get('job.id'), 'job'), '/revert'); + + // FIXME is there a better place for this? + const [jobName] = JSON.parse(jobVersion.get('job.id')); + + return this.ajax(url, 'POST', { + data: { + JobID: jobName, + JobVersion: jobVersion.number, + }, + }); + } +} diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index 25499648823d..add40cc081ed 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -1,6 +1,7 @@ import Component from '@ember/component'; import { action, computed } from '@ember/object'; import { classNames } from '@ember-decorators/component'; +import { task } from 'ember-concurrency'; import classic from 'ember-classic-decorator'; const changeTypes = ['Added', 'Deleted', 'Edited']; @@ -30,10 +31,24 @@ export default class JobVersion extends Component { ); } + @computed('version.{number,job.version}') + get isCurrent() { + return this.get('version.number') === this.get('version.job.version'); + } + @action toggleDiff() { this.toggleProperty('isOpen'); } + + @task(function*() { + try { + yield this.version.revertTo(); + } catch (e) { + // FIXME what is to be done + } + }) + revertTo; } const flatten = (accumulator, array) => accumulator.concat(array); diff --git a/ui/app/models/job-version.js b/ui/app/models/job-version.js index 155fb85a1ef1..cc37430073e7 100644 --- a/ui/app/models/job-version.js +++ b/ui/app/models/job-version.js @@ -7,4 +7,8 @@ export default class JobVersion extends Model { @attr('date') submitTime; @attr('number') number; @attr() diff; + + revertTo() { + return this.store.adapterFor('job-version').revertTo(this); + } } diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 6aa4647a46a0..23939c73d4a3 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -8,11 +8,17 @@ Submitted {{format-ts this.version.submitTime}} - {{#if this.version.diff}} - - {{else}} - No Changes - {{/if}} +
+ {{#if (not this.isCurrent)}} +   + {{/if}} + + {{#if this.version.diff}} + + {{else}} + No Changes + {{/if}} +
{{#if this.isOpen}}
diff --git a/ui/mirage/config.js b/ui/mirage/config.js index f739a353b690..6fe7baa65119 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -180,6 +180,11 @@ export default function() { return okEmpty(); }); + this.post('/job/:id/revert', function() { + // FIXME return something more realistic? + return okEmpty(); + }); + this.post('/job/:id/scale', function({ jobs }, { params }) { return this.serialize(jobs.find(params.id)); }); diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index cd2c224d6020..40b315671075 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -41,6 +41,33 @@ module('Acceptance | job versions', function(hooks) { assert.equal(versionRow.submitTime, formattedSubmitTime, 'Submit time'); }); + test('all versions but the current one have a button to revert to that version', async function(assert) { + let versionRowToRevertTo; + + Versions.versions.forEach((versionRow) => { + if (versionRow.number === job.version) { + assert.ok(versionRow.revertToButton.isHidden); + } else { + assert.ok(versionRow.revertToButton.isPresent); + + versionRowToRevertTo = versionRow; + } + }); + + if (versionRowToRevertTo) { + await versionRowToRevertTo.revertToButton.click(); + + const revertRequest = this.server.pretender.handledRequests.find(request => request.url.includes('revert')); + + assert.equal(revertRequest.url, `/v1/job/${job.id}/revert`); + + assert.deepEqual(JSON.parse(revertRequest.requestBody), { + JobID: job.id, + JobVersion: versionRowToRevertTo.number, + }); + } + }); + test('when the job for the versions is not found, an error message is shown, but the URL persists', async function(assert) { await Versions.visit({ id: 'not-a-real-job' }); diff --git a/ui/tests/pages/jobs/job/versions.js b/ui/tests/pages/jobs/job/versions.js index 1b58b9557f76..d1a312a9ffed 100644 --- a/ui/tests/pages/jobs/job/versions.js +++ b/ui/tests/pages/jobs/job/versions.js @@ -1,4 +1,5 @@ import { create, collection, text, visitable } from 'ember-cli-page-object'; +import { getter } from 'ember-cli-page-object/macros'; import error from 'nomad-ui/tests/pages/components/error'; @@ -9,6 +10,13 @@ export default create({ text: text(), stability: text('[data-test-version-stability]'), submitTime: text('[data-test-version-submit-time]'), + revertToButton: { + scope: '[data-test-revert-to-button]', + }, + + number: getter(function() { + return parseInt(this.text.match(/#(\d+)/)[1]); + }), }), error: error(), From 504aa817b7b03e5971d13f2e4b601b70705cfdb6 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Thu, 8 Apr 2021 15:55:25 -0500 Subject: [PATCH 02/15] Fix template linting errors --- ui/app/templates/components/job-version.hbs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 23939c73d4a3..84069274ce6e 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -9,9 +9,9 @@ {{format-ts this.version.submitTime}}
- {{#if (not this.isCurrent)}} -   - {{/if}} + {{#unless this.isCurrent}} +   + {{/unless}} {{#if this.version.diff}} From 1289d1f0ffc297adf13b7372b80a3bacd85c6180 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Mon, 12 Apr 2021 16:04:43 -0500 Subject: [PATCH 03/15] Add preliminary error-handling --- ui/app/components/job-version.js | 6 ++++- ui/app/controllers/jobs/job/versions.js | 12 ++++++++++ .../components/job-versions-stream.hbs | 2 +- ui/app/templates/jobs/job/versions.hbs | 16 +++++++++++++- ui/tests/acceptance/job-versions-test.js | 22 +++++++++++++++++++ ui/tests/pages/layout.js | 7 ++++++ 6 files changed, 62 insertions(+), 3 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index add40cc081ed..a800961c478a 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -2,6 +2,7 @@ import Component from '@ember/component'; import { action, computed } from '@ember/object'; import { classNames } from '@ember-decorators/component'; import { task } from 'ember-concurrency'; +import messageForError from 'nomad-ui/utils/message-from-adapter-error'; import classic from 'ember-classic-decorator'; const changeTypes = ['Added', 'Deleted', 'Edited']; @@ -45,7 +46,10 @@ export default class JobVersion extends Component { try { yield this.version.revertTo(); } catch (e) { - // FIXME what is to be done + this.handleError({ + title: 'Could Not Revert', + description: messageForError(e, 'revert'), + }); } }) revertTo; diff --git a/ui/app/controllers/jobs/job/versions.js b/ui/app/controllers/jobs/job/versions.js index ca1989177096..19d8e76c69a9 100644 --- a/ui/app/controllers/jobs/job/versions.js +++ b/ui/app/controllers/jobs/job/versions.js @@ -1,9 +1,21 @@ import Controller from '@ember/controller'; import WithNamespaceResetting from 'nomad-ui/mixins/with-namespace-resetting'; import { alias } from '@ember/object/computed'; +import { action } from '@ember/object'; import classic from 'ember-classic-decorator'; @classic export default class VersionsController extends Controller.extend(WithNamespaceResetting) { + error = null; + @alias('model') job; + + onDismiss() { + this.set('error', null); + } + + @action + handleError(errorObject) { + this.set('error', errorObject); + } } diff --git a/ui/app/templates/components/job-versions-stream.hbs b/ui/app/templates/components/job-versions-stream.hbs index c05caa48a612..0528fc292c63 100644 --- a/ui/app/templates/components/job-versions-stream.hbs +++ b/ui/app/templates/components/job-versions-stream.hbs @@ -5,6 +5,6 @@ {{/if}}
  • - +
    • {{/each}} diff --git a/ui/app/templates/jobs/job/versions.hbs b/ui/app/templates/jobs/job/versions.hbs index dc643705345b..5a657876188d 100644 --- a/ui/app/templates/jobs/job/versions.hbs +++ b/ui/app/templates/jobs/job/versions.hbs @@ -1,5 +1,19 @@ {{page-title "Job " this.job.name " versions"}}
    - + {{#if this.error}} +
    {{!-- FIXME hard-coded colour? hmm --}} +
    +
    +

    {{this.error.title}}

    +

    {{this.error.description}}

    +
    +
    + +
    +
    +
    + {{/if}} + +
    diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index 40b315671075..ade58971f1eb 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -4,6 +4,7 @@ import { setupApplicationTest } from 'ember-qunit'; import { setupMirage } from 'ember-cli-mirage/test-support'; import a11yAudit from 'nomad-ui/tests/helpers/a11y-audit'; import Versions from 'nomad-ui/tests/pages/jobs/job/versions'; +import Layout from 'nomad-ui/tests/pages/layout'; import moment from 'moment'; let job; @@ -68,6 +69,27 @@ module('Acceptance | job versions', function(hooks) { } }); + test('when reversion fails, the error message from the API is piped through to the alert', async function(assert) { + const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0]; + + if (versionRowToRevertTo) { + const message = 'A plaintext error message'; + server.pretender.post('/v1/job/:id/revert', () => [500, {}, message]); + + await versionRowToRevertTo.revertToButton.click(); + + assert.ok(Layout.inlineError.isShown); + assert.ok(Layout.inlineError.title.includes('Could Not Revert')); + assert.equal(Layout.inlineError.message, message); + + await Layout.inlineError.dismiss(); + + assert.notOk(Layout.inlineError.isShown); + } else { + assert.expect(0); + } + }); + test('when the job for the versions is not found, an error message is shown, but the URL persists', async function(assert) { await Versions.visit({ id: 'not-a-real-job' }); diff --git a/ui/tests/pages/layout.js b/ui/tests/pages/layout.js index cb1fcbbe8c26..1f66c5e8b67f 100644 --- a/ui/tests/pages/layout.js +++ b/ui/tests/pages/layout.js @@ -99,4 +99,11 @@ export default create({ title: text('[data-test-error-title]'), message: text('[data-test-error-message]'), }, + + inlineError: { + isShown: isPresent('[data-test-inline-error]'), + title: text('[data-test-inline-error-title]'), + message: text('[data-test-inline-error-body]'), + dismiss: clickable('[data-test-inline-error-close]'), + }, }); From b22c433382143b8e032a067306e6ce9650956a16 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Mon, 12 Apr 2021 16:10:58 -0500 Subject: [PATCH 04/15] Add reload to check for updated version --- ui/app/components/job-version.js | 1 + ui/mirage/config.js | 8 ++++++-- ui/tests/acceptance/job-versions-test.js | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index a800961c478a..256c80ecc0b3 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -45,6 +45,7 @@ export default class JobVersion extends Component { @task(function*() { try { yield this.version.revertTo(); + yield this.version.job.reload(); } catch (e) { this.handleError({ title: 'Could Not Revert', diff --git a/ui/mirage/config.js b/ui/mirage/config.js index 6fe7baa65119..1641fca8c6c4 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -180,8 +180,12 @@ export default function() { return okEmpty(); }); - this.post('/job/:id/revert', function() { - // FIXME return something more realistic? + this.post('/job/:id/revert', function({ jobs }, { requestBody }) { + const { JobID, JobVersion } = JSON.parse(requestBody); + const job = jobs.find(JobID); + job.version = JobVersion; + job.save(); + return okEmpty(); }); diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index ade58971f1eb..108eccd0a85d 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -66,6 +66,9 @@ module('Acceptance | job versions', function(hooks) { JobID: job.id, JobVersion: versionRowToRevertTo.number, }); + + // The job should reload and have a new version number + assert.ok(versionRowToRevertTo.revertToButton.isHidden); } }); From f9220bf76734d2c2a9b0b9f2db62c6bae9cfba23 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Mon, 12 Apr 2021 16:29:18 -0500 Subject: [PATCH 05/15] =?UTF-8?q?Add=20warning=20when=20reversion=20hasn?= =?UTF-8?q?=E2=80=99t=20changed=20version?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ui/app/components/job-version.js | 13 +++++++++++++ ui/app/controllers/jobs/job/versions.js | 14 +++++++++++++- ui/app/templates/jobs/job/versions.hbs | 4 ++-- ui/tests/acceptance/job-versions-test.js | 19 +++++++++++++++++++ ui/tests/pages/layout.js | 3 +++ 5 files changed, 50 insertions(+), 3 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index 256c80ecc0b3..37e4100ffc10 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -44,10 +44,23 @@ export default class JobVersion extends Component { @task(function*() { try { + const versionBeforeReversion = this.get('version.job.version'); + yield this.version.revertTo(); yield this.version.job.reload(); + + const versionAfterRevision = this.get('version.job.version'); + + if (versionBeforeReversion === versionAfterRevision) { + this.handleError({ + level: 'warn', + title: 'Reversion Had No Effect', + description: 'Reverting to an identical older version doesn’t produce a new version', + }); + } } catch (e) { this.handleError({ + level: 'danger', title: 'Could Not Revert', description: messageForError(e, 'revert'), }); diff --git a/ui/app/controllers/jobs/job/versions.js b/ui/app/controllers/jobs/job/versions.js index 19d8e76c69a9..087100ea0396 100644 --- a/ui/app/controllers/jobs/job/versions.js +++ b/ui/app/controllers/jobs/job/versions.js @@ -1,15 +1,27 @@ import Controller from '@ember/controller'; import WithNamespaceResetting from 'nomad-ui/mixins/with-namespace-resetting'; import { alias } from '@ember/object/computed'; -import { action } from '@ember/object'; +import { action, computed } from '@ember/object'; import classic from 'ember-classic-decorator'; +const alertClassFallback = 'is-info'; + +const errorLevelToAlertClass = { + danger: 'is-danger', + warn: 'is-warning', +}; + @classic export default class VersionsController extends Controller.extend(WithNamespaceResetting) { error = null; @alias('model') job; + @computed('error.level') + get errorLevelClass() { + return errorLevelToAlertClass[this.get('error.level')] || alertClassFallback; + } + onDismiss() { this.set('error', null); } diff --git a/ui/app/templates/jobs/job/versions.hbs b/ui/app/templates/jobs/job/versions.hbs index 5a657876188d..d6829bd849cd 100644 --- a/ui/app/templates/jobs/job/versions.hbs +++ b/ui/app/templates/jobs/job/versions.hbs @@ -2,14 +2,14 @@
    {{#if this.error}} -
    {{!-- FIXME hard-coded colour? hmm --}} +

    {{this.error.title}}

    {{this.error.description}}

    - +
    diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index 108eccd0a85d..343e85b84899 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -82,6 +82,7 @@ module('Acceptance | job versions', function(hooks) { await versionRowToRevertTo.revertToButton.click(); assert.ok(Layout.inlineError.isShown); + assert.ok(Layout.inlineError.isDanger); assert.ok(Layout.inlineError.title.includes('Could Not Revert')); assert.equal(Layout.inlineError.message, message); @@ -93,6 +94,24 @@ module('Acceptance | job versions', function(hooks) { } }); + test('when reversion has no effect, the error message explains', async function(assert) { + const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0]; + + if (versionRowToRevertTo) { + // The default Mirage implementation updates the job version as passed in, this does nothing + server.pretender.post('/v1/job/:id/revert', () => [200, {}, '']); + + await versionRowToRevertTo.revertToButton.click(); + + assert.ok(Layout.inlineError.isShown); + assert.ok(Layout.inlineError.isWarning); + assert.ok(Layout.inlineError.title.includes('Reversion Had No Effect')); + assert.equal(Layout.inlineError.message, 'Reverting to an identical older version doesn’t produce a new version'); + } else { + assert.expect(0); + } + }); + test('when the job for the versions is not found, an error message is shown, but the URL persists', async function(assert) { await Versions.visit({ id: 'not-a-real-job' }); diff --git a/ui/tests/pages/layout.js b/ui/tests/pages/layout.js index 1f66c5e8b67f..c2a34c343f0e 100644 --- a/ui/tests/pages/layout.js +++ b/ui/tests/pages/layout.js @@ -105,5 +105,8 @@ export default create({ title: text('[data-test-inline-error-title]'), message: text('[data-test-inline-error-body]'), dismiss: clickable('[data-test-inline-error-close]'), + + isDanger: hasClass('is-danger', '[data-test-inline-error]'), + isWarning: hasClass('is-warning', '[data-test-inline-error]'), }, }); From c9871ccfcf6d3190379260803774fb6f485f8578 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Tue, 13 Apr 2021 13:27:07 -0500 Subject: [PATCH 06/15] Change reversion to use two-step button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 😖 re the ever-growing styling interface --- ui/app/components/two-step-button.js | 2 +- ui/app/styles/components/two-step-button.scss | 12 ++++++++++-- ui/app/templates/components/job-version.hbs | 17 +++++++++++++++-- ui/tests/acceptance/job-versions-test.js | 9 ++++++--- ui/tests/pages/jobs/job/versions.js | 6 +++--- 5 files changed, 35 insertions(+), 11 deletions(-) diff --git a/ui/app/components/two-step-button.js b/ui/app/components/two-step-button.js index 4bf934eadb45..70ff3401062d 100644 --- a/ui/app/components/two-step-button.js +++ b/ui/app/components/two-step-button.js @@ -9,7 +9,7 @@ import classic from 'ember-classic-decorator'; @classic @classNames('two-step-button') -@classNameBindings('inlineText:has-inline-text') +@classNameBindings('inlineText:has-inline-text', 'fadingBackground:has-fading-background') export default class TwoStepButton extends Component { idleText = ''; cancelText = ''; diff --git a/ui/app/styles/components/two-step-button.scss b/ui/app/styles/components/two-step-button.scss index c4dfcbb2665f..3a1aeb9e47d8 100644 --- a/ui/app/styles/components/two-step-button.scss +++ b/ui/app/styles/components/two-step-button.scss @@ -14,11 +14,19 @@ } .confirmation-text { - position: static; - margin-right: 0.5ch; + position: absolute; + left: auto; + right: 0; + top: auto; + margin-right: 100%; } } + &.has-fading-background .confirmation-text { + padding: 0.5rem 0 0.5rem 4rem; + background: linear-gradient(to left, white, white 90%, transparent 100%); + } + .confirmation-text { position: absolute; left: 0; diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 84069274ce6e..8a8f47fe0e34 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -10,11 +10,24 @@
    {{#unless this.isCurrent}} -   + {{/unless}} {{#if this.version.diff}} - + {{else}} No Changes {{/if}} diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index 343e85b84899..bb970617cf2c 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -56,7 +56,8 @@ module('Acceptance | job versions', function(hooks) { }); if (versionRowToRevertTo) { - await versionRowToRevertTo.revertToButton.click(); + await versionRowToRevertTo.revertToButton.idle(); + await versionRowToRevertTo.revertToButton.confirm(); const revertRequest = this.server.pretender.handledRequests.find(request => request.url.includes('revert')); @@ -79,7 +80,8 @@ module('Acceptance | job versions', function(hooks) { const message = 'A plaintext error message'; server.pretender.post('/v1/job/:id/revert', () => [500, {}, message]); - await versionRowToRevertTo.revertToButton.click(); + await versionRowToRevertTo.revertToButton.idle(); + await versionRowToRevertTo.revertToButton.confirm(); assert.ok(Layout.inlineError.isShown); assert.ok(Layout.inlineError.isDanger); @@ -101,7 +103,8 @@ module('Acceptance | job versions', function(hooks) { // The default Mirage implementation updates the job version as passed in, this does nothing server.pretender.post('/v1/job/:id/revert', () => [200, {}, '']); - await versionRowToRevertTo.revertToButton.click(); + await versionRowToRevertTo.revertToButton.idle(); + await versionRowToRevertTo.revertToButton.confirm(); assert.ok(Layout.inlineError.isShown); assert.ok(Layout.inlineError.isWarning); diff --git a/ui/tests/pages/jobs/job/versions.js b/ui/tests/pages/jobs/job/versions.js index d1a312a9ffed..7964648fcd54 100644 --- a/ui/tests/pages/jobs/job/versions.js +++ b/ui/tests/pages/jobs/job/versions.js @@ -1,6 +1,7 @@ import { create, collection, text, visitable } from 'ember-cli-page-object'; import { getter } from 'ember-cli-page-object/macros'; +import twoStepButton from 'nomad-ui/tests/pages/components/two-step-button'; import error from 'nomad-ui/tests/pages/components/error'; export default create({ @@ -10,9 +11,8 @@ export default create({ text: text(), stability: text('[data-test-version-stability]'), submitTime: text('[data-test-version-submit-time]'), - revertToButton: { - scope: '[data-test-revert-to-button]', - }, + + revertToButton: twoStepButton('[data-test-revert-to]'), number: getter(function() { return parseInt(this.text.match(/#(\d+)/)[1]); From a7011551df5c28456f89b26ec3d650500e38d68a Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Tue, 13 Apr 2021 13:43:56 -0500 Subject: [PATCH 07/15] Change changes/no changes to be fixed width MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Placing this in boxed-section: questionable? Should there be job-version.scss? 🧐 --- ui/app/styles/components/boxed-section.scss | 5 +++++ ui/app/templates/components/job-version.hbs | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/ui/app/styles/components/boxed-section.scss b/ui/app/styles/components/boxed-section.scss index f960bac5a876..bcebd868f410 100644 --- a/ui/app/styles/components/boxed-section.scss +++ b/ui/app/styles/components/boxed-section.scss @@ -19,6 +19,11 @@ margin-left: auto; } + .is-fixed-width { + display: inline-block; + width: 8em; + } + &.is-compact { padding: 0.75em; } diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index 8a8f47fe0e34..b794dcd2c9d1 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -27,9 +27,9 @@ {{/unless}} {{#if this.version.diff}} - + {{else}} - No Changes +
    No Changes
    {{/if}}
    From b36f24dd7d47a46b7652be35ea11dd0d539d4660 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Wed, 14 Apr 2021 09:36:44 -0500 Subject: [PATCH 08/15] Remove note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This happens twice in watchable-namespace-ids, maybe not quite widely-used enough to extract? 🤔 --- ui/app/adapters/job-version.js | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/app/adapters/job-version.js b/ui/app/adapters/job-version.js index c51763c187db..5281f24f028f 100644 --- a/ui/app/adapters/job-version.js +++ b/ui/app/adapters/job-version.js @@ -8,7 +8,6 @@ export default class JobVersionAdapter extends ApplicationAdapter { const url = addToPath(jobAdapter.urlForFindRecord(jobVersion.get('job.id'), 'job'), '/revert'); - // FIXME is there a better place for this? const [jobName] = JSON.parse(jobVersion.get('job.id')); return this.ajax(url, 'POST', { From 917828af19d3ea1f66f8d7eda18c50045ab992a1 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Wed, 14 Apr 2021 09:43:20 -0500 Subject: [PATCH 09/15] Fix typo --- ui/app/components/job-version.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index 37e4100ffc10..53a6d33b3eb6 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -49,9 +49,9 @@ export default class JobVersion extends Component { yield this.version.revertTo(); yield this.version.job.reload(); - const versionAfterRevision = this.get('version.job.version'); + const versionAfterReversion = this.get('version.job.version'); - if (versionBeforeReversion === versionAfterRevision) { + if (versionBeforeReversion === versionAfterReversion) { this.handleError({ level: 'warn', title: 'Reversion Had No Effect', From d0ecfec15c37e56faa1ea7a684049adee7cc9989 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Wed, 14 Apr 2021 09:43:52 -0500 Subject: [PATCH 10/15] Fix whitespace --- ui/app/adapters/job-version.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/ui/app/adapters/job-version.js b/ui/app/adapters/job-version.js index 5281f24f028f..eb2393a9489d 100644 --- a/ui/app/adapters/job-version.js +++ b/ui/app/adapters/job-version.js @@ -2,12 +2,10 @@ import ApplicationAdapter from './application'; import addToPath from 'nomad-ui/utils/add-to-path'; export default class JobVersionAdapter extends ApplicationAdapter { - revertTo(jobVersion) { const jobAdapter = this.store.adapterFor('job'); const url = addToPath(jobAdapter.urlForFindRecord(jobVersion.get('job.id'), 'job'), '/revert'); - const [jobName] = JSON.parse(jobVersion.get('job.id')); return this.ajax(url, 'POST', { From 8ac25586fbc5b525a4508f84672c0ac451c0d170 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Wed, 14 Apr 2021 13:49:28 -0500 Subject: [PATCH 11/15] Add permissions-checking --- ui/app/templates/components/job-version.hbs | 40 +++++++++++++-------- ui/tests/acceptance/job-versions-test.js | 31 ++++++++++++++++ ui/tests/pages/jobs/job/versions.js | 3 +- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/ui/app/templates/components/job-version.hbs b/ui/app/templates/components/job-version.hbs index b794dcd2c9d1..fc219e964cda 100644 --- a/ui/app/templates/components/job-version.hbs +++ b/ui/app/templates/components/job-version.hbs @@ -10,20 +10,32 @@
    {{#unless this.isCurrent}} - + {{#if (can "run job")}} + + {{else}} + + {{/if}} {{/unless}} {{#if this.version.diff}} diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index bb970617cf2c..dac0c94e8945 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -18,6 +18,9 @@ module('Acceptance | job versions', function(hooks) { job = server.create('job', { createAllocations: false }); versions = server.db.jobVersions.where({ jobId: job.id }); + const managementToken = server.create('token'); + window.localStorage.nomadTokenSecret = managementToken.secretId; + await Versions.visit({ id: job.id }); }); @@ -130,3 +133,31 @@ module('Acceptance | job versions', function(hooks) { assert.equal(Versions.error.title, 'Not Found', 'Error message is for 404'); }); }); + +module('Acceptance | job versions (with client token)', function(hooks) { + setupApplicationTest(hooks); + setupMirage(hooks); + + hooks.beforeEach(async function() { + job = server.create('job', { createAllocations: false }); + versions = server.db.jobVersions.where({ jobId: job.id }); + + server.create('token'); + const clientToken = server.create('token'); + window.localStorage.nomadTokenSecret = clientToken.secretId; + + await Versions.visit({ id: job.id }); + }); + + test('reversion buttons are disabled when the token lacks permissions', async function(assert) { + const versionRowWithReversion = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0]; + + if (versionRowWithReversion) { + assert.ok(versionRowWithReversion.revertToButtonIsDisabled); + } else { + assert.expect(0); + } + + window.localStorage.clear(); + }); +}); diff --git a/ui/tests/pages/jobs/job/versions.js b/ui/tests/pages/jobs/job/versions.js index 7964648fcd54..416406f00e9a 100644 --- a/ui/tests/pages/jobs/job/versions.js +++ b/ui/tests/pages/jobs/job/versions.js @@ -1,4 +1,4 @@ -import { create, collection, text, visitable } from 'ember-cli-page-object'; +import { attribute, create, collection, text, visitable } from 'ember-cli-page-object'; import { getter } from 'ember-cli-page-object/macros'; import twoStepButton from 'nomad-ui/tests/pages/components/two-step-button'; @@ -13,6 +13,7 @@ export default create({ submitTime: text('[data-test-version-submit-time]'), revertToButton: twoStepButton('[data-test-revert-to]'), + revertToButtonIsDisabled: attribute('disabled', '[data-test-revert-to]'), number: getter(function() { return parseInt(this.text.match(/#(\d+)/)[1]); From b510957eebddfe1e9e60b861472b7d7dcf6bfca8 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Wed, 14 Apr 2021 15:59:08 -0500 Subject: [PATCH 12/15] Add job-watching on versions route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this, if the job’s current version is updated outside the UI, the “Revert” button won’t be added to the formerly- current version. --- ui/app/routes/jobs/job/versions.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ui/app/routes/jobs/job/versions.js b/ui/app/routes/jobs/job/versions.js index c6ab5684bdec..2923c321c77b 100644 --- a/ui/app/routes/jobs/job/versions.js +++ b/ui/app/routes/jobs/job/versions.js @@ -1,6 +1,6 @@ import Route from '@ember/routing/route'; import { collect } from '@ember/object/computed'; -import { watchRelationship } from 'nomad-ui/utils/properties/watch'; +import { watchRecord, watchRelationship } from 'nomad-ui/utils/properties/watch'; import WithWatchers from 'nomad-ui/mixins/with-watchers'; export default class VersionsRoute extends Route.extend(WithWatchers) { @@ -11,10 +11,13 @@ export default class VersionsRoute extends Route.extend(WithWatchers) { startWatchers(controller, model) { if (model) { - controller.set('watcher', this.watchVersions.perform(model)); + controller.set('watcher', this.watch.perform(model)); + controller.set('watchVersions', this.watchVersions.perform(model)); } } + @watchRecord('job') watch; @watchRelationship('versions') watchVersions; - @collect('watchVersions') watchers; + + @collect('watch', 'watchVersions') watchers; } From 3a219ee7d3ce1014be27deab58e1dc937bd4f2fd Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Mon, 19 Apr 2021 09:22:26 -0500 Subject: [PATCH 13/15] Add redirection after successful reversion As suggested by @DingoEatingFuzz. --- ui/app/components/job-version.js | 9 +++++++++ ui/tests/acceptance/job-versions-test.js | 6 +++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/ui/app/components/job-version.js b/ui/app/components/job-version.js index 53a6d33b3eb6..d3ba75a7510e 100644 --- a/ui/app/components/job-version.js +++ b/ui/app/components/job-version.js @@ -1,6 +1,7 @@ import Component from '@ember/component'; import { action, computed } from '@ember/object'; import { classNames } from '@ember-decorators/component'; +import { inject as service } from '@ember/service'; import { task } from 'ember-concurrency'; import messageForError from 'nomad-ui/utils/message-from-adapter-error'; import classic from 'ember-classic-decorator'; @@ -16,6 +17,8 @@ export default class JobVersion extends Component { // Passes through to the job-diff component verbose = true; + @service router; + @computed('version.diff') get changeCount() { const diff = this.get('version.diff'); @@ -57,6 +60,12 @@ export default class JobVersion extends Component { title: 'Reversion Had No Effect', description: 'Reverting to an identical older version doesn’t produce a new version', }); + } else { + const job = this.get('version.job'); + + this.router.transitionTo('jobs.job', job.get('plainId'), { + queryParams: { namespace: job.get('namespace.name') }, + }); } } catch (e) { this.handleError({ diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index dac0c94e8945..3393f861f4ca 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -59,6 +59,7 @@ module('Acceptance | job versions', function(hooks) { }); if (versionRowToRevertTo) { + const versionNumberRevertingTo = versionRowToRevertTo.number; await versionRowToRevertTo.revertToButton.idle(); await versionRowToRevertTo.revertToButton.confirm(); @@ -68,11 +69,10 @@ module('Acceptance | job versions', function(hooks) { assert.deepEqual(JSON.parse(revertRequest.requestBody), { JobID: job.id, - JobVersion: versionRowToRevertTo.number, + JobVersion: versionNumberRevertingTo, }); - // The job should reload and have a new version number - assert.ok(versionRowToRevertTo.revertToButton.isHidden); + assert.equal(currentURL(), `/jobs/${job.id}`); } }); From c30917f42e8cff45ac33946055642f0a86581737 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Mon, 19 Apr 2021 17:31:02 -0500 Subject: [PATCH 14/15] Update reversion call test to be namespace-aware --- ui/tests/acceptance/job-versions-test.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/ui/tests/acceptance/job-versions-test.js b/ui/tests/acceptance/job-versions-test.js index 3393f861f4ca..d9b973e49cdc 100644 --- a/ui/tests/acceptance/job-versions-test.js +++ b/ui/tests/acceptance/job-versions-test.js @@ -8,6 +8,7 @@ import Layout from 'nomad-ui/tests/pages/layout'; import moment from 'moment'; let job; +let namespace; let versions; module('Acceptance | job versions', function(hooks) { @@ -15,13 +16,16 @@ module('Acceptance | job versions', function(hooks) { setupMirage(hooks); hooks.beforeEach(async function() { - job = server.create('job', { createAllocations: false }); + server.create('namespace'); + namespace = server.create('namespace'); + + job = server.create('job', { namespaceId: namespace.id, createAllocations: false }); versions = server.db.jobVersions.where({ jobId: job.id }); const managementToken = server.create('token'); window.localStorage.nomadTokenSecret = managementToken.secretId; - await Versions.visit({ id: job.id }); + await Versions.visit({ id: job.id, namespace: namespace.id }); }); test('it passes an accessibility audit', async function(assert) { @@ -65,14 +69,14 @@ module('Acceptance | job versions', function(hooks) { const revertRequest = this.server.pretender.handledRequests.find(request => request.url.includes('revert')); - assert.equal(revertRequest.url, `/v1/job/${job.id}/revert`); + assert.equal(revertRequest.url, `/v1/job/${job.id}/revert?namespace=${namespace.id}`); assert.deepEqual(JSON.parse(revertRequest.requestBody), { JobID: job.id, JobVersion: versionNumberRevertingTo, }); - assert.equal(currentURL(), `/jobs/${job.id}`); + assert.equal(currentURL(), `/jobs/${job.id}?namespace=${namespace.id}`); } }); From 4c9ec9c08d49f8a1960c1ea3c6d5aaceafecccf3 Mon Sep 17 00:00:00 2001 From: Buck Doyle Date: Tue, 20 Apr 2021 08:10:29 -0500 Subject: [PATCH 15/15] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5805c5240ac3..05c06f80f6b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ IMPROVEMENTS: * networking: Added support for user-defined iptables rules on the NOMAD-ADMIN chain. [[GH-10181](https://github.com/hashicorp/nomad/issues/10181)] * networking: Added support for interpolating host network names with node attributes. [[GH-10196](https://github.com/hashicorp/nomad/issues/10196)] * nomad/structs: Removed deprecated Node.Drain field, added API extensions to restore it [[GH-10202](https://github.com/hashicorp/nomad/issues/10202)] + * ui: Added a job reversion button [[GH-10336](https://github.com/hashicorp/nomad/pull/10336)] BUG FIXES: * core (Enterprise): Update licensing library to v0.0.11 to include race condition fix. [[GH-10253](https://github.com/hashicorp/nomad/issues/10253)]