From 334e16a6cf2d149cf0319a0733cf20aa2d517bfe Mon Sep 17 00:00:00 2001 From: John Cowen Date: Tue, 4 Jun 2019 15:57:35 +0100 Subject: [PATCH] ui: Ensures nested policy forms are reset properly (#5838) 1. All {{ivy-codemirror}} components need 'refreshing' when they become visible via our own `didAppear` method on the `{{code-editor}}` component (also see:) - https://github.com/hashicorp/consul/pull/4190#discussion_r193270223 - https://github.com/hashicorp/consul/pull/4789/files/73db111db8324e1c6710791b38ba1cd2bab08f15#r225264296 2. On initial investigation, it looks like the component we are using for the code editor doesn't distinguish between setting its `value` programatically and a `keyup` event, i.e. an interaction from the user. We currently pretend that whenever its `value` changes, it is a `keyup` event. This means that when we reset the `value` to `""` programmatically for form resetting purposes, a 'pretend keyup' event would also be fired, which would in turn kick off the validation, which would fail and show an error message for empty values in other fields of the form - something that is perfectly valid if you haven't typed anything yet. We solved this by checking for `isPristine` on fields that are allowed to be empty before you have typed anything. --- ui-v2/app/components/policy-selector.js | 3 ++ ui-v2/app/components/role-selector.js | 18 ++++++--- .../app/templates/components/policy-form.hbs | 4 +- .../templates/components/policy-selector.hbs | 2 +- .../dc/acls/policies/as-many/reset.feature | 37 +++++++++++++++++++ .../dc/acls/policies/as-many/reset-steps.js | 10 +++++ ui-v2/tests/steps.js | 2 + ui-v2/tests/steps/assertions/form.js | 21 +++++++++++ 8 files changed, 88 insertions(+), 9 deletions(-) create mode 100644 ui-v2/tests/acceptance/dc/acls/policies/as-many/reset.feature create mode 100644 ui-v2/tests/acceptance/steps/dc/acls/policies/as-many/reset-steps.js create mode 100644 ui-v2/tests/steps/assertions/form.js diff --git a/ui-v2/app/components/policy-selector.js b/ui-v2/app/components/policy-selector.js index 295afce42..66210aaec 100644 --- a/ui-v2/app/components/policy-selector.js +++ b/ui-v2/app/components/policy-selector.js @@ -61,6 +61,9 @@ export default ChildSelectorComponent.extend({ } }, actions: { + open: function(e) { + this.refreshCodeEditor(e, e.target.parentElement); + }, loadItem: function(e, item, items) { const target = e.target; // the Details expander toggle, only load on opening diff --git a/ui-v2/app/components/role-selector.js b/ui-v2/app/components/role-selector.js index f1243ad17..8d88ea36e 100644 --- a/ui-v2/app/components/role-selector.js +++ b/ui-v2/app/components/role-selector.js @@ -1,25 +1,25 @@ import ChildSelectorComponent from './child-selector'; import { inject as service } from '@ember/service'; import { get, set } from '@ember/object'; - import { alias } from '@ember/object/computed'; import { CallableEventSource as EventSource } from 'consul-ui/utils/dom/event-source'; export default ChildSelectorComponent.extend({ repo: service('repository/role/component'), + dom: service('dom'), name: 'role', type: 'role', classNames: ['role-selector'], state: 'role', + // You have to alias data. + // If you just set it, it loses its reference? + policy: alias('policyForm.data'), init: function() { this._super(...arguments); this.policyForm = get(this, 'formContainer').form('policy'); this.source = new EventSource(); }, - // You have to alias data - // is you just set it it loses its reference? - policy: alias('policyForm.data'), actions: { reset: function(e) { this._super(...arguments); @@ -30,9 +30,15 @@ export default ChildSelectorComponent.extend({ }, change: function() { const event = get(this, 'dom').normalizeEvent(...arguments); - switch (event.target.name) { + const target = event.target; + switch (target.name) { case 'role[state]': - set(this, 'state', event.target.value); + set(this, 'state', target.value); + if (target.value === 'policy') { + get(this, 'dom') + .component('.code-editor', target.nextElementSibling) + .didAppear(); + } break; default: this._super(...arguments); diff --git a/ui-v2/app/templates/components/policy-form.hbs b/ui-v2/app/templates/components/policy-form.hbs index c573ee5a1..6b20175e3 100644 --- a/ui-v2/app/templates/components/policy-form.hbs +++ b/ui-v2/app/templates/components/policy-form.hbs @@ -18,13 +18,13 @@ {{/each}} {{/yield-slot}} - {{!TODO: potentially call trigger something else}} {{!the modal has to go here so that if you provide a slot to trigger it doesn't get rendered}} - {{#modal-dialog data-test-policy-form name="new-policy-toggle"}} + {{#modal-dialog data-test-policy-form onopen=(action 'open') name="new-policy-toggle"}} {{#block-slot 'header'}}

New Policy

{{/block-slot}} diff --git a/ui-v2/tests/acceptance/dc/acls/policies/as-many/reset.feature b/ui-v2/tests/acceptance/dc/acls/policies/as-many/reset.feature new file mode 100644 index 000000000..12e5eea6e --- /dev/null +++ b/ui-v2/tests/acceptance/dc/acls/policies/as-many/reset.feature @@ -0,0 +1,37 @@ +@setupApplicationTest +Feature: dc / acls / policies / as many / reset: Reset policy form + Background: + Given 1 datacenter model with the value "datacenter" + And 1 [Model] model from yaml + --- + Policies: ~ + ServiceIdentities: ~ + --- + When I visit the [Model] page for yaml + --- + dc: datacenter + [Model]: key + --- + Then the url should be /datacenter/acls/[Model]s/key + And I click policies.create + Scenario: Adding a new policy as a child of [Model] + Then I fill in the policies.form form with yaml + --- + Name: New-Policy + Description: New Policy Description + Rules: operator {} + --- + And I click cancel on the policies.form + And I click policies.create + Then I see the policies.form form with yaml + --- + Name: "" + Description: "" + Rules: "" + --- + Where: + ------------- + | Model | + | token | + | role | + ------------- diff --git a/ui-v2/tests/acceptance/steps/dc/acls/policies/as-many/reset-steps.js b/ui-v2/tests/acceptance/steps/dc/acls/policies/as-many/reset-steps.js new file mode 100644 index 000000000..550f68858 --- /dev/null +++ b/ui-v2/tests/acceptance/steps/dc/acls/policies/as-many/reset-steps.js @@ -0,0 +1,10 @@ +import steps from '../../../../steps'; + +// step definitions that are shared between features should be moved to the +// tests/acceptance/steps/steps.js file + +export default function(assert) { + return steps(assert).then('I should find a file', function() { + assert.ok(true, this.step); + }); +} diff --git a/ui-v2/tests/steps.js b/ui-v2/tests/steps.js index 3bec04174..a34bce897 100644 --- a/ui-v2/tests/steps.js +++ b/ui-v2/tests/steps.js @@ -8,6 +8,7 @@ import assertHttp from './steps/assertions/http'; import assertModel from './steps/assertions/model'; import assertPage from './steps/assertions/page'; import assertDom from './steps/assertions/dom'; +import assertForm from './steps/assertions/form'; // const dont = `( don't| shouldn't| can't)?`; @@ -79,6 +80,7 @@ export default function(assert, library, pages, utils) { assertModel(library, assert, find, getCurrentPage, pauseUntil, utils.pluralize); assertPage(library, assert, find, getCurrentPage); assertDom(library, assert, pauseUntil, utils.find, utils.currentURL); + assertForm(library, assert, find, getCurrentPage); return library.given(["I'm using a legacy token"], function(number, model, data) { window.localStorage['consul:token'] = JSON.stringify({ AccessorID: null, SecretID: 'id' }); diff --git a/ui-v2/tests/steps/assertions/form.js b/ui-v2/tests/steps/assertions/form.js new file mode 100644 index 000000000..9c4ddec69 --- /dev/null +++ b/ui-v2/tests/steps/assertions/form.js @@ -0,0 +1,21 @@ +export default function(scenario, assert, find, currentPage) { + scenario.then('I see the $property form with yaml\n$yaml', function(property, data) { + try { + let obj; + try { + obj = find(property); + } catch (e) { + obj = currentPage(); + } + return Object.keys(data).reduce(function(prev, item, i, arr) { + const name = `${obj.prefix || property}[${item}]`; + const $el = document.querySelector(`[name="${name}"]`); + const actual = $el.value; + const expected = data[item]; + assert.strictEqual(actual, expected, `Expected settings to be ${expected} was ${actual}`); + }, obj); + } catch (e) { + throw e; + } + }); +}