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): Add template Id to workflow metadata #8088

Merged
merged 5 commits into from
Dec 22, 2023
Merged

feat(editor): Add template Id to workflow metadata #8088

merged 5 commits into from
Dec 22, 2023
+48 −5

Conversation

MiloradFilipovic
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic commented Dec 19, 2023 •
edited by mutdmour
Loading

Summary

Adding a link between the workflow and the template it originated from by saving templateId in the workflow metadata

Related tickets and issues

ADO-1537

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@MiloradFilipovic MiloradFilipovic self-assigned this Dec 19, 2023
@cypress Cypress
Copy link

cypress bot commented Dec 19, 2023 •
edited
Loading

3 flaky tests on run #3481 ↗︎

0 316 5 0 Flakiness 3

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 MiloradFilipovic 🗃️ e2e/*
Project: n8n Commit: b4031e8b47
Status: Passed Duration: 07:18 💡
Started: Dec 22, 2023 1:17 PM Ended: Dec 22, 2023 1:24 PM
Flakiness  24-ndv-paired-item.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > resolves expression with default item when input node is not parent, while still pairing items Screenshots Video
Flakiness  19-execution.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Execution > should test manual workflow stop Screenshots Video
Flakiness  29-templates.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Templates > should save template id with the workflow Screenshots Video

Review all test suite changes for PR #8088 ↗︎

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 19, 2023
mutdmour
mutdmour reviewed Dec 20, 2023
View reviewed changes
cypress/e2e/29-templates.cy.ts Outdated
workflowPage.actions.saveWorkflowOnButtonClick();

cy.wait('@saveWorkflow').then((interception) => {
expect(interception.request.body.meta.templateId).to.equal(templateId);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really a good e2e test? seems to be more of an integration test. it's too coupled to the api request and does not tell you it was stored or used correctly.. Could you think of a better approach, even though it does not seem we are using this yet.. maybe check the json of loaded workflow or when copying/pasting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point on the API coupling. The problem with meta is that it has to rely for something under the hood (like you mentioned). Goal here is to test if template user chooses is correctly saved, but I guess we can check that also by re-opening the workflow, will try something like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

ya this might be one of those scenarios where a unit test is better.. but nice solution here..

packages/editor-ui/src/Interface.ts
@@ -272,6 +271,8 @@ export interface INewWorkflowData {

export interface WorkflowMetadata {
onboardingId?: string;
templateId?: string;
instanceId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

are we adding instance id now to the metadata? we intentionally avoided adding that before when saving.. only to be added when sharing..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not adding instanceId on any new places in this PR, I moved it here so we can re-use this type in IWorkflowToShare and clean up the typing around that a bit more.

@MiloradFilipovic
Merge branch 'master' into ADO-1537-save-templateId-with-workflow
c5747ed 
* master: (22 commits)
  fix(editor): Make keyboard shortcuts more strict; don't accept extra Ctrl/Alt/Shift keys (#8024)
  fix(core):  Downgrade Rudderstack SDK (no-changelog) (#8107)
  fix(editor): Move versions check to init function and refactor store (no-changelog) (#8067)
  refactor(editor): Add telemetry for SSO/SAML (no-changelog) (#8102)
  fix(editor): Ensure execution data overrides pinned data when copying in executions view (#8009)
  fix(editor): Fix copy/paste issue when switch node is in workflow (#8103)
  feat(editor): Upgrade frontend tooling to address a few vulnerabilities (#8100)
  feat(editor): De-duplicate frontend devDependencies (no-changelog) (#8094)
  refactor(core): Improve test-webhooks (no-changelog) (#8069)
  refactor: Add telemetry for RBAC (no-changelog) (#8056)
  feat(core): Upgrade Rudderstack SDK (no-changelog) (#8090)
  fix: Upgrade axios to address CVE-2023-45857 (#7713)
  fix(core): Do not display error when stopping jobless execution in queue mode (#8007)
  feat(editor): Gracefully ignore invalid payloads in postMessage handler (#8096)
  feat(editor): Add lead enrichment suggestions to workflow list (#8042)
  refactor(Discord Node): Stop reporting to Sentry inaccessible guild error (no-changelog) (#8095)
  feat: Add opt-in enterprise license trial checkbox (no-changelog) (#7826)
  ci: Remove unnecessary async/await, enable await-thenable linting rule (no-changelog) (#8076)
  refactor(editor): Add telemetry for log streaming (no-changelog) (#8075)
  fix(core): Use relative imports for dynamic imports in SecurityAuditService (#8086)
  ...
@MiloradFilipovic
⚡ Using clipboard instead of API to check template ID
b4031e8
@github-actions GitHub Actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@MiloradFilipovic MiloradFilipovic merged commit 517b050 into master Dec 22, 2023
36 checks passed
@MiloradFilipovic MiloradFilipovic deleted the ADO-1537-save-templateId-with-workflow branch December 22, 2023 14:07
jamesfdavis added a commit to jamesfdavis/n8n that referenced this pull request Dec 24, 2023
@jamesfdavis
Merge remote-tracking branch 'upstream/master'
4575e81 
* upstream/master: (32 commits)
  refactor(core): Move `typeorm` operators from `PruningService` to `ExecutionRepository` (no-changelog) (n8n-io#8145)
  refactor(core): Move more `typeorm` operators to repositories (no-changelog) (n8n-io#8143)
  refactor(core): Delete unused code, and fix typings in tests (no-changelog) (n8n-io#8142)
  docs: Improve documentation for the toTitleCase function (no-changelog) (n8n-io#8140)
  refactor(core): Move all base URLs to UrlService (no-changelog) (n8n-io#8141)
  feat(editor): Add template Id to workflow metadata (n8n-io#8088)
  fix(editor): Avoid sanitizing output to search node data (n8n-io#8126)
  fix(editor): Prevent browser zoom when scrolling inside sticky edit mode (n8n-io#8116)
  refactor(core): Move some `typeorm` operators to repositories (no-changelog) (n8n-io#8139)
  feat(editor): Add node execution status indicator to output panel (n8n-io#8124)
  feat(core): Unify application components shutdown (n8n-io#8097)
  refactor(core): Upgrade more dependencies to remove axios 0.x (no-changelog) (n8n-io#8105)
  refactor(core): Move active workflows endpoints to a decorated controller class (no-changelog) (n8n-io#8101)
  fix(editor): Prevent canvas undo/redo when NDV is open (n8n-io#8118)
  fix(editor): Fix operation change failing in certain conditions (n8n-io#8114)
  fix(Asana Node): Omit body from GET, HEAD, and DELETE requests (n8n-io#8057)
  fix(Redis Trigger Node): Activating a workflow with a Redis trigger fails (n8n-io#8129)
  refactor(Peekalink Node): Stricter typing for Peekalink api call + Tests (no-changelog) (n8n-io#8125)
  fix(core): Remove circular dependency in WorkflowService and ActiveWorkflowRunner (n8n-io#8128)
  fix: Show public API upgrade CTA when feature is not enabled (n8n-io#8109)
  ...
This was referenced Dec 27, 2023
Closed
Merged
ivov added a commit that referenced this pull request Jan 3, 2024
@github-actions @ivov
🚀 Release 1.23.0 (#8205)
0535035 
#
[1.23.0](https://github.com/n8n-io/n8n/compare/n8n@1.22.0...n8n@1.23.0)
(2024-01-03)


### Bug Fixes

* **Asana Node:** Omit body from GET, HEAD, and DELETE requests
([#8057](#8057))
([15ffd4f](15ffd4f))
* **core:** Better input validation for the changeRole endpoint
([#8189](#8189))
([cfe9525](cfe9525))
* **core:** Fix issue that pinnedData is not used with Test-Webhooks
([#8123](#8123))
([fa8bd8b](fa8bd8b))
* **core:** Handle empty executions table in pruning in migrations
([#8121](#8121))
([ffaa30d](ffaa30d))
* **core:** Remove circular dependency in WorkflowService and
ActiveWorkflowRunner
([#8128](#8128))
([21788d9](21788d9))
* **core:** Use pinned data only for manual mode
([#8164](#8164))
([ea7e76f](ea7e76f))
* **Discord Node:** Remove unnecessary requirement on parameters
([#8060](#8060))
([ef3a577](ef3a577))
* **editor:** Avoid sanitizing output to search node data
([#8126](#8126))
([c83d9f4](c83d9f4))
* **editor:** Enable explicit undo keyboard shortcut across all code
editors ([#8178](#8178))
([cf7f668](cf7f668))
* **editor:** Fix operation change failing in certain conditions
([#8114](#8114))
([711fa2b](711fa2b))
* **editor:** Fix templates view layout
([#8196](#8196))
([d01e42a](d01e42a))
* **editor:** Fix UI urls when hosted behind a path prefix
([#8198](#8198))
([5c078f1](5c078f1))
* **editor:** Prevent browser zoom when scrolling inside sticky edit
mode ([#8116](#8116))
([e928210](e928210))
* **editor:** Prevent canvas undo/redo when NDV is open
([#8118](#8118))
([39e45d8](39e45d8))
* **editor:** Prevent storing pairedItem data inside of pinData
([#8173](#8173))
([405e267](405e267))
* **GitHub Node:** Fix issue that File->Get did not run once per item
([#8190](#8190))
([11cda41](11cda41))
* **Invoice Ninja Node:** Fix issue with custom invoice numbers not
working with v5 ([#8200](#8200))
([3b6ae2d](3b6ae2d))
* **Microsoft Excel 365 Node:** Ensure arg is string during worksheet
table search ([#8154](#8154))
([8e873ca](8e873ca))
* **Notion Node:** Ensure arg is string during page ID extraction
([#8153](#8153))
([e94b8a6](e94b8a6))
* **Redis Trigger Node:** Activating a workflow with a Redis trigger
fails ([#8129](#8129))
([a169b74](a169b74))
* **Schedule Trigger Node:** Use the correct `moment` import
([#8185](#8185))
([17a4e2e](17a4e2e))
* Show public API upgrade CTA when feature is not enabled
([#8109](#8109))
([e9c7fd7](e9c7fd7))


### Features

* **core:** Add closeFunction support to Sub-Nodes
([#7708](#7708))
([bec0fae](bec0fae))
* **core:** Add user.profile.beforeUpdate hook
([#8144](#8144))
([e126ed7](e126ed7))
* **core:** Improvements/overhaul for nodes working with binary data
([#7651](#7651))
([5e16dd4](5e16dd4))
* **core:** Remove discontinued crypto-js
([#8104](#8104))
([01e9a79](01e9a79))
* **core:** Unify application components shutdown
([#8097](#8097))
([3a881be](3a881be))
* **editor:** Add node execution status indicator to output panel
([#8124](#8124))
([ab74bad](ab74bad))
* **editor:** Add template Id to workflow metadata
([#8088](#8088))
([517b050](517b050))
* **Home Assistant Node:** Use the new Home Assistant logo
([#8150](#8150))
([518a99e](518a99e))
* **Qdrant Vector Store Node:** Qdrant vector store support
([#8080](#8080))
([66460f6](66460f6))
* **Wordpress Node:** Add option to ignore error when using self signed
certificates ([#8199](#8199))
([65c8e12](65c8e12))

Co-authored-by: ivov <ivov@users.noreply.github.com>
@Joffcom
Copy link
Member

Joffcom commented Jan 3, 2024

Got released with n8n@1.23.0

@tomi tomi mentioned this pull request Jan 4, 2024
Merged
3 tasks
tomi added a commit that referenced this pull request Jan 5, 2024
@tomi
fix: Add template id to workflows created from templates (no-changelo…
8a78ae1 
…g) (#8226)

In #8088 template ID was added to workflow metadata, but it was missing
from workflows that were created using the template credential setup.
This fixes that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mutdmour mutdmour mutdmour approved these changes

Assignees

@MiloradFilipovic MiloradFilipovic

Labels
n8n team Authored by the n8n team 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 @Joffcom @mutdmour