Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(editor): Enable pinning main output with error and always allow unpinning #11452

Merged
merged 10 commits into from
Nov 7, 2024
Merged

fix(editor): Enable pinning main output with error and always allow unpinning #11452

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
306d13d
enable pinning with error output and always unpin
CharlieKolb Oct 29, 2024
dd354ff
Add tests
CharlieKolb Oct 31, 2024
abee2f7
Merge remote-tracking branch 'origin/master' into ADO-2343/community-…
CharlieKolb Nov 4, 2024
aae7ccc
Update packages/editor-ui/src/composables/usePinnedData.ts
CharlieKolb Nov 4, 2024
1cb39c7
PR Feedback
CharlieKolb Nov 5, 2024
4f95885
throw error instead
CharlieKolb Nov 5, 2024
a63af7f
self review
CharlieKolb Nov 5, 2024
ec5b581
Merge remote-tracking branch 'origin/master' into ADO-2343/community-…
CharlieKolb Nov 5, 2024
f03b325
more tests
CharlieKolb Nov 5, 2024
4db633a
PR Feedback
CharlieKolb Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions packages/editor-ui/src/components/RunData.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,11 @@ export default defineComponent({
return false;
}

if (this.outputIndex !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more safe here to check the type instead of the output index? Because, for example, if the UX changes and suddenly error output is on top, this would be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this assumption is a bit wacky. It's rooted in the explicit effort elsewhere in the code to add the error path after node specific outputs, but it might not hold long-term.

So it turns out we actually don't need this check here, as this line

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

Line 263 in 73b0a80

if (!this.rawInputData.length && !this.pinnedData.hasData.value) {
has !this.rawInputData.length, which causes us to return early for errors.
I don't fully understand this, does this happen to align, or can we rely on this semantically?

I can still add the check, though I'd move it to canPinNode by adding an index parameter if that works for you.

// Only allow pinning of the main output
return false;
}

const canPinNode = usePinnedData(this.node).canPinNode(false);

return (
Expand Down Expand Up @@ -1213,9 +1218,7 @@ export default defineComponent({
<template>
<div :class="['run-data', $style.container]" @mouseover="activatePane">
<n8n-callout
v-if="
canPinData && pinnedData.hasData.value && !editMode.enabled && !isProductionExecutionPreview
"
v-if="pinnedData.hasData.value && !editMode.enabled && !isProductionExecutionPreview"
theme="secondary"
icon="thumbtack"
:class="$style.pinnedDataCallout"
Expand Down
77 changes: 76 additions & 1 deletion packages/editor-ui/src/composables/usePinnedData.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ import { setActivePinia, createPinia } from 'pinia';
import { ref } from 'vue';
import { usePinnedData } from '@/composables/usePinnedData';
import type { INodeUi } from '@/Interface';
import { MAX_PINNED_DATA_SIZE } from '@/constants';
import { HTTP_REQUEST_NODE_TYPE, IF_NODE_TYPE, MAX_PINNED_DATA_SIZE } from '@/constants';
import { useWorkflowsStore } from '@/stores/workflows.store';
import { useTelemetry } from '@/composables/useTelemetry';
import { NodeConnectionType, type INodeTypeDescription } from 'n8n-workflow';

vi.mock('@/composables/useToast', () => ({ useToast: vi.fn(() => ({ showError: vi.fn() })) }));
vi.mock('@/composables/useI18n', () => ({
Expand All @@ -17,6 +18,13 @@ vi.mock('@/composables/useExternalHooks', () => ({
})),
}));

const getNodeType = vi.fn();
vi.mock('@/stores/nodeTypes.store', () => ({
useNodeTypesStore: vi.fn(() => ({
getNodeType,
})),
}));

describe('usePinnedData', () => {
beforeEach(() => {
setActivePinia(createPinia());
Expand Down Expand Up @@ -133,4 +141,71 @@ describe('usePinnedData', () => {
expect(spy).toHaveBeenCalled();
});
});

describe('canPinData()', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding Unit tests. Would it also be useful to add some e2e tests as well for this bug?

Copy link
Contributor Author

@CharlieKolb CharlieKolb Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially if we now drop the condition in the vue file or move it to the composable this unit test should now have us fully covered.

I'm a bit unclear still on when to add e2e tests, I understand we're looking to move away from them "by default" and that there is a real cost associated?

Copy link
Contributor

@mutdmour mutdmour Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no rule. My general rule is if the bug depends on the interaction of multiple modules, then better to have an e2e test for it.

afterEach(() => {
vi.clearAllMocks();
});

it('allows pin on single output', async () => {
const node = ref({
name: 'single output node',
typeVersion: 1,
type: HTTP_REQUEST_NODE_TYPE,

parameters: {},
onError: 'stopWorkflow',
} as INodeUi);
getNodeType.mockReturnValue(makeNodeType([NodeConnectionType.Main], HTTP_REQUEST_NODE_TYPE));

const { canPinNode } = usePinnedData(node);

expect(canPinNode()).toBe(true);
});

it('allows pin on one main and one error output', async () => {
const node = ref({
name: 'single output node',
typeVersion: 1,
type: HTTP_REQUEST_NODE_TYPE,
parameters: {},
onError: 'continueErrorOutput',
} as INodeUi);
getNodeType.mockReturnValue(makeNodeType([NodeConnectionType.Main], HTTP_REQUEST_NODE_TYPE));

const { canPinNode } = usePinnedData(node);

expect(canPinNode()).toBe(true);
});

it('does not allow pin on two main outputs', async () => {
const node = ref({
name: 'single output node',
typeVersion: 1,
type: IF_NODE_TYPE,
parameters: {},
onError: 'stopWorkflow',
} as INodeUi);
getNodeType.mockReturnValue(
makeNodeType([NodeConnectionType.Main, NodeConnectionType.Main], IF_NODE_TYPE),
);

const { canPinNode } = usePinnedData(node);

expect(canPinNode()).toBe(false);
});
});
});

const makeNodeType = (outputs: NodeConnectionType[], name: string) =>
({
displayName: name,
name,
version: [1],
inputs: [],
outputs,
properties: [],
defaults: { color: '', name: '' },
group: [],
description: '',
}) as INodeTypeDescription;
16 changes: 9 additions & 7 deletions packages/editor-ui/src/composables/usePinnedData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,16 @@ export function usePinnedData(
if (!nodeType || (checkDataEmpty && dataToPin.length === 0)) return false;

const workflow = workflowsStore.getCurrentWorkflow();
const outputs = NodeHelpers.getNodeOutputs(workflow, targetNode, nodeType);
const mainOutputs = outputs.filter((output) =>
typeof output === 'string'
? output === NodeConnectionType.Main
: output.type === NodeConnectionType.Main,
);
const mainOutputs = NodeHelpers.getNodeOutputs(workflow, targetNode, nodeType)
.map((output) => (typeof output === 'string' ? { type: output } : output))
.filter((output) => output.type === NodeConnectionType.Main);
CharlieKolb marked this conversation as resolved.
Show resolved Hide resolved

// Outputs are pinnable if there is exactly one main output and optionally one error output
const pinnableMainOutputs =
mainOutputs.length === 1 ||
(mainOutputs.length === 2 && mainOutputs[1]?.category === 'error');

return mainOutputs.length === 1 && !PIN_DATA_NODE_TYPES_DENYLIST.includes(targetNode.type);
return pinnableMainOutputs && !PIN_DATA_NODE_TYPES_DENYLIST.includes(targetNode.type);
}

function isValidJSON(data: string): boolean {
Expand Down