From 771e58d85f2732e57681592381d330c18fbf95f1 Mon Sep 17 00:00:00 2001 From: Jai <41024828+ChaiWithJai@users.noreply.github.com> Date: Tue, 17 Jan 2023 10:15:34 -0500 Subject: [PATCH] ui: add error handling and form validation to job create template (#15767) * ui: handle server side errors * ui: show error to prevent duplicate * refact: conditional namespace * ui: save as template flow (#15787) --- ui/app/components/job-editor.js | 1 + ui/app/controllers/jobs/run.js | 12 ++ ui/app/controllers/jobs/run/index.js | 9 ++ ui/app/controllers/jobs/run/templates/new.js | 43 +++++-- ui/app/routes/jobs/run/templates/new.js | 16 ++- ui/app/styles/core/forms.scss | 10 +- ui/app/templates/components/job-editor.hbs | 1 + ui/app/templates/jobs/run/index.hbs | 2 +- ui/app/templates/jobs/run/templates/new.hbs | 32 ++++-- ui/tests/acceptance/job-run-test.js | 111 +++++++++++++++++++ 10 files changed, 214 insertions(+), 23 deletions(-) create mode 100644 ui/app/controllers/jobs/run.js diff --git a/ui/app/components/job-editor.js b/ui/app/components/job-editor.js index f070462d6d1..91e4d80d07e 100644 --- a/ui/app/components/job-editor.js +++ b/ui/app/components/job-editor.js @@ -19,6 +19,7 @@ export default class JobEditor extends Component { job = null; onSubmit() {} + handleSaveAsTemplate() {} @computed('_context') get context() { diff --git a/ui/app/controllers/jobs/run.js b/ui/app/controllers/jobs/run.js new file mode 100644 index 00000000000..2fa1c80a4e1 --- /dev/null +++ b/ui/app/controllers/jobs/run.js @@ -0,0 +1,12 @@ +import Controller from '@ember/controller'; +import { action } from '@ember/object'; +import { tracked } from '@glimmer/tracking'; + +export default class JobsRunTemplatesController extends Controller { + @tracked jsonTemplate = null; + + @action + setTemplate(template) { + this.jsonTemplate = template; + } +} diff --git a/ui/app/controllers/jobs/run/index.js b/ui/app/controllers/jobs/run/index.js index 794fa2f7438..00084a843d5 100644 --- a/ui/app/controllers/jobs/run/index.js +++ b/ui/app/controllers/jobs/run/index.js @@ -1,4 +1,6 @@ +import { getOwner } from '@ember/application'; import Controller from '@ember/controller'; +import { action } from '@ember/object'; import { inject as service } from '@ember/service'; export default class RunController extends Controller { @@ -6,6 +8,13 @@ export default class RunController extends Controller { queryParams = ['template']; + @action + handleSaveAsTemplate() { + getOwner(this) + .lookup('controller:jobs.run') + .setTemplate(this.model._newDefinition); + } + onSubmit(id, namespace) { this.router.transitionTo('jobs.job', `${id}@${namespace || 'default'}`); } diff --git a/ui/app/controllers/jobs/run/templates/new.js b/ui/app/controllers/jobs/run/templates/new.js index dd0c60cdbe8..942ebad3c2c 100644 --- a/ui/app/controllers/jobs/run/templates/new.js +++ b/ui/app/controllers/jobs/run/templates/new.js @@ -3,9 +3,12 @@ import Controller from '@ember/controller'; import { inject as service } from '@ember/service'; import { action } from '@ember/object'; import { tracked } from '@glimmer/tracking'; +import { trimPath } from '../../../../helpers/trim-path'; export default class JobsRunTemplatesController extends Controller { @service router; + @service store; + @service system; @tracked templateName = null; @tracked templateNamespace = 'default'; @@ -19,6 +22,17 @@ export default class JobsRunTemplatesController extends Controller { return namespaces; } + get isDuplicateTemplate() { + const templates = this.store.peekAll('variable'); + const templateName = trimPath([`nomad/job-templates/${this.templateName}`]); + + return !!templates + .without(this.model) + .find( + (v) => v.path === templateName && v.namespace === this.templateNamespace + ); + } + @action updateKeyValue(key, value) { if (this.keyValues.find((kv) => kv.key === key)) { @@ -46,16 +60,27 @@ export default class JobsRunTemplatesController extends Controller { this.model.set('keyValues', this.keyValues); this.model.set('path', `nomad/job-templates/${this.templateName}`); this.model.setAndTrimPath(); - await this.model.save({ adapterOptions: { overwrite } }); - this.flashMessages.add({ - title: 'Job template saved', - message: `${this.templateName} successfully saved`, - type: 'success', - destroyOnClick: false, - timeout: 5000, - }); + try { + await this.model.save({ adapterOptions: { overwrite } }); - this.router.transitionTo('jobs.run.templates'); + this.flashMessages.add({ + title: 'Job template saved', + message: `${this.templateName} successfully saved`, + type: 'success', + destroyOnClick: false, + timeout: 5000, + }); + + this.router.transitionTo('jobs.run.templates'); + } catch (e) { + this.flashMessages.add({ + title: 'Job template cannot be saved.', + message: e, + type: 'error', + destroyOnClick: false, + timeout: 5000, + }); + } } } diff --git a/ui/app/routes/jobs/run/templates/new.js b/ui/app/routes/jobs/run/templates/new.js index c9e05897485..fcc614c542a 100644 --- a/ui/app/routes/jobs/run/templates/new.js +++ b/ui/app/routes/jobs/run/templates/new.js @@ -1,3 +1,4 @@ +import { getOwner } from '@ember/application'; import Route from '@ember/routing/route'; import { inject as service } from '@ember/service'; import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; @@ -22,8 +23,21 @@ export default class RunRoute extends Route { try { // When variables are created with a namespace attribute, it is verified against // available namespaces to prevent redirecting to a non-existent namespace. - await this.store.findAll('namespace'); + await Promise.all([ + this.store.query('variable', { + prefix: 'nomad/job-templates', + namespace: '*', + }), + this.store.findAll('namespace'), + ]); + // When navigating from jobs.run.index using "Save as Template" + const json = getOwner(this).lookup('controller:jobs.run').jsonTemplate; + if (json) { + return this.store.createRecord('variable', { + keyValues: [{ key: 'template', value: json }], + }); + } return this.store.createRecord('variable'); } catch (e) { notifyForbidden(this)(e); diff --git a/ui/app/styles/core/forms.scss b/ui/app/styles/core/forms.scss index b39d03c84d6..85814baa77b 100644 --- a/ui/app/styles/core/forms.scss +++ b/ui/app/styles/core/forms.scss @@ -104,11 +104,19 @@ .new-job-template { & > div { margin-bottom: 1rem; - align-items: end; } .path-input { height: 2.25em; + &.error { + color: $red; + border-color: $red; + } + + + p { + position: relative; + animation: slide-in 0.3s ease-out; + } } .input-dropdown-row { diff --git a/ui/app/templates/components/job-editor.hbs b/ui/app/templates/components/job-editor.hbs index da07f3f2e9a..47c817ea891 100644 --- a/ui/app/templates/components/job-editor.hbs +++ b/ui/app/templates/components/job-editor.hbs @@ -63,6 +63,7 @@ {{#if (can "write variable" path="*" namespace="*")}} + {{/if}} diff --git a/ui/app/templates/jobs/run/index.hbs b/ui/app/templates/jobs/run/index.hbs index 7bb74880bc6..0ef4f3e446f 100644 --- a/ui/app/templates/jobs/run/index.hbs +++ b/ui/app/templates/jobs/run/index.hbs @@ -1,5 +1,5 @@ {{page-title "Run a job"}}
- +
\ No newline at end of file diff --git a/ui/app/templates/jobs/run/templates/new.hbs b/ui/app/templates/jobs/run/templates/new.hbs index 76951212a1c..0278f308aaa 100644 --- a/ui/app/templates/jobs/run/templates/new.hbs +++ b/ui/app/templates/jobs/run/templates/new.hbs @@ -1,7 +1,7 @@ {{page-title "Create a custom template"}}
-
+
- {{#if this.namespaceOptions.length}} - + {{#if this.system.shouldShowNamespaces}} + {{/if}}
diff --git a/ui/tests/acceptance/job-run-test.js b/ui/tests/acceptance/job-run-test.js index a7bcea9231d..0487266f346 100644 --- a/ui/tests/acceptance/job-run-test.js +++ b/ui/tests/acceptance/job-run-test.js @@ -1,3 +1,4 @@ +import AdapterError from '@ember-data/adapter/error'; import { click, currentRouteName, @@ -6,6 +7,10 @@ import { } from '@ember/test-helpers'; import { assign } from '@ember/polyfills'; import { module, test } from 'qunit'; +import { + selectChoose, + clickTrigger, +} from 'ember-power-select/test-support/helpers'; import { setupApplicationTest } from 'ember-qunit'; import { setupMirage } from 'ember-cli-mirage/test-support'; import a11yAudit from 'nomad-ui/tests/helpers/a11y-audit'; @@ -310,5 +315,111 @@ module('Acceptance | job run', function (hooks) { .dom('[data-test-template-card=foo]') .exists('The newly created template appears in the list.'); }); + + test('a toast notification alerts the user if there is an error saving the newly created job template', async function (assert) { + assert.expect(5); + // Arrange + await JobRun.visit(); + await click('[data-test-choose-template]'); + + // Assert + assert + .dom('[data-test-empty-templates-list-headline]') + .exists('No templates are listed if none have been created.'); + + await click('[data-test-create-inline]'); + assert.equal(currentRouteName(), 'jobs.run.templates.new'); + assert + .dom('[data-test-save-template]') + .isDisabled('the save button should be disabled if no path is set'); + + await fillIn('[data-test-template-name]', 'try@'); + await fillIn('[data-test-template-description]', 'foo-bar-baz'); + const codeMirror = getCodeMirrorInstance('[data-test-template-json]'); + codeMirror.setValue(jsonJob()); + + server.put('/var/:varId?cas=0', function () { + return new AdapterError({ + detail: `invalid path "nomad/job-templates/try@"`, + status: 500, + }); + }); + + await click('[data-test-save-template]'); + assert.equal( + currentRouteName(), + 'jobs.run.templates.new', + 'We do not navigate away from the page if an error is returned by the API.' + ); + assert + .dom('.flash-message.alert-error') + .exists('A toast error message pops up.'); + }); + + test('a user cannot create a job template if one with the same name and namespace already exists', async function (assert) { + assert.expect(4); + // Arrange + await JobRun.visit(); + await click('[data-test-choose-template]'); + server.create('variable', { + path: 'nomad/job-templates/foo', + namespace: 'default', + id: 'nomad/job-templates/foo', + }); + server.create('namespace', { id: 'test' }); + + // Assert + assert + .dom('[data-test-empty-templates-list-headline]') + .exists('No templates are listed if none have been created.'); + + await click('[data-test-create-inline]'); + assert.equal(currentRouteName(), 'jobs.run.templates.new'); + + await fillIn('[data-test-template-name]', 'foo'); + assert + .dom('[data-test-duplicate-error]') + .exists('an error message alerts the user'); + + await clickTrigger('[data-test-namespace-facet]'); + await selectChoose('[data-test-namespace-facet]', 'test'); + + assert + .dom('[data-test-duplicate-error]') + .doesNotExist( + 'an error disappears when name or namespace combination is unique' + ); + }); + + test('a user can save code from the editor as a template', async function (assert) { + assert.expect(4); + // Arrange + await JobRun.visit(); + await JobRun.editor.editor.fillIn(jsonJob()); + + await click('[data-test-save-as-template]'); + assert.equal( + currentRouteName(), + 'jobs.run.templates.new', + 'We navigate template creation page.' + ); + + // Assert + assert + .dom('[data-test-template-name]') + .hasNoText('No template name is prefilled.'); + assert + .dom('[data-test-template-description]') + .hasNoText('No template description is prefilled.'); + + const codeMirror = getCodeMirrorInstance('[data-test-template-json]'); + const json = codeMirror.getValue(); + + assert.equal( + json, + jsonJob(), + 'Template is filled out with text from the editor.' + ); + }); }); });