From cb759aa22e33db2c451786a32cfe23eca2dc1bd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Bosi?= <71827178+bosiraphael@users.noreply.github.com> Date: Thu, 13 Nov 2025 12:01:11 +0100 Subject: [PATCH] Fix concurrent page layout edition bug (#15740) This PR fixes a bug which could happen if two people were editing a page layout at the same time. If one person deletes a widget or a tab and saves first, and the second person saves after but doesn't delete this widget or tab, an error is raised. This is because, in the backend, a diff is computed to know which tab or widget to create, update or delete. But the logic was maid without considering soft deletion. So, when the second person saves, the diff tries to create the widget or tab which had been soft deleted by the first use. Since they have the same id, a duplicate primary key error is raised. Now we always consider that the last user to save has the truth. So we restore the tab/widget and update it. --- .../__tests__/page-layout-tab.service.spec.ts | 4 +- .../page-layout-widget.service.spec.ts | 2 +- .../services/page-layout-tab.service.ts | 3 +- .../services/page-layout-update.service.ts | 39 +++++++++- .../services/page-layout-widget.service.ts | 3 +- .../compute-diff-between-objects.test.ts | 73 +++++++++++++++++-- .../src/utils/compute-diff-between-objects.ts | 30 +++++--- 7 files changed, 131 insertions(+), 23 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-tab.service.spec.ts b/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-tab.service.spec.ts index 9b6128a0fd9..e7247c54c40 100644 --- a/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-tab.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-tab.service.spec.ts @@ -111,10 +111,10 @@ describe('PageLayoutTabService', () => { where: { pageLayoutId, pageLayout: { workspaceId }, - deletedAt: expect.anything(), }, order: { position: 'ASC' }, relations: ['widgets'], + withDeleted: false, }); expect(result).toEqual(expectedTabs); }); @@ -154,10 +154,10 @@ describe('PageLayoutTabService', () => { where: { pageLayoutId, pageLayout: { workspaceId }, - deletedAt: expect.anything(), }, order: { position: 'ASC' }, relations: ['widgets'], + withDeleted: false, }); expect(result).toEqual(expectedTabs); }); diff --git a/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-widget.service.spec.ts b/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-widget.service.spec.ts index 8109e5ee2af..4827a0a1166 100644 --- a/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-widget.service.spec.ts +++ b/packages/twenty-server/src/engine/core-modules/page-layout/services/__tests__/page-layout-widget.service.spec.ts @@ -107,9 +107,9 @@ describe('PageLayoutWidgetService', () => { where: { pageLayoutTabId, workspaceId, - deletedAt: IsNull(), }, order: { createdAt: 'ASC' }, + withDeleted: false, }); expect(result).toEqual(expectedWidgets); }); diff --git a/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-tab.service.ts b/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-tab.service.ts index ed346597c21..6cd1bbd871c 100644 --- a/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-tab.service.ts +++ b/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-tab.service.ts @@ -39,6 +39,7 @@ export class PageLayoutTabService { workspaceId: string, pageLayoutId: string, transactionManager?: EntityManager, + withDeleted = false, ): Promise { const repository = this.getPageLayoutTabRepository(transactionManager); @@ -46,10 +47,10 @@ export class PageLayoutTabService { where: { pageLayoutId, pageLayout: { workspaceId }, - deletedAt: IsNull(), }, order: { position: 'ASC' }, relations: ['widgets'], + withDeleted, }); } diff --git a/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-update.service.ts b/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-update.service.ts index 618914a7d77..760547d2b0d 100644 --- a/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-update.service.ts +++ b/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-update.service.ts @@ -6,7 +6,6 @@ import { DataSource, EntityManager } from 'typeorm'; import { CreatePageLayoutWidgetInput } from 'src/engine/core-modules/page-layout/dtos/inputs/create-page-layout-widget.input'; import { UpdatePageLayoutTabWithWidgetsInput } from 'src/engine/core-modules/page-layout/dtos/inputs/update-page-layout-tab-with-widgets.input'; import { UpdatePageLayoutWidgetWithIdInput } from 'src/engine/core-modules/page-layout/dtos/inputs/update-page-layout-widget-with-id.input'; -import { UpdatePageLayoutWidgetInput } from 'src/engine/core-modules/page-layout/dtos/inputs/update-page-layout-widget.input'; import { UpdatePageLayoutWithTabsInput } from 'src/engine/core-modules/page-layout/dtos/inputs/update-page-layout-with-tabs.input'; import { PageLayoutTabEntity } from 'src/engine/core-modules/page-layout/entities/page-layout-tab.entity'; import { PageLayoutWidgetEntity } from 'src/engine/core-modules/page-layout/entities/page-layout-widget.entity'; @@ -131,11 +130,13 @@ export class PageLayoutUpdateService { workspaceId, pageLayoutId, transactionManager, + true, ); const { toCreate: entitiesToCreate, toUpdate: entitiesToUpdate, + toRestoreAndUpdate: entitiesToRestoreAndUpdate, idsToDelete, } = computeDiffBetweenObjects< PageLayoutTabEntity, @@ -165,6 +166,23 @@ export class PageLayoutUpdateService { ); } + for (const tabToRestoreAndUpdate of entitiesToRestoreAndUpdate) { + await this.pageLayoutTabService.restore( + tabToRestoreAndUpdate.id, + workspaceId, + transactionManager, + ); + + const { widgets: _widgets, ...updateData } = tabToRestoreAndUpdate; + + await this.pageLayoutTabService.update( + tabToRestoreAndUpdate.id, + workspaceId, + updateData, + transactionManager, + ); + } + for (const tabToCreate of entitiesToCreate) { await this.pageLayoutTabService.create( { @@ -197,11 +215,13 @@ export class PageLayoutUpdateService { workspaceId, tabId, transactionManager, + true, ); const { toCreate: entitiesToCreate, toUpdate: entitiesToUpdate, + toRestoreAndUpdate: entitiesToRestoreAndUpdate, idsToDelete, } = computeDiffBetweenObjects< PageLayoutWidgetEntity, @@ -231,7 +251,22 @@ export class PageLayoutUpdateService { await this.pageLayoutWidgetService.update( widgetUpdate.id, workspaceId, - widgetUpdate as UpdatePageLayoutWidgetInput, + widgetUpdate, + transactionManager, + ); + } + + for (const widgetToRestoreAndUpdate of entitiesToRestoreAndUpdate) { + await this.pageLayoutWidgetService.restore( + widgetToRestoreAndUpdate.id, + workspaceId, + transactionManager, + ); + + await this.pageLayoutWidgetService.update( + widgetToRestoreAndUpdate.id, + workspaceId, + widgetToRestoreAndUpdate, transactionManager, ); } diff --git a/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-widget.service.ts b/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-widget.service.ts index b6908a15ed7..05442277fe8 100644 --- a/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-widget.service.ts +++ b/packages/twenty-server/src/engine/core-modules/page-layout/services/page-layout-widget.service.ts @@ -46,6 +46,7 @@ export class PageLayoutWidgetService { workspaceId: string, pageLayoutTabId: string, transactionManager?: EntityManager, + withDeleted = false, ): Promise { const repository = this.getPageLayoutWidgetRepository(transactionManager); @@ -53,9 +54,9 @@ export class PageLayoutWidgetService { where: { pageLayoutTabId, workspaceId, - deletedAt: IsNull(), }, order: { createdAt: 'ASC' }, + withDeleted, }); } diff --git a/packages/twenty-shared/src/utils/__tests__/compute-diff-between-objects.test.ts b/packages/twenty-shared/src/utils/__tests__/compute-diff-between-objects.test.ts index 663c4a27b06..0ca3238d210 100644 --- a/packages/twenty-shared/src/utils/__tests__/compute-diff-between-objects.test.ts +++ b/packages/twenty-shared/src/utils/__tests__/compute-diff-between-objects.test.ts @@ -3,8 +3,8 @@ import { computeDiffBetweenObjects } from '../compute-diff-between-objects'; describe('computeDiffBetweenObjects', () => { it('should return the correct diff', () => { const existingObjects = [ - { id: '1', name: 'Object 1' }, - { id: '2', name: 'Object 2' }, + { id: '1', name: 'Object 1', deletedAt: null }, + { id: '2', name: 'Object 2', deletedAt: null }, ]; const receivedObjects = [ { id: '1', name: 'Object 1' }, @@ -20,12 +20,13 @@ describe('computeDiffBetweenObjects', () => { expect(diff).toEqual({ toCreate: [{ id: '3', name: 'Object 3' }], toUpdate: [], + toRestoreAndUpdate: [], idsToDelete: ['2'], }); }); it('should return the correct diff when the properties to compare are empty', () => { - const existingObjects = [{ id: '1', name: 'Object 1' }]; + const existingObjects = [{ id: '1', name: 'Object 1', deletedAt: null }]; const receivedObjects = [{ id: '1', name: 'Object 1' }]; const diff = computeDiffBetweenObjects({ @@ -37,12 +38,13 @@ describe('computeDiffBetweenObjects', () => { expect(diff).toEqual({ toCreate: [], toUpdate: [], + toRestoreAndUpdate: [], idsToDelete: [], }); }); it('should return the correct diff when the existing objects are empty', () => { - const existingObjects: { id: string; name: string }[] = []; + const existingObjects: { id: string; name: string; deletedAt: null }[] = []; const receivedObjects = [{ id: '1', name: 'Object 1' }]; const diff = computeDiffBetweenObjects({ @@ -54,12 +56,13 @@ describe('computeDiffBetweenObjects', () => { expect(diff).toEqual({ toCreate: [{ id: '1', name: 'Object 1' }], toUpdate: [], + toRestoreAndUpdate: [], idsToDelete: [], }); }); it('should return the correct diff when the received objects are empty', () => { - const existingObjects = [{ id: '1', name: 'Object 1' }]; + const existingObjects = [{ id: '1', name: 'Object 1', deletedAt: null }]; const receivedObjects: { id: string; name: string }[] = []; const diff = computeDiffBetweenObjects({ @@ -71,6 +74,66 @@ describe('computeDiffBetweenObjects', () => { expect(diff).toEqual({ toCreate: [], toUpdate: [], + toRestoreAndUpdate: [], + idsToDelete: ['1'], + }); + }); + + it('should detect updates when properties change', () => { + const existingObjects = [{ id: '1', name: 'Object 1', deletedAt: null }]; + const receivedObjects = [{ id: '1', name: 'Updated Object 1' }]; + + const diff = computeDiffBetweenObjects({ + existingObjects, + receivedObjects, + propertiesToCompare: ['name'], + }); + + expect(diff).toEqual({ + toCreate: [], + toUpdate: [{ id: '1', name: 'Updated Object 1' }], + toRestoreAndUpdate: [], + idsToDelete: [], + }); + }); + + it('should restore and update deleted objects', () => { + const existingObjects = [ + { id: '1', name: 'Object 1', deletedAt: new Date('2024-01-01') }, + ]; + const receivedObjects = [{ id: '1', name: 'Restored Object 1' }]; + + const diff = computeDiffBetweenObjects({ + existingObjects, + receivedObjects, + propertiesToCompare: ['name'], + }); + + expect(diff).toEqual({ + toCreate: [], + toUpdate: [], + toRestoreAndUpdate: [{ id: '1', name: 'Restored Object 1' }], + idsToDelete: [], + }); + }); + + it('should not include deleted objects in idsToDelete', () => { + const existingObjects = [ + { id: '1', name: 'Object 1', deletedAt: null }, + { id: '2', name: 'Object 2', deletedAt: new Date('2024-01-01') }, + ]; + const receivedObjects: { id: string; name: string }[] = []; + + const diff = computeDiffBetweenObjects({ + existingObjects, + receivedObjects, + propertiesToCompare: ['name'], + }); + + expect(diff).toEqual({ + toCreate: [], + toUpdate: [], + toRestoreAndUpdate: [], idsToDelete: ['1'], }); }); diff --git a/packages/twenty-shared/src/utils/compute-diff-between-objects.ts b/packages/twenty-shared/src/utils/compute-diff-between-objects.ts index 3572ce0e9d0..78d652c043d 100644 --- a/packages/twenty-shared/src/utils/compute-diff-between-objects.ts +++ b/packages/twenty-shared/src/utils/compute-diff-between-objects.ts @@ -4,6 +4,7 @@ import deepEqual from 'deep-equal'; type Diff = { toCreate: T[]; toUpdate: T[]; + toRestoreAndUpdate: T[]; idsToDelete: string[]; }; @@ -29,7 +30,7 @@ type ComputeDiffBetweenObjectsParams< }; export const computeDiffBetweenObjects = < - T extends { id: string }, + T extends { id: string; deletedAt: Date | null }, K extends { id: string }, >({ existingObjects, @@ -38,6 +39,7 @@ export const computeDiffBetweenObjects = < }: ComputeDiffBetweenObjectsParams): Diff => { const toCreate: K[] = []; const toUpdate: K[] = []; + const toRestoreAndUpdate: K[] = []; const existingEntitiesMap = new Map( existingObjects.map((entity) => [entity.id, entity]), @@ -50,18 +52,22 @@ export const computeDiffBetweenObjects = < const existingEntity = existingEntitiesMap.get(receivedObject.id); if (isDefined(existingEntity)) { - const comparableExistingEntity = extractProperties( - existingEntity, - propertiesToCompare, - ); + if (isDefined(existingEntity.deletedAt)) { + toRestoreAndUpdate.push(receivedObject); + } else { + const comparableExistingEntity = extractProperties( + existingEntity, + propertiesToCompare, + ); - const comparableReceivedEntity = extractProperties( - receivedObject, - propertiesToCompare, - ); + const comparableReceivedEntity = extractProperties( + receivedObject, + propertiesToCompare, + ); - if (!deepEqual(comparableExistingEntity, comparableReceivedEntity)) { - toUpdate.push(receivedObject); + if (!deepEqual(comparableExistingEntity, comparableReceivedEntity)) { + toUpdate.push(receivedObject); + } } } else { toCreate.push(receivedObject); @@ -69,12 +75,14 @@ export const computeDiffBetweenObjects = < } const idsToDelete = existingObjects + .filter((existingEntity) => !isDefined(existingEntity.deletedAt)) .filter((existingEntity) => !receivedEntitiesMap.has(existingEntity.id)) .map((entity) => entity.id); return { toCreate, toUpdate, + toRestoreAndUpdate, idsToDelete, }; };