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

feat(editor): Execute sub-workflow UX and copy updates (no-changelog) #12834

Merged
merged 16 commits into from
Jan 28, 2025
Merged

feat(editor): Execute sub-workflow UX and copy updates (no-changelog) #12834

merged 16 commits into from
Jan 28, 2025

Conversation

MiloradFilipovic
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic commented Jan 24, 2025 •
edited
Loading

Summary

  • Adding empty fields message to resource mapper
  • Adding missing workflow data (name and id) to LoadWorkflowNodeContext
  • For detailed requirements, check Linear ticket description

Related Linear tickets, Github issues, and Community forum posts

Closes ADO-3103

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@MiloradFilipovic MiloradFilipovic self-assigned this Jan 24, 2025
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team node/improvement New feature or request ui Enhancement in /editor-ui or /design-system labels Jan 24, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.01796% with 30 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ls/ToolWorkflow/v2/methods/localResourceMapping.ts 0.00% 11 Missing ⚠️
...ow/ExecuteWorkflow/methods/localResourceMapping.ts 18.18% 9 Missing ⚠️
.../workflowInputsResourceMapping/GenericFunctions.ts 11.11% 8 Missing ⚠️
...nodes/tools/ToolWorkflow/v2/ToolWorkflowV2.node.ts 0.00% 1 Missing ⚠️
...chain/nodes/tools/ToolWorkflow/v2/methods/index.ts 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@MiloradFilipovic MiloradFilipovic changed the title feat(editor): Updating copy around execute workflow nodes (no-changelog) feat(editor): Execute sub-workflow UX and copy updates (no-changelog) Jan 27, 2025
CharlieKolb
CharlieKolb approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@CharlieKolb CharlieKolb left a comment

Choose a reason for hiding this comment

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

Love the resourceMapper tests 🙌 Happy to do the one copy change from notion as a follow up if you want to merge this first

packages/nodes-base/nodes/ExecuteWorkflow/ExecuteWorkflow/ExecuteWorkflow.node.ts Show resolved Hide resolved
packages/nodes-base/utils/workflowInputsResourceMapping/GenericFunctions.ts Outdated Show resolved Hide resolved
@cypress Cypress
Copy link

cypress bot commented Jan 28, 2025 •
edited
Loading

n8n    Run #8980

Run Properties:  status check failed Failed #8980  •  git commit 10ac7e370a: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 MiloradFilipovic 🗃️ e2e/*
Project n8n
Branch Review ADO-3103-workflow-inputs-copy-updates
Run status status check failed Failed #8980
Run duration 04m 25s
Commit git commit 10ac7e370a: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 MiloradFilipovic 🗃️ e2e/*
Committer Milorad FIlipović
View all properties for this run ↗︎
Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 1
Tests that did not run due to a failure in a mocha hook  Skipped 7
Tests that passed  Passing 225
View all changes introduced in this branch ↗︎

Tests for review

Failed  45-workflow-selector-parameter.cy.ts • 1 failed test

View Output Video

Test Artifacts
Workflow Selector Parameter > should render sub-workflows list Test Replay Screenshots Video
Failed  48-subworkflow-inputs.cy.ts • 1 failed test

View Output Video

Test Artifacts
Sub-workflow creation and typed usage > works with type-checked values Test Replay Screenshots Video
Failed  20-workflow-executions.cy.ts • 0 failed tests

View Output

Test Artifacts
Failed  11-inline-expression-editor.cy.ts • 0 failed tests

View Output

Test Artifacts
Failed  6-code-node.cy.ts • 0 failed tests

View Output

Test Artifacts

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

Flakiness  e2e/45-ai-assistant.cy.ts • 1 flaky test

View Output Video

Test Artifacts
AI Assistant::enabled > should resize assistant chat down Test Replay Screenshots Video

@github-actions GitHub Actions
Copy link
Contributor

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

CharlieKolb
CharlieKolb previously approved these changes Jan 28, 2025
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

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

CharlieKolb
CharlieKolb approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@CharlieKolb CharlieKolb left a comment

Choose a reason for hiding this comment

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

Nice refactoring!

packages/nodes-base/nodes/ExecuteWorkflow/ExecuteWorkflow/methods/localResourceMapping.ts
@@ -0,0 +1,25 @@
import type { ILocalLoadOptionsFunctions, ResourceMapperFields } from 'n8n-workflow';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go into the GenericFunctions file we ended up putting all the shared logic? This is straight up duplication otherwise I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, the second sentence in the emptyFieldNotice is different, I see :o I'll leave this up to you then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it ended up being 50-50 reusable so I decided to leave the two implementations since it's in the spirit of this refactor (leaving the message definition to each node).

@MiloradFilipovic MiloradFilipovic merged commit de49c23 into master Jan 28, 2025
98 of 101 checks passed
@MiloradFilipovic MiloradFilipovic deleted the ADO-3103-workflow-inputs-copy-updates branch January 28, 2025 10:33
@github-actions GitHub Actions
Copy link
Contributor

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

@CharlieKolb CharlieKolb mentioned this pull request Jan 28, 2025
Merged
4 tasks
@janober
Copy link
Member

janober commented Jan 29, 2025

Got released with [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharlieKolb CharlieKolb CharlieKolb approved these changes

Assignees

@MiloradFilipovic MiloradFilipovic

Labels
n8n team Authored by the n8n team node/improvement New feature or request Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MiloradFilipovic @janober @CharlieKolb