From f782f4dcd81e002766702a156c70b2751560175d Mon Sep 17 00:00:00 2001 From: Marie <51697796+ijreilly@users.noreply.github.com> Date: Thu, 15 May 2025 14:44:31 +0200 Subject: [PATCH] Fix CSV import upsert (#12048) Fixes https://github.com/twentyhq/twenty/issues/11864 and https://github.com/twentyhq/core-team-issues/issues/908 We should not send `createManyXXX` mutations with FE-forged ids in the payload if we want to do an upsert, because that 1) prevents records from being merged 2) triggers optimistic rendering while we can't know before-hand which records will actually be created and which records will only be updated Also noticed createdBy was being overriden even for records we are updating and not creating, which did not seem right, so fixed that too --- .../__tests__/useCreateManyRecords.test.tsx | 38 ++++++++++++++++ .../hooks/useCreateManyRecords.ts | 45 ++++++++++++------- ...phql-query-create-many-resolver.service.ts | 34 +++++++++++--- .../twenty-orm/utils/format-data.util.ts | 2 +- 4 files changed, 96 insertions(+), 23 deletions(-) diff --git a/packages/twenty-front/src/modules/object-record/hooks/__tests__/useCreateManyRecords.test.tsx b/packages/twenty-front/src/modules/object-record/hooks/__tests__/useCreateManyRecords.test.tsx index 82ff949fe..1523b99c1 100644 --- a/packages/twenty-front/src/modules/object-record/hooks/__tests__/useCreateManyRecords.test.tsx +++ b/packages/twenty-front/src/modules/object-record/hooks/__tests__/useCreateManyRecords.test.tsx @@ -40,6 +40,20 @@ const mocks = [ }, })), }, + { + request: { + query, + variables: { + data: input, + upsert: true, + }, + }, + result: jest.fn(() => ({ + data: { + createPeople: response, + }, + })), + }, ]; const Wrapper = getJestMetadataAndApolloMocksWrapper({ @@ -69,4 +83,28 @@ describe('useCreateManyRecords', () => { expect(mocks[0].result).toHaveBeenCalled(); expect(mockRefetchAggregateQueries).toHaveBeenCalledTimes(1); }); + + it('does not indicate id in request variables when upsert is true because we cant know if it will be an insert or an update', async () => { + const { result } = renderHook( + () => + useCreateManyRecords({ + objectNameSingular: CoreObjectNameSingular.Person, + }), + { + wrapper: Wrapper, + }, + ); + + await act(async () => { + const res = await result.current.createManyRecords(input, true); + expect(res).toEqual(response); + }); + + // Verify that the mutation was called with data without IDs + expect(mocks[1].request.variables.data).toEqual(input); + mocks[1].request.variables.data.forEach((record: any) => { + expect(record).not.toHaveProperty('id'); + }); + expect(mockRefetchAggregateQueries).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts b/packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts index 7880d575c..6cd54ae67 100644 --- a/packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts +++ b/packages/twenty-front/src/modules/object-record/hooks/useCreateManyRecords.ts @@ -27,6 +27,10 @@ type PartialObjectRecordWithId = Partial & { id: string; }; +type PartialObjectRecordWithOptionalId = Partial & { + id?: string; +}; + type useCreateManyRecordsProps = { objectNameSingular: string; recordGqlFields?: RecordGqlOperationGqlRecordFields; @@ -75,16 +79,20 @@ export const useCreateManyRecords = < recordsToCreate: Partial[], upsert?: boolean, ) => { - const sanitizedCreateManyRecordsInput: PartialObjectRecordWithId[] = []; + const sanitizedCreateManyRecordsInput: PartialObjectRecordWithOptionalId[] = + []; const recordOptimisticRecordsInput: PartialObjectRecordWithId[] = []; recordsToCreate.forEach((recordToCreate) => { - const idForCreation = recordToCreate?.id ?? v4(); + const shouldDoOptimisticEffect = upsert !== true; + const idForCreation = shouldDoOptimisticEffect + ? (recordToCreate?.id ?? v4()) + : undefined; const sanitizedRecord = { ...sanitizeRecordInput({ objectMetadataItem, recordInput: recordToCreate, }), - id: idForCreation, + ...(isDefined(idForCreation) ? { id: idForCreation } : {}), }; const baseOptimisticRecordInputCreatedBy: | { createdBy: FieldActorForInputValue } @@ -96,22 +104,25 @@ export const useCreateManyRecords = < }, } : undefined; - const optimisticRecordInput = { - ...computeOptimisticRecordFromInput({ - cache: apolloClient.cache, - objectMetadataItem, - objectMetadataItems, - currentWorkspaceMember: currentWorkspaceMember, - recordInput: { - ...baseOptimisticRecordInputCreatedBy, - ...recordToCreate, - }, - }), - id: idForCreation, - }; sanitizedCreateManyRecordsInput.push(sanitizedRecord); - recordOptimisticRecordsInput.push(optimisticRecordInput); + + if (shouldDoOptimisticEffect) { + const optimisticRecordInput = { + ...computeOptimisticRecordFromInput({ + cache: apolloClient.cache, + objectMetadataItem, + objectMetadataItems, + currentWorkspaceMember: currentWorkspaceMember, + recordInput: { + ...baseOptimisticRecordInputCreatedBy, + ...recordToCreate, + }, + }), + id: idForCreation as string, + }; + recordOptimisticRecordsInput.push(optimisticRecordInput); + } }); const recordsCreatedInCache = recordOptimisticRecordsInput diff --git a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts index 5418b9295..8391c57ba 100644 --- a/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts +++ b/packages/twenty-server/src/engine/api/graphql/graphql-query-runner/resolvers/graphql-query-create-many-resolver.service.ts @@ -266,15 +266,20 @@ export class GraphqlQueryCreateManyResolverService extends GraphqlQueryBaseResol for (const record of recordsToUpdate) { const recordId = record.id as string; - // TODO: we should align update and insert - // For insert, formating is done in the server - // While for update, formatting is done at the resolver level - - const formattedRecord = formatData( + // we should not update an existing record's createdBy value + const recordWithoutCreatedByUpdate = this.getRecordWithoutCreatedBy( record, objectMetadataItemWithFieldMaps, ); + const formattedRecord = formatData( + recordWithoutCreatedByUpdate, + objectMetadataItemWithFieldMaps, + ); + + // TODO: we should align update and insert + // For insert, formating is done in the server + // While for update, formatting is done at the resolver level await repository.update(recordId, formattedRecord); result.identifiers.push({ id: recordId }); result.generatedMaps.push({ id: recordId }); @@ -362,6 +367,25 @@ export class GraphqlQueryCreateManyResolverService extends GraphqlQueryBaseResol ); } + private getRecordWithoutCreatedBy( + record: Partial, + objectMetadataItemWithFieldMaps: ObjectMetadataItemWithFieldMaps, + ) { + let recordWithoutCreatedByUpdate = record; + + if ( + 'createdBy' in record && + objectMetadataItemWithFieldMaps.fieldsByName['createdBy']?.isCustom === + false + ) { + const { createdBy: _createdBy, ...recordWithoutCreatedBy } = record; + + recordWithoutCreatedByUpdate = recordWithoutCreatedBy; + } + + return recordWithoutCreatedByUpdate; + } + async validate( args: CreateManyResolverArgs>, options: WorkspaceQueryRunnerOptions, diff --git a/packages/twenty-server/src/engine/twenty-orm/utils/format-data.util.ts b/packages/twenty-server/src/engine/twenty-orm/utils/format-data.util.ts index ac9bb9805..05db3e9cf 100644 --- a/packages/twenty-server/src/engine/twenty-orm/utils/format-data.util.ts +++ b/packages/twenty-server/src/engine/twenty-orm/utils/format-data.util.ts @@ -1,5 +1,5 @@ -import { capitalize } from 'twenty-shared/utils'; import { FieldMetadataType } from 'twenty-shared/types'; +import { capitalize } from 'twenty-shared/utils'; import { FieldMetadataInterface } from 'src/engine/metadata-modules/field-metadata/interfaces/field-metadata.interface';