feat(editor): Prevent saving of workflow when canvas is loading (#6497)

* feat(editor): Prevent saving of pristine workflow

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Prevent saving if loading

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Fix 7-workflow-actions spec

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Restrict delay intercept to GET only

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Wait for WF patch

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Add helper to remove all active WFs in e2e

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Use META_KEY env var

* Remove cy.wait

* Delete debugging DB reset console log

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

* Fix clashin mixins `isReadOnly` property

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

---------

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

cypress/e2e/7-workflow-actions.cy.ts

@@ -5,6 +5,7 @@ import {
 	SCHEDULE_TRIGGER_NODE_NAME,
 } from '../constants';
 import { WorkflowPage as WorkflowPageClass } from '../pages/workflow';
+import { WorkflowsPage as WorkflowsPageClass } from '../pages/workflows';
 
 const NEW_WORKFLOW_NAME = 'Something else';
 const IMPORT_WORKFLOW_URL = 'https://gist.githubusercontent.com/OlegIvaniv/010bd3f45c8a94f8eb7012e663a8b671/raw/3afea1aec15573cc168d9af7e79395bd76082906/test-workflow.json';
@@ -12,6 +13,7 @@ const DUPLICATE_WORKFLOW_NAME = 'Duplicated workflow';
 const DUPLICATE_WORKFLOW_TAG = 'Duplicate';
 
 const WorkflowPage = new WorkflowPageClass();
+const WorkflowPages = new WorkflowsPageClass();
 
 describe('Workflow Actions', () => {
 	before(() => {
@@ -66,6 +68,42 @@ describe('Workflow Actions', () => {
 			.should('eq', NEW_WORKFLOW_NAME);
 	});
 
+	it('should not save workflow if canvas is loading', () => {
+		let interceptCalledCount = 0;
+
+    // There's no way in Cypress to check if intercept was not called
+		// so we'll count the number of times it was called
+    cy.intercept('PATCH', '/rest/workflows/*', () => {
+      interceptCalledCount++;
+    }).as('saveWorkflow');
+
+		WorkflowPage.actions.addNodeToCanvas(MANUAL_TRIGGER_NODE_NAME);
+		WorkflowPage.actions.saveWorkflowOnButtonClick();
+		cy.intercept(
+			{
+				url: '/rest/workflows/*',
+				method: 'GET',
+				middleware: true,
+			},
+			(req) => {
+				// Delay the response to give time for the save to be triggered
+				req.on('response', async (res) => {
+					await new Promise((resolve) => setTimeout(resolve, 2000))
+					res.send();
+				})
+			}
+		)
+		cy.reload();
+		cy.get('.el-loading-mask').should('exist');
+		cy.get('body').type(META_KEY, { release: false }).type('s');
+		cy.get('body').type(META_KEY, { release: false }).type('s');
+		cy.get('body').type(META_KEY, { release: false }).type('s');
+		cy.wrap(null).then(() => expect(interceptCalledCount).to.eq(0));
+		WorkflowPage.actions.addNodeToCanvas(SCHEDULE_TRIGGER_NODE_NAME);
+		cy.get('body').type(META_KEY, { release: false }).type('s');
+		cy.wait('@saveWorkflow');
+		cy.wrap(null).then(() => expect(interceptCalledCount).to.eq(1));
+	})
 	it('should copy nodes', () => {
 		WorkflowPage.actions.addNodeToCanvas(SCHEDULE_TRIGGER_NODE_NAME);
 		WorkflowPage.actions.addNodeToCanvas(CODE_NODE_NAME);
@@ -110,64 +148,70 @@ describe('Workflow Actions', () => {
 	});
 
 	it('should update workflow settings', () => {
-		WorkflowPage.actions.visit();
-		// Open settings dialog
-		WorkflowPage.actions.saveWorkflowOnButtonClick();
-		WorkflowPage.getters.workflowMenu().should('be.visible');
-		WorkflowPage.getters.workflowMenu().click();
-		WorkflowPage.getters.workflowMenuItemSettings().should('be.visible');
-		WorkflowPage.getters.workflowMenuItemSettings().click();
-		// Change all settings
-		WorkflowPage.getters.workflowSettingsErrorWorkflowSelect().find('li').should('have.length', 7);
-		WorkflowPage.getters
-			.workflowSettingsErrorWorkflowSelect()
-			.find('li')
-			.last()
-			.click({ force: true });
-		WorkflowPage.getters.workflowSettingsTimezoneSelect().find('li').should('exist');
-		WorkflowPage.getters.workflowSettingsTimezoneSelect().find('li').eq(1).click({ force: true });
-		WorkflowPage.getters
-			.workflowSettingsSaveFiledExecutionsSelect()
-			.find('li')
-			.should('have.length', 3);
-		WorkflowPage.getters
-			.workflowSettingsSaveFiledExecutionsSelect()
-			.find('li')
-			.last()
-			.click({ force: true });
-		WorkflowPage.getters
-			.workflowSettingsSaveSuccessExecutionsSelect()
-			.find('li')
-			.should('have.length', 3);
-		WorkflowPage.getters
-			.workflowSettingsSaveSuccessExecutionsSelect()
-			.find('li')
-			.last()
-			.click({ force: true });
-		WorkflowPage.getters
-			.workflowSettingsSaveManualExecutionsSelect()
-			.find('li')
-			.should('have.length', 3);
-		WorkflowPage.getters
-			.workflowSettingsSaveManualExecutionsSelect()
-			.find('li')
-			.last()
-			.click({ force: true });
-		WorkflowPage.getters
-			.workflowSettingsSaveExecutionProgressSelect()
-			.find('li')
-			.should('have.length', 3);
-		WorkflowPage.getters
-			.workflowSettingsSaveExecutionProgressSelect()
-			.find('li')
-			.last()
-			.click({ force: true });
-		WorkflowPage.getters.workflowSettingsTimeoutWorkflowSwitch().click();
-		WorkflowPage.getters.workflowSettingsTimeoutForm().find('input').first().type('1');
-		// Save settings
-		WorkflowPage.getters.workflowSettingsSaveButton().click();
-		WorkflowPage.getters.workflowSettingsModal().should('not.exist');
-		WorkflowPage.getters.successToast().should('exist');
+		cy.visit(WorkflowPages.url);
+		WorkflowPages.getters.workflowCards().then((cards) => {
+			const totalWorkflows = cards.length;
+
+			WorkflowPage.actions.visit();
+			// Open settings dialog
+			WorkflowPage.actions.saveWorkflowOnButtonClick();
+			WorkflowPage.getters.workflowMenu().should('be.visible');
+			WorkflowPage.getters.workflowMenu().click();
+			WorkflowPage.getters.workflowMenuItemSettings().should('be.visible');
+			WorkflowPage.getters.workflowMenuItemSettings().click();
+			// Change all settings
+			// totalWorkflows + 1 (current workflow) + 1 (no workflow option)
+			WorkflowPage.getters.workflowSettingsErrorWorkflowSelect().find('li').should('have.length', totalWorkflows + 2);
+			WorkflowPage.getters
+				.workflowSettingsErrorWorkflowSelect()
+				.find('li')
+				.last()
+				.click({ force: true });
+			WorkflowPage.getters.workflowSettingsTimezoneSelect().find('li').should('exist');
+			WorkflowPage.getters.workflowSettingsTimezoneSelect().find('li').eq(1).click({ force: true });
+			WorkflowPage.getters
+				.workflowSettingsSaveFiledExecutionsSelect()
+				.find('li')
+				.should('have.length', 3);
+			WorkflowPage.getters
+				.workflowSettingsSaveFiledExecutionsSelect()
+				.find('li')
+				.last()
+				.click({ force: true });
+			WorkflowPage.getters
+				.workflowSettingsSaveSuccessExecutionsSelect()
+				.find('li')
+				.should('have.length', 3);
+			WorkflowPage.getters
+				.workflowSettingsSaveSuccessExecutionsSelect()
+				.find('li')
+				.last()
+				.click({ force: true });
+			WorkflowPage.getters
+				.workflowSettingsSaveManualExecutionsSelect()
+				.find('li')
+				.should('have.length', 3);
+			WorkflowPage.getters
+				.workflowSettingsSaveManualExecutionsSelect()
+				.find('li')
+				.last()
+				.click({ force: true });
+			WorkflowPage.getters
+				.workflowSettingsSaveExecutionProgressSelect()
+				.find('li')
+				.should('have.length', 3);
+			WorkflowPage.getters
+				.workflowSettingsSaveExecutionProgressSelect()
+				.find('li')
+				.last()
+				.click({ force: true });
+			WorkflowPage.getters.workflowSettingsTimeoutWorkflowSwitch().click();
+			WorkflowPage.getters.workflowSettingsTimeoutForm().find('input').first().type('1');
+			// Save settings
+			WorkflowPage.getters.workflowSettingsSaveButton().click();
+			WorkflowPage.getters.workflowSettingsModal().should('not.exist');
+			WorkflowPage.getters.successToast().should('exist');
+		})
 	});
 
 	it('should not be able to delete unsaved workflow', () => {

cypress/pages/workflow.ts

@@ -177,7 +177,7 @@ export class WorkflowPage extends BasePage {
 		},
 		saveWorkflowUsingKeyboardShortcut: () => {
 			cy.intercept('POST', '/rest/workflows').as('createWorkflow');
-			cy.get('body').type('{meta}', { release: false }).type('s');
+			cy.get('body').type(META_KEY, { release: false }).type('s');
 		},
 		deleteNode: (name: string) => {
 			this.getters.canvasNodeByName(name).first().click();

cypress/pages/workflows.ts

@@ -36,8 +36,10 @@ export class WorkflowsPage extends BasePage {
 			cy.visit(this.url);
 			this.getters.workflowCardActions(name).click();
 			this.getters.workflowDeleteButton().click();
+			cy.intercept('DELETE', '/rest/workflows/*').as('deleteWorkflow');
 
 			cy.get('button').contains('delete').click();
+			cy.wait('@deleteWorkflow');
 		},
 	};
 }

packages/cli/src/api/e2e.api.ts

@@ -12,6 +12,7 @@ import { Container } from 'typedi';
 import config from '@/config';
 import * as Db from '@/Db';
 import type { Role } from '@db/entities/Role';
+import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
 import { RoleRepository } from '@db/repositories';
 import { hashPassword } from '@/UserManagement/UserManagementHelper';
 import { eventBus } from '@/eventbus/MessageEventBus/MessageEventBus';
@@ -108,10 +109,18 @@ const resetLogStreaming = async () => {
 	}
 };
 
+const removeActiveWorkflows = async () => {
+	const workflowRunner = Container.get(ActiveWorkflowRunner);
+
+	workflowRunner.removeAllQueuedWorkflowActivations();
+	await workflowRunner.removeAll();
+};
+
 export const e2eController = Router();
 
 e2eController.post('/db/reset', async (req, res) => {
 	await resetLogStreaming();
+	await removeActiveWorkflows();
 	await truncateAll();
 	await setupUserManagement();
 

packages/editor-ui/src/components/ExpressionEdit.vue

@@ -45,7 +45,7 @@
 						<div class="expression-editor ph-no-capture">
 							<ExpressionEditorModalInput
 								:value="value"
-								:isReadOnly="isReadOnly"
+								:isReadOnly="isReadOnlyRoute"
 								:path="path"
 								@change="valueChanged"
 								@close="closeDialog"

packages/editor-ui/src/components/NodeCredentials.vue

@@ -15,7 +15,7 @@
 				color="text-dark"
 				data-test-id="credentials-label"
 			>
-				<div v-if="readonly || isReadOnly">
+				<div v-if="readonly || isReadOnlyRoute">
 					<n8n-input
 						:value="getSelectedName(credentialTypeDescription.name)"
 						disabled

packages/editor-ui/src/components/RunData.vue

@@ -7,7 +7,7 @@
 			:class="$style.pinnedDataCallout"
 		>
 			{{ $locale.baseText('runData.pindata.thisDataIsPinned') }}
-			<span class="ml-4xs" v-if="!isReadOnly">
+			<span class="ml-4xs" v-if="!isReadOnlyRoute">
 				<n8n-link
 					theme="secondary"
 					size="small"
@@ -59,7 +59,7 @@
 					data-test-id="ndv-run-data-display-mode"
 				/>
 				<n8n-icon-button
-					v-if="canPinData && !isReadOnly"
+					v-if="canPinData && !isReadOnlyRoute"
 					v-show="!editMode.enabled"
 					:title="$locale.baseText('runData.editOutput')"
 					:circle="false"
@@ -99,7 +99,9 @@
 						type="tertiary"
 						:active="hasPinData"
 						icon="thumbtack"
-						:disabled="editMode.enabled || (inputData.length === 0 && !hasPinData) || isReadOnly"
+						:disabled="
+							editMode.enabled || (inputData.length === 0 && !hasPinData) || isReadOnlyRoute
+						"
 						@click="onTogglePinData({ source: 'pin-icon-click' })"
 						data-test-id="ndv-pin-data"
 					/>
@@ -917,7 +919,7 @@ export default defineComponent({
 			if (
 				value &&
 				value.length > 0 &&
-				!this.isReadOnly &&
+				!this.isReadOnlyRoute &&
 				!localStorage.getItem(LOCAL_STORAGE_PIN_DATA_DISCOVERY_NDV_FLAG)
 			) {
 				this.pinDataDiscoveryComplete();

packages/editor-ui/src/components/RunDataJsonActions.vue

@@ -212,7 +212,7 @@ export default defineComponent({
 				copy_type: copyType,
 				workflow_id: this.workflowsStore.workflowId,
 				pane: this.paneType,
-				in_execution_log: this.isReadOnly,
+				in_execution_log: this.isReadOnlyRoute,
 			});
 
 			this.copyToClipboard(value);

packages/editor-ui/src/mixins/genericHelpers.ts

@@ -17,7 +17,7 @@ export const genericHelpers = defineComponent({
 		};
 	},
 	computed: {
-		isReadOnly(): boolean {
+		isReadOnlyRoute(): boolean {
 			return ![VIEWS.WORKFLOW, VIEWS.NEW_WORKFLOW, VIEWS.LOG_STREAMING_SETTINGS].includes(
 				this.$route.name as VIEWS,
 			);
@@ -50,7 +50,7 @@ export const genericHelpers = defineComponent({
 			return { date, time };
 		},
 		editAllowedCheck(): boolean {
-			if (this.isReadOnly) {
+			if (this.isReadOnlyRoute) {
 				this.showMessage({
 					// title: 'Workflow can not be changed!',
 					title: this.$locale.baseText('genericHelpers.showMessage.title'),

packages/editor-ui/src/mixins/workflowHelpers.ts

@@ -43,6 +43,7 @@ import type {
 
 import { externalHooks } from '@/mixins/externalHooks';
 import { nodeHelpers } from '@/mixins/nodeHelpers';
+import { genericHelpers } from '@/mixins/genericHelpers';
 import { useToast, useMessage } from '@/composables';
 
 import { isEqual } from 'lodash-es';
@@ -329,7 +330,7 @@ function executeData(
 }
 
 export const workflowHelpers = defineComponent({
-	mixins: [externalHooks, nodeHelpers],
+	mixins: [externalHooks, nodeHelpers, genericHelpers],
 	setup() {
 		return {
 			...useToast(),
@@ -699,6 +700,7 @@ export const workflowHelpers = defineComponent({
 			forceSave = false,
 		): Promise<boolean> {
 			const currentWorkflow = id || this.$route.params.name;
+			const isLoading = this.loadingService !== null;
 
 			if (!currentWorkflow || ['new', PLACEHOLDER_EMPTY_WORKFLOW_ID].includes(currentWorkflow)) {
 				return this.saveAsNewWorkflow({ name, tags }, redirect);
@@ -706,6 +708,9 @@ export const workflowHelpers = defineComponent({
 
 			// Workflow exists already so update it
 			try {
+				if (!forceSave && isLoading) {
+					return true;
+				}
 				this.uiStore.addActiveAction('workflowSaving');
 
 				const workflowDataRequest: IWorkflowDataUpdate = await this.getWorkflowDataToSave();

packages/editor-ui/src/views/NodeView.vue

@@ -54,7 +54,7 @@
 							@run="onNodeRun"
 							:key="`${nodeData.id}_node`"
 							:name="nodeData.name"
-							:isReadOnly="isReadOnly || readOnlyEnv"
+							:isReadOnly="isReadOnlyRoute || readOnlyEnv"
 							:instance="instance"
 							:isActive="!!activeNode && activeNode.name === nodeData.name"
 							:hideActions="pullConnActive"
@@ -76,7 +76,7 @@
 							@removeNode="(name) => removeNode(name, true)"
 							:key="`${nodeData.id}_sticky`"
 							:name="nodeData.name"
-							:isReadOnly="isReadOnly || readOnlyEnv"
+							:isReadOnly="isReadOnlyRoute || readOnlyEnv"
 							:instance="instance"
 							:isActive="!!activeNode && activeNode.name === nodeData.name"
 							:nodeViewScale="nodeViewScale"
@@ -87,7 +87,7 @@
 				</div>
 			</div>
 			<node-details-view
-				:readOnly="isReadOnly || readOnlyEnv"
+				:readOnly="isReadOnlyRoute || readOnlyEnv"
 				:renaming="renamingActive"
 				:isProductionExecutionPreview="isProductionExecutionPreview"
 				@valueChanged="valueChanged"
@@ -95,14 +95,14 @@
 				@saveKeyboardShortcut="onSaveKeyboardShortcut"
 			/>
 			<node-creation
-				v-if="!isReadOnly && !readOnlyEnv"
+				v-if="!isReadOnlyRoute && !readOnlyEnv"
 				:create-node-active="createNodeActive"
 				:node-view-scale="nodeViewScale"
 				@toggleNodeCreator="onToggleNodeCreator"
 				@addNode="onAddNode"
 			/>
 			<canvas-controls />
-			<div class="workflow-execute-wrapper" v-if="!isReadOnly">
+			<div class="workflow-execute-wrapper" v-if="!isReadOnlyRoute">
 				<span
 					@mouseenter="showTriggerMissingToltip(true)"
 					@mouseleave="showTriggerMissingToltip(false)"
@@ -149,7 +149,7 @@
 				/>
 
 				<n8n-icon-button
-					v-if="!isReadOnly && workflowExecution && !workflowRunning && !allTriggersDisabled"
+					v-if="!isReadOnlyRoute && workflowExecution && !workflowRunning && !allTriggersDisabled"
 					:title="$locale.baseText('nodeView.deletesTheCurrentExecutionData')"
 					icon="trash"
 					size="large"
@@ -961,7 +961,7 @@ export default defineComponent({
 				e.stopPropagation();
 				e.preventDefault();
 
-				if (this.isReadOnly) {
+				if (this.isReadOnlyRoute) {
 					return;
 				}
 
@@ -1015,13 +1015,13 @@ export default defineComponent({
 			} else if (e.key === 'Tab') {
 				this.onToggleNodeCreator({
 					source: NODE_CREATOR_OPEN_SOURCES.TAB,
-					createNodeActive: !this.createNodeActive && !this.isReadOnly,
+					createNodeActive: !this.createNodeActive && !this.isReadOnlyRoute,
 				});
 			} else if (e.key === this.controlKeyCode) {
 				this.ctrlKeyPressed = true;
 			} else if (e.key === ' ') {
 				this.moveCanvasKeyPressed = true;
-			} else if (e.key === 'F2' && !this.isReadOnly) {
+			} else if (e.key === 'F2' && !this.isReadOnlyRoute) {
 				const lastSelectedNode = this.lastSelectedNode;
 				if (lastSelectedNode !== null && lastSelectedNode.type !== STICKY_NODE_TYPE) {
 					void this.callDebounced(
@@ -1067,7 +1067,7 @@ export default defineComponent({
 				const lastSelectedNode = this.lastSelectedNode;
 
 				if (lastSelectedNode !== null) {
-					if (lastSelectedNode.type === STICKY_NODE_TYPE && this.isReadOnly) {
+					if (lastSelectedNode.type === STICKY_NODE_TYPE && this.isReadOnlyRoute) {
 						return;
 					}
 					this.ndvStore.activeNodeName = lastSelectedNode.name;
@@ -1307,7 +1307,7 @@ export default defineComponent({
 		},
 
 		cutSelectedNodes() {
-			const deleteCopiedNodes = !this.isReadOnly;
+			const deleteCopiedNodes = !this.isReadOnlyRoute;
 			this.copySelectedNodes(deleteCopiedNodes);
 			if (deleteCopiedNodes) {
 				this.deleteSelectedNodes();
@@ -2162,7 +2162,7 @@ export default defineComponent({
 				if (!this.suspendRecordingDetachedConnections) {
 					this.historyStore.pushCommandToUndo(new AddConnectionCommand(connectionData));
 				}
-				if (!this.isReadOnly) {
+				if (!this.isReadOnlyRoute) {
 					NodeViewUtils.addConnectionActionsOverlay(
 						info.connection,
 						() => {
@@ -2211,7 +2211,7 @@ export default defineComponent({
 				}
 
 				if (
-					this.isReadOnly ||
+					this.isReadOnlyRoute ||
 					this.readOnlyEnv ||
 					this.enterTimer ||
 					!connection ||
@@ -2242,7 +2242,7 @@ export default defineComponent({
 				}
 
 				if (
-					this.isReadOnly ||
+					this.isReadOnlyRoute ||
 					this.readOnlyEnv ||
 					!connection ||
 					this.activeConnection?.id !== connection.id
@@ -2609,7 +2609,7 @@ export default defineComponent({
 			// Create connections in DOM
 			this.instance?.connect({
 				uuids: uuid,
-				detachable: !this.isReadOnly,
+				detachable: !this.isReadOnlyRoute,
 			});
 
 			setTimeout(() => {