From 9f51a5deb2da84e229aae4229bf915f3edbb0d34 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 15 Aug 2022 11:56:09 -0400 Subject: [PATCH] UI variables made to be unique by namespace and path (#14072) * Starting on namespaced id * Traversal for variables uniqued by namespace * Delog * Basic CRUD complete w namespaces included * Correct secvar breadcrumb joining and testfix now that namespaces are included * Testfixes with namespaces in place * Namespace-aware duplicate path warning * Duplicate path warning test additions * Trimpath reimplemented on dupe check * Solves a bug where slash was not being passed to the can write check * PR fixes * variable paths integration test fix now uses store * Seems far less hacky in retrospect * PR feedback addressed * test fixes after inclusion of path as local non-model var * Prevent confusion by dropping namespace from QPs on PUT, since its already in .data * Solves a harsh bug where you have namespace access but no secvars access (#14098) * Solves a harsh bug where you have namespace access but no secvars access * Lint cleanup * Remove unneeded condition --- ui/app/adapters/variable.js | 36 +++++--- ui/app/components/secure-variable-form.hbs | 5 +- ui/app/components/secure-variable-form.js | 50 +++++----- ui/app/components/variable-paths.hbs | 2 +- ui/app/components/variable-paths.js | 4 +- ui/app/controllers/variables/path.js | 25 +++-- ui/app/controllers/variables/variable.js | 8 +- ui/app/helpers/trim-path.js | 5 +- ui/app/models/variable.js | 4 +- ui/app/router.js | 2 +- ui/app/routes/variables.js | 6 +- ui/app/routes/variables/index.js | 20 ++-- ui/app/routes/variables/path.js | 20 ++-- ui/app/routes/variables/variable.js | 2 +- ui/app/serializers/variable.js | 9 +- ui/app/templates/variables/index.hbs | 50 +++++----- ui/app/templates/variables/path.hbs | 91 ++++++++++--------- ui/mirage/config.js | 8 +- ui/mirage/factories/variable.js | 6 +- ui/mirage/scenarios/default.js | 30 ++++-- ui/tests/acceptance/secure-variables-test.js | 16 ++-- .../components/secure-variable-form-test.js | 42 +++++++-- .../components/variable-paths-test.js | 48 ++++++---- ui/tests/unit/adapters/variable-test.js | 2 +- 24 files changed, 291 insertions(+), 200 deletions(-) diff --git a/ui/app/adapters/variable.js b/ui/app/adapters/variable.js index bccceaf45..b6db2ea37 100644 --- a/ui/app/adapters/variable.js +++ b/ui/app/adapters/variable.js @@ -8,13 +8,10 @@ export default class VariableAdapter extends ApplicationAdapter { // PUT instead of POST on create; // /v1/var instead of /v1/vars on create (urlForFindRecord) - createRecord(_store, _type, snapshot) { + createRecord(_store, type, snapshot) { let data = this.serialize(snapshot); - return this.ajax( - this.urlForFindRecord(snapshot.id, snapshot.modelName), - 'PUT', - { data } - ); + let baseUrl = this.buildURL(type.modelName, data.ID); + return this.ajax(baseUrl, 'PUT', { data }); } urlForFindAll(modelName) { @@ -27,21 +24,30 @@ export default class VariableAdapter extends ApplicationAdapter { return pluralize(baseUrl); } - urlForFindRecord(id, modelName, snapshot) { - const namespace = snapshot?.attr('namespace') || 'default'; - - let baseUrl = this.buildURL(modelName, id, snapshot); + urlForFindRecord(identifier, modelName, snapshot) { + const { namespace, id } = _extractIDAndNamespace(identifier, snapshot); + let baseUrl = this.buildURL(modelName, id); return `${baseUrl}?namespace=${namespace}`; } - urlForUpdateRecord(id, modelName) { - return this.buildURL(modelName, id); + urlForUpdateRecord(identifier, modelName, snapshot) { + const { id } = _extractIDAndNamespace(identifier, snapshot); + let baseUrl = this.buildURL(modelName, id); + return `${baseUrl}`; } - urlForDeleteRecord(id, modelName, snapshot) { - const namespace = snapshot?.attr('namespace') || 'default'; - + urlForDeleteRecord(identifier, modelName, snapshot) { + const { namespace, id } = _extractIDAndNamespace(identifier, snapshot); const baseUrl = this.buildURL(modelName, id); return `${baseUrl}?namespace=${namespace}`; } } + +function _extractIDAndNamespace(identifier, snapshot) { + const namespace = snapshot?.attr('namespace') || 'default'; + const id = snapshot?.attr('path') || identifier; + return { + namespace, + id, + }; +} diff --git a/ui/app/components/secure-variable-form.hbs b/ui/app/components/secure-variable-form.hbs index 16852784a..8c4449957 100644 --- a/ui/app/components/secure-variable-form.hbs +++ b/ui/app/components/secure-variable-form.hbs @@ -16,18 +16,17 @@ {{#if this.duplicatePathWarning}}

There is already a Secure Variable located at - {{@model.path}} + {{this.path}} .
Please choose a different path, or diff --git a/ui/app/components/secure-variable-form.js b/ui/app/components/secure-variable-form.js index bc6d048c6..e76d09ca3 100644 --- a/ui/app/components/secure-variable-form.js +++ b/ui/app/components/secure-variable-form.js @@ -23,18 +23,15 @@ export default class SecureVariableFormComponent extends Component { @service router; @service store; - /** - * @typedef {Object} DuplicatePathWarning - * @property {string} path - */ - - /** - * @type {DuplicatePathWarning} - */ - @tracked duplicatePathWarning = null; @tracked variableNamespace = null; @tracked namespaceOptions = null; + @tracked path = ''; + constructor() { + super(...arguments); + set(this, 'path', this.args.model.path); + } + @action setNamespace(namespace) { this.variableNamespace = namespace; @@ -52,9 +49,8 @@ export default class SecureVariableFormComponent extends Component { get shouldDisableSave() { const disallowedPath = - this.args.model?.path?.startsWith('nomad/') && - !this.args.model?.path?.startsWith('nomad/jobs'); - return !!this.JSONError || !this.args.model?.path || disallowedPath; + this.path?.startsWith('nomad/') && !this.path?.startsWith('nomad/jobs'); + return !!this.JSONError || !this.path || disallowedPath; } /** @@ -94,19 +90,28 @@ export default class SecureVariableFormComponent extends Component { ]); } - @action - validatePath(e) { - const value = trimPath([e.target.value]); + /** + * @typedef {Object} DuplicatePathWarning + * @property {string} path + */ + + /** + * @type {DuplicatePathWarning} + */ + get duplicatePathWarning() { const existingVariables = this.args.existingVariables || []; + const pathValue = trimPath([this.path]); let existingVariable = existingVariables .without(this.args.model) - .find((v) => v.path === value); + .find( + (v) => v.path === pathValue && v.namespace === this.variableNamespace + ); if (existingVariable) { - this.duplicatePathWarning = { + return { path: existingVariable.path, }; } else { - this.duplicatePathWarning = null; + return null; } } @@ -144,7 +149,7 @@ export default class SecureVariableFormComponent extends Component { if (e.type === 'submit') { e.preventDefault(); } - // TODO: temp, hacky way to force translation to tabular keyValues + if (this.view === 'json') { this.translateAndValidateItems('table'); } @@ -168,20 +173,21 @@ export default class SecureVariableFormComponent extends Component { } this.args.model.set('keyValues', this.keyValues); + this.args.model.set('path', this.path); this.args.model.setAndTrimPath(); await this.args.model.save(); this.flashMessages.add({ title: 'Secure Variable saved', - message: `${this.args.model.path} successfully saved`, + message: `${this.path} successfully saved`, type: 'success', destroyOnClick: false, timeout: 5000, }); - this.router.transitionTo('variables.variable', this.args.model.path); + this.router.transitionTo('variables.variable', this.args.model.id); } catch (error) { this.flashMessages.add({ - title: `Error saving ${this.args.model.path}`, + title: `Error saving ${this.path}`, message: error, type: 'error', destroyOnClick: false, diff --git a/ui/app/components/variable-paths.hbs b/ui/app/components/variable-paths.hbs index 668af2325..e4aea29bd 100644 --- a/ui/app/components/variable-paths.hbs +++ b/ui/app/components/variable-paths.hbs @@ -35,7 +35,7 @@ {{#if (can "read variable" path=file.absoluteFilePath namespace=file.variable.namespace)}} {{file.name}} diff --git a/ui/app/components/variable-paths.js b/ui/app/components/variable-paths.js index ea361dfc0..5252dfece 100644 --- a/ui/app/components/variable-paths.js +++ b/ui/app/components/variable-paths.js @@ -26,9 +26,9 @@ export default class VariablePathsComponent extends Component { } @action - async handleFileClick({ path, variable: { namespace } }) { + async handleFileClick({ path, variable: { id, namespace } }) { if (this.can.can('read variable', null, { path, namespace })) { - this.router.transitionTo('variables.variable', path); + this.router.transitionTo('variables.variable', id); } } } diff --git a/ui/app/controllers/variables/path.js b/ui/app/controllers/variables/path.js index 5b36c7fa9..8e312cbdb 100644 --- a/ui/app/controllers/variables/path.js +++ b/ui/app/controllers/variables/path.js @@ -4,16 +4,23 @@ import { action } from '@ember/object'; const ALL_NAMESPACE_WILDCARD = '*'; export default class VariablesPathController extends Controller { + get absolutePath() { + return this.model?.absolutePath || ''; + } get breadcrumbs() { - let crumbs = []; - this.model.absolutePath.split('/').reduce((m, n) => { - crumbs.push({ - label: n, - args: [`variables.path`, m + n], - }); - return m + n + '/'; - }, []); - return crumbs; + if (this.absolutePath) { + let crumbs = []; + this.absolutePath.split('/').reduce((m, n) => { + crumbs.push({ + label: n, + args: [`variables.path`, m + n], + }); + return m + n + '/'; + }, []); + return crumbs; + } else { + return []; + } } @controller variables; diff --git a/ui/app/controllers/variables/variable.js b/ui/app/controllers/variables/variable.js index bbb578fdd..c9dbd2366 100644 --- a/ui/app/controllers/variables/variable.js +++ b/ui/app/controllers/variables/variable.js @@ -3,12 +3,14 @@ import Controller from '@ember/controller'; export default class VariablesVariableController extends Controller { get breadcrumbs() { let crumbs = []; - this.params.path.split('/').reduce((m, n) => { + let id = decodeURI(this.params.id.split('@').slice(0, -1).join('@')); // remove namespace + let namespace = this.params.id.split('@').slice(-1)[0]; + id.split('/').reduce((m, n) => { crumbs.push({ label: n, args: - m + n === this.params.path // If the last crumb, link to the var itself - ? [`variables.variable`, m + n] + m + n === id // If the last crumb, link to the var itself + ? [`variables.variable`, `${m + n}@${namespace}`] : [`variables.path`, m + n], }); return m + n + '/'; diff --git a/ui/app/helpers/trim-path.js b/ui/app/helpers/trim-path.js index c9eb495c9..3b18cdfeb 100644 --- a/ui/app/helpers/trim-path.js +++ b/ui/app/helpers/trim-path.js @@ -6,11 +6,12 @@ import Helper from '@ember/component/helper'; * @param {Array} params * @returns {string} */ -export function trimPath([path = '']) { +export function trimPath([path]) { + if (!path) return; if (path.startsWith('/')) { path = trimPath([path.slice(1)]); } - if (path.endsWith('/')) { + if (path?.endsWith('/')) { path = trimPath([path.slice(0, -1)]); } return path; diff --git a/ui/app/models/variable.js b/ui/app/models/variable.js index 1e808f900..9d68a5182 100644 --- a/ui/app/models/variable.js +++ b/ui/app/models/variable.js @@ -67,7 +67,9 @@ export default class VariableModel extends Model { */ setAndTrimPath() { this.set('path', trimPath([this.path])); - this.set('id', this.get('path')); + if (!this.get('id')) { + this.set('id', `${this.get('path')}@${this.get('namespace')}`); + } } /** diff --git a/ui/app/router.js b/ui/app/router.js index ba7cfc5d5..dbedd67db 100644 --- a/ui/app/router.js +++ b/ui/app/router.js @@ -84,7 +84,7 @@ Router.map(function () { this.route( 'variable', { - path: '/var/*path', + path: '/var/*id', }, function () { this.route('edit'); diff --git a/ui/app/routes/variables.js b/ui/app/routes/variables.js index a1ca65917..702430021 100644 --- a/ui/app/routes/variables.js +++ b/ui/app/routes/variables.js @@ -1,7 +1,7 @@ import Route from '@ember/routing/route'; import { inject as service } from '@ember/service'; import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state'; -import notifyError from 'nomad-ui/utils/notify-error'; +import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; import PathTree from 'nomad-ui/utils/path-tree'; export default class VariablesRoute extends Route.extend(WithForbiddenState) { @@ -30,13 +30,13 @@ export default class VariablesRoute extends Route.extend(WithForbiddenState) { { namespace }, { reload: true } ); - return { variables, pathTree: new PathTree(variables), }; } catch (e) { - notifyError(this)(e); + notifyForbidden(this)(e); + return e; } } } diff --git a/ui/app/routes/variables/index.js b/ui/app/routes/variables/index.js index 4eac11a8b..81ddea51e 100644 --- a/ui/app/routes/variables/index.js +++ b/ui/app/routes/variables/index.js @@ -1,11 +1,19 @@ import Route from '@ember/routing/route'; +import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state'; +import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; -export default class VariablesIndexRoute extends Route { +export default class VariablesIndexRoute extends Route.extend( + WithForbiddenState +) { model() { - const { variables, pathTree } = this.modelFor('variables'); - return { - variables, - root: pathTree.paths.root, - }; + if (this.modelFor('variables').errors) { + notifyForbidden(this)(this.modelFor('variables')); + } else { + const { variables, pathTree } = this.modelFor('variables'); + return { + variables, + root: pathTree.paths.root, + }; + } } } diff --git a/ui/app/routes/variables/path.js b/ui/app/routes/variables/path.js index 97a99386b..220bd2754 100644 --- a/ui/app/routes/variables/path.js +++ b/ui/app/routes/variables/path.js @@ -1,12 +1,20 @@ import Route from '@ember/routing/route'; -export default class VariablesPathRoute extends Route { +import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state'; +import notifyForbidden from 'nomad-ui/utils/notify-forbidden'; +export default class VariablesPathRoute extends Route.extend( + WithForbiddenState +) { model({ absolutePath }) { - const treeAtPath = - this.modelFor('variables').pathTree.findPath(absolutePath); - if (treeAtPath) { - return { treeAtPath, absolutePath }; + if (this.modelFor('variables').errors) { + notifyForbidden(this)(this.modelFor('variables')); } else { - return { absolutePath }; + const treeAtPath = + this.modelFor('variables').pathTree.findPath(absolutePath); + if (treeAtPath) { + return { treeAtPath, absolutePath }; + } else { + return { absolutePath }; + } } } } diff --git a/ui/app/routes/variables/variable.js b/ui/app/routes/variables/variable.js index 3e01dafed..c49f520d8 100644 --- a/ui/app/routes/variables/variable.js +++ b/ui/app/routes/variables/variable.js @@ -9,7 +9,7 @@ export default class VariablesVariableRoute extends Route.extend( @service store; model(params) { return this.store - .findRecord('variable', decodeURIComponent(params.path), { + .findRecord('variable', decodeURIComponent(params.id), { reload: true, }) .catch(notifyForbidden(this)); diff --git a/ui/app/serializers/variable.js b/ui/app/serializers/variable.js index b021510f8..142ac660e 100644 --- a/ui/app/serializers/variable.js +++ b/ui/app/serializers/variable.js @@ -3,12 +3,16 @@ import ApplicationSerializer from './application'; @classic export default class VariableSerializer extends ApplicationSerializer { - primaryKey = 'Path'; separateNanos = ['CreateTime', 'ModifyTime']; + normalize(typeHash, hash) { + // ID is a composite of both the job ID and the namespace the job is in + hash.ID = `${hash.Path}@${hash.Namespace || 'default'}`; + return super.normalize(typeHash, hash); + } + // Transform API's Items object into an array of a KeyValue objects normalizeFindRecordResponse(store, typeClass, hash, id, ...args) { - // TODO: prevent items-less saving at API layer if (!hash.Items) { hash.Items = { '': '' }; } @@ -31,6 +35,7 @@ export default class VariableSerializer extends ApplicationSerializer { // Transform our KeyValues array into an Items object serialize(snapshot, options) { const json = super.serialize(snapshot, options); + json.ID = json.Path; json.Items = json.KeyValues.reduce((acc, { key, value }) => { acc[key] = value; return acc; diff --git a/ui/app/templates/variables/index.hbs b/ui/app/templates/variables/index.hbs index f6a3db40b..dbee80dba 100644 --- a/ui/app/templates/variables/index.hbs +++ b/ui/app/templates/variables/index.hbs @@ -35,32 +35,36 @@ - {{#if this.hasVariables}} - + {{#if this.isForbidden}} + {{else}} -

- {{#if (eq this.namespaceSelection "*")}} -

- No Secure Variables -

- {{#if (can "write variable" path="*" namespace=this.namespaceSelection)}} + {{#if this.hasVariables}} + + {{else}} +
+ {{#if (eq this.namespaceSelection "*")}} +

+ No Secure Variables +

+ {{#if (can "write variable" path="*" namespace=this.namespaceSelection)}} +

+ Get started by creating a new secure variable +

+ {{/if}} + {{else}} +

+ No Matches +

- Get started by creating a new secure variable + No paths or variables match the namespace + + {{this.namespaceSelection}} +

{{/if}} - {{else}} -

- No Matches -

-

- No paths or variables match the namespace - - {{this.namespaceSelection}} - -

- {{/if}} -
+
+ {{/if}} {{/if}} diff --git a/ui/app/templates/variables/path.hbs b/ui/app/templates/variables/path.hbs index 95fb011f7..f4838b423 100644 --- a/ui/app/templates/variables/path.hbs +++ b/ui/app/templates/variables/path.hbs @@ -1,4 +1,4 @@ -{{page-title "Secure Variables: " this.model.absolutePath}} +{{page-title "Secure Variables: " this.absolutePath}} {{#each this.breadcrumbs as |crumb|}} {{/each}} @@ -15,52 +15,55 @@ /> {{/if}}
- {{#if (can "write variable" path=this.model.absolutePath namespace=this.namespaceSelection)}} - - Create Secure Variable - - {{else}} - - {{/if}} - + {{#if (can "write variable" path=(concat this.absolutePath "/") namespace=this.namespaceSelection)}} + + Create Secure Variable + + {{else}} + + {{/if}}
-{{#if this.model.treeAtPath}} - -{{else}} -
- {{#if (eq this.namespaceSelection "*")}} -

- Path /{{this.model.absolutePath}} contains no variables -

-

- To get started, create a new secure variable here, or go back to the Secure Variables root directory. -

+ {{#if this.isForbidden}} + {{else}} -

- No Matches -

-

- No paths or variables match the namespace - - {{this.namespaceSelection}} - -

+ {{#if this.model.treeAtPath}} + + {{else}} +
+ {{#if (eq this.namespaceSelection "*")}} +

+ Path /{{this.absolutePath}} contains no variables +

+

+ To get started, create a new secure variable here, or go back to the Secure Variables root directory. +

+ {{else}} +

+ No Matches +

+

+ No paths or variables match the namespace + + {{this.namespaceSelection}} + +

+ {{/if}} +
+ {{/if}} {{/if}} -
-{{/if}} diff --git a/ui/mirage/config.js b/ui/mirage/config.js index d10f1eaa3..3bf133b98 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -853,9 +853,9 @@ export default function () { this.put('/var/:id', function (schema, request) { const { Path, Namespace, Items } = JSON.parse(request.requestBody); return server.create('variable', { - Path, - Namespace, - Items, + path: Path, + namespace: Namespace, + items: Items, id: Path, }); }); @@ -863,7 +863,7 @@ export default function () { this.delete('/var/:id', function (schema, request) { const { id } = request.params; server.db.variables.remove(id); - return okEmpty(); + return ''; }); //#endregion Secure Variables diff --git a/ui/mirage/factories/variable.js b/ui/mirage/factories/variable.js index 77eb0fd3b..ad1dd8449 100644 --- a/ui/mirage/factories/variable.js +++ b/ui/mirage/factories/variable.js @@ -25,15 +25,11 @@ export default Factory.extend({ }, afterCreate(variable, server) { - if (!variable.namespaceId) { + if (!variable.namespace) { const namespace = pickOne(server.db.jobs)?.namespace || 'default'; variable.update({ namespace, }); - } else { - variable.update({ - namespace: variable.namespaceId, - }); } }, }); diff --git a/ui/mirage/scenarios/default.js b/ui/mirage/scenarios/default.js index 125bc0e17..76dd7f780 100644 --- a/ui/mirage/scenarios/default.js +++ b/ui/mirage/scenarios/default.js @@ -80,17 +80,17 @@ function smallCluster(server) { server.create('variable', { id: `nomad/jobs/${variableLinkedJob.id}/${variableLinkedGroup.name}/${variableLinkedTask.name}`, - namespaceId: variableLinkedJob.namespace, + namespace: variableLinkedJob.namespace, }); server.create('variable', { id: `nomad/jobs/${variableLinkedJob.id}/${variableLinkedGroup.name}`, - namespaceId: variableLinkedJob.namespace, + namespace: variableLinkedJob.namespace, }); server.create('variable', { id: `nomad/jobs/${variableLinkedJob.id}`, - namespaceId: variableLinkedJob.namespace, + namespace: variableLinkedJob.namespace, }); // #region evaluations @@ -200,24 +200,36 @@ function variableTestCluster(server) { 'w/x/y/foo9', 'w/x/y/z/foo10', 'w/x/y/z/bar11', - 'just some arbitrary file', - 'another arbitrary file', - 'another arbitrary file again', ].forEach((path) => server.create('variable', { id: path })); server.create('variable', { id: `nomad/jobs/${variableLinkedJob.id}/${variableLinkedGroup.name}/${variableLinkedTask.name}`, - namespaceId: variableLinkedJob.namespace, + namespace: variableLinkedJob.namespace, }); server.create('variable', { id: `nomad/jobs/${variableLinkedJob.id}/${variableLinkedGroup.name}`, - namespaceId: variableLinkedJob.namespace, + namespace: variableLinkedJob.namespace, }); server.create('variable', { id: `nomad/jobs/${variableLinkedJob.id}`, - namespaceId: variableLinkedJob.namespace, + namespace: variableLinkedJob.namespace, + }); + + server.create('variable', { + id: 'just some arbitrary file', + namespace: 'namespace-2', + }); + + server.create('variable', { + id: 'another arbitrary file', + namespace: 'namespace-2', + }); + + server.create('variable', { + id: 'another arbitrary file again', + namespace: 'namespace-2', }); } diff --git a/ui/tests/acceptance/secure-variables-test.js b/ui/tests/acceptance/secure-variables-test.js index 7cf0a1727..66474d795 100644 --- a/ui/tests/acceptance/secure-variables-test.js +++ b/ui/tests/acceptance/secure-variables-test.js @@ -103,9 +103,8 @@ module('Acceptance | secure variables', function (hooks) { await percySnapshot(assert); await click(fooLink); - assert.equal( - currentURL(), - '/variables/var/a/b/c/foo0', + assert.ok( + currentURL().includes('/variables/var/a/b/c/foo0'), 'correctly traverses to a deeply nested variable file' ); const deleteButton = find('[data-test-delete-button] button'); @@ -173,9 +172,8 @@ module('Acceptance | secure variables', function (hooks) { assert.ok(nonJobLink, 'non-job file is present'); await click(nonJobLink); - assert.equal( - currentURL(), - '/variables/var/just some arbitrary file', + assert.ok( + currentURL().includes('/variables/var/just some arbitrary file'), 'correctly traverses to a non-job file' ); let relatedEntitiesBox = find('.related-entities'); @@ -335,7 +333,10 @@ module('Acceptance | secure variables', function (hooks) { await click('button[type="submit"]'); assert.dom('.flash-message.alert-success').exists(); - assert.equal(currentURL(), '/variables/var/foo/bar'); + assert.ok( + currentURL().includes('/variables/var/foo'), + 'drops you back off to the parent page' + ); }); test('it passes an accessibility audit', async function (assert) { @@ -514,7 +515,6 @@ module('Acceptance | secure variables', function (hooks) { await typeIn('[data-test-var-key]', 'kiki'); await typeIn('[data-test-var-value]', 'do you love me'); await click('[data-test-submit-var]'); - assert.equal( currentRouteName(), 'variables.variable.index', diff --git a/ui/tests/integration/components/secure-variable-form-test.js b/ui/tests/integration/components/secure-variable-form-test.js index 8e18c159a..43da21e7b 100644 --- a/ui/tests/integration/components/secure-variable-form-test.js +++ b/ui/tests/integration/components/secure-variable-form-test.js @@ -7,6 +7,10 @@ import { setupMirage } from 'ember-cli-mirage/test-support'; import setupCodeMirror from 'nomad-ui/tests/helpers/codemirror'; import { codeFillable, code } from 'nomad-ui/tests/pages/helpers/codemirror'; import percySnapshot from '@percy/ember'; +import { + selectChoose, + clickTrigger, +} from 'ember-power-select/test-support/helpers'; module('Integration | Component | secure-variable-form', function (hooks) { setupRenderingTest(hooks); @@ -219,6 +223,8 @@ module('Integration | Component | secure-variable-form', function (hooks) { module('Validation', function () { test('warns when you try to create a path that already exists', async function (assert) { + this.server.createList('namespace', 3); + this.set( 'mockedModel', server.create('variable', { @@ -227,23 +233,41 @@ module('Integration | Component | secure-variable-form', function (hooks) { }) ); - this.set( - 'existingVariables', - server.createList('variable', 1, { - path: 'baz/bat', - }) - ); + server.create('variable', { + path: 'baz/bat', + }); + server.create('variable', { + path: 'baz/bat/qux', + namespace: server.db.namespaces[2].id, + }); + + this.set('existingVariables', server.db.variables.toArray()); await render( hbs`` ); - await typeIn('.path-input', 'baz/bat'); - assert.dom('.duplicate-path-error').exists(); - assert.dom('.path-input').hasClass('error'); await typeIn('.path-input', 'foo/bar'); assert.dom('.duplicate-path-error').doesNotExist(); assert.dom('.path-input').doesNotHaveClass('error'); + + document.querySelector('.path-input').value = ''; // clear current input + await typeIn('.path-input', 'baz/bat'); + assert.dom('.duplicate-path-error').exists(); + assert.dom('.path-input').hasClass('error'); + + await clickTrigger('[data-test-variable-namespace-filter]'); + await selectChoose( + '[data-test-variable-namespace-filter]', + server.db.namespaces[2].id + ); + assert.dom('.duplicate-path-error').doesNotExist(); + assert.dom('.path-input').doesNotHaveClass('error'); + + document.querySelector('.path-input').value = ''; // clear current input + await typeIn('.path-input', 'baz/bat/qux'); + assert.dom('.duplicate-path-error').exists(); + assert.dom('.path-input').hasClass('error'); }); test('warns you when you set a key with . in it', async function (assert) { diff --git a/ui/tests/integration/components/variable-paths-test.js b/ui/tests/integration/components/variable-paths-test.js index fedfbf0df..887ed3b52 100644 --- a/ui/tests/integration/components/variable-paths-test.js +++ b/ui/tests/integration/components/variable-paths-test.js @@ -6,26 +6,34 @@ import { hbs } from 'ember-cli-htmlbars'; import { componentA11yAudit } from 'nomad-ui/tests/helpers/a11y-audit'; import pathTree from 'nomad-ui/utils/path-tree'; import Service from '@ember/service'; - -const PATHSTRINGS = [ - { path: '/foo/bar/baz' }, - { path: '/foo/bar/bay' }, - { path: '/foo/bar/bax' }, - { path: '/a/b' }, - { path: '/a/b/c' }, - { path: '/a/b/canary' }, - { path: '/a/b/canine' }, - { path: '/a/b/chipmunk' }, - { path: '/a/b/c/d' }, - { path: '/a/b/c/dalmation/index' }, - { path: '/a/b/c/doberman/index' }, - { path: '/a/b/c/dachshund/index' }, - { path: '/a/b/c/dachshund/poppy' }, -]; -const tree = new pathTree(PATHSTRINGS); +let tree; module('Integration | Component | variable-paths', function (hooks) { setupRenderingTest(hooks); + hooks.beforeEach(function () { + this.store = this.owner.lookup('service:store'); + + const PATHSTRINGS = [ + { path: '/foo/bar/baz' }, + { path: '/foo/bar/bay' }, + { path: '/foo/bar/bax' }, + { path: '/a/b' }, + { path: '/a/b/c' }, + { path: '/a/b/canary' }, + { path: '/a/b/canine' }, + { path: '/a/b/chipmunk' }, + { path: '/a/b/c/d' }, + { path: '/a/b/c/dalmation/index' }, + { path: '/a/b/c/doberman/index' }, + { path: '/a/b/c/dachshund/index' }, + { path: '/a/b/c/dachshund/poppy' }, + ].map((x) => { + const varInstance = this.store.createRecord('variable', x); + varInstance.setAndTrimPath(); + return varInstance; + }); + tree = new pathTree(PATHSTRINGS); + }); test('it renders without data', async function (assert) { assert.expect(2); @@ -109,21 +117,21 @@ module('Integration | Component | variable-paths', function (hooks) { .dom('tbody tr:first-child td:first-child a') .hasAttribute( 'href', - '/ui/variables/var/foo/bar/baz', + '/ui/variables/var/foo/bar/baz@default', 'Correctly links the first file' ); assert .dom('tbody tr:nth-child(2) td:first-child a') .hasAttribute( 'href', - '/ui/variables/var/foo/bar/bay', + '/ui/variables/var/foo/bar/bay@default', 'Correctly links the second file' ); assert .dom('tbody tr:nth-child(3) td:first-child a') .hasAttribute( 'href', - '/ui/variables/var/foo/bar/bax', + '/ui/variables/var/foo/bar/bax@default', 'Correctly links the third file' ); assert diff --git a/ui/tests/unit/adapters/variable-test.js b/ui/tests/unit/adapters/variable-test.js index 1f9ca8f61..bf650a6de 100644 --- a/ui/tests/unit/adapters/variable-test.js +++ b/ui/tests/unit/adapters/variable-test.js @@ -12,7 +12,7 @@ module('Unit | Adapter | Variable', function (hooks) { // we're incorrectly passing an object with a `Model` interface // we should be passing a `Snapshot` // hacky fix to rectify the issue - newVariable.attr = () => 'default'; + newVariable.attr = () => {}; assert.equal( this.subject().urlForFindAll('variable'),