fix(core): Comply with custom default for workflow saving settings (#…

…7634) https://linear.app/n8n/issue/PAY-982
n8n-io · Nov 7, 2023 · 48c068f · 48c068f
1 parent ac87701
commit 48c068f
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 29 deletions.
diff --git a/packages/cli/src/executionLifecycleHooks/toSaveSettings.ts b/packages/cli/src/executionLifecycleHooks/toSaveSettings.ts
@@ -1,20 +1,24 @@
 import config from '@/config';
 import type { IWorkflowSettings } from 'n8n-workflow';
 
-const DEFAULTS = {
-	ERROR: config.getEnv('executions.saveDataOnError'),
-	SUCCESS: config.getEnv('executions.saveDataOnSuccess'),
-	MANUAL: config.getEnv('executions.saveDataManualExecutions'),
-};
-
 /**
  * Return whether a workflow execution is configured to be saved or not,
  * for error executions, success executions, and manual executions.
  */
 export function toSaveSettings(workflowSettings: IWorkflowSettings = {}) {
+	const DEFAULTS = {
+		ERROR: config.getEnv('executions.saveDataOnError'),
+		SUCCESS: config.getEnv('executions.saveDataOnSuccess'),
+		MANUAL: config.getEnv('executions.saveDataManualExecutions'),
+	};
+
 	return {
-		error: workflowSettings.saveDataErrorExecution !== 'none' ?? DEFAULTS.ERROR !== 'none',
-		success: workflowSettings.saveDataSuccessExecution !== 'none' ?? DEFAULTS.SUCCESS !== 'none',
+		error: workflowSettings.saveDataErrorExecution
+			? workflowSettings.saveDataErrorExecution !== 'none'
+			: DEFAULTS.ERROR !== 'none',
+		success: workflowSettings.saveDataSuccessExecution
+			? workflowSettings.saveDataSuccessExecution !== 'none'
+			: DEFAULTS.SUCCESS !== 'none',
 		manual: workflowSettings?.saveManualExecutions ?? DEFAULTS.MANUAL,
 	};
 }
diff --git a/packages/cli/test/unit/execution.lifecycle.test.ts b/packages/cli/test/unit/execution.lifecycle.test.ts
@@ -145,43 +145,96 @@ for (const mode of ['filesystem-v2', 's3'] as const) {
 describe('toSaveSettings()', () => {
 	afterEach(() => {
 		jest.restoreAllMocks();
+		config.load(config.default);
 	});
 
-	it('should set `error` based on workflow settings', () => {
-		const saveSettings = toSaveSettings({ saveDataErrorExecution: 'all' });
+	describe('when setting `error`', () => {
+		it('should favor workflow settings over defaults', () => {
+			config.set('executions.saveDataOnError', 'none');
 
-		expect(saveSettings.error).toBe(true);
+			const saveSettings = toSaveSettings({ saveDataErrorExecution: 'all' });
 
-		const _saveSettings = toSaveSettings({ saveDataErrorExecution: 'none' });
+			expect(saveSettings.error).toBe(true);
 
-		expect(_saveSettings.error).toBe(false);
-	});
+			config.set('executions.saveDataOnError', 'all');
+
+			const _saveSettings = toSaveSettings({ saveDataErrorExecution: 'none' });
+
+			expect(_saveSettings.error).toBe(false);
+		});
 
-	it('should set `success` based on workflow settings', () => {
-		const saveSettings = toSaveSettings({ saveDataSuccessExecution: 'all' });
+		it('should fall back to default if no workflow setting', () => {
+			config.set('executions.saveDataOnError', 'all');
 
-		expect(saveSettings.success).toBe(true);
+			const saveSettings = toSaveSettings();
 
-		const _saveSettings = toSaveSettings({ saveDataSuccessExecution: 'none' });
+			expect(saveSettings.error).toBe(true);
 
-		expect(_saveSettings.success).toBe(false);
+			config.set('executions.saveDataOnError', 'none');
+
+			const _saveSettings = toSaveSettings();
+
+			expect(_saveSettings.error).toBe(false);
+		});
 	});
 
-	it('should set `manual` based on workflow settings', () => {
-		const saveSettings = toSaveSettings({ saveManualExecutions: true });
+	describe('when setting `success`', () => {
+		it('should favor workflow settings over defaults', () => {
+			config.set('executions.saveDataOnSuccess', 'none');
+
+			const saveSettings = toSaveSettings({ saveDataSuccessExecution: 'all' });
+
+			expect(saveSettings.success).toBe(true);
+
+			config.set('executions.saveDataOnSuccess', 'all');
+
+			const _saveSettings = toSaveSettings({ saveDataSuccessExecution: 'none' });
+
+			expect(_saveSettings.success).toBe(false);
+		});
+
+		it('should fall back to default if no workflow setting', () => {
+			config.set('executions.saveDataOnSuccess', 'all');
+
+			const saveSettings = toSaveSettings();
 
-		expect(saveSettings.manual).toBe(true);
+			expect(saveSettings.success).toBe(true);
 
-		const _saveSettings = toSaveSettings({ saveManualExecutions: false });
+			config.set('executions.saveDataOnSuccess', 'none');
 
-		expect(_saveSettings.manual).toBe(false);
+			const _saveSettings = toSaveSettings();
+
+			expect(_saveSettings.success).toBe(false);
+		});
 	});
 
-	it('should return defaults if no workflow settings', async () => {
-		const saveSettings = toSaveSettings();
+	describe('when setting `manual`', () => {
+		it('should favor workflow settings over defaults', () => {
+			config.set('executions.saveDataManualExecutions', false);
+
+			const saveSettings = toSaveSettings({ saveManualExecutions: true });
+
+			expect(saveSettings.manual).toBe(true);
+
+			config.set('executions.saveDataManualExecutions', true);
+
+			const _saveSettings = toSaveSettings({ saveManualExecutions: false });
+
+			expect(_saveSettings.manual).toBe(false);
+		});
+
+		it('should fall back to default if no workflow setting', () => {
+			config.set('executions.saveDataManualExecutions', true);
+
+			const saveSettings = toSaveSettings();
+
+			expect(saveSettings.manual).toBe(true);
 
-		expect(saveSettings.error).toBe(true);
-		expect(saveSettings.success).toBe(true);
-		expect(saveSettings.manual).toBe(true);
+			config.set('executions.saveDataManualExecutions', false);
+
+			const _saveSettings = toSaveSettings();
+
+			expect(_saveSettings.manual).toBe(false);
+		});
 	});
 });