Fix: multi-select default values validation #12271
Conversation
…thod for validating multi-select default values.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR fixes validation issues with multi-select default values in the field metadata system, specifically addressing the inability to remove defaulted option values for standard objects.
- Added separate validation logic for multi-select fields in
packages/twenty-server/src/engine/metadata-modules/field-metadata/field-metadata-validation.service.ts - Implemented validation to ensure all multi-select default values exist within the options array
- Added support for handling null/empty default values in multi-select fields
- Enhanced type safety with stricter validation checks for field metadata options
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:5378 This environment will automatically shut down when the PR is closed or after 5 hours. |
There was a problem hiding this comment.
Hey @abdulrahmancodes ! Hope you're doing great, thanks for your contribution.
In a nutshell what you've done is great and works ! but IMO we should go deeper and integrate tho other existing service
I think such refactor is very prone to TDD.
That's why I would recommend starting by writing twenty-server integration tests in the first place, we can have a call where we could describe each tests use case we wanna cover through the defaultValue validation
Main review points:
- Moving the enum fieldMetadataType ( SELECT | MULTI_SELECT | RATING ) validation logic in
FieldMetadataEnumValidationService/ defaultValuekey should be trimmed usingtrimAndRemoveDuplicatedWhitespacesFromObjectStringProperties( see existing usage I think we could merge thedefaultValuewithin theprepareCustomFieldMetadataOptions)- Adding integrations tests to for each raised exceptions in
update-create-one-field-metadata-select-tests-cases.ts
| // @ts-expect-error legacy noImplicitAny | ||
| enumOptions.some((option) => option.to === formattedDefaultValue)) | ||
| ) { | ||
| if (isValid) { |
There was a problem hiding this comment.
Nitpick: Would rather throw an exception if invalid as early return
| fieldType === FieldMetadataType.SELECT || | ||
| fieldType === FieldMetadataType.MULTI_SELECT | ||
| ) { | ||
| if (fieldType === FieldMetadataType.SELECT) { |
There was a problem hiding this comment.
Remark: If the default value validation is only scoped to ENUM field metadata type ( SELECT || MULTI_SELECT || RATING ) then can moove this in the FieldMetadataEnumValidationService
There was a problem hiding this comment.
Nitpick: Using switch statement
There was a problem hiding this comment.
Question: unsure what we should do about the RATING fieldMetadata type, to be determined
| return false; | ||
| } | ||
|
|
||
| const enumOptions = options.map((option) => option.value); |
There was a problem hiding this comment.
Suggestion: Please follow the validator pattern from the FieldMetadataEnumValidationService that makes the following assertions more explicit IMO
There was a problem hiding this comment.
Suggestion: The defaultValue should also be validate through the value validators and trimmer too
|
|
||
| return ( | ||
| enumOptions.includes(formattedValue) || | ||
| // @ts-expect-error legacy noImplicitAny |
There was a problem hiding this comment.
Request: This @ts-expect-error is inaccurate, the related error is not an implicit any
to does not exist on string, from my understanding the below some always returns false
enumOptions.some((option) => option.to === formattedValue)| const enumOptions = options.map((option) => option.value); | ||
| private validateMultiSelectDefaultValue( | ||
| options: FieldMetadataOptions<T>, | ||
| defaultValues: FieldMetadataDefaultValue<T>, |
There was a problem hiding this comment.
Note: We should refactor the FieldMetadataDefaultValue as is imo too verbose
Create accurately named and exported abstraction for each ternary occurence that could be used here for example
export type FieldMetadataDefaultValue<
T extends FieldMetadataType = FieldMetadataType,
> =
IsExactly<T, FieldMetadataType> extends true
? ExtractValueType<UnionOfValues<FieldMetadataDefaultValueMapping>> | null
: T extends keyof FieldMetadataDefaultValueMapping
? ExtractValueType<FieldMetadataDefaultValueMapping[T]> | null
: never;…ue validation logic
…from string properties
…ming and validation for default values. Introduced a utility function to sanitize input and updated validation logic for both single and multi-select default values.
abdulrahmancodes
force-pushed
the
fix/multi-select-default-value-validation
branch
from
385a636 to
d628377
Compare
abdulrahmancodes
force-pushed
the
fix/multi-select-default-value-validation
branch
from
385a636 to
7197be2
Compare
|
Hey @abdulrahmancodes please let me know once you would like me to review your PR |
Hey @prastoin ! Sure. Will let you know as soon as I'm done with the changes. |
…n trim-and-remove-duplicated-whitespaces utility tests.
…s to avoid variable shadowing
…fault value validation
| }, | ||
| }, | ||
| { | ||
| title: 'should fail with duplicate multi-select default values', |
| @@ -123,6 +124,46 @@ const successfulTestCases: UpdateCreateFieldMetadataSelectTestCase[] = [ | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Remark: I agree that I've missed testing the multi-select over there, I'm afraid that mixing SELECT and MULTI_SELECT specific scoped tests cases becomes hardly maintainable.
Would you be up to do some peer on this section ? From my point of view I would see commonTestCases between SELECT MULTI_SELECT and afterwards dedicated tests file with their own jest hooks implementation etc.
|
Hey @prastoin ! Sure, happy to sync on this, just let me know when works for you. |
…pes for any type and for mapped types
Reaching out on discord in order to plan the best schedule for both of us ! |
abdulrahmancodes
force-pushed
the
fix/multi-select-default-value-validation
branch
from
95fe946 to
454c1fb
Compare
| @@ -94,6 +94,22 @@ describe('trim-and-remove-duplicated-whitespaces-from-object-string-properties', | |||
| expected: { name: ' John Doe ', description: 'this is a test' }, | |||
| }, | |||
| }, | |||
| { | |||
| title: 'should strip quotes from string properties', | |||
There was a problem hiding this comment.
Request: We should not, doing so will break below queries. its required for the defaultValue to be formatted as 'Option'
Related issue twentyhq/core-team-issues#941
UpdateHad a call with @abdulrahmancodes, we should implement dedicated We should create a new test suite that will handle the The validation enum typing should be scoped to the Actions
|
Screen.Recording.2025-05-24.at.11.25.18.AM.mov
Closes #12277