-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: FieldMetadata default value and options better validation #2785
Conversation
4a2b3fa
to
6a3d351
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't generate something smaller for now... As it's map file and bundled sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, LGTM 👏
server/src/metadata/field-metadata/validators/is-field-metadata-options.validator.ts
Outdated
Show resolved
Hide resolved
@Args('input') input: UpdateOneFieldMetadataInput, | ||
@AuthWorkspace() { id: workspaceId }: Workspace, | ||
) { | ||
return this.fieldMetadataService.updateOne(input.id, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we lost a few checks such as rejecting updates on standard fields (except for isActive) + updates on certain fields for custom or am I missed something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Weiko I'll take a look on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I've fix the issue
…a-options.validator.ts Co-authored-by: Weiko <corentin@twenty.com>
ac2161e
to
a0fc109
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -159,6 +159,15 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit | |||
throw new NotFoundException('Field does not exist'); | |||
} | |||
|
|||
if (existingFieldMetadata.isCustom === false) { | |||
// We can only update the isActive field for standard fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: not a blocker but it could be a bit deceiving for the api user since we transform the input and not throw any error if something else than isActive is changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Weiko Yes it's a quick change, we can maybe create another default validator for isCustom
properties, but you told me that the front-end send all the input informations and I didn't want to break it
This PR, in how
defaultValue
andoptions
were validated.It nos use asynchronous validators to support
create
andupdate
validation based on the type of the field.