diff --git a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.handler.ts b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.handler.ts index 8a432ecea7..a6f58d44eb 100644 --- a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.handler.ts +++ b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.handler.ts @@ -27,7 +27,6 @@ import { import { WorkflowsService } from '@/workflows/workflows.services'; import { InternalHooks } from '@/InternalHooks'; import { RoleService } from '@/services/role.service'; -import { isWorkflowHistoryLicensed } from '@/workflows/workflowHistory/workflowHistoryHelper.ee'; import { WorkflowHistoryService } from '@/workflows/workflowHistory/workflowHistory.service.ee'; export = { @@ -47,13 +46,11 @@ export = { const createdWorkflow = await createWorkflow(workflow, req.user, role); - if (isWorkflowHistoryLicensed()) { - await Container.get(WorkflowHistoryService).saveVersion( - req.user, - createdWorkflow, - createdWorkflow.id, - ); - } + await Container.get(WorkflowHistoryService).saveVersion( + req.user, + createdWorkflow, + createdWorkflow.id, + ); await Container.get(ExternalHooks).run('workflow.afterCreate', [createdWorkflow]); void Container.get(InternalHooks).onWorkflowCreated(req.user, createdWorkflow, true); @@ -202,7 +199,7 @@ export = { const updatedWorkflow = await getWorkflowById(sharedWorkflow.workflowId); - if (isWorkflowHistoryLicensed() && updatedWorkflow) { + if (updatedWorkflow) { await Container.get(WorkflowHistoryService).saveVersion( req.user, updatedWorkflow, diff --git a/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts b/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts index a8f6e59b46..c9a7cdd348 100644 --- a/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts +++ b/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts @@ -6,6 +6,7 @@ import { SharedWorkflowRepository } from '@/databases/repositories'; import { WorkflowHistoryRepository } from '@db/repositories/workflowHistory.repository'; import { Service } from 'typedi'; import { isWorkflowHistoryEnabled } from './workflowHistoryHelper.ee'; +import { getLogger } from '@/Logger'; export class SharedWorkflowNotFoundError extends Error {} export class HistoryVersionNotFoundError extends Error {} @@ -66,13 +67,20 @@ export class WorkflowHistoryService { async saveVersion(user: User, workflow: WorkflowEntity, workflowId: string) { if (isWorkflowHistoryEnabled()) { - await this.workflowHistoryRepository.insert({ - authors: user.firstName + ' ' + user.lastName, - connections: workflow.connections, - nodes: workflow.nodes, - versionId: workflow.versionId, - workflowId, - }); + try { + await this.workflowHistoryRepository.insert({ + authors: user.firstName + ' ' + user.lastName, + connections: workflow.connections, + nodes: workflow.nodes, + versionId: workflow.versionId, + workflowId, + }); + } catch (e) { + getLogger().error( + `Failed to save workflow history version for workflow ${workflowId}`, + e as Error, + ); + } } } } diff --git a/packages/cli/src/workflows/workflows.controller.ee.ts b/packages/cli/src/workflows/workflows.controller.ee.ts index a23f3d9613..c2d75dafe8 100644 --- a/packages/cli/src/workflows/workflows.controller.ee.ts +++ b/packages/cli/src/workflows/workflows.controller.ee.ts @@ -22,6 +22,7 @@ import { RoleService } from '@/services/role.service'; import * as utils from '@/utils'; import { listQueryMiddleware } from '@/middlewares'; import { TagService } from '@/services/tag.service'; +import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee'; // eslint-disable-next-line @typescript-eslint/naming-convention export const EEWorkflowController = express.Router(); @@ -186,6 +187,12 @@ EEWorkflowController.post( ); } + await Container.get(WorkflowHistoryService).saveVersion( + req.user, + savedWorkflow, + savedWorkflow.id, + ); + if (tagIds && !config.getEnv('workflowTagsDisabled') && savedWorkflow.tags) { savedWorkflow.tags = Container.get(TagService).sortByRequestOrder(savedWorkflow.tags, { requestOrder: tagIds, diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 80732655ae..c6f79b4922 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -26,7 +26,6 @@ import { RoleService } from '@/services/role.service'; import * as utils from '@/utils'; import { listQueryMiddleware } from '@/middlewares'; import { TagService } from '@/services/tag.service'; -import { isWorkflowHistoryLicensed } from './workflowHistory/workflowHistoryHelper.ee'; import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee'; export const workflowsController = express.Router(); @@ -101,13 +100,11 @@ workflowsController.post( throw new ResponseHelper.InternalServerError('Failed to save workflow'); } - if (isWorkflowHistoryLicensed()) { - await Container.get(WorkflowHistoryService).saveVersion( - req.user, - savedWorkflow, - savedWorkflow.id, - ); - } + await Container.get(WorkflowHistoryService).saveVersion( + req.user, + savedWorkflow, + savedWorkflow.id, + ); if (tagIds && !config.getEnv('workflowTagsDisabled') && savedWorkflow.tags) { savedWorkflow.tags = Container.get(TagService).sortByRequestOrder(savedWorkflow.tags, { diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index a9cd3aeb77..02e8b5ba26 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -33,7 +33,6 @@ import { WorkflowRepository } from '@/databases/repositories'; import { RoleService } from '@/services/role.service'; import { OwnershipService } from '@/services/ownership.service'; import { isStringArray, isWorkflowIdValid } from '@/utils'; -import { isWorkflowHistoryLicensed } from './workflowHistory/workflowHistoryHelper.ee'; import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee'; import { BinaryDataService } from 'n8n-core'; @@ -222,13 +221,19 @@ export class WorkflowsService { ); } + let onlyActiveUpdate = false; + if ( - Object.keys(workflow).length === 3 && - workflow.id !== undefined && - workflow.versionId !== undefined && - workflow.active !== undefined + (Object.keys(workflow).length === 3 && + workflow.id !== undefined && + workflow.versionId !== undefined && + workflow.active !== undefined) || + (Object.keys(workflow).length === 2 && + workflow.versionId !== undefined && + workflow.active !== undefined) ) { // we're just updating the active status of the workflow, don't update the versionId + onlyActiveUpdate = true; } else { // Update the workflow's version workflow.versionId = uuid(); @@ -301,7 +306,7 @@ export class WorkflowsService { ); } - if (isWorkflowHistoryLicensed() && workflow.versionId !== shared.workflow.versionId) { + if (!onlyActiveUpdate && workflow.versionId !== shared.workflow.versionId) { await Container.get(WorkflowHistoryService).saveVersion(user, workflow, workflowId); } diff --git a/packages/cli/test/integration/shared/testDb.ts b/packages/cli/test/integration/shared/testDb.ts index 287feb6d0f..7187a67fb1 100644 --- a/packages/cli/test/integration/shared/testDb.ts +++ b/packages/cli/test/integration/shared/testDb.ts @@ -447,7 +447,7 @@ export async function createManyWorkflows( * @param user user to assign the workflow to */ export async function createWorkflow(attributes: Partial = {}, user?: User) { - const { active, name, nodes, connections } = attributes; + const { active, name, nodes, connections, versionId } = attributes; const workflowEntity = Db.collections.Workflow.create({ active: active ?? false, @@ -463,6 +463,7 @@ export async function createWorkflow(attributes: Partial = {}, u }, ], connections: connections ?? {}, + versionId: versionId ?? uuid(), ...attributes, }); diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index 6c63bbc8ae..f09c716537 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -12,6 +12,10 @@ import { createWorkflow, getGlobalMemberRole, getGlobalOwnerRole } from './share import type { SaveCredentialFunction } from './shared/types'; import { makeWorkflow } from './shared/utils/'; import { randomCredentialPayload } from './shared/random'; +import { License } from '@/License'; +import { WorkflowHistoryRepository } from '@/databases/repositories'; +import Container from 'typedi'; +import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; let owner: User; let member: User; @@ -21,6 +25,12 @@ let authMemberAgent: SuperAgentTest; let authAnotherMemberAgent: SuperAgentTest; let saveCredential: SaveCredentialFunction; +const licenseLike = utils.mockInstance(License, { + isWorkflowHistoryLicensed: jest.fn().mockReturnValue(false), + isWithinUsersLimit: jest.fn().mockReturnValue(true), +}); +const activeWorkflowRunnerLike = utils.mockInstance(ActiveWorkflowRunner); + const sharingSpy = jest.spyOn(UserManagementHelpers, 'isSharingEnabled').mockReturnValue(true); const testServer = utils.setupTestServer({ endpointGroups: ['workflows'], @@ -46,7 +56,11 @@ beforeAll(async () => { }); beforeEach(async () => { - await testDb.truncate(['Workflow', 'SharedWorkflow']); + activeWorkflowRunnerLike.add.mockReset(); + activeWorkflowRunnerLike.remove.mockReset(); + + await testDb.truncate(['Workflow', 'SharedWorkflow', WorkflowHistoryRepository]); + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); }); describe('router should switch based on flag', () => { @@ -399,6 +413,96 @@ describe('POST /workflows', () => { const response = await authAnotherMemberAgent.post('/workflows').send(workflow); expect(response.statusCode).toBe(200); }); + + test('Should create workflow history version when licensed', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(true); + const payload = { + name: 'testing', + nodes: [ + { + id: 'uuid-1234', + parameters: {}, + name: 'Start', + type: 'n8n-nodes-base.start', + typeVersion: 1, + position: [240, 300], + }, + ], + connections: {}, + staticData: null, + settings: { + saveExecutionProgress: true, + saveManualExecutions: true, + saveDataErrorExecution: 'all', + saveDataSuccessExecution: 'all', + executionTimeout: 3600, + timezone: 'America/New_York', + }, + active: false, + }; + + const response = await authOwnerAgent.post('/workflows').send(payload); + + expect(response.statusCode).toBe(200); + + const { + data: { id }, + } = response.body; + + expect(id).toBeDefined(); + expect( + await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }), + ).toBe(1); + const historyVersion = await Container.get(WorkflowHistoryRepository).findOne({ + where: { + workflowId: id, + }, + }); + expect(historyVersion).not.toBeNull(); + expect(historyVersion!.connections).toEqual(payload.connections); + expect(historyVersion!.nodes).toEqual(payload.nodes); + }); + + test('Should not create workflow history version when not licensed', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); + const payload = { + name: 'testing', + nodes: [ + { + id: 'uuid-1234', + parameters: {}, + name: 'Start', + type: 'n8n-nodes-base.start', + typeVersion: 1, + position: [240, 300], + }, + ], + connections: {}, + staticData: null, + settings: { + saveExecutionProgress: true, + saveManualExecutions: true, + saveDataErrorExecution: 'all', + saveDataSuccessExecution: 'all', + executionTimeout: 3600, + timezone: 'America/New_York', + }, + active: false, + }; + + const response = await authOwnerAgent.post('/workflows').send(payload); + + expect(response.statusCode).toBe(200); + + const { + data: { id }, + } = response.body; + + expect(id).toBeDefined(); + expect( + await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }), + ).toBe(0); + }); }); describe('PATCH /workflows/:id - validate credential permissions to user', () => { @@ -831,3 +935,160 @@ describe('getSharedWorkflowIds', () => { expect(sharedWorkflowIds).toContain(workflow3.id); }); }); + +describe('PATCH /workflows/:id - workflow history', () => { + test('Should create workflow history version when licensed', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(true); + const workflow = await testDb.createWorkflow({}, owner); + const payload = { + name: 'name updated', + versionId: workflow.versionId, + nodes: [ + { + id: 'uuid-1234', + parameters: {}, + name: 'Start', + type: 'n8n-nodes-base.start', + typeVersion: 1, + position: [240, 300], + }, + { + id: 'uuid-1234', + parameters: {}, + name: 'Cron', + type: 'n8n-nodes-base.cron', + typeVersion: 1, + position: [400, 300], + }, + ], + connections: {}, + staticData: '{"id":1}', + settings: { + saveExecutionProgress: false, + saveManualExecutions: false, + saveDataErrorExecution: 'all', + saveDataSuccessExecution: 'all', + executionTimeout: 3600, + timezone: 'America/New_York', + }, + }; + + const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload); + + const { + data: { id }, + } = response.body; + + expect(response.statusCode).toBe(200); + + expect(id).toBe(workflow.id); + expect( + await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }), + ).toBe(1); + const historyVersion = await Container.get(WorkflowHistoryRepository).findOne({ + where: { + workflowId: id, + }, + }); + expect(historyVersion).not.toBeNull(); + expect(historyVersion!.connections).toEqual(payload.connections); + expect(historyVersion!.nodes).toEqual(payload.nodes); + }); + + test('Should not create workflow history version when not licensed', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); + const workflow = await testDb.createWorkflow({}, owner); + const payload = { + name: 'name updated', + versionId: workflow.versionId, + nodes: [ + { + id: 'uuid-1234', + parameters: {}, + name: 'Start', + type: 'n8n-nodes-base.start', + typeVersion: 1, + position: [240, 300], + }, + { + id: 'uuid-1234', + parameters: {}, + name: 'Cron', + type: 'n8n-nodes-base.cron', + typeVersion: 1, + position: [400, 300], + }, + ], + connections: {}, + staticData: '{"id":1}', + settings: { + saveExecutionProgress: false, + saveManualExecutions: false, + saveDataErrorExecution: 'all', + saveDataSuccessExecution: 'all', + executionTimeout: 3600, + timezone: 'America/New_York', + }, + }; + + const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload); + + const { + data: { id }, + } = response.body; + + expect(response.statusCode).toBe(200); + + expect(id).toBe(workflow.id); + expect( + await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }), + ).toBe(0); + }); +}); + +describe('PATCH /workflows/:id - activate workflow', () => { + test('should activate workflow without changing version ID', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); + const workflow = await testDb.createWorkflow({}, owner); + const payload = { + versionId: workflow.versionId, + active: true, + }; + + const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload); + + expect(response.statusCode).toBe(200); + expect(activeWorkflowRunnerLike.add).toBeCalled(); + + const { + data: { id, versionId, active }, + } = response.body; + + expect(id).toBe(workflow.id); + expect(versionId).toBe(workflow.versionId); + expect(active).toBe(true); + }); + + test('should deactivate workflow without changing version ID', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); + const workflow = await testDb.createWorkflow({ active: true }, owner); + const payload = { + versionId: workflow.versionId, + active: false, + }; + + const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload); + + expect(response.statusCode).toBe(200); + expect(activeWorkflowRunnerLike.add).not.toBeCalled(); + expect(activeWorkflowRunnerLike.remove).toBeCalled(); + + const { + data: { id, versionId, active }, + } = response.body; + + expect(id).toBe(workflow.id); + expect(versionId).toBe(workflow.versionId); + expect(active).toBe(false); + }); +}); diff --git a/packages/cli/test/integration/workflows.controller.test.ts b/packages/cli/test/integration/workflows.controller.test.ts index cb49e8d9a1..2a816de1db 100644 --- a/packages/cli/test/integration/workflows.controller.test.ts +++ b/packages/cli/test/integration/workflows.controller.test.ts @@ -13,6 +13,7 @@ import Container from 'typedi'; import type { ListQuery } from '@/requests'; import { License } from '@/License'; import { WorkflowHistoryRepository } from '@/databases/repositories'; +import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; let owner: User; let authOwnerAgent: SuperAgentTest; @@ -27,12 +28,15 @@ const licenseLike = utils.mockInstance(License, { isWithinUsersLimit: jest.fn().mockReturnValue(true), }); +const activeWorkflowRunnerLike = utils.mockInstance(ActiveWorkflowRunner); + beforeAll(async () => { owner = await testDb.createOwner(); authOwnerAgent = testServer.authAgentFor(owner); }); beforeEach(async () => { + jest.resetAllMocks(); await testDb.truncate(['Workflow', 'SharedWorkflow', 'Tag', WorkflowHistoryRepository]); licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); }); @@ -207,7 +211,7 @@ describe('GET /workflows', () => { createdAt: any(String), updatedAt: any(String), tags: [{ id: any(String), name: 'A' }], - versionId: null, + versionId: any(String), ownedBy: { id: owner.id }, }), objectContaining({ @@ -217,7 +221,7 @@ describe('GET /workflows', () => { createdAt: any(String), updatedAt: any(String), tags: [], - versionId: null, + versionId: any(String), ownedBy: { id: owner.id }, }), ]), @@ -423,6 +427,7 @@ describe('PATCH /workflows/:id', () => { const workflow = await testDb.createWorkflow({}, owner); const payload = { name: 'name updated', + versionId: workflow.versionId, nodes: [ { id: 'uuid-1234', @@ -480,6 +485,7 @@ describe('PATCH /workflows/:id', () => { const workflow = await testDb.createWorkflow({}, owner); const payload = { name: 'name updated', + versionId: workflow.versionId, nodes: [ { id: 'uuid-1234', @@ -523,4 +529,49 @@ describe('PATCH /workflows/:id', () => { await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }), ).toBe(0); }); + + test('should activate workflow without changing version ID', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); + const workflow = await testDb.createWorkflow({}, owner); + const payload = { + versionId: workflow.versionId, + active: true, + }; + + const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload); + + expect(response.statusCode).toBe(200); + expect(activeWorkflowRunnerLike.add).toBeCalled(); + + const { + data: { id, versionId, active }, + } = response.body; + + expect(id).toBe(workflow.id); + expect(versionId).toBe(workflow.versionId); + expect(active).toBe(true); + }); + + test('should deactivate workflow without changing version ID', async () => { + licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false); + const workflow = await testDb.createWorkflow({ active: true }, owner); + const payload = { + versionId: workflow.versionId, + active: false, + }; + + const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload); + + expect(response.statusCode).toBe(200); + expect(activeWorkflowRunnerLike.add).not.toBeCalled(); + expect(activeWorkflowRunnerLike.remove).toBeCalled(); + + const { + data: { id, versionId, active }, + } = response.body; + + expect(id).toBe(workflow.id); + expect(versionId).toBe(workflow.versionId); + expect(active).toBe(false); + }); });