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: Ado 1296 spike credential setup in templates #7786

Merged
merged 23 commits into from
Nov 27, 2023
Merged

feat: Ado 1296 spike credential setup in templates #7786

merged 23 commits into from
Nov 27, 2023

Conversation

tomi
Copy link
Contributor

@tomi tomi commented Nov 23, 2023 •
edited
Loading

Introduces following changes:

  • Add a 'Setup template credentials' view to setup the credentials of a template before it is created

Linear ticket: https://linear.app/n8n/issue/ADO-1296/feature-credential-setup-in-templates
Handoff notes: https://www.notion.so/n8n/Handoff-a1150c38f6e042db91fdf61c56900967

Stacked PRs:

Kapture.2023-11-23.at.10.49.08.mp4

@tomi tomi changed the title Ado 1296 spike credential setup in templates feat: Ado 1296 spike credential setup in templates Nov 23, 2023
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Nov 23, 2023
MiloradFilipovic
MiloradFilipovic reviewed Nov 23, 2023
View reviewed changes
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Looks really solid! 👏
I left a few comments, but mostly about refactoring.

packages/editor-ui/src/Interface.ts Show resolved Hide resolved
packages/editor-ui/src/components/CredentialPicker/CredentialPicker.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/components/CredentialPicker/CredentialPicker.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/components/CredentialPicker/CredentialPicker.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/components/CredentialPicker/CredentialPicker.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/utils/templates/templateActions.ts Show resolved Hide resolved
packages/editor-ui/src/views/SetupWorkflowFromTemplateView/SetupWorkflowFromTemplateView.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/views/SetupWorkflowFromTemplateView/SetupWorkflowFromTemplateView.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/views/SetupWorkflowFromTemplateView/SetupWorkflowFromTemplateView.vue Outdated Show resolved Hide resolved
packages/editor-ui/src/views/SetupWorkflowFromTemplateView/SetupWorkflowFromTemplateView.vue Outdated Show resolved Hide resolved
This was referenced Nov 23, 2023
Merged
Merged
@tomi
Copy link
Contributor Author

tomi commented Nov 24, 2023

@MiloradFilipovic addressed all the comments. Please have another look 🙏

MiloradFilipovic
MiloradFilipovic reviewed Nov 24, 2023
View reviewed changes
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for addressing all of the points 🍦
The code looks good to go, I just left one nitpick, but looks like credentials for LangChain nodes are looking weird. Can you check that?
image

packages/editor-ui/src/components/CredentialPicker/CredentialPicker.vue Outdated
@@ -0,0 +1,132 @@
<template>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but we started writing <template> after <script setup> in out components. I think we should stick to that standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check! I was actually wondering which is the correct way to go with, as I've seen both. Will change!

@tomi
Copy link
Contributor Author

tomi commented Nov 24, 2023

Nice work, thanks for addressing all of the points 🍦 The code looks good to go, I just left one nitpick, but looks like credentials for LangChain nodes are looking weird. Can you check that? image

I think it's because the AI branch hasn't been merged yet, and the node types are missing. Same happens if you try to use that template in prod at the moment.

MiloradFilipovic
MiloradFilipovic previously approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Looking good

@cypress Cypress
Copy link

cypress bot commented Nov 27, 2023 •
edited
Loading

2 flaky tests on run #3043 ↗︎

0 283 5 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 tomi 🗃️ e2e/*
Project: n8n Commit: 34f005ae59
Status: Passed Duration: 06:26 💡
Started: Nov 27, 2023 2:14 PM Ended: Nov 27, 2023 2:20 PM
Flakiness  12-canvas.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Canvas Node Manipulation and Navigation > should add merge node and test connections Screenshots Video
Flakiness  26-resource-locator.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Resource Locator > should retrieve list options when other params throw errors Screenshots Video

Review all test suite changes for PR #7786 ↗︎

@github-actions GitHub Actions
Copy link
Contributor

✅ All Cypress E2E specs passed

tomi added a commit that referenced this pull request Nov 27, 2023
@tomi
feat(editor): Add telemetry for template credential setup (#7794)
882172e 
Part of #7786

Adds telemetry for the setup template credentials feature.

Spec:
https://www.notion.so/n8n/Handoff-a1150c38f6e042db91fdf61c56900967?pvs=4#e495f63ebb56415b8eb5af3ec537e8e0
@tomi
feat(editor): Move template credentials setup behind feature flag (#7787
34f005a 
)

To enable the flag do:

```js
localStorage.setItem('template-credentials-setup', 'true')
```
MiloradFilipovic
MiloradFilipovic approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Re-approving after merge

@github-actions GitHub Actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@tomi tomi merged commit aae45b0 into master Nov 27, 2023
19 checks passed
@tomi tomi deleted the ado-1296-spike-credential-setup-in-templates branch November 27, 2023 14:30
@github-actions github-actions bot mentioned this pull request Nov 29, 2023
Merged
ivov added a commit that referenced this pull request Nov 29, 2023
@github-actions @ivov
🚀 Release 1.19.0 (#7863)
303cf31 
#
[1.19.0](https://github.com/n8n-io/n8n/compare/n8n@1.18.0...n8n@1.19.0)
(2023-11-29)


### Bug Fixes

* **core:** Ensure member and admin cannot be promoted to owner
([#7830](#7830))
([9b87a59](9b87a59)),
closes
[/linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint#comment-62355f6](https://github.com//linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint/issues/comment-62355f6)
* **core:** Prevent error messages due to statistics about data loading
([#7824](#7824))
([847f6ac](847f6ac))
* **core:** Tighten checks for multi-main setup usage
([#7788](#7788))
([fdb2c18](fdb2c18))
* **core:** Use AbortController to notify nodes to abort execution
([#6141](#6141))
([d2c18c5](d2c18c5))
* **editor:** Add telemetry to workflow history
([#7811](#7811))
([d497041](d497041))
* **editor:** Allow owners and admins to share workflows and credentials
they don't own ([#7833](#7833))
([3ab3ec9](3ab3ec9))
* **editor:** Disable context menu actions in read-only mode
([#7789](#7789))
([902beff](902beff))
* **editor:** Fix cloud plan data loading on instance
([#7841](#7841))
([8b99384](8b99384))
* **editor:** Fix credential icon for old node type version
([#7843](#7843))
([4074107](4074107))
* **editor:** Fix icon for unknown node type
([#7842](#7842))
([28ac5a7](28ac5a7))
* **editor:** Fix mouse position in workflow previews
([#7853](#7853))
([c063398](c063398))
* **editor:** Show nice error when environment is not set up
([#7778](#7778))
([5835e05](5835e05))
* **editor:** Suppress dev server websocket messages in workflow view
([#7808](#7808))
([685ffd7](685ffd7))
* **Google Sheets Node:** Read operation execute for each item
([#7800](#7800))
([d548872](d548872))
* **HTTP Request Node:** Enable expressions for binary input data fields
([#7782](#7782))
([6208af0](6208af0))
* **Microsoft SQL Node:** Prevent double escaping table name
([#7801](#7801))
([73ec753](73ec753))


### Features

* Add AI tool building capabilities
([#7336](#7336))
([87def60](87def60))
* Add initial scope checks via decorators
([#7737](#7737))
([a37f1cb](a37f1cb))
* Ado 1296 spike credential setup in templates
([#7786](#7786))
([aae45b0](aae45b0))
* **core:** Add Support for custom CORS origins for webhooks
([#7455](#7455))
([99a9ea4](99a9ea4))
* **core:** Allow user role modification
([#7797](#7797))
([7a86d36](7a86d36))
* **core:** Set up endpoint for all existing roles with license flag
([#7834](#7834))
([2356fb0](2356fb0))
* **editor:** Add node name and version to NDV node settings
([#7731](#7731))
([da85198](da85198))
* **editor:** Add routing middleware, permission checks, RBAC store,
RBAC component ([#7702](#7702))
([67a8891](67a8891))
* **editor:** Replace middleware for Role checks with Scope checks
([#7847](#7847))
([72852a6](72852a6))
* **editor:** Show avatars for users currently working on the same
workflow ([#7763](#7763))
([77bc8ec](77bc8ec))
* **Notion Node:** Option to simplify output in getChildBlocks operation
([#7791](#7791))
([d667bca](d667bca))
* **Slack Node:** Add support for getting the profile of a user
([#7829](#7829))
([90bb6ba](90bb6ba))

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

netroy commented Nov 29, 2023

Reminder: If a PR title doesn't contain (no-changelog), please use a title that reads well in the changelog. Ado 1296 spike credential setup in templates just looks weird in the changelog/release-notes.

@janober
Copy link
Member

janober commented Dec 1, 2023

Got released with n8n@1.19.4

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

@MiloradFilipovic MiloradFilipovic MiloradFilipovic approved these changes

Assignees
No one assigned
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.

None yet
4 participants
@tomi @netroy @janober @MiloradFilipovic