From ad553e967b28a071ca0380d3476a2fab48fb4d8e Mon Sep 17 00:00:00 2001 From: Patrik Date: Fri, 17 Jan 2025 09:42:46 -0500 Subject: [PATCH] fix: updates field validation error messages to use labels if applicable (#10601) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What? Previously, field error messages displayed in toast notifications used the field path to reference fields that failed validation. This path-based approach was necessary to distinguish between fields that might share the same name when nested inside arrays, groups, rows, or collapsible fields. However, the human readability of these paths was lacking, especially for unnamed fields like rows and collapsible fields. For example: - A text field inside a row could display as: `_index-0.text` - A text field nested within multiple arrays could display as: `items.0.subArray.0.text` These outputs are technically correct but not user-friendly. ### Why? While the previous format was helpful for pinpointing the specific field that caused the validation error, it could be more user-friendly and clearer to read. The goal is to maintain the same level of accuracy while improving the readability for both developers and content editors. ### How? To improve readability, the following changes were made: 1. Use Field Labels Instead of Field Paths: - The ValidationError component now uses the label prop from the field config (if available) instead of the field’s name. - If a label is provided, it will be used in the error message. - If no label exists, it will fall back to the field’s name. 2. Remove _index from Paths for Unnamed Fields (In the validationError component only): - For unnamed fields like rows and collapsibles, the _index prefix is now stripped from the output to make it cleaner. - Instead of `_index-0.text`, it now outputs just `Text`. 3. Reformat the Error Path for Readability: - The error message format has been improved to be more human-readable, showing the field hierarchy in a structured way with array indices converted to 1-based numbers. #### Example transformation: ##### Before: The following fields are invalid: `items.0.subArray.0.text` ##### After: The following fields are invalid: `Items 1 > SubArray 1 > Text` --- .../payload/src/errors/ValidationError.ts | 5 +- .../src/fields/hooks/beforeChange/promise.ts | 10 ++ .../src/utilities/getFormattedLabel.ts | 16 +++ .../src/utilities/getTranslatedLabel.ts | 15 +++ test/collections-graphql/int.spec.ts | 4 +- test/database/int.spec.ts | 2 +- test/fields/collections/Array/index.ts | 18 +++ test/fields/collections/Collapsible/index.ts | 6 + test/fields/int.spec.ts | 114 ++++++++++++++++-- test/relationships/int.spec.ts | 4 +- test/versions/int.spec.ts | 2 +- 11 files changed, 180 insertions(+), 16 deletions(-) create mode 100644 packages/payload/src/utilities/getFormattedLabel.ts create mode 100644 packages/payload/src/utilities/getTranslatedLabel.ts diff --git a/packages/payload/src/errors/ValidationError.ts b/packages/payload/src/errors/ValidationError.ts index 850974414..5a107f313 100644 --- a/packages/payload/src/errors/ValidationError.ts +++ b/packages/payload/src/errors/ValidationError.ts @@ -3,12 +3,15 @@ import type { TFunction } from '@payloadcms/translations' import { en } from '@payloadcms/translations/languages/en' import httpStatus from 'http-status' +import type { LabelFunction, StaticLabel } from '../config/types.js' + import { APIError } from './APIError.js' // This gets dynamically reassigned during compilation export let ValidationErrorName = 'ValidationError' export type ValidationFieldError = { + label?: LabelFunction | StaticLabel // The error message to display for this field message: string path: string @@ -35,7 +38,7 @@ export class ValidationError extends APIError<{ : en.translations.error.followingFieldsInvalid_other super( - `${message} ${results.errors.map((f) => f.path).join(', ')}`, + `${message} ${results.errors.map((f) => f.label || f.path).join(', ')}`, httpStatus.BAD_REQUEST, results, ) diff --git a/packages/payload/src/fields/hooks/beforeChange/promise.ts b/packages/payload/src/fields/hooks/beforeChange/promise.ts index c37f05011..a2877da50 100644 --- a/packages/payload/src/fields/hooks/beforeChange/promise.ts +++ b/packages/payload/src/fields/hooks/beforeChange/promise.ts @@ -8,6 +8,8 @@ import type { Field, TabAsField } from '../../config/types.js' import { MissingEditorProp } from '../../../errors/index.js' import { deepMergeWithSourceArrays } from '../../../utilities/deepMerge.js' +import { getFormattedLabel } from '../../../utilities/getFormattedLabel.js' +import { getTranslatedLabel } from '../../../utilities/getTranslatedLabel.js' import { fieldAffectsData, tabHasName } from '../../config/types.js' import { getFieldPaths } from '../../getFieldPaths.js' import { getExistingRowDoc } from './getExistingRowDoc.js' @@ -159,7 +161,15 @@ export const promise = async ({ ) if (typeof validationResult === 'string') { + const label = getTranslatedLabel(field?.label || field?.name, req.i18n) + + const fieldLabel = + Array.isArray(parentPath) && parentPath.length > 0 + ? getFormattedLabel([...parentPath, label]) + : label + errors.push({ + label: fieldLabel, message: validationResult, path: fieldPath.join('.'), }) diff --git a/packages/payload/src/utilities/getFormattedLabel.ts b/packages/payload/src/utilities/getFormattedLabel.ts new file mode 100644 index 000000000..7e1241e16 --- /dev/null +++ b/packages/payload/src/utilities/getFormattedLabel.ts @@ -0,0 +1,16 @@ +export const getFormattedLabel = (path: (number | string)[]): string => { + return path + .filter((pathSegment) => !(typeof pathSegment === 'string' && pathSegment.includes('_index'))) + .reduce((acc, part) => { + if (typeof part === 'number' || (typeof part === 'string' && !isNaN(Number(part)))) { + // Convert index to 1-based and format as "Array 01", "Array 02", etc. + const fieldName = acc.pop() + acc.push(`${fieldName} ${Number(part) + 1}`) + } else { + // Capitalize field names + acc.push(part.charAt(0).toUpperCase() + part.slice(1)) + } + return acc + }, []) + .join(' > ') +} diff --git a/packages/payload/src/utilities/getTranslatedLabel.ts b/packages/payload/src/utilities/getTranslatedLabel.ts new file mode 100644 index 000000000..50ab92f2d --- /dev/null +++ b/packages/payload/src/utilities/getTranslatedLabel.ts @@ -0,0 +1,15 @@ +import { getTranslation, type I18n } from '@payloadcms/translations' + +import type { LabelFunction, StaticLabel } from '../config/types.js' + +export const getTranslatedLabel = (label: LabelFunction | StaticLabel, i18n?: I18n): string => { + if (typeof label === 'function') { + return label({ t: i18n.t }) + } + + if (typeof label === 'object') { + return getTranslation(label, i18n) + } + + return label +} diff --git a/test/collections-graphql/int.spec.ts b/test/collections-graphql/int.spec.ts index 4f167ae23..42435b1d1 100644 --- a/test/collections-graphql/int.spec.ts +++ b/test/collections-graphql/int.spec.ts @@ -1159,7 +1159,7 @@ describe('collections-graphql', () => { }) .then((res) => res.json()) expect(Array.isArray(errors)).toBe(true) - expect(errors[0].message).toEqual('The following field is invalid: min') + expect(errors[0].message).toEqual('The following field is invalid: Min') expect(typeof errors[0].locations).toBeDefined() }) @@ -1207,7 +1207,7 @@ describe('collections-graphql', () => { expect(errors[1].extensions.data.errors[0].path).toEqual('email') expect(Array.isArray(errors[2].locations)).toEqual(true) - expect(errors[2].message).toEqual('The following field is invalid: email') + expect(errors[2].message).toEqual('The following field is invalid: Email') expect(errors[2].path[0]).toEqual('test4') expect(errors[2].extensions.name).toEqual('ValidationError') expect(errors[2].extensions.data.errors[0].message).toEqual( diff --git a/test/database/int.spec.ts b/test/database/int.spec.ts index c739a3adf..c7daa70f0 100644 --- a/test/database/int.spec.ts +++ b/test/database/int.spec.ts @@ -892,7 +892,7 @@ describe('database', () => { errorMessage = e.message } - await expect(errorMessage).toBe('The following field is invalid: title') + await expect(errorMessage).toBe('The following field is invalid: Title') }) it('should return proper deeply nested field validation errors', async () => { diff --git a/test/fields/collections/Array/index.ts b/test/fields/collections/Array/index.ts index df1abd2d1..869f3a92f 100644 --- a/test/fields/collections/Array/index.ts +++ b/test/fields/collections/Array/index.ts @@ -54,6 +54,24 @@ const ArrayFields: CollectionConfig = { name: 'text', type: 'text', }, + { + name: 'textTwo', + label: 'Second text field', + type: 'text', + required: true, + defaultValue: 'default', + }, + { + type: 'row', + fields: [ + { + name: 'textInRow', + type: 'text', + required: true, + defaultValue: 'default', + }, + ], + }, ], type: 'array', }, diff --git a/test/fields/collections/Collapsible/index.ts b/test/fields/collections/Collapsible/index.ts index 2893becb1..de6f480bb 100644 --- a/test/fields/collections/Collapsible/index.ts +++ b/test/fields/collections/Collapsible/index.ts @@ -36,6 +36,12 @@ const CollapsibleFields: CollectionConfig = { name: 'textWithinSubGroup', type: 'text', }, + { + name: 'requiredTextWithinSubGroup', + type: 'text', + required: true, + defaultValue: 'required text', + }, ], }, ], diff --git a/test/fields/int.spec.ts b/test/fields/int.spec.ts index 5f0de2ee4..ef90dd0af 100644 --- a/test/fields/int.spec.ts +++ b/test/fields/int.spec.ts @@ -486,6 +486,20 @@ describe('Fields', () => { }) }) + describe('rows', () => { + it('show proper validation error message on text field within row field', async () => { + await expect(async () => + payload.create({ + collection: 'row-fields', + data: { + id: 'some-id', + title: '', + }, + }), + ).rejects.toThrow('The following field is invalid: Title within a row') + }) + }) + describe('timestamps', () => { const tenMinutesAgo = new Date(Date.now() - 1000 * 60 * 10) let doc @@ -693,7 +707,7 @@ describe('Fields', () => { min: 5, }, }), - ).rejects.toThrow('The following field is invalid: min') + ).rejects.toThrow('The following field is invalid: Min') }) it('should not create number above max', async () => { await expect(async () => @@ -703,7 +717,7 @@ describe('Fields', () => { max: 15, }, }), - ).rejects.toThrow('The following field is invalid: max') + ).rejects.toThrow('The following field is invalid: Max') }) it('should not create number below 0', async () => { @@ -714,7 +728,7 @@ describe('Fields', () => { positiveNumber: -5, }, }), - ).rejects.toThrow('The following field is invalid: positiveNumber') + ).rejects.toThrow('The following field is invalid: Positive Number') }) it('should not create number above 0', async () => { @@ -725,7 +739,7 @@ describe('Fields', () => { negativeNumber: 5, }, }), - ).rejects.toThrow('The following field is invalid: negativeNumber') + ).rejects.toThrow('The following field is invalid: Negative Number') }) it('should not create a decimal number below min', async () => { await expect(async () => @@ -735,7 +749,7 @@ describe('Fields', () => { decimalMin: -0.25, }, }), - ).rejects.toThrow('The following field is invalid: decimalMin') + ).rejects.toThrow('The following field is invalid: Decimal Min') }) it('should not create a decimal number above max', async () => { @@ -746,7 +760,7 @@ describe('Fields', () => { decimalMax: 1.5, }, }), - ).rejects.toThrow('The following field is invalid: decimalMax') + ).rejects.toThrow('The following field is invalid: Decimal Max') }) it('should localize an array of numbers using hasMany', async () => { const localizedHasMany = [5, 10] @@ -1128,7 +1142,7 @@ describe('Fields', () => { min: 5, }, }), - ).rejects.toThrow('The following field is invalid: min') + ).rejects.toThrow('The following field is invalid: Min') expect(doc.point).toEqual(point) expect(doc.localized).toEqual(localized) @@ -1662,6 +1676,46 @@ describe('Fields', () => { expect(res.id).toBe(doc.id) }) + + it('show proper validation error on text field in nested array', async () => { + await expect(async () => + payload.create({ + collection, + data: { + items: [ + { + text: 'required', + subArray: [ + { + textTwo: '', + }, + ], + }, + ], + }, + }), + ).rejects.toThrow('The following field is invalid: Items 1 > SubArray 1 > Second text field') + }) + + it('show proper validation error on text field in row field in nested array', async () => { + await expect(async () => + payload.create({ + collection, + data: { + items: [ + { + text: 'required', + subArray: [ + { + textInRow: '', + }, + ], + }, + ], + }, + }), + ).rejects.toThrow('The following field is invalid: Items 1 > SubArray 1 > Text In Row') + }) }) describe('group', () => { @@ -2168,6 +2222,28 @@ describe('Fields', () => { expect(res.camelCaseTab.array[0].text).toBe('text') expect(res.camelCaseTab.array[0].array[0].text).toBe('nested') }) + + it('should show proper validation error message on text field within array within tab', async () => { + await expect(async () => + payload.update({ + id: document.id, + collection: tabsFieldsSlug, + data: { + array: [ + { + text: 'one', + }, + { + text: 'two', + }, + { + text: '', + }, + ], + }, + }), + ).rejects.toThrow('The following field is invalid: Array 3 > Text') + }) }) describe('blocks', () => { @@ -2429,6 +2505,26 @@ describe('Fields', () => { }) }) + describe('collapsible', () => { + it('show proper validation error message for fields nested in collapsible', async () => { + await expect(async () => + payload.create({ + collection: 'collapsible-fields', + data: { + text: 'required', + group: { + subGroup: { + requiredTextWithinSubGroup: '', + }, + }, + }, + }), + ).rejects.toThrow( + 'The following field is invalid: Group > SubGroup > Required Text Within Sub Group', + ) + }) + }) + describe('json', () => { it('should save json data', async () => { const json = { foo: 'bar' } @@ -2450,7 +2546,7 @@ describe('Fields', () => { json: '{ bad input: true }', }, }), - ).rejects.toThrow('The following field is invalid: json') + ).rejects.toThrow('The following field is invalid: Json') }) it('should validate json schema', async () => { @@ -2461,7 +2557,7 @@ describe('Fields', () => { json: { foo: 'bad' }, }, }), - ).rejects.toThrow('The following field is invalid: json') + ).rejects.toThrow('The following field is invalid: Json') }) it('should save empty json objects', async () => { diff --git a/test/relationships/int.spec.ts b/test/relationships/int.spec.ts index feb05f71b..58834286a 100644 --- a/test/relationships/int.spec.ts +++ b/test/relationships/int.spec.ts @@ -562,7 +562,7 @@ describe('Relationships', () => { // @ts-expect-error Sending bad data to test error handling customIdRelation: 1234, }), - ).rejects.toThrow('The following field is invalid: customIdRelation') + ).rejects.toThrow('The following field is invalid: Custom Id Relation') }) it('should validate the format of number id relationships', async () => { @@ -571,7 +571,7 @@ describe('Relationships', () => { // @ts-expect-error Sending bad data to test error handling customIdNumberRelation: 'bad-input', }), - ).rejects.toThrow('The following field is invalid: customIdNumberRelation') + ).rejects.toThrow('The following field is invalid: Custom Id Number Relation') }) it('should allow update removing a relationship', async () => { diff --git a/test/versions/int.spec.ts b/test/versions/int.spec.ts index 7a40f83ae..8ac7bdfd0 100644 --- a/test/versions/int.spec.ts +++ b/test/versions/int.spec.ts @@ -686,7 +686,7 @@ describe('Versions', () => { expect(updateManyResult.docs).toHaveLength(0) expect(updateManyResult.errors).toStrictEqual([ - { id: doc.id, message: 'The following field is invalid: title' }, + { id: doc.id, message: 'The following field is invalid: Title' }, ]) }) })