From 155aa4995e9db2ca65dd177538d989513647e0e9 Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Mon, 15 Sep 2025 10:20:33 +0200 Subject: [PATCH] Handle edge deletion from loop (#14489) Update delete edge endpoint. It should handles the deletion between the iterator and the loop step ids --- .../workflow-version-edge.resolver.ts | 8 +- ...low-version-edge.workspace-service.spec.ts | 169 +++++++++++++++++- ...workflow-version-edge.workspace-service.ts | 117 +++++++++++- .../types/WorkflowStepCreationOptions.ts | 2 +- .../utils/__tests__/insert-step.spec.ts | 8 +- .../utils/insert-step.ts | 2 +- 6 files changed, 285 insertions(+), 21 deletions(-) diff --git a/packages/twenty-server/src/engine/core-modules/workflow/resolvers/workflow-version-edge.resolver.ts b/packages/twenty-server/src/engine/core-modules/workflow/resolvers/workflow-version-edge.resolver.ts index f3094b19aa2..1d5cf56468d 100644 --- a/packages/twenty-server/src/engine/core-modules/workflow/resolvers/workflow-version-edge.resolver.ts +++ b/packages/twenty-server/src/engine/core-modules/workflow/resolvers/workflow-version-edge.resolver.ts @@ -56,13 +56,19 @@ export class WorkflowVersionEdgeResolver { async deleteWorkflowVersionEdge( @AuthWorkspace() { id: workspaceId }: Workspace, @Args('input') - { source, target, workflowVersionId }: CreateWorkflowVersionEdgeInput, + { + source, + target, + workflowVersionId, + sourceConnectionOptions, + }: CreateWorkflowVersionEdgeInput, ): Promise { return this.workflowVersionEdgeWorkspaceService.deleteWorkflowVersionEdge({ source, target, workflowVersionId, workspaceId, + sourceConnectionOptions, }); } } diff --git a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/__tests__/workflow-version-edge.workspace-service.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/__tests__/workflow-version-edge.workspace-service.spec.ts index be98ffba328..27cb34a625a 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/__tests__/workflow-version-edge.workspace-service.spec.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/__tests__/workflow-version-edge.workspace-service.spec.ts @@ -242,7 +242,7 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { ); }); - it('should add target to initialLoopStepIds when shouldInsertToLoop is true', async () => { + it('should add target to initialLoopStepIds when isConnectedToLoop is true', async () => { const result = await service.createWorkflowVersionEdge({ source: 'iterator-step', target: 'step-3', @@ -251,7 +251,7 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { sourceConnectionOptions: { connectedStepType: WorkflowActionType.ITERATOR, settings: { - shouldInsertToLoop: true, + isConnectedToLoop: true, }, }, }); @@ -291,7 +291,7 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { sourceConnectionOptions: { connectedStepType: WorkflowActionType.ITERATOR, settings: { - shouldInsertToLoop: true, + isConnectedToLoop: true, }, }, }); @@ -321,7 +321,7 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { sourceConnectionOptions: { connectedStepType: WorkflowActionType.ITERATOR, settings: { - shouldInsertToLoop: true, + isConnectedToLoop: true, }, }, }); @@ -331,7 +331,7 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { ); }); - it('should create normal edge when shouldInsertToLoop is false', async () => { + it('should create normal edge when isConnectedToLoop is false', async () => { const result = await service.createWorkflowVersionEdge({ source: 'iterator-step', target: 'step-3', @@ -340,7 +340,7 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { sourceConnectionOptions: { connectedStepType: WorkflowActionType.ITERATOR, settings: { - shouldInsertToLoop: false, + isConnectedToLoop: false, }, }, }); @@ -590,6 +590,163 @@ describe('WorkflowVersionEdgeWorkspaceService', () => { }); }); + describe('with sourceConnectionOptions', () => { + describe('with iterator step', () => { + const mockIteratorStep = { + id: 'iterator-step', + type: WorkflowActionType.ITERATOR, + settings: { + errorHandlingOptions: { + continueOnFailure: { value: false }, + retryOnFailure: { value: false }, + }, + input: { + initialLoopStepIds: ['step-2', 'step-3'], + }, + }, + nextStepIds: ['step-1'], + } as WorkflowAction; + + const mockStepsWithIterator = [mockIteratorStep, ...mockSteps]; + + const mockWorkflowVersionWithIterator = { + ...mockWorkflowVersion, + steps: mockStepsWithIterator, + }; + + beforeEach(() => { + workflowCommonWorkspaceService.getWorkflowVersionOrFail.mockResolvedValue( + mockWorkflowVersionWithIterator, + ); + }); + + it('should throw if source step is not an iterator when connectedStepType is ITERATOR', async () => { + const call = async () => + await service.deleteWorkflowVersionEdge({ + source: 'step-1', + target: 'step-2', + workflowVersionId: mockWorkflowVersionId, + workspaceId: mockWorkspaceId, + sourceConnectionOptions: { + connectedStepType: WorkflowActionType.ITERATOR, + settings: { + isConnectedToLoop: true, + }, + }, + }); + + await expect(call).rejects.toThrow( + `Source step 'step-1' is not an iterator`, + ); + }); + + it('should remove target from initialLoopStepIds when isConnectedToLoop is true', async () => { + const result = await service.deleteWorkflowVersionEdge({ + source: 'iterator-step', + target: 'step-2', + workflowVersionId: mockWorkflowVersionId, + workspaceId: mockWorkspaceId, + sourceConnectionOptions: { + connectedStepType: WorkflowActionType.ITERATOR, + settings: { + isConnectedToLoop: true, + }, + }, + }); + + expect( + mockWorkflowVersionWorkspaceRepository.update, + ).toHaveBeenCalledWith(mockWorkflowVersionId, { + steps: expect.arrayContaining([ + expect.objectContaining({ + id: 'iterator-step', + settings: expect.objectContaining({ + input: expect.objectContaining({ + initialLoopStepIds: ['step-3'], + }), + }), + }), + ]), + }); + + expect(result).toEqual({ + triggerNextStepIds: ['step-1'], + stepsNextStepIds: { + 'iterator-step': ['step-1'], + 'step-1': ['step-2'], + 'step-2': [], + 'step-3': [], + }, + }); + }); + + it('should not update if target is not in initialLoopStepIds when isConnectedToLoop is true', async () => { + const result = await service.deleteWorkflowVersionEdge({ + source: 'iterator-step', + target: 'step-1', + workflowVersionId: mockWorkflowVersionId, + workspaceId: mockWorkspaceId, + sourceConnectionOptions: { + connectedStepType: WorkflowActionType.ITERATOR, + settings: { + isConnectedToLoop: true, + }, + }, + }); + + expect( + mockWorkflowVersionWorkspaceRepository.update, + ).not.toHaveBeenCalled(); + + expect(result).toEqual({ + triggerNextStepIds: ['step-1'], + stepsNextStepIds: { + 'iterator-step': ['step-1'], + 'step-1': ['step-2'], + 'step-2': [], + 'step-3': [], + }, + }); + }); + + it('should remove target from nextStepIds when isConnectedToLoop is false', async () => { + const result = await service.deleteWorkflowVersionEdge({ + source: 'iterator-step', + target: 'step-1', + workflowVersionId: mockWorkflowVersionId, + workspaceId: mockWorkspaceId, + sourceConnectionOptions: { + connectedStepType: WorkflowActionType.ITERATOR, + settings: { + isConnectedToLoop: false, + }, + }, + }); + + expect( + mockWorkflowVersionWorkspaceRepository.update, + ).toHaveBeenCalledWith(mockWorkflowVersionId, { + steps: expect.arrayContaining([ + expect.objectContaining({ + id: 'iterator-step', + nextStepIds: [], + }), + ]), + }); + + expect(result).toEqual({ + triggerNextStepIds: ['step-1'], + stepsNextStepIds: { + 'iterator-step': [], + 'step-1': ['step-2'], + 'step-2': [], + 'step-3': [], + }, + }); + }); + }); + }); + describe('with filter steps', () => { it('should delete the filter step when deleting edge from trigger to target through filter', async () => { const mockStepsWithFilter = [ diff --git a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/workflow-version-edge.workspace-service.ts b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/workflow-version-edge.workspace-service.ts index 05422efe88c..4f6243366a4 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/workflow-version-edge.workspace-service.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-edge/workflow-version-edge.workspace-service.ts @@ -96,11 +96,13 @@ export class WorkflowVersionEdgeWorkspaceService { target, workflowVersionId, workspaceId, + sourceConnectionOptions, }: { source: string; target: string; workflowVersionId: string; workspaceId: string; + sourceConnectionOptions?: WorkflowStepConnectionOptions; }): Promise { const workflowVersionRepository = await this.twentyORMGlobalManager.getRepositoryForWorkspace( @@ -147,6 +149,7 @@ export class WorkflowVersionEdgeWorkspaceService { target, workflowVersion, workflowVersionRepository, + sourceConnectionOptions, }); } } @@ -270,7 +273,7 @@ export class WorkflowVersionEdgeWorkspaceService { ); } - if (sourceConnectionOptions.settings.shouldInsertToLoop) { + if (sourceConnectionOptions.settings.isConnectedToLoop) { const currentInitialLoopStepIds = sourceStep.settings.input.initialLoopStepIds; @@ -394,6 +397,7 @@ export class WorkflowVersionEdgeWorkspaceService { target, workflowVersion, workflowVersionRepository, + sourceConnectionOptions, }: { trigger: WorkflowTrigger | null; steps: WorkflowAction[]; @@ -401,6 +405,7 @@ export class WorkflowVersionEdgeWorkspaceService { target: string; workflowVersion: WorkflowVersionWorkspaceEntity; workflowVersionRepository: WorkspaceRepository; + sourceConnectionOptions?: WorkflowStepConnectionOptions; }): Promise { const sourceStep = steps.find((step) => step.id === source); @@ -411,7 +416,17 @@ export class WorkflowVersionEdgeWorkspaceService { ); } - if (!sourceStep.nextStepIds?.includes(target)) { + // TODO: Remove this once we start using filters as regular steps + const isIteratorWithLoopTarget = + isDefined(sourceConnectionOptions) && + sourceConnectionOptions.connectedStepType === + WorkflowActionType.ITERATOR && + sourceConnectionOptions.settings.isConnectedToLoop; + + if ( + !sourceStep.nextStepIds?.includes(target) && + !isIteratorWithLoopTarget + ) { return await this.handleFilterBetweenSourceAndTarget({ trigger, steps, @@ -422,12 +437,30 @@ export class WorkflowVersionEdgeWorkspaceService { }); } - const updatedSourceStep = { - ...sourceStep, - nextStepIds: sourceStep.nextStepIds?.filter( - (nextStepId: string) => nextStepId !== target, - ), - }; + const { updatedSourceStep, shouldPersist } = isDefined( + sourceConnectionOptions, + ) + ? this.buildUpdatedSourceStepWithOptions({ + sourceStep, + target, + sourceConnectionOptions, + }) + : { + updatedSourceStep: { + ...sourceStep, + nextStepIds: sourceStep.nextStepIds?.filter( + (nextStepId: string) => nextStepId !== target, + ), + }, + shouldPersist: true, + }; + + if (!shouldPersist) { + return computeWorkflowVersionStepChanges({ + trigger, + steps, + }); + } const updatedSteps = steps.map((step) => { if (step.id === source) { @@ -550,6 +583,74 @@ export class WorkflowVersionEdgeWorkspaceService { }); } + private buildUpdatedSourceStepWithOptions({ + sourceStep, + target, + sourceConnectionOptions, + }: { + sourceStep: WorkflowAction; + target: string; + sourceConnectionOptions: WorkflowStepConnectionOptions; + }): { + updatedSourceStep: WorkflowAction; + shouldPersist: boolean; + } { + switch (sourceConnectionOptions.connectedStepType) { + case WorkflowActionType.ITERATOR: + if (sourceStep.type !== WorkflowActionType.ITERATOR) { + throw new WorkflowVersionEdgeException( + `Source step '${sourceStep.id}' is not an iterator`, + WorkflowVersionEdgeExceptionCode.INVALID_REQUEST, + ); + } + + if (sourceConnectionOptions.settings.isConnectedToLoop) { + const currentInitialLoopStepIds = + sourceStep.settings.input.initialLoopStepIds; + + if (!currentInitialLoopStepIds?.includes(target)) { + return { + updatedSourceStep: sourceStep, + shouldPersist: false, + }; + } + + return { + updatedSourceStep: { + ...sourceStep, + settings: { + ...sourceStep.settings, + input: { + ...sourceStep.settings.input, + initialLoopStepIds: currentInitialLoopStepIds.filter( + (id) => id !== target, + ), + }, + }, + }, + shouldPersist: true, + }; + } + + return { + updatedSourceStep: { + ...sourceStep, + nextStepIds: sourceStep.nextStepIds?.filter((id) => id !== target), + }, + shouldPersist: true, + }; + + default: + return { + updatedSourceStep: { + ...sourceStep, + nextStepIds: sourceStep.nextStepIds?.filter((id) => id !== target), + }, + shouldPersist: true, + }; + } + } + private findFilterBetweenNodes({ steps, sourceNextStepIds, diff --git a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/types/WorkflowStepCreationOptions.ts b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/types/WorkflowStepCreationOptions.ts index 088c15b0001..b0c81095092 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/types/WorkflowStepCreationOptions.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/types/WorkflowStepCreationOptions.ts @@ -3,7 +3,7 @@ import { type WorkflowActionType } from 'src/modules/workflow/workflow-executor/ type WorkflowIteratorStepConnectionOptions = { connectedStepType: WorkflowActionType.ITERATOR; settings: { - shouldInsertToLoop: boolean; + isConnectedToLoop: boolean; }; }; diff --git a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/__tests__/insert-step.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/__tests__/insert-step.spec.ts index 0d81832d596..6c481c7006c 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/__tests__/insert-step.spec.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/__tests__/insert-step.spec.ts @@ -178,7 +178,7 @@ describe('insertStep', () => { }); }); - it('should add step to iterator initialLoopStepIds when shouldInsertToLoop is true', () => { + it('should add step to iterator initialLoopStepIds when isConnectedToLoop is true', () => { const existingTrigger = createMockTrigger(['1']); const newStep = createMockAction('new'); @@ -190,7 +190,7 @@ describe('insertStep', () => { parentStepConnectionOptions: { connectedStepType: WorkflowActionType.ITERATOR, settings: { - shouldInsertToLoop: true, + isConnectedToLoop: true, }, }, }); @@ -204,7 +204,7 @@ describe('insertStep', () => { ]); }); - it('should not add step to iterator initialLoopStepIds when shouldInsertToLoop is false', () => { + it('should not add step to iterator initialLoopStepIds when isConnectedToLoop is false', () => { const existingTrigger = createMockTrigger(['1']); const newStep = createMockAction('new'); @@ -216,7 +216,7 @@ describe('insertStep', () => { parentStepConnectionOptions: { connectedStepType: WorkflowActionType.ITERATOR, settings: { - shouldInsertToLoop: false, + isConnectedToLoop: false, }, }, }); diff --git a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/insert-step.ts b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/insert-step.ts index 3b316a3225a..b6c1d1ca86f 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/insert-step.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/insert-step.ts @@ -173,7 +173,7 @@ const updateStepsWithOptions = ({ switch (parentStepConnectionOptions.connectedStepType) { case WorkflowActionType.ITERATOR: - if (!parentStepConnectionOptions.settings.shouldInsertToLoop) { + if (!parentStepConnectionOptions.settings.isConnectedToLoop) { break; }