fix(editor): Enable pinning main output with error and always allow unpinning #11452
Conversation
n8n-assistant
bot
added
n8n team
CharlieKolb
changed the title
…issue-cant-unpin-output-in-http-module
| @@ -288,6 +288,11 @@ export default defineComponent({ | |||
| return false; | |||
| } | |||
|
|
|||
| if (this.outputIndex !== 0) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.
| @@ -133,4 +141,71 @@ describe('usePinnedData', () => { | |||
| expect(spy).toHaveBeenCalled(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('canPinData()', () => { | |||
There was a problem hiding this comment.
Thank you for adding Unit tests. Would it also be useful to add some e2e tests as well for this bug?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Co-authored-by: Mutasem Aldmour <[email protected]>
…issue-cant-unpin-output-in-http-module
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
| const output = outputs[outputIndex]; | ||
|
|
||
| if (outputs[outputIndex] === undefined) | ||
| throw new Error(`Out of bounds index ${outputIndex} provided to canPinNode`); |
There was a problem hiding this comment.
This could brick the UI. So might be better to have a sensible default rather than break UI.
For example adding a node with no outputs.
There was a problem hiding this comment.
Oh nice catch, I had assumed we always have an output.
|
✅ All Cypress E2E specs passed |
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
ADO-2343/community-issue-cant-unpin-output-in-http-module
|
| Run status |
|
| Run duration | 04m 28s |
| Commit |
|
| Committer | Charlie Kolb |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
461
|
| View all changes introduced in this branch ↗︎ | |
CharlieKolb
deleted the
ADO-2343/community-issue-cant-unpin-output-in-http-module
branch
…npinning (#11452) Co-authored-by: Mutasem Aldmour <[email protected]>
|
Got released with |
Summary
Allow pinning the main output even if we have an error branch from the
OnErrorsetting.Also always allow unpinning existing pinned data.
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-2343/community-issue-cant-unpin-output-in-http-module
#10066
#10240
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)