From d5ba1a059b7a67154f17f8ad3fcfe66c5c031059 Mon Sep 17 00:00:00 2001 From: Jon Date: Mon, 18 Nov 2024 11:54:20 +0000 Subject: [PATCH 1/5] fix(Google Sheets Trigger Node): Fix issue with regex showing correct sheet as invalid (#11770) --- .../Google/Sheet/GoogleSheetsTrigger.node.ts | 8 +- .../Google/Sheet/test/v2/utils/utils.test.ts | 81 ++++++++++++++++++- .../Sheet/v2/actions/sheet/Sheet.resource.ts | 8 +- packages/nodes-base/nodes/Google/constants.ts | 3 + 4 files changed, 89 insertions(+), 11 deletions(-) diff --git a/packages/nodes-base/nodes/Google/Sheet/GoogleSheetsTrigger.node.ts b/packages/nodes-base/nodes/Google/Sheet/GoogleSheetsTrigger.node.ts index 1558b7fd1918f..8993760e574e5 100644 --- a/packages/nodes-base/nodes/Google/Sheet/GoogleSheetsTrigger.node.ts +++ b/packages/nodes-base/nodes/Google/Sheet/GoogleSheetsTrigger.node.ts @@ -7,7 +7,7 @@ import type { } from 'n8n-workflow'; import { NodeConnectionType, NodeOperationError } from 'n8n-workflow'; -import { GOOGLE_DRIVE_FILE_URL_REGEX } from '../constants'; +import { GOOGLE_DRIVE_FILE_URL_REGEX, GOOGLE_SHEETS_SHEET_URL_REGEX } from '../constants'; import { apiRequest } from './v2/transport'; import { sheetsSearch, spreadSheetsSearch } from './v2/methods/listSearch'; import { GoogleSheet } from './v2/helpers/GoogleSheet'; @@ -137,15 +137,13 @@ export class GoogleSheetsTrigger implements INodeType { type: 'string', extractValue: { type: 'regex', - regex: - 'https:\\/\\/docs\\.google\\.com/spreadsheets\\/d\\/[0-9a-zA-Z\\-_]+\\/edit\\#gid=([0-9]+)', + regex: GOOGLE_SHEETS_SHEET_URL_REGEX, }, validation: [ { type: 'regex', properties: { - regex: - 'https:\\/\\/docs\\.google\\.com/spreadsheets\\/d\\/[0-9a-zA-Z\\-_]+\\/edit\\#gid=([0-9]+)', + regex: GOOGLE_SHEETS_SHEET_URL_REGEX, errorMessage: 'Not a valid Sheet URL', }, }, diff --git a/packages/nodes-base/nodes/Google/Sheet/test/v2/utils/utils.test.ts b/packages/nodes-base/nodes/Google/Sheet/test/v2/utils/utils.test.ts index 033f7249f8116..c725d03bb9e85 100644 --- a/packages/nodes-base/nodes/Google/Sheet/test/v2/utils/utils.test.ts +++ b/packages/nodes-base/nodes/Google/Sheet/test/v2/utils/utils.test.ts @@ -1,9 +1,16 @@ -import type { IExecuteFunctions, INode, ResourceMapperField } from 'n8n-workflow'; +import { + NodeOperationError, + type IExecuteFunctions, + type INode, + type ResourceMapperField, +} from 'n8n-workflow'; + import { GoogleSheet } from '../../../v2/helpers/GoogleSheet'; import { addRowNumber, autoMapInputData, checkForSchemaChanges, + getSpreadsheetId, prepareSheetData, removeEmptyColumns, removeEmptyRows, @@ -11,6 +18,8 @@ import { trimToFirstEmptyRow, } from '../../../v2/helpers/GoogleSheets.utils'; +import { GOOGLE_SHEETS_SHEET_URL_REGEX } from '../../../../constants'; + describe('Test Google Sheets, addRowNumber', () => { it('should add row nomber', () => { const data = [ @@ -444,3 +453,73 @@ describe('Test Google Sheets, checkForSchemaChanges', () => { ).toThrow("Column names were updated after the node's setup"); }); }); + +describe('Test Google Sheets, getSpreadsheetId', () => { + let mockNode: INode; + + beforeEach(() => { + mockNode = { name: 'Google Sheets' } as INode; + jest.clearAllMocks(); + }); + + it('should throw an error if value is empty', () => { + expect(() => getSpreadsheetId(mockNode, 'url', '')).toThrow(NodeOperationError); + }); + + it('should return the ID from a valid URL', () => { + const url = + 'https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit#gid=0'; + const result = getSpreadsheetId(mockNode, 'url', url); + expect(result).toBe('1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms'); + }); + + it('should return an empty string for an invalid URL', () => { + const url = 'https://docs.google.com/spreadsheets/d/'; + const result = getSpreadsheetId(mockNode, 'url', url); + expect(result).toBe(''); + }); + + it('should return the value for documentIdType byId or byList', () => { + const value = '1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms'; + expect(getSpreadsheetId(mockNode, 'id', value)).toBe(value); + expect(getSpreadsheetId(mockNode, 'list', value)).toBe(value); + }); +}); + +describe('Test Google Sheets, Google Sheets Sheet URL Regex', () => { + const regex = new RegExp(GOOGLE_SHEETS_SHEET_URL_REGEX); + + it('should match a valid Google Sheets URL', () => { + const urls = [ + 'https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit#gid=0', + 'https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit#gid=123456', + 'https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit?gid=654321#gid=654321', + ]; + for (const url of urls) { + expect(regex.test(url)).toBe(true); + } + }); + + it('should not match an invalid Google Sheets URL', () => { + const url = 'https://docs.google.com/spreadsheets/d/'; + expect(regex.test(url)).toBe(false); + }); + + it('should not match a URL that does not match the pattern', () => { + const url = + 'https://example.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit#gid=0'; + expect(regex.test(url)).toBe(false); + }); + + it('should extract the gid from a valid Google Sheets URL', () => { + const urls = [ + 'https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit#gid=12345', + 'https://docs.google.com/spreadsheets/d/1BxiMVs0XRA5nFMdKvBdBZjgmUUqptlbs74OgvE2upms/edit?gid=12345#gid=12345', + ]; + for (const url of urls) { + const match = url.match(regex); + expect(match).not.toBeNull(); + expect(match?.[1]).toBe('12345'); + } + }); +}); diff --git a/packages/nodes-base/nodes/Google/Sheet/v2/actions/sheet/Sheet.resource.ts b/packages/nodes-base/nodes/Google/Sheet/v2/actions/sheet/Sheet.resource.ts index 3cb7866aa5dee..d46e72811a076 100644 --- a/packages/nodes-base/nodes/Google/Sheet/v2/actions/sheet/Sheet.resource.ts +++ b/packages/nodes-base/nodes/Google/Sheet/v2/actions/sheet/Sheet.resource.ts @@ -1,5 +1,5 @@ import type { INodeProperties } from 'n8n-workflow'; -import { GOOGLE_DRIVE_FILE_URL_REGEX } from '../../../../constants'; +import { GOOGLE_DRIVE_FILE_URL_REGEX, GOOGLE_SHEETS_SHEET_URL_REGEX } from '../../../../constants'; import * as append from './append.operation'; import * as appendOrUpdate from './appendOrUpdate.operation'; import * as clear from './clear.operation'; @@ -156,15 +156,13 @@ export const descriptions: INodeProperties[] = [ type: 'string', extractValue: { type: 'regex', - regex: - 'https:\\/\\/docs\\.google.com\\/spreadsheets\\/d\\/[0-9a-zA-Z\\-_]+.*\\#gid=([0-9]+)', + regex: GOOGLE_SHEETS_SHEET_URL_REGEX, }, validation: [ { type: 'regex', properties: { - regex: - 'https:\\/\\/docs\\.google.com\\/spreadsheets\\/d\\/[0-9a-zA-Z\\-_]+.*\\#gid=([0-9]+)', + regex: GOOGLE_SHEETS_SHEET_URL_REGEX, errorMessage: 'Not a valid Sheet URL', }, }, diff --git a/packages/nodes-base/nodes/Google/constants.ts b/packages/nodes-base/nodes/Google/constants.ts index 374cb345b5e1c..e0cd0555c924b 100644 --- a/packages/nodes-base/nodes/Google/constants.ts +++ b/packages/nodes-base/nodes/Google/constants.ts @@ -3,3 +3,6 @@ export const GOOGLE_DRIVE_FILE_URL_REGEX = export const GOOGLE_DRIVE_FOLDER_URL_REGEX = 'https:\\/\\/drive\\.google\\.com(?:\\/.*|)\\/folders\\/([0-9a-zA-Z\\-_]+)(?:\\/.*|)'; + +export const GOOGLE_SHEETS_SHEET_URL_REGEX = + 'https:\\/\\/docs\\.google\\.com\\/spreadsheets\\/d\\/[0-9a-zA-Z\\-_]+.*\\#gid=([0-9]+)'; From 61696c3db313cdc97925af728ff5c68415f9b6b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 18 Nov 2024 12:58:26 +0100 Subject: [PATCH 2/5] feat(core): Improve handling of manual executions with wait nodes (#11750) Co-authored-by: Michael Kret --- packages/@n8n/api-types/src/push/execution.ts | 8 + .../__tests__/save-execution-progress.test.ts | 32 ---- .../save-execution-progress.ts | 2 +- .../to-save-settings.ts | 28 +-- packages/cli/src/webhooks/webhook-helpers.ts | 5 + .../src/workflow-execute-additional-data.ts | 38 ++-- .../cli/templates/form-trigger.handlebars | 8 - packages/core/src/WorkflowExecute.ts | 5 +- .../editor-ui/src/components/InputPanel.vue | 5 +- .../src/components/NodeExecuteButton.vue | 17 +- .../editor-ui/src/components/OutputPanel.vue | 2 +- packages/editor-ui/src/components/RunData.vue | 19 +- .../src/composables/usePushConnection.ts | 5 +- .../src/composables/useRunWorkflow.test.ts | 75 +------- .../src/composables/useRunWorkflow.ts | 164 +----------------- .../src/stores/workflows.store.test.ts | 29 ++-- .../editor-ui/src/stores/workflows.store.ts | 90 +++++++--- .../src/utils/executionUtils.test.ts | 148 +++++++++++++++- .../editor-ui/src/utils/executionUtils.ts | 15 +- packages/editor-ui/src/views/NodeView.v2.vue | 15 +- packages/editor-ui/src/views/NodeView.vue | 17 +- 21 files changed, 325 insertions(+), 402 deletions(-) diff --git a/packages/@n8n/api-types/src/push/execution.ts b/packages/@n8n/api-types/src/push/execution.ts index 3c7459dec5715..9c723e2817f8a 100644 --- a/packages/@n8n/api-types/src/push/execution.ts +++ b/packages/@n8n/api-types/src/push/execution.ts @@ -12,6 +12,13 @@ type ExecutionStarted = { }; }; +type ExecutionWaiting = { + type: 'executionWaiting'; + data: { + executionId: string; + }; +}; + type ExecutionFinished = { type: 'executionFinished'; data: { @@ -45,6 +52,7 @@ type NodeExecuteAfter = { export type ExecutionPushMessage = | ExecutionStarted + | ExecutionWaiting | ExecutionFinished | ExecutionRecovered | NodeExecuteBefore diff --git a/packages/cli/src/execution-lifecycle-hooks/__tests__/save-execution-progress.test.ts b/packages/cli/src/execution-lifecycle-hooks/__tests__/save-execution-progress.test.ts index b0db5becac714..d89f2fb734d2f 100644 --- a/packages/cli/src/execution-lifecycle-hooks/__tests__/save-execution-progress.test.ts +++ b/packages/cli/src/execution-lifecycle-hooks/__tests__/save-execution-progress.test.ts @@ -1,5 +1,4 @@ import { - deepCopy, ErrorReporterProxy, type IRunExecutionData, type ITaskData, @@ -87,37 +86,6 @@ test('should update execution when saving progress is enabled', async () => { expect(reporterSpy).not.toHaveBeenCalled(); }); -test('should update execution when saving progress is disabled, but waitTill is defined', async () => { - jest.spyOn(fnModule, 'toSaveSettings').mockReturnValue({ - ...commonSettings, - progress: false, - }); - - const reporterSpy = jest.spyOn(ErrorReporterProxy, 'error'); - - executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse); - - const args = deepCopy(commonArgs); - args[4].waitTill = new Date(); - await saveExecutionProgress(...args); - - expect(executionRepository.updateExistingExecution).toHaveBeenCalledWith('some-execution-id', { - data: { - executionData: undefined, - resultData: { - lastNodeExecuted: 'My Node', - runData: { - 'My Node': [{}], - }, - }, - startData: {}, - }, - status: 'running', - }); - - expect(reporterSpy).not.toHaveBeenCalled(); -}); - test('should report error on failure', async () => { jest.spyOn(fnModule, 'toSaveSettings').mockReturnValue({ ...commonSettings, diff --git a/packages/cli/src/execution-lifecycle-hooks/save-execution-progress.ts b/packages/cli/src/execution-lifecycle-hooks/save-execution-progress.ts index 6cd1cfd08f78c..ca9899e1ec1a2 100644 --- a/packages/cli/src/execution-lifecycle-hooks/save-execution-progress.ts +++ b/packages/cli/src/execution-lifecycle-hooks/save-execution-progress.ts @@ -16,7 +16,7 @@ export async function saveExecutionProgress( ) { const saveSettings = toSaveSettings(workflowData.settings); - if (!saveSettings.progress && !executionData.waitTill) return; + if (!saveSettings.progress) return; const logger = Container.get(Logger); diff --git a/packages/cli/src/execution-lifecycle-hooks/to-save-settings.ts b/packages/cli/src/execution-lifecycle-hooks/to-save-settings.ts index 7a25adaeba912..a7af8f3ddc233 100644 --- a/packages/cli/src/execution-lifecycle-hooks/to-save-settings.ts +++ b/packages/cli/src/execution-lifecycle-hooks/to-save-settings.ts @@ -18,20 +18,20 @@ export function toSaveSettings(workflowSettings: IWorkflowSettings = {}) { PROGRESS: config.getEnv('executions.saveExecutionProgress'), }; + const { + saveDataErrorExecution = DEFAULTS.ERROR, + saveDataSuccessExecution = DEFAULTS.SUCCESS, + saveManualExecutions = DEFAULTS.MANUAL, + saveExecutionProgress = DEFAULTS.PROGRESS, + } = workflowSettings; + return { - error: workflowSettings.saveDataErrorExecution - ? workflowSettings.saveDataErrorExecution !== 'none' - : DEFAULTS.ERROR !== 'none', - success: workflowSettings.saveDataSuccessExecution - ? workflowSettings.saveDataSuccessExecution !== 'none' - : DEFAULTS.SUCCESS !== 'none', - manual: - workflowSettings === undefined || workflowSettings.saveManualExecutions === 'DEFAULT' - ? DEFAULTS.MANUAL - : (workflowSettings.saveManualExecutions ?? DEFAULTS.MANUAL), - progress: - workflowSettings === undefined || workflowSettings.saveExecutionProgress === 'DEFAULT' - ? DEFAULTS.PROGRESS - : (workflowSettings.saveExecutionProgress ?? DEFAULTS.PROGRESS), + error: saveDataErrorExecution === 'DEFAULT' ? DEFAULTS.ERROR : saveDataErrorExecution === 'all', + success: + saveDataSuccessExecution === 'DEFAULT' + ? DEFAULTS.SUCCESS + : saveDataSuccessExecution === 'all', + manual: saveManualExecutions === 'DEFAULT' ? DEFAULTS.MANUAL : saveManualExecutions, + progress: saveExecutionProgress === 'DEFAULT' ? DEFAULTS.PROGRESS : saveExecutionProgress, }; } diff --git a/packages/cli/src/webhooks/webhook-helpers.ts b/packages/cli/src/webhooks/webhook-helpers.ts index 72628b8351695..6110584f7e555 100644 --- a/packages/cli/src/webhooks/webhook-helpers.ts +++ b/packages/cli/src/webhooks/webhook-helpers.ts @@ -464,6 +464,11 @@ export async function executeWebhook( projectId: project?.id, }; + // When resuming from a wait node, copy over the pushRef from the execution-data + if (!runData.pushRef) { + runData.pushRef = runExecutionData.pushRef; + } + let responsePromise: IDeferredPromise | undefined; if (responseMode === 'responseNode') { responsePromise = createDeferredPromise(); diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index 08d6ba09e41a6..97322f4fe0230 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -307,7 +307,7 @@ function hookFunctionsPush(): IWorkflowExecuteHooks { }, ], workflowExecuteAfter: [ - async function (this: WorkflowHooks): Promise { + async function (this: WorkflowHooks, fullRunData: IRun): Promise { const { pushRef, executionId } = this; if (pushRef === undefined) return; @@ -318,7 +318,9 @@ function hookFunctionsPush(): IWorkflowExecuteHooks { workflowId, }); - pushInstance.send('executionFinished', { executionId }, pushRef); + const pushType = + fullRunData.status === 'waiting' ? 'executionWaiting' : 'executionFinished'; + pushInstance.send(pushType, { executionId }, pushRef); }, ], }; @@ -430,22 +432,21 @@ function hookFunctionsSave(): IWorkflowExecuteHooks { (executionStatus === 'success' && !saveSettings.success) || (executionStatus !== 'success' && !saveSettings.error); - if (shouldNotSave && !fullRunData.waitTill) { - if (!fullRunData.waitTill && !isManualMode) { - executeErrorWorkflow( - this.workflowData, - fullRunData, - this.mode, - this.executionId, - this.retryOf, - ); - await Container.get(ExecutionRepository).hardDelete({ - workflowId: this.workflowData.id, - executionId: this.executionId, - }); + if (shouldNotSave && !fullRunData.waitTill && !isManualMode) { + executeErrorWorkflow( + this.workflowData, + fullRunData, + this.mode, + this.executionId, + this.retryOf, + ); - return; - } + await Container.get(ExecutionRepository).hardDelete({ + workflowId: this.workflowData.id, + executionId: this.executionId, + }); + + return; } // Although it is treated as IWorkflowBase here, it's being instantiated elsewhere with properties that may be sensitive @@ -1110,6 +1111,9 @@ export function getWorkflowHooksWorkerMain( hookFunctions.nodeExecuteAfter = []; hookFunctions.workflowExecuteAfter = [ async function (this: WorkflowHooks, fullRunData: IRun): Promise { + // Don't delete executions before they are finished + if (!fullRunData.finished) return; + const executionStatus = determineFinalExecutionStatus(fullRunData); const saveSettings = toSaveSettings(this.workflowData.settings); diff --git a/packages/cli/templates/form-trigger.handlebars b/packages/cli/templates/form-trigger.handlebars index 57d93cb29144b..02611f5b5b285 100644 --- a/packages/cli/templates/form-trigger.handlebars +++ b/packages/cli/templates/form-trigger.handlebars @@ -740,14 +740,6 @@ } return; - }).then(() => { - window.addEventListener('storage', function(event) { - if (event.key === 'n8n_redirect_to_next_form_test_page' && event.newValue) { - const newUrl = event.newValue; - localStorage.removeItem('n8n_redirect_to_next_form_test_page'); - window.location.replace(newUrl); - } - }); }) .catch(function (error) { console.error('Error:', error); diff --git a/packages/core/src/WorkflowExecute.ts b/packages/core/src/WorkflowExecute.ts index 2ae12908aca69..c6e0316038cc7 100644 --- a/packages/core/src/WorkflowExecute.ts +++ b/packages/core/src/WorkflowExecute.ts @@ -916,7 +916,6 @@ export class WorkflowExecute { let nodeSuccessData: INodeExecutionData[][] | null | undefined; let runIndex: number; let startTime: number; - let taskData: ITaskData; if (this.runExecutionData.startData === undefined) { this.runExecutionData.startData = {}; @@ -1446,13 +1445,13 @@ export class WorkflowExecute { this.runExecutionData.resultData.runData[executionNode.name] = []; } - taskData = { + const taskData: ITaskData = { hints: executionHints, startTime, executionTime: new Date().getTime() - startTime, source: !executionData.source ? [] : executionData.source.main, metadata: executionData.metadata, - executionStatus: 'success', + executionStatus: this.runExecutionData.waitTill ? 'waiting' : 'success', }; if (executionError !== undefined) { diff --git a/packages/editor-ui/src/components/InputPanel.vue b/packages/editor-ui/src/components/InputPanel.vue index 169c61de46f97..a3ddc8ed64203 100644 --- a/packages/editor-ui/src/components/InputPanel.vue +++ b/packages/editor-ui/src/components/InputPanel.vue @@ -212,7 +212,10 @@ const activeNodeType = computed(() => { return nodeTypesStore.getNodeType(activeNode.value.type, activeNode.value.typeVersion); }); -const waitingMessage = computed(() => waitingNodeTooltip()); +const waitingMessage = computed(() => { + const parentNode = parentNodes.value[0]; + return parentNode && waitingNodeTooltip(workflowsStore.getNodeByName(parentNode.name)); +}); watch( inputMode, diff --git a/packages/editor-ui/src/components/NodeExecuteButton.vue b/packages/editor-ui/src/components/NodeExecuteButton.vue index 43b4dfa7dcfd4..f801f0701c45e 100644 --- a/packages/editor-ui/src/components/NodeExecuteButton.vue +++ b/packages/editor-ui/src/components/NodeExecuteButton.vue @@ -65,7 +65,7 @@ const lastPopupCountUpdate = ref(0); const codeGenerationInProgress = ref(false); const router = useRouter(); -const { runWorkflow, runWorkflowResolvePending, stopCurrentExecution } = useRunWorkflow({ router }); +const { runWorkflow, stopCurrentExecution } = useRunWorkflow({ router }); const workflowsStore = useWorkflowsStore(); const externalHooks = useExternalHooks(); @@ -353,17 +353,10 @@ async function onClick() { telemetry.track('User clicked execute node button', telemetryPayload); await externalHooks.run('nodeExecuteButton.onClick', telemetryPayload); - if (workflowsStore.isWaitingExecution) { - await runWorkflowResolvePending({ - destinationNode: props.nodeName, - source: 'RunData.ExecuteNodeButton', - }); - } else { - await runWorkflow({ - destinationNode: props.nodeName, - source: 'RunData.ExecuteNodeButton', - }); - } + await runWorkflow({ + destinationNode: props.nodeName, + source: 'RunData.ExecuteNodeButton', + }); emit('execute'); } diff --git a/packages/editor-ui/src/components/OutputPanel.vue b/packages/editor-ui/src/components/OutputPanel.vue index 0503ad5c94f45..0ca7e3830eab4 100644 --- a/packages/editor-ui/src/components/OutputPanel.vue +++ b/packages/editor-ui/src/components/OutputPanel.vue @@ -352,7 +352,7 @@ const activatePane = () => {