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(core): Adjust starter node priority for manual executions with pinned activators #8305

Closed
wants to merge 13 commits into from
Closed

fix(core): Adjust starter node priority for manual executions with pinned activators #8305

wants to merge 13 commits into from

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Jan 11, 2024 •
edited
Loading

Story: https://linear.app/n8n/issue/PAY-1099

This PR adjusts starter node priority for manual executions with pinned activators, prioritizing n8n-nodes-base.webhook over other activators. Also, this PR consolidates the search for starter nodes for manual executions and subworkflow executions.

Out of scope - these legacy methods are also due for consolidation:

  • WorkflowHelpers.getExecutionStartNode()
  • Workflow.getStartNode()
  • Workflow.__getStartNode()

@ivov ivov marked this pull request as draft January 11, 2024 14:06
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jan 11, 2024
@ivov ivov changed the title fix(core): Consolidate starting node search and fix starting node priority fix(core): Adjust starter node priority for manual executions with pinned activators Jan 12, 2024
ivov
ivov commented Jan 12, 2024
View reviewed changes
packages/workflow/src/Workflow.ts

// partial manual execution

const [firstStartNodeName] = startNodeNames;
Copy link
Contributor Author

@ivov ivov Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This preserves existing behavior - if the partial manual execution has 2+ start nodes, we search only the zeroth start node's parents for a pinned activator. If we had 2+ start nodes without a common ancestor and so if we end up finding multiple pinned activators, we'd still need to return one to comply with existing usage, so I assume that picking the zeroth starter node here is correct, even though arbitrary.

@ivov ivov marked this pull request as ready for review January 12, 2024 14:23
krynble
krynble reviewed Jan 16, 2024
View reviewed changes
packages/workflow/src/Workflow.ts Outdated Show resolved Hide resolved
packages/workflow/src/Workflow.ts Outdated Show resolved Hide resolved
packages/workflow/src/Workflow.ts Outdated Show resolved Hide resolved
@ivov ivov requested a review from krynble January 16, 2024 11:35
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@cypress Cypress
Copy link

cypress bot commented Jan 16, 2024 •
edited
Loading

8 failed tests on run #3809 ↗︎

8 243 5 0 Flakiness 0

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n Commit: ad32ec38a1
Status: Failed Duration: 06:23 💡
Started: Jan 17, 2024 4:22 PM Ended: Jan 17, 2024 4:29 PM
Failed  5-ndv.cy.ts • 4 failed tests

View Output Video

Test Artifacts
NDV > should properly show node execution indicator Screenshots Video
NDV > test output schema view > should switch to output schema view and validate it Screenshots Video
NDV > test output schema view > should collapse and expand nested schema object Screenshots Video
NDV > test output schema view > should not display pagination for schema Screenshots Video
Failed  13-pinning.cy.ts • 1 failed test

View Output Video

Test Artifacts
Data pinning > Should be able to reference paired items in a node located before pinned data Test Replay Screenshots Video
Failed  12-canvas-actions.cy.ts • 1 failed test

View Output Video

Test Artifacts
Canvas Actions > should execute node Test Replay Screenshots Video
Failed  6-code-node.cy.ts • 1 failed test

View Output Video

Test Artifacts
Code node > Code editor > should execute the placeholder successfully in both modes Test Replay Screenshots Video
Failed  24-ndv-paired-item.cy.ts • 1 failed test

View Output Video

Test Artifacts
NDV > can resolve expression with paired item in multi-input node Test Replay Screenshots Video

The first 5 failed specs are shown, see all 29 specs in Cypress Cloud.

Review all test suite changes for PR #8305 ↗︎

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

3 similar comments
@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@netroy netroy mentioned this pull request Jan 18, 2024
Merged
@ivov
Copy link
Contributor Author

ivov commented Jan 18, 2024

Closing in favor of #8386

@ivov ivov closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krynble krynble krynble approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivov @krynble