From d4f3fba56e66a9316f2641f2f21196367c571db1 Mon Sep 17 00:00:00 2001
From: claire bontempo <68122737+hellobontempo@users.noreply.github.com>
Date: Wed, 25 May 2022 11:22:36 -0700
Subject: [PATCH] UI/Fix form validation issues (#15560)

* clean up validators

* fix getter overriding user input

* add changelog

* remove asString option

* move invalid check up

* remove asString everywhere

* revert input value defaults

* undo form disabling if validation errors

* address comments

* remove or

* add validation message to form, create pseudo loading icon

* whole alert disappears with refresh

* glimmerize alert-inline

* add tests

* rename variables for consistency

* spread attributes to glimmerized component

* address comments

* add validation test
---
 changelog/15560.txt                           |  3 +
 ui/app/components/mount-backend-form.js       | 23 ++++--
 ui/app/decorators/model-validations.js        | 12 ++-
 ui/app/models/secret-engine.js                |  2 +-
 ui/app/models/secret-v2.js                    |  2 +-
 .../components/mount-backend-form.hbs         |  7 +-
 ui/app/utils/validators.js                    | 22 +++---
 ui/lib/core/addon/components/alert-inline.hbs | 19 +++++
 ui/lib/core/addon/components/alert-inline.js  | 65 ++++++++++------
 ui/lib/core/addon/components/form-field.js    |  2 +-
 .../templates/components/alert-inline.hbs     |  8 --
 .../components/alert-inline-test.js           | 77 +++++++++++++++++--
 .../unit/decorators/model-validations-test.js | 29 +++++++
 ui/tests/unit/utils/validators-test.js        | 44 ++++++++---
 14 files changed, 245 insertions(+), 70 deletions(-)
 create mode 100644 changelog/15560.txt
 create mode 100644 ui/lib/core/addon/components/alert-inline.hbs
 delete mode 100644 ui/lib/core/addon/templates/components/alert-inline.hbs

diff --git a/changelog/15560.txt b/changelog/15560.txt
new file mode 100644
index 000000000..387d2a848
--- /dev/null
+++ b/changelog/15560.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ui: fix form validations ignoring default values and disabling submit button 
+```
diff --git a/ui/app/components/mount-backend-form.js b/ui/app/components/mount-backend-form.js
index b23a7ffc5..263981141 100644
--- a/ui/app/components/mount-backend-form.js
+++ b/ui/app/components/mount-backend-form.js
@@ -47,7 +47,7 @@ export default Component.extend({
 
   // validation related properties
   modelValidations: null,
-  isFormInvalid: false,
+  invalidFormAlert: null,
 
   mountIssue: false,
 
@@ -88,10 +88,24 @@ export default Component.extend({
     }
   },
 
+  checkModelValidity(model) {
+    const { isValid, state, invalidFormMessage } = model.validate();
+    this.setProperties({
+      modelValidations: state,
+      invalidFormAlert: invalidFormMessage,
+    });
+
+    return isValid;
+  },
+
   mountBackend: task(
     waitFor(function* () {
       const mountModel = this.mountModel;
       const { type, path } = mountModel;
+      // only submit form if validations pass
+      if (!this.checkModelValidity(mountModel)) {
+        return;
+      }
       let capabilities = null;
       try {
         capabilities = yield this.store.findRecord('capabilities', `${path}/config`);
@@ -120,7 +134,6 @@ export default Component.extend({
       } catch (err) {
         if (err.httpStatus === 403) {
           this.mountIssue = true;
-          this.set('isFormInvalid', this.mountIssue);
           this.flashMessages.danger(
             'You do not have access to the sys/mounts endpoint. The secret engine was not mounted.'
           );
@@ -163,12 +176,8 @@ export default Component.extend({
   actions: {
     onKeyUp(name, value) {
       this.mountModel.set(name, value);
-      const { isValid, state } = this.mountModel.validate();
-      this.setProperties({
-        modelValidations: state,
-        isFormInvalid: !isValid,
-      });
     },
+
     onTypeChange(path, value) {
       if (path === 'type') {
         this.wizard.set('componentState', value);
diff --git a/ui/app/decorators/model-validations.js b/ui/app/decorators/model-validations.js
index fae6e8f70..8520811da 100644
--- a/ui/app/decorators/model-validations.js
+++ b/ui/app/decorators/model-validations.js
@@ -69,6 +69,7 @@ export function withModelValidations(validations) {
       validate() {
         let isValid = true;
         const state = {};
+        let errorCount = 0;
 
         for (const key in this._validations) {
           const rules = this._validations[key];
@@ -110,9 +111,18 @@ export function withModelValidations(validations) {
               }
             }
           }
+          errorCount += state[key].errors.length;
           state[key].isValid = !state[key].errors.length;
         }
-        return { isValid, state };
+
+        return { isValid, state, invalidFormMessage: this.generateErrorCountMessage(errorCount) };
+      }
+
+      generateErrorCountMessage(errorCount) {
+        if (errorCount < 1) return null;
+        // returns count specific message: 'There is an error/are N errors with this form.'
+        let isPlural = errorCount > 1 ? `are ${errorCount} errors` : false;
+        return `There ${isPlural ? isPlural : 'is an error'} with this form.`;
       }
     };
   };
diff --git a/ui/app/models/secret-engine.js b/ui/app/models/secret-engine.js
index a51983fd8..2fb064c37 100644
--- a/ui/app/models/secret-engine.js
+++ b/ui/app/models/secret-engine.js
@@ -12,7 +12,7 @@ const LIST_EXCLUDED_BACKENDS = ['system', 'identity'];
 const validations = {
   path: [{ type: 'presence', message: "Path can't be blank." }],
   maxVersions: [
-    { type: 'number', options: { asString: true }, message: 'Maximum versions must be a number.' },
+    { type: 'number', message: 'Maximum versions must be a number.' },
     { type: 'length', options: { min: 1, max: 16 }, message: 'You cannot go over 16 characters.' },
   ],
 };
diff --git a/ui/app/models/secret-v2.js b/ui/app/models/secret-v2.js
index 1f95d6e3d..c4945fcce 100644
--- a/ui/app/models/secret-v2.js
+++ b/ui/app/models/secret-v2.js
@@ -8,7 +8,7 @@ import { withModelValidations } from 'vault/decorators/model-validations';
 
 const validations = {
   maxVersions: [
-    { type: 'number', options: { asString: true }, message: 'Maximum versions must be a number.' },
+    { type: 'number', message: 'Maximum versions must be a number.' },
     { type: 'length', options: { min: 1, max: 16 }, message: 'You cannot go over 16 characters.' },
   ],
 };
diff --git a/ui/app/templates/components/mount-backend-form.hbs b/ui/app/templates/components/mount-backend-form.hbs
index 10f2a8a47..4bc343cb7 100644
--- a/ui/app/templates/components/mount-backend-form.hbs
+++ b/ui/app/templates/components/mount-backend-form.hbs
@@ -67,7 +67,7 @@
           type="submit"
           data-test-mount-submit="true"
           class="button is-primary {{if this.mountBackend.isRunning 'loading'}}"
-          disabled={{or this.mountBackend.isRunning this.isFormInvalid}}
+          disabled={{this.mountBackend.isRunning}}
         >
           {{#if (eq this.mountType "auth")}}
             Enable Method
@@ -81,6 +81,11 @@
           Back
         </button>
       </div>
+      {{#if this.invalidFormAlert}}
+        <div class="control">
+          <AlertInline @type="danger" @paddingTop={{true}} @message={{this.invalidFormAlert}} @mimicRefresh={{true}} />
+        </div>
+      {{/if}}
     {{else}}
       <button
         data-test-mount-next
diff --git a/ui/app/utils/validators.js b/ui/app/utils/validators.js
index 8f2aed978..7533c6f9f 100644
--- a/ui/app/utils/validators.js
+++ b/ui/app/utils/validators.js
@@ -3,20 +3,20 @@ import { isPresent } from '@ember/utils';
 export const presence = (value) => isPresent(value);
 
 export const length = (value, { nullable = false, min, max } = {}) => {
-  let isValid = nullable;
-  if (typeof value === 'string') {
-    const underMin = min && value.length < min;
-    const overMax = max && value.length > max;
-    isValid = underMin || overMax ? false : true;
+  if (!min && !max) return;
+  // value could be an integer if the attr has a default value of some number
+  const valueLength = value?.toString().length;
+  if (valueLength) {
+    const underMin = min && valueLength < min;
+    const overMax = max && valueLength > max;
+    return underMin || overMax ? false : true;
   }
-  return isValid;
+  return nullable;
 };
 
-export const number = (value, { nullable = false, asString } = {}) => {
-  if (!value) return nullable;
-  if (typeof value === 'string' && !asString) {
-    return false;
-  }
+export const number = (value, { nullable = false } = {}) => {
+  // since 0 is falsy, !value returns true even though 0 is a valid number
+  if (!value && value !== 0) return nullable;
   return !isNaN(value);
 };
 
diff --git a/ui/lib/core/addon/components/alert-inline.hbs b/ui/lib/core/addon/components/alert-inline.hbs
new file mode 100644
index 000000000..2cd80383d
--- /dev/null
+++ b/ui/lib/core/addon/components/alert-inline.hbs
@@ -0,0 +1,19 @@
+<div
+  {{did-update this.refresh @message}}
+  class={{concat "message-inline" this.paddingTop this.isMarginless this.sizeSmall}}
+  data-test-inline-alert
+  ...attributes
+>
+  {{#if this.isRefreshing}}
+    <Icon @name="loading" class="loading" />
+  {{else}}
+    <Icon @name={{this.alertType.glyph}} class={{this.alertType.glyphClass}} />
+    <p class={{this.textClass}} data-test-inline-error-message>
+      {{#if (has-block)}}
+        {{yield}}
+      {{else}}
+        {{@message}}
+      {{/if}}
+    </p>
+  {{/if}}
+</div>
\ No newline at end of file
diff --git a/ui/lib/core/addon/components/alert-inline.js b/ui/lib/core/addon/components/alert-inline.js
index 89e850133..ec670d849 100644
--- a/ui/lib/core/addon/components/alert-inline.js
+++ b/ui/lib/core/addon/components/alert-inline.js
@@ -1,7 +1,8 @@
-import Component from '@ember/component';
-import { computed } from '@ember/object';
+import Component from '@glimmer/component';
+import { action } from '@ember/object';
+import { later } from '@ember/runloop';
+import { tracked } from '@glimmer/tracking';
 import { messageTypes } from 'core/helpers/message-types';
-import layout from '../templates/components/alert-inline';
 
 /**
  * @module AlertInline
@@ -12,31 +13,51 @@ import layout from '../templates/components/alert-inline';
  * <AlertInline @type="danger" @message="{{model.keyId}} is not a valid lease ID"/>
  * ```
  *
- * @param type=null{String} - The alert type. This comes from the message-types helper.
+ * @param type=null{String} - The alert type passed to the message-types helper.
  * @param [message=null]{String} - The message to display within the alert.
- * @param [sizeSmall=false]{Boolean} - Whether or not to display a small font with padding below of alert message.
  * @param [paddingTop=false]{Boolean} - Whether or not to add padding above component.
  * @param [isMarginless=false]{Boolean} - Whether or not to remove margin bottom below component.
+ * @param [sizeSmall=false]{Boolean} - Whether or not to display a small font with padding below of alert message.
+ * @param [mimicRefresh=false]{Boolean} - If true will display a loading icon when attributes change (e.g. when a form submits and the alert message changes).
  */
 
-export default Component.extend({
-  layout,
-  type: null,
-  message: null,
-  sizeSmall: false,
-  paddingTop: false,
-  classNames: ['message-inline'],
-  classNameBindings: ['sizeSmall:size-small', 'paddingTop:padding-top', 'isMarginless:is-marginless'],
+export default class AlertInlineComponent extends Component {
+  @tracked isRefreshing = false;
 
-  textClass: computed('type', function () {
-    if (this.type == 'danger') {
-      return messageTypes([this.type]).glyphClass;
+  get mimicRefresh() {
+    return this.args.mimicRefresh || false;
+  }
+
+  get paddingTop() {
+    return this.args.paddingTop ? ' padding-top' : '';
+  }
+
+  get isMarginless() {
+    return this.args.isMarginless ? ' is-marginless' : '';
+  }
+
+  get sizeSmall() {
+    return this.args.sizeSmall ? ' size-small' : '';
+  }
+
+  get textClass() {
+    if (this.args.type === 'danger') {
+      return this.alertType.glyphClass;
     }
+    return null;
+  }
 
-    return;
-  }),
+  get alertType() {
+    return messageTypes([this.args.type]);
+  }
 
-  alertType: computed('type', function () {
-    return messageTypes([this.type]);
-  }),
-});
+  @action
+  refresh() {
+    if (this.mimicRefresh) {
+      this.isRefreshing = true;
+      later(() => {
+        this.isRefreshing = false;
+      }, 200);
+    }
+  }
+}
diff --git a/ui/lib/core/addon/components/form-field.js b/ui/lib/core/addon/components/form-field.js
index f85dcae04..81408e49c 100644
--- a/ui/lib/core/addon/components/form-field.js
+++ b/ui/lib/core/addon/components/form-field.js
@@ -85,7 +85,7 @@ export default class FormFieldComponent extends Component {
   get validationError() {
     const validations = this.args.modelValidations || {};
     const state = validations[this.valuePath];
-    return state && !state.isValid ? state.errors.join('. ') : null;
+    return state && !state.isValid ? state.errors.join(' ') : null;
   }
 
   onChange() {
diff --git a/ui/lib/core/addon/templates/components/alert-inline.hbs b/ui/lib/core/addon/templates/components/alert-inline.hbs
deleted file mode 100644
index 57ead04ea..000000000
--- a/ui/lib/core/addon/templates/components/alert-inline.hbs
+++ /dev/null
@@ -1,8 +0,0 @@
-<Icon @name={{this.alertType.glyph}} class={{this.alertType.glyphClass}} />
-<p class={{this.textClass}} data-test-inline-error-message>
-  {{#if (has-block)}}
-    {{yield}}
-  {{else}}
-    {{@message}}
-  {{/if}}
-</p>
\ No newline at end of file
diff --git a/ui/tests/integration/components/alert-inline-test.js b/ui/tests/integration/components/alert-inline-test.js
index 898d7d90f..6397c165d 100644
--- a/ui/tests/integration/components/alert-inline-test.js
+++ b/ui/tests/integration/components/alert-inline-test.js
@@ -1,17 +1,82 @@
 import { module, test } from 'qunit';
 import { setupRenderingTest } from 'ember-qunit';
-import { render } from '@ember/test-helpers';
+import { render, settled, find, waitUntil } from '@ember/test-helpers';
 import hbs from 'htmlbars-inline-precompile';
 
 module('Integration | Component | alert-inline', function (hooks) {
   setupRenderingTest(hooks);
 
-  test('it renders', async function (assert) {
-    // Set any properties with this.set('myProperty', 'value');
-    // Handle any actions with this.set('myAction', function(val) { ... });
+  hooks.beforeEach(function () {
+    this.set('message', 'some very important alert');
+  });
 
-    await render(hbs`{{alert-inline type="danger" message="test message"}}`);
+  test('it renders alert message with correct class args', async function (assert) {
+    await render(hbs`
+    <AlertInline
+      @paddingTop={{true}}
+      @isMarginless={{true}}
+      @sizeSmall={{true}}
+      @message={{this.message}}
+    />
+    `);
+    assert.dom('[data-test-inline-error-message]').hasText('some very important alert');
+    assert
+      .dom('[data-test-inline-alert]')
+      .hasAttribute('class', 'message-inline padding-top is-marginless size-small');
+  });
 
-    assert.dom(this.element).hasText('test message');
+  test('it yields to block text', async function (assert) {
+    await render(hbs`
+    <AlertInline @message={{this.message}}> 
+      A much more important alert
+    </AlertInline>
+    `);
+    assert.dom('[data-test-inline-error-message]').hasText('A much more important alert');
+  });
+
+  test('it renders correctly for type=danger', async function (assert) {
+    this.set('type', 'danger');
+    await render(hbs`
+    <AlertInline 
+      @type={{this.type}}
+      @message={{this.message}}
+    />
+    `);
+    assert
+      .dom('[data-test-inline-error-message]')
+      .hasAttribute('class', 'has-text-danger', 'has danger text');
+    assert.dom('[data-test-icon="x-square-fill"]').exists('danger icon exists');
+  });
+
+  test('it renders correctly for type=warning', async function (assert) {
+    this.set('type', 'warning');
+    await render(hbs`
+    <AlertInline 
+      @type={{this.type}}
+      @message={{this.message}}
+    />
+    `);
+    assert.dom('[data-test-inline-error-message]').doesNotHaveAttribute('class', 'does not have styled text');
+    assert.dom('[data-test-icon="alert-triangle-fill"]').exists('warning icon exists');
+  });
+
+  test('it mimics loading when message changes', async function (assert) {
+    await render(hbs`
+    <AlertInline 
+      @message={{this.message}}
+      @mimicRefresh={{true}} 
+    />
+    `);
+    assert
+      .dom('[data-test-inline-error-message]')
+      .hasText('some very important alert', 'it renders original message');
+
+    this.set('message', 'some changed alert!!!');
+    await waitUntil(() => find('[data-test-icon="loading"]'));
+    assert.ok(find('[data-test-icon="loading"]'), 'it shows loading icon when message changes');
+    await settled();
+    assert
+      .dom('[data-test-inline-error-message]')
+      .hasText('some changed alert!!!', 'it shows updated message');
   });
 });
diff --git a/ui/tests/unit/decorators/model-validations-test.js b/ui/tests/unit/decorators/model-validations-test.js
index e036a31bf..c34d0e313 100644
--- a/ui/tests/unit/decorators/model-validations-test.js
+++ b/ui/tests/unit/decorators/model-validations-test.js
@@ -12,6 +12,7 @@ const createClass = (validations) => {
   const foo = Foo.extend({
     modelName: 'bar',
     foo: null,
+    integer: null,
   });
   return new foo();
 };
@@ -81,4 +82,32 @@ module('Unit | Decorators | ModelValidations', function (hooks) {
       'Correct state returned when property is valid'
     );
   });
+
+  test('invalid form message has correct error count', function (assert) {
+    const message = 'This field is required';
+    const messageII = 'This field must be a number';
+    const validations = {
+      foo: [{ type: 'presence', message }],
+      integer: [{ type: 'number', messageII }],
+    };
+    const fooClass = createClass(validations);
+    const v1 = fooClass.validate();
+    assert.equal(
+      v1.invalidFormMessage,
+      'There are 2 errors with this form.',
+      'error message says form as 2 errors'
+    );
+
+    fooClass.integer = 9;
+    const v2 = fooClass.validate();
+    assert.equal(
+      v2.invalidFormMessage,
+      'There is an error with this form.',
+      'error message says form has an error'
+    );
+
+    fooClass.foo = true;
+    const v3 = fooClass.validate();
+    assert.equal(v3.invalidFormMessage, null, 'invalidFormMessage is null when form is valid');
+  });
 });
diff --git a/ui/tests/unit/utils/validators-test.js b/ui/tests/unit/utils/validators-test.js
index d18b150e2..dec5e975b 100644
--- a/ui/tests/unit/utils/validators-test.js
+++ b/ui/tests/unit/utils/validators-test.js
@@ -6,10 +6,18 @@ module('Unit | Util | validators', function (hooks) {
   setupTest(hooks);
 
   test('it should validate presence', function (assert) {
-    let isValid = validators.presence(null);
-    assert.false(isValid);
-    isValid = validators.presence(true);
-    assert.true(isValid);
+    let isValid;
+    const check = (value) => (isValid = validators.presence(value));
+    check(null);
+    assert.false(isValid, 'Invalid when value is null');
+    check('');
+    assert.false(isValid, 'Invalid when value is empty string');
+    check(true);
+    assert.true(isValid, 'Valid when value is true');
+    check(0);
+    assert.true(isValid, 'Valid when value is 0 as integer');
+    check('0');
+    assert.true(isValid, 'Valid when value is 0 as string');
   });
 
   test('it should validate length', function (assert) {
@@ -22,30 +30,44 @@ module('Unit | Util | validators', function (hooks) {
     check(null);
     assert.false(isValid, 'Invalid when nullable is false');
     check('12');
-    assert.false(isValid, 'Invalid when not min length');
+    assert.false(isValid, 'Invalid when string not min length');
     check('123456');
-    assert.false(isValid, 'Invalid when over max length');
+    assert.false(isValid, 'Invalid when string over max length');
     check('1234');
-    assert.true(isValid, 'Valid when in between min and max length');
+    assert.true(isValid, 'Valid when string between min and max length');
+    check(12);
+    assert.false(isValid, 'Invalid when integer not min length');
+    check(123456);
+    assert.false(isValid, 'Invalid when integer over max length');
+    check(1234);
+    assert.true(isValid, 'Valid when integer between min and max length');
+    options.min = 1;
+    check(0);
+    assert.true(isValid, 'Valid when integer is 0 and min is 1');
+    check('0');
+    assert.true(isValid, 'Valid when string is 0 and min is 1');
   });
 
   test('it should validate number', function (assert) {
     let isValid;
-    const options = { nullable: true, asString: false };
+    const options = { nullable: true };
     const check = (prop) => (isValid = validators.number(prop, options));
     check(null);
     assert.true(isValid, 'Valid when nullable is true');
     options.nullable = false;
     check(null);
     assert.false(isValid, 'Invalid when nullable is false');
-    check('9');
-    assert.false(isValid, 'Invalid for string when asString is false');
     check(9);
     assert.true(isValid, 'Valid for number');
-    options.asString = true;
     check('9');
     assert.true(isValid, 'Valid for number as string');
     check('foo');
     assert.false(isValid, 'Invalid for string that is not a number');
+    check('12foo');
+    assert.false(isValid, 'Invalid for string that contains a number');
+    check(0);
+    assert.true(isValid, 'Valid for 0 as an integer');
+    check('0');
+    assert.true(isValid, 'Valid for 0 as a string');
   });
 });