-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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.
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
feat: Enable cred setup for workflows created from templates (no-changelog) #8240
feat: Enable cred setup for workflows created from templates (no-changelog) #8240
Conversation
export type AppCredentialCount = { | ||
appName: string; | ||
count: number; | ||
}; | ||
|
||
//#region Getter functions | ||
|
||
export const groupNodeCredentialsByKey = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here has been mainly moved useCredentialSetupState.ts
|
||
//#region Types | ||
|
||
export type NodeCredentials = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these have been moved (with slight modifications) from setupTemplate.store.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Looks good. Just a small question to address
@@ -227,6 +227,7 @@ export default defineComponent({ | |||
|
|||
.modal-content { | |||
overflow: hidden; | |||
overflow-y: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test this across different modals? Could be a problematic change. Putting this behind a prop might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked a few modals but not all. I think it's more problematic if content gets hidden if there's not enough vertical space
|
||
<div> | ||
<ol :class="$style.appCredentialsContainer"> | ||
<SetupTemplateFormStep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 Love this. Such complex functionality, hidden away and reused.
[CREDENTIAL_SELECT_MODAL_KEY]: { | ||
open: false, | ||
}, | ||
...Object.fromEntries( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
|
2 flaky tests on run #3623 ↗︎Details:
|
Test | Artifacts | |
---|---|---|
Sharing > should work for admin role on credentials created by others (also can share it with themselves) |
Screenshots
Video
|
29-templates.cy.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
Templates > should save template id with the workflow |
Screenshots
Video
|
Review all test suite changes for PR #8240 ↗︎
|
1 similar comment
|
✅ All Cypress E2E specs passed |
…8245) ## Summary Follow-up to #8240 Adds e2e tests for the template credential setup in workflow editor ## Related tickets and issues https://linear.app/n8n/issue/ADO-1463/feature-enable-users-to-close-and-re-open-the-setup
Summary
Enable users to open credential setup for workflows that have been created from templates if they skip it.
Next steps (will be their own PRs):
Kapture.2024-01-05.at.13.05.25.mp4
Related tickets and issues
https://linear.app/n8n/issue/ADO-1637/feature-support-template-credential-setup-for-http-request-nodes-that
Review / Merge checklist
(no-changelog)
otherwise. (conventions)