From e8db58836843244b579b0ecdf8cc3bbcb3ce90eb Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Thu, 15 Feb 2024 11:54:09 -0500 Subject: [PATCH] ui: Warning and better error handling for invalid-named variables and job templates (#19989) * Warning and better error handling for invalid-named variables and job templates * Warning and better error handling for invalid-named variables and job templates * Tests for variable pathname warnings * Only show the bad-name warning if the variable is being created and path is editable --- .changelog/19989.txt | 3 ++ ui/app/adapters/variable.js | 10 +++++ ui/app/components/variable-form.hbs | 9 +++- ui/app/components/variable-form.js | 24 ++++++++--- ui/app/controllers/jobs/run/templates/new.js | 17 +++++++- ui/app/templates/jobs/run/templates/new.hbs | 5 +++ .../components/variable-form-test.js | 41 +++++++++++++++++++ 7 files changed, 102 insertions(+), 7 deletions(-) create mode 100644 .changelog/19989.txt diff --git a/.changelog/19989.txt b/.changelog/19989.txt new file mode 100644 index 00000000000..fcdf612eb27 --- /dev/null +++ b/.changelog/19989.txt @@ -0,0 +1,3 @@ +```release-note:improvement +ui: Improve error and warning messages for invalid variable and job template paths/names +``` diff --git a/ui/app/adapters/variable.js b/ui/app/adapters/variable.js index 14de0eca9a0..ee023f1e1d3 100644 --- a/ui/app/adapters/variable.js +++ b/ui/app/adapters/variable.js @@ -6,6 +6,7 @@ // @ts-check import ApplicationAdapter from './application'; import AdapterError from '@ember-data/adapter/error'; +import InvalidError from '@ember-data/adapter/error'; import { pluralize } from 'ember-inflector'; import classic from 'ember-classic-decorator'; import { ConflictError } from '@ember-data/adapter/error'; @@ -141,6 +142,15 @@ export default class VariableAdapter extends ApplicationAdapter { { detail: _normalizeConflictErrorObject(payload), status: 409 }, ]); } + if (status === 400) { + return new InvalidError([ + { + detail: + 'Invalid name. Name must contain only alphanumeric or "-", "_", "~", or "/" characters, and be fewer than 128 characters in length.', + status: 400, + }, + ]); + } return super.handleResponse(...arguments); } } diff --git a/ui/app/components/variable-form.hbs b/ui/app/components/variable-form.hbs index cab126f6295..686c25cc6b2 100644 --- a/ui/app/components/variable-form.hbs +++ b/ui/app/components/variable-form.hbs @@ -55,7 +55,7 @@ @isRequired={{true}} @value={{this.path}} placeholder="nomad/jobs/my-job/my-group/my-task" - @isInvalid={{this.duplicatePathWarning}} + @isInvalid={{or this.duplicatePathWarning (and @model.isNew this.hasInvalidPath)}} {{on "input" (action this.setModelPath)}} disabled={{not @model.isNew}} {{autofocus}} @@ -79,6 +79,13 @@ . {{/if}} + {{#if @model.isNew}} + {{#if this.hasInvalidPath}} + + Path must contain only alphanumeric or "-", "_", "~", or "/" characters, and be fewer than 128 characters in length. + + {{/if}} + {{/if}} {{#if this.isJobTemplateVariable}} Use this variable to generate job templates with diff --git a/ui/app/components/variable-form.js b/ui/app/components/variable-form.js index 0fd3e84f12b..51d00f08d22 100644 --- a/ui/app/components/variable-form.js +++ b/ui/app/components/variable-form.js @@ -154,6 +154,11 @@ export default class VariableFormComponent extends Component { } } + get hasInvalidPath() { + let pathNameRegex = new RegExp('^[a-zA-Z0-9-_~/]{1,128}$'); + return !pathNameRegex.test(trimPath([this.path])); + } + @action validateKey(entry, e) { const value = e.target.value; @@ -269,18 +274,27 @@ export default class VariableFormComponent extends Component { this.removeExitHandler(); this.router.transitionTo('variables.variable', this.args.model.id); - } catch (error) { - notifyConflict(this)(error); + } catch (e) { + notifyConflict(this)(e); if (!this.hasConflict) { + let errorMessage = e; + if (e.errors && e.errors.length > 0) { + const nameInvalidError = e.errors.find((err) => err.status === 400); + if (nameInvalidError) { + errorMessage = nameInvalidError.detail; + } + } + + console.log('caught an error', e); this.notifications.add({ title: `Error saving ${this.path}`, - message: error, + message: errorMessage, color: 'critical', sticky: true, }); } else { - if (error.errors[0]?.detail) { - set(this, 'conflictingVariable', error.errors[0].detail); + if (e.errors[0]?.detail) { + set(this, 'conflictingVariable', e.errors[0].detail); } window.scrollTo(0, 0); // because the k/v list may be long, ensure the user is snapped to top to read error } diff --git a/ui/app/controllers/jobs/run/templates/new.js b/ui/app/controllers/jobs/run/templates/new.js index 65ad30d3ff7..59bce48f629 100644 --- a/ui/app/controllers/jobs/run/templates/new.js +++ b/ui/app/controllers/jobs/run/templates/new.js @@ -36,6 +36,11 @@ export default class JobsRunTemplatesNewController extends Controller { ); } + get hasInvalidName() { + let pathNameRegex = new RegExp('^[a-zA-Z0-9-_~/]{1,128}$'); + return !pathNameRegex.test(this.templateName); + } + @action updateKeyValue(key, value) { if (this.model.keyValues.find((kv) => kv.key === key)) { @@ -74,9 +79,19 @@ export default class JobsRunTemplatesNewController extends Controller { this.router.transitionTo('jobs.run.templates'); } catch (e) { + let errorMessage = + 'An unexpected error occurred when saving your Job template.'; + console.log('caught', e); + if (e.errors && e.errors.length > 0) { + const nameInvalidError = e.errors.find((err) => err.status === 400); + if (nameInvalidError) { + errorMessage = nameInvalidError.detail; + } + } + this.notifications.add({ title: 'Job template cannot be saved.', - message: e, + message: errorMessage, color: 'critical', }); } diff --git a/ui/app/templates/jobs/run/templates/new.hbs b/ui/app/templates/jobs/run/templates/new.hbs index 1a59f90e719..1ad146067f9 100644 --- a/ui/app/templates/jobs/run/templates/new.hbs +++ b/ui/app/templates/jobs/run/templates/new.hbs @@ -28,6 +28,11 @@ There is already a templated named {{this.templateName}}.

{{/if}} + {{#if this.hasInvalidName}} +

+ Template name must contain only alphanumeric or "-", "_", "~", or "/" characters, and be fewer than 128 characters in length. +

+ {{/if}} {{#if this.system.shouldShowNamespaces}}