From b41dd63667f3cea4025b135765ee57dfa9676012 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Wed, 9 Apr 2025 21:25:19 -0700 Subject: [PATCH 1/3] Log Invisible Validation Errors To prevent bugs such as we had with validating the session overview, where an error in a field that isn't currently open or showing errors prevents validation, this now logs those errors to the console. This isn't visible much to users, as it's in the console, but I used the intl service to translate them into nice errors anyway. --- .../addon/classes/yup-validations.js | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/ilios-common/addon/classes/yup-validations.js b/packages/ilios-common/addon/classes/yup-validations.js index 9e7ae04bf8..df9d902aff 100644 --- a/packages/ilios-common/addon/classes/yup-validations.js +++ b/packages/ilios-common/addon/classes/yup-validations.js @@ -3,6 +3,7 @@ import { getProperties } from '@ember/object'; import { object, setLocale } from 'yup'; import { restartableTask, timeout } from 'ember-concurrency'; import { modifier } from 'ember-modifier'; +import { getOwner } from '@ember/application'; const DEBOUNCE_MS = 100; @@ -13,6 +14,7 @@ export default class YupValidations { context; schema; shape; + intl; @tracked error; @tracked showAllErrors = false; @@ -22,6 +24,7 @@ export default class YupValidations { this.context = context; this.shape = shape; this.schema = object().shape(shape); + this.intl = getOwner(context).lookup('service:intl'); } get errorsByKey() { @@ -86,7 +89,26 @@ export default class YupValidations { } async isValid() { - return (await this.#validate()) === true; + const isValid = await this.#validate(); + if (isValid === true) { + return true; + } + //find errors that are not visible and log them + //this makes it easier to find invisible errors when debugging + const invisibleErrors = Object.keys(this.errorsByKey).filter( + (key) => !this.visibleErrors.includes(key), + ); + if (invisibleErrors.length) { + invisibleErrors.forEach((key) => { + const errors = this.errorsByKey[key].map(({ messageKey, values }) => { + values.description = key; + return this.intl.t(messageKey, values); + }); + console.warn('Invisible errors prevented validation:\n ' + errors.join('\n ')); + }); + } + + return false; } addErrorDisplaysFor = (fields) => { From d63ad34cf433809ca5715cc3e702354073e363ff Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Thu, 10 Apr 2025 22:39:46 -0700 Subject: [PATCH 2/3] Improve Handling of Values in Yup We use the `values` key to pass information to the translation, it should always be an object and if it's not set (for instance in a `test` type of validation where it's developer specified) we should account for that and not expect it to be present. This is what we do in the validations message component. --- packages/ilios-common/addon/classes/yup-validations.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/ilios-common/addon/classes/yup-validations.js b/packages/ilios-common/addon/classes/yup-validations.js index df9d902aff..d4e9a69cce 100644 --- a/packages/ilios-common/addon/classes/yup-validations.js +++ b/packages/ilios-common/addon/classes/yup-validations.js @@ -101,6 +101,9 @@ export default class YupValidations { if (invisibleErrors.length) { invisibleErrors.forEach((key) => { const errors = this.errorsByKey[key].map(({ messageKey, values }) => { + if (!values) { + values = {}; + } values.description = key; return this.intl.t(messageKey, values); }); @@ -212,7 +215,7 @@ function isEmail() { return { path: validationParams.path, messageKey: 'errors.email', - values: [], + values: {}, }; }; } @@ -222,7 +225,7 @@ function isURL() { return { path: validationParams.path, messageKey: 'errors.url', - values: [], + values: {}, }; }; } @@ -230,7 +233,7 @@ function isURL() { function notType() { return ({ type, path }) => { let messageKey, - values = []; + values = {}; switch (type) { case 'number': messageKey = 'errors.notANumber'; From eba2127fb4071e5a7a2cda456cc9b7d25818bdc4 Mon Sep 17 00:00:00 2001 From: Jonathan Johnson Date: Fri, 11 Apr 2025 11:08:49 -0700 Subject: [PATCH 3/3] Improve Owner Handling Sometimes we validate a non-ember thing (like ProposedUser) and it doesn't have an owner. In that case we can pass one in. --- .../frontend/app/components/bulk-new-users.js | 66 ++++++++++--------- .../addon/classes/yup-validations.js | 8 ++- 2 files changed, 43 insertions(+), 31 deletions(-) diff --git a/packages/frontend/app/components/bulk-new-users.js b/packages/frontend/app/components/bulk-new-users.js index 7b33351587..c099401083 100644 --- a/packages/frontend/app/components/bulk-new-users.js +++ b/packages/frontend/app/components/bulk-new-users.js @@ -11,6 +11,7 @@ import { findById, mapBy } from 'ilios-common/utils/array-helpers'; import { TrackedAsyncData } from 'ember-async-data'; import YupValidations from 'ilios-common/classes/yup-validations'; import { string } from 'yup'; +import { getOwner } from '@ember/application'; export default class BulkNewUsersComponent extends Component { @service flashMessages; @@ -203,6 +204,7 @@ export default class BulkNewUsersComponent extends Component { password: isPresent(arr[8]) ? arr[8] : null, }, existingUsernames, + getOwner(this), ); }); const notHeaderRow = proposedUsers.filter( @@ -346,37 +348,12 @@ export default class BulkNewUsersComponent extends Component { class ProposedUser { addedViaIlios = true; enabled = true; + owner = null; + validations = null; + existingUsernames = []; - validations = new YupValidations(this, { - firstName: string().required().min(1).max(50), - middleName: string().nullable().min(1).max(20), - lastName: string().required().min(1).max(50), - username: string() - .nullable() - .min(1) - .max(100) - .test( - 'is-username-unique', - (d) => { - return { - path: d.path, - messageKey: 'errors.exclusion', - }; - }, - (value) => value == null || !this.existingUsernames.includes(this.username), - ), - password: string().when('username', { - is: (username) => !!username, // Check if the username field has a value - then: (schema) => schema.required(), - otherwise: (schema) => schema.notRequired(), - }), - campusId: string().nullable().min(1).max(16), - otherId: string().nullable().min(1).max(16), - email: string().nullable().email(), - phone: string().nullable().min(1).max(20), - }); - - constructor(userObj, existingUsernames) { + constructor(userObj, existingUsernames, owner) { + this.owner = owner; this.firstName = userObj.firstName; this.lastName = userObj.lastName; this.middleName = userObj.middleName; @@ -389,6 +366,35 @@ class ProposedUser { this.existingUsernames = existingUsernames; + this.validations = new YupValidations(this, { + firstName: string().required().min(1).max(50), + middleName: string().nullable().min(1).max(20), + lastName: string().required().min(1).max(50), + username: string() + .nullable() + .min(1) + .max(100) + .test( + 'is-username-unique', + (d) => { + return { + path: d.path, + messageKey: 'errors.exclusion', + }; + }, + (value) => value == null || !this.existingUsernames.includes(this.username), + ), + password: string().when('username', { + is: (username) => !!username, // Check if the username field has a value + then: (schema) => schema.required(), + otherwise: (schema) => schema.notRequired(), + }), + campusId: string().nullable().min(1).max(16), + otherId: string().nullable().min(1).max(16), + email: string().nullable().email(), + phone: string().nullable().min(1).max(20), + }); + this.validations.addErrorDisplayForAllFields(); } diff --git a/packages/ilios-common/addon/classes/yup-validations.js b/packages/ilios-common/addon/classes/yup-validations.js index d4e9a69cce..007426dd13 100644 --- a/packages/ilios-common/addon/classes/yup-validations.js +++ b/packages/ilios-common/addon/classes/yup-validations.js @@ -24,7 +24,13 @@ export default class YupValidations { this.context = context; this.shape = shape; this.schema = object().shape(shape); - this.intl = getOwner(context).lookup('service:intl'); + const owner = context.owner ?? getOwner(context); + if (!owner) { + throw new Error( + 'No owner found for YupValidations. Either the context does not have an owner or the owner is not set.', + ); + } + this.intl = owner.lookup('service:intl'); } get errorsByKey() {