From 2fc300a63c31f9dd2306c39ef66d39e08cfb4d1c Mon Sep 17 00:00:00 2001 From: Paul Rastoin <45004772+prastoin@users.noreply.github.com> Date: Tue, 24 Jun 2025 20:05:27 +0200 Subject: [PATCH] Remove number from label identifier list (#12831) --- ...ettingsObjectFieldActiveActionDropdown.tsx | 5 +- .../SettingsObjectFieldItemTableRow.tsx | 8 +- ...SettingsDataModelObjectIdentifiersForm.tsx | 4 +- .../dtos/update-object.input.ts | 4 + .../before-update-one-object.hook.spec.ts | 56 ----------- .../hooks/before-update-one-object.hook.ts | 56 +---------- .../object-metadata.service.ts | 9 ++ ...data-identifier-field-metadata-id.utils.ts | 94 ++++++++++++++++++ ...e-object-metadata.integration-spec.ts.snap | 34 +++++++ ...te-one-object-metadata.integration-spec.ts | 96 +++++++++++++++++++ .../LabelIdentifierFieldMetadataTypes.ts | 3 +- packages/twenty-shared/src/constants/index.ts | 1 + packages/twenty-shared/src/utils/index.ts | 1 + .../isLabelIdentifierFieldMetadataTypes.ts | 7 ++ 14 files changed, 258 insertions(+), 120 deletions(-) create mode 100644 packages/twenty-server/src/engine/metadata-modules/utils/validate-metadata-identifier-field-metadata-id.utils.ts create mode 100644 packages/twenty-server/test/integration/metadata/suites/object-metadata/__snapshots__/failing-update-one-object-metadata.integration-spec.ts.snap create mode 100644 packages/twenty-server/test/integration/metadata/suites/object-metadata/failing-update-one-object-metadata.integration-spec.ts rename packages/{twenty-front/src/modules/object-metadata => twenty-shared/src}/constants/LabelIdentifierFieldMetadataTypes.ts (55%) create mode 100644 packages/twenty-shared/src/utils/validation/isLabelIdentifierFieldMetadataTypes.ts diff --git a/packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsObjectFieldActiveActionDropdown.tsx b/packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsObjectFieldActiveActionDropdown.tsx index a460c0f00..9397aec80 100644 --- a/packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsObjectFieldActiveActionDropdown.tsx +++ b/packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsObjectFieldActiveActionDropdown.tsx @@ -3,6 +3,7 @@ import { DropdownContent } from '@/ui/layout/dropdown/components/DropdownContent import { DropdownMenuItemsContainer } from '@/ui/layout/dropdown/components/DropdownMenuItemsContainer'; import { GenericDropdownContentWidth } from '@/ui/layout/dropdown/constants/GenericDropdownContentWidth'; import { useDropdown } from '@/ui/layout/dropdown/hooks/useDropdown'; +import { isDefined } from 'twenty-shared/utils'; import { IconArchive, IconDotsVertical, @@ -65,14 +66,14 @@ export const SettingsObjectFieldActiveActionDropdown = ({ LeftIcon={isCustomField ? IconPencil : IconEye} onClick={handleEdit} /> - {!!onSetAsLabelIdentifier && ( + {isDefined(onSetAsLabelIdentifier) && ( )} - {!!onDeactivate && ( + {isDefined(onDeactivate) && ( - LABEL_IDENTIFIER_FIELD_METADATA_TYPES.includes(type) || + isLabelIdentifierFieldMetadataTypes(type) || objectMetadataItem.labelIdentifierFieldMetadataId === id, ) .map>((fieldMetadataItem) => ({ diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/dtos/update-object.input.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/dtos/update-object.input.ts index 96da3d4a7..1d887c6ea 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/dtos/update-object.input.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/dtos/update-object.input.ts @@ -1,12 +1,14 @@ import { Field, InputType } from '@nestjs/graphql'; import { BeforeUpdateOne } from '@ptc-org/nestjs-query-graphql'; +import { Type } from 'class-transformer'; import { IsBoolean, IsNotEmpty, IsOptional, IsString, IsUUID, + ValidateNested, } from 'class-validator'; import { UUIDScalarType } from 'src/engine/api/graphql/workspace-schema-builder/graphql-types/scalars'; @@ -76,6 +78,8 @@ export class UpdateObjectPayload { @InputType() @BeforeUpdateOne(BeforeUpdateOneObject) export class UpdateOneObjectInput { + @Type(() => UpdateObjectPayload) + @ValidateNested() @Field(() => UpdateObjectPayload) update: UpdateObjectPayload; diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/__tests__/before-update-one-object.hook.spec.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/__tests__/before-update-one-object.hook.spec.ts index 518c86d2e..4b4e0e817 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/__tests__/before-update-one-object.hook.spec.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/__tests__/before-update-one-object.hook.spec.ts @@ -656,34 +656,6 @@ describe('BeforeUpdateOneObject', () => { expect(result).toEqual(instance); }); - it('should throw BadRequestException if label identifier field does not exist', async () => { - const labelIdentifierFieldId = 'nonexistent-field-id'; - const instance: UpdateOneInputType = { - id: mockObjectId, - update: { - labelIdentifierFieldMetadataId: labelIdentifierFieldId, - }, - }; - - const mockObject: Partial = { - id: mockObjectId, - isCustom: true, - }; - - jest - .spyOn(objectMetadataService, 'findOneWithinWorkspace') - .mockResolvedValue(mockObject as ObjectMetadataEntity); - - jest.spyOn(fieldMetadataRepository, 'findBy').mockResolvedValue([]); - - await expect( - hook.run(instance as UpdateOneInputType, { - workspaceId: mockWorkspaceId, - locale: undefined, - }), - ).rejects.toThrow('This label identifier does not exist'); - }); - it('should validate image identifier field correctly for custom objects', async () => { const imageIdentifierFieldId = 'image-field-id'; const instance: UpdateOneInputType = { @@ -722,32 +694,4 @@ describe('BeforeUpdateOneObject', () => { expect(result).toEqual(instance); }); - - it('should throw BadRequestException if image identifier field does not exist', async () => { - const imageIdentifierFieldId = 'nonexistent-field-id'; - const instance: UpdateOneInputType = { - id: mockObjectId, - update: { - imageIdentifierFieldMetadataId: imageIdentifierFieldId, - }, - }; - - const mockObject: Partial = { - id: mockObjectId, - isCustom: true, - }; - - jest - .spyOn(objectMetadataService, 'findOneWithinWorkspace') - .mockResolvedValue(mockObject as ObjectMetadataEntity); - - jest.spyOn(fieldMetadataRepository, 'findBy').mockResolvedValue([]); - - await expect( - hook.run(instance as UpdateOneInputType, { - workspaceId: mockWorkspaceId, - locale: undefined, - }), - ).rejects.toThrow('This image identifier does not exist'); - }); }); diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts index 5486c63b7..37dce526c 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/hooks/before-update-one-object.hook.ts @@ -12,7 +12,7 @@ import { } from '@ptc-org/nestjs-query-graphql'; import { APP_LOCALES, SOURCE_LOCALE } from 'twenty-shared/translations'; import { isDefined } from 'twenty-shared/utils'; -import { Equal, In, Repository } from 'typeorm'; +import { Repository } from 'typeorm'; import { generateMessageId } from 'src/engine/core-modules/i18n/utils/generateMessageId'; import { FieldMetadataEntity } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; @@ -57,8 +57,6 @@ export class BeforeUpdateOneObject return this.handleStandardObjectUpdate(instance, objectMetadata, locale); } - await this.validateIdentifierFields(instance, workspaceId); - return instance; } @@ -437,56 +435,4 @@ export class BeforeUpdateOneObject locale, ); } - - private async validateIdentifierFields( - instance: UpdateOneInputType, - workspaceId: string, - ): Promise { - if ( - !instance.update.labelIdentifierFieldMetadataId && - !instance.update.imageIdentifierFieldMetadataId - ) { - return; - } - - const fields = await this.fieldMetadataRepository.findBy({ - workspaceId: Equal(workspaceId), - objectMetadataId: Equal(instance.id.toString()), - id: In( - [ - instance.update.labelIdentifierFieldMetadataId, - instance.update.imageIdentifierFieldMetadataId, - ].filter((id) => id !== null), - ), - }); - - const fieldIds = fields.map((field) => field.id); - - this.validateLabelIdentifier(instance, fieldIds); - this.validateImageIdentifier(instance, fieldIds); - } - - private validateLabelIdentifier( - instance: UpdateOneInputType, - fieldIds: string[], - ): void { - if ( - instance.update.labelIdentifierFieldMetadataId && - !fieldIds.includes(instance.update.labelIdentifierFieldMetadataId) - ) { - throw new BadRequestException('This label identifier does not exist'); - } - } - - private validateImageIdentifier( - instance: UpdateOneInputType, - fieldIds: string[], - ): void { - if ( - instance.update.imageIdentifierFieldMetadataId && - !fieldIds.includes(instance.update.imageIdentifierFieldMetadataId) - ) { - throw new BadRequestException('This image identifier does not exist'); - } - } } diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts index f7b2a6cf0..c7341aec7 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/object-metadata.service.ts @@ -34,6 +34,7 @@ import { import { RemoteTableRelationsService } from 'src/engine/metadata-modules/remote-server/remote-table/remote-table-relations/remote-table-relations.service'; import { SearchVectorService } from 'src/engine/metadata-modules/search-vector/search-vector.service'; import { ObjectMetadataItemWithFieldMaps } from 'src/engine/metadata-modules/types/object-metadata-item-with-field-maps'; +import { validateMetadataIdentifierFieldMetadataIds } from 'src/engine/metadata-modules/utils/validate-metadata-identifier-field-metadata-id.utils'; import { validateNameAndLabelAreSyncOrThrow } from 'src/engine/metadata-modules/utils/validate-name-and-label-are-sync-or-throw.util'; import { validatesNoOtherObjectWithSameNameExistsOrThrows } from 'src/engine/metadata-modules/utils/validate-no-other-object-with-same-name-exists-or-throw.util'; import { WorkspaceMetadataCacheService } from 'src/engine/metadata-modules/workspace-metadata-cache/services/workspace-metadata-cache.service'; @@ -300,6 +301,14 @@ export class ObjectMetadataService extends TypeOrmQueryService boolean; + label: string; +}; + +type ValidateMetadataIdentifierFieldMetadataIdOrThrowArgs = { + fieldMetadataId: string; + fieldMetadataItems: FieldMetadataEntity[] | FieldMetadataInterface[]; + validators: Validator[]; +}; +const validatorRunner = ({ + fieldMetadataId, + fieldMetadataItems, + validators, +}: ValidateMetadataIdentifierFieldMetadataIdOrThrowArgs): void => { + const matchingFieldMetadata = fieldMetadataItems.find( + (fieldMetadata) => fieldMetadata.id === fieldMetadataId, + ); + + validators.forEach(({ label, validator }) => { + if (validator({ fieldMetadataId, matchingFieldMetadata })) { + throw new ObjectMetadataException( + label, + ObjectMetadataExceptionCode.INVALID_OBJECT_INPUT, + ); + } + }); +}; + +type ValidateMetadataIdentifierFieldMetadataIdsArgs = { + labelIdentifierFieldMetadataId: string | undefined; + imageIdentifierFieldMetadataId: string | undefined; + fieldMetadataItems: FieldMetadataEntity[] | FieldMetadataInterface[]; +}; +export const validateMetadataIdentifierFieldMetadataIds = ({ + imageIdentifierFieldMetadataId, + labelIdentifierFieldMetadataId, + fieldMetadataItems, +}: ValidateMetadataIdentifierFieldMetadataIdsArgs) => { + const isMatchingFieldMetadataDefined: Validator['validator'] = ({ + matchingFieldMetadata, + }) => !isDefined(matchingFieldMetadata); + + if (isDefined(labelIdentifierFieldMetadataId)) { + validatorRunner({ + fieldMetadataId: labelIdentifierFieldMetadataId, + fieldMetadataItems, + validators: [ + { + validator: isMatchingFieldMetadataDefined, + label: + 'labelIdentifierFieldMetadataId validation failed: related field metadata not found', + }, + { + validator: ({ matchingFieldMetadata }) => + isDefined(matchingFieldMetadata) && + !isLabelIdentifierFieldMetadataTypes(matchingFieldMetadata.type), + label: + 'labelIdentifierFieldMetadataId validation failed: it must be a TEXT or FULL_NAME field metadata type id', + }, + ], + }); + } + + if (isDefined(imageIdentifierFieldMetadataId)) { + validatorRunner({ + fieldMetadataId: imageIdentifierFieldMetadataId, + fieldMetadataItems, + validators: [ + { + validator: isMatchingFieldMetadataDefined, + label: + 'imageIdentifierFieldMetadataId validation failed: related field metadata not found', + }, + ], + }); + } +}; diff --git a/packages/twenty-server/test/integration/metadata/suites/object-metadata/__snapshots__/failing-update-one-object-metadata.integration-spec.ts.snap b/packages/twenty-server/test/integration/metadata/suites/object-metadata/__snapshots__/failing-update-one-object-metadata.integration-spec.ts.snap new file mode 100644 index 000000000..f8932e59e --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/object-metadata/__snapshots__/failing-update-one-object-metadata.integration-spec.ts.snap @@ -0,0 +1,34 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`Object metadata update should fail when labelIdentifier is not a TEXT or NAME field 1`] = ` +[ + { + "extensions": { + "code": "BAD_USER_INPUT", + }, + "message": "labelIdentifierFieldMetadataId validation failed: it must be a TEXT or FULL_NAME field metadata type id", + }, +] +`; + +exports[`Object metadata update should fail when labelIdentifier is not a known field metadata id 1`] = ` +[ + { + "extensions": { + "code": "BAD_USER_INPUT", + }, + "message": "labelIdentifierFieldMetadataId validation failed: related field metadata not found", + }, +] +`; + +exports[`Object metadata update should fail when labelIdentifier is not a uuid 1`] = ` +[ + { + "extensions": { + "code": "BAD_USER_INPUT", + }, + "message": "labelIdentifierFieldMetadataId must be a UUID", + }, +] +`; diff --git a/packages/twenty-server/test/integration/metadata/suites/object-metadata/failing-update-one-object-metadata.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/object-metadata/failing-update-one-object-metadata.integration-spec.ts new file mode 100644 index 000000000..5368c03ed --- /dev/null +++ b/packages/twenty-server/test/integration/metadata/suites/object-metadata/failing-update-one-object-metadata.integration-spec.ts @@ -0,0 +1,96 @@ +import { createOneFieldMetadata } from 'test/integration/metadata/suites/field-metadata/utils/create-one-field-metadata.util'; +import { createOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/create-one-object-metadata.util'; +import { deleteOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/delete-one-object-metadata.util'; +import { getMockCreateObjectInput } from 'test/integration/metadata/suites/object-metadata/utils/generate-mock-create-object-metadata-input'; +import { updateOneObjectMetadata } from 'test/integration/metadata/suites/object-metadata/utils/update-one-object-metadata.util'; +import { EachTestingContext } from 'twenty-shared/testing'; +import { FieldMetadataType } from 'twenty-shared/types'; + +import { UpdateObjectPayload } from 'src/engine/metadata-modules/object-metadata/dtos/update-object.input'; + +type TestingRuntimeContext = { + objectMetadataId: string; + numberFieldMetadataId: string; +}; + +type CreateOneObjectMetadataItemTestingContext = EachTestingContext< + | ((args: TestingRuntimeContext) => Partial) + | Partial +>[]; + +const labelIdentifierFailingTestsUseCase: CreateOneObjectMetadataItemTestingContext = + [ + { + title: 'when labelIdentifier is not a uuid', + context: { + labelIdentifierFieldMetadataId: 'not-a-uuid', + }, + }, + { + title: 'when labelIdentifier is not a known field metadata id', + context: { + labelIdentifierFieldMetadataId: '42422020-f49c-4159-8751-76a24f47b360', + }, + }, + { + title: 'when labelIdentifier is not a TEXT or NAME field', + context: ({ numberFieldMetadataId }) => ({ + labelIdentifierFieldMetadataId: numberFieldMetadataId, + }), + }, + ]; + +const allTestsUseCases = [...labelIdentifierFailingTestsUseCase]; + +describe('Object metadata update should fail', () => { + let objectMetadataId: string; + let numberFieldMetadataId: string; + + beforeAll(async () => { + const { data } = await createOneObjectMetadata({ + input: getMockCreateObjectInput(), + }); + + objectMetadataId = data.createOneObject.id; + + const { + data: { createOneField }, + } = await createOneFieldMetadata({ + input: { + objectMetadataId: objectMetadataId, + name: 'testName', + label: 'Test name', + isLabelSyncedWithName: true, + type: FieldMetadataType.NUMBER, + }, + }); + + numberFieldMetadataId = createOneField.id; + }); + + afterAll(async () => { + await deleteOneObjectMetadata({ + input: { + idToDelete: objectMetadataId, + }, + }); + }); + + it.each(allTestsUseCases)('$title', async ({ context }) => { + const updatePayload = + typeof context === 'function' + ? context({ numberFieldMetadataId, objectMetadataId }) + : context; + + const { errors } = await updateOneObjectMetadata({ + input: { + idToUpdate: objectMetadataId, + updatePayload, + }, + expectToFail: true, + }); + + expect(errors).toBeDefined(); + expect(errors).toMatchSnapshot(); + }); +}); diff --git a/packages/twenty-front/src/modules/object-metadata/constants/LabelIdentifierFieldMetadataTypes.ts b/packages/twenty-shared/src/constants/LabelIdentifierFieldMetadataTypes.ts similarity index 55% rename from packages/twenty-front/src/modules/object-metadata/constants/LabelIdentifierFieldMetadataTypes.ts rename to packages/twenty-shared/src/constants/LabelIdentifierFieldMetadataTypes.ts index b164fc620..66eefa316 100644 --- a/packages/twenty-front/src/modules/object-metadata/constants/LabelIdentifierFieldMetadataTypes.ts +++ b/packages/twenty-shared/src/constants/LabelIdentifierFieldMetadataTypes.ts @@ -1,7 +1,6 @@ -import { FieldMetadataType } from '~/generated-metadata/graphql'; +import { FieldMetadataType } from "@/types"; export const LABEL_IDENTIFIER_FIELD_METADATA_TYPES = [ - FieldMetadataType.NUMBER, FieldMetadataType.TEXT, FieldMetadataType.FULL_NAME, ]; diff --git a/packages/twenty-shared/src/constants/index.ts b/packages/twenty-shared/src/constants/index.ts index e2ae5ead0..293129327 100644 --- a/packages/twenty-shared/src/constants/index.ts +++ b/packages/twenty-shared/src/constants/index.ts @@ -10,6 +10,7 @@ export { FIELD_FOR_TOTAL_COUNT_AGGREGATE_OPERATION } from './FieldForTotalCountAggregateOperation'; export { MAX_OPTIONS_TO_DISPLAY } from './FieldMetadataMaxOptionsToDisplay'; export { FIELD_RESTRICTED_ADDITIONAL_PERMISSIONS_REQUIRED } from './FieldRestrictedAdditionalPermissionsRequired'; +export { LABEL_IDENTIFIER_FIELD_METADATA_TYPES } from './LabelIdentifierFieldMetadataTypes'; export { PermissionsOnAllObjectRecords } from './PermissionsOnAllObjectRecords'; export { QUERY_MAX_RECORDS } from './QueryMaxRecords'; export { STANDARD_OBJECT_RECORDS_UNDER_OBJECT_RECORDS_PERMISSIONS } from './StandardObjectRecordsUnderObjectRecordsPermissions'; diff --git a/packages/twenty-shared/src/utils/index.ts b/packages/twenty-shared/src/utils/index.ts index 6a11da1bf..41c2fdd5e 100644 --- a/packages/twenty-shared/src/utils/index.ts +++ b/packages/twenty-shared/src/utils/index.ts @@ -25,6 +25,7 @@ export { getUrlHostnameOrThrow } from './url/getUrlHostnameOrThrow'; export { isValidHostname } from './url/isValidHostname'; export { isValidUrl } from './url/isValidUrl'; export { isDefined } from './validation/isDefined'; +export { isLabelIdentifierFieldMetadataTypes } from './validation/isLabelIdentifierFieldMetadataTypes'; export { isValidLocale } from './validation/isValidLocale'; export { isValidUuid } from './validation/isValidUuid'; export { isValidVariable } from './validation/isValidVariable'; diff --git a/packages/twenty-shared/src/utils/validation/isLabelIdentifierFieldMetadataTypes.ts b/packages/twenty-shared/src/utils/validation/isLabelIdentifierFieldMetadataTypes.ts new file mode 100644 index 000000000..9e6c31862 --- /dev/null +++ b/packages/twenty-shared/src/utils/validation/isLabelIdentifierFieldMetadataTypes.ts @@ -0,0 +1,7 @@ +import { LABEL_IDENTIFIER_FIELD_METADATA_TYPES } from '@/constants/LabelIdentifierFieldMetadataTypes'; +import { FieldMetadataType } from '@/types'; + +export const isLabelIdentifierFieldMetadataTypes = ( + value: FieldMetadataType, +): value is (typeof LABEL_IDENTIFIER_FIELD_METADATA_TYPES)[number] => + LABEL_IDENTIFIER_FIELD_METADATA_TYPES.includes(value);