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.

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

⚡Implementing customizable node credentials position #3349

Closed

Conversation

MiloradFilipovic
Copy link
Contributor

🛑 DO NOT MERGE UNTIL #3228 HAS BEEN MERGED 🛑

Node's credentials field position can now be customized using the dedicated setting

credentials_position
.

ivov and others added 4 commits May 19, 2022 14:47
* ✨ Added node credentials type proxy. Changed node credentials input order.

* ⚡ Add computed property from versioning branch

* 🐛 Fix cred ref lost and unsaved

* ⚡ Make options consistent with cred type names

* ⚡ Use prop to set component order

* ⚡ Use constant and version

* ⚡ Fix rendering for generic auth creds

* ⚡ Mark as required on first selection

* ⚡ Implement discoverability flow

* ⚡ Mark as required on subsequent selections

* ⚡ Fix marking as required after cred deletion

* ⚡ Refactor to clean up

* ⚡ Detect position automatically

* ⚡ Add i18n to option label

* ⚡ Hide subtitle for custom action

* ⚡ Detect active credential type

* ⚡ Prop drilling to re-render select

* 🔥 Remove unneeded property

* ✏️ Rename arg

* 🔥 Remove unused import

* 🔥 Remove unneeded getters

* 🔥 Remove unused import

* ⚡ Generalize cred component positioning

* ⚡ Set up request

* 🐛 Fix edge case in endpoint

* ⚡ Display scopes alert box

* ⏪ Revert "Generalize cred comp positioning"

This reverts commit 75eea89.

* ⚡ Consolidate HTTPRN check

* ⚡ Fix hue percentage to degree

* 🔥 Remove unused import

* 🔥 Remove unused import

* 🔥 Remove unused class

* 🔥 Remove unused import

* 📘 Create type for HTTPRN v2 auth params

* ✏️ Rename check

* 🔥 Remove unused import

* ✏️ Add i18n to `reportUnsetCredential()`

* ⚡ Refactor Alex's spacing changes

* ⚡ Post-merge fixes

* ⚡ Add docs link

* 🔥 Exclude Notion OAuth cred

* ✏️ Update copy

* ✏️ Rename param

* 🎨 Reposition notice and simplify styling

* ✏️ Update copy

* ✏️ Update copy

* ⚡ Hide params during custom action

* ⚡ Show notice if any cred type supported

* 🐛 Prevent scopes text overflow

* 🔥 Remove superfluous check

* ✏️ Break up docstring

* 🎨 Tweak notice styling

* ⚡ Reorder cred param in Webhook node

* ✏️ Shorten cred name in scopes notice

* 🧪 Update Notice snapshots

* 🐛 Fix check when `globalRole` is `undefined`

* ⏪ Revert 3f2c4a6

* ⚡ Apply feedback from Product

* 🧪 Update snapshot

* ⚡ Adjust regex expansion pattern for singular

* 🔥 Remove unused import

* 🔥 Remove logging

* ⚡ Make `somethingElse` key more unique

* ⚡ Move something else to constants

* ⚡ Consolidate notice component

* ⚡ Apply latest feedback

* 🧪 Update tests

* 🧪 Update snapshot

* ✏️ Fix singular version

* 🧪 Finalize tests

* ✏️ Rename constant

* 🧪 Expand tests

* 🔥 Remove `truncate` prop

* 🚚 Move scopes fetching to store

* 🚚 Move method to component

* ⚡ Use constant

* ⚡ Refactor `Notice` component

* 🧪 Update tests

* 🔥 Remove unused keys

* ⚡ Inject custom API call option

* 🔥 Remove unused props

* 🎨 Use `compact` prop

* 🧪 Update snapshots

* 🚚 Move scopes to store

* 🚚 Move `nodeCredentialTypes` to parent

* ✏️ Rename cred types per branding

* 🐛 Clear scopes when none

* ⚡ Add default

* 🚚 Move `newHttpRequestNodeCredentialType` to parent

* 🔥 Remove test data

* ⚡ Separate lines for readability

* ⚡ Change reference from node to node name

* ✏️ Rename i18n keys

* ⚡ Refactor OAuth check

* 🔥 Remove unused key

* 🚚 Move `OAuth1/2 API` to i18n

* ⚡ Refactor `skipCheck`

* ⚡ Add `stopPropagation` and `preventDefault`

* 🚚 Move active credential scopes logic to store

* 🎨 Fix spacing for `NodeWebhooks` component

* ⚡ Implement feedback

* ⚡ Update HTTPRN default and issue copy

* Refactor to use `CredentialsSelect` param (#3304)

* ⚡ Refactor into cred type param

* ⚡ Componentize scopes notice

* 🔥 Remove unused data

* 🔥 Remove unused `loadOptions`

* ⚡ Componentize `NodeCredentialType`

* 🐛 Fix param validation

* 🔥 Remove dup methods

* ⚡ Refactor all references to `isHttpRequestNodeV2`

* 🎨 Fix styling

* 🔥 Remove unused import

* 🔥 Remove unused properties

* 🎨 Fix spacing for Pipedrive Trigger node

* 🎨 Undo Webhook node styling change

* 🔥 Remove unused style

* ⚡ Cover `httpHeaderAuth` edge case

* 🐛 Fix `this.node` reference

* 🚚 Rename to `credentialsSelect`

* 🐛 Fix mistaken renaming

* ⚡ Set one attribute per line

* ⚡ Move condition to instantiation site

* 🚚 Rename prop

* ⚡ Refactor away `prepareScopesNotice`

* ✏️ Rename i18n keys

* ✏️ Update i18n calls

* ✏️ Add more i18n keys

* 🔥 Remove unused props

* ✏️ Add explanatory comment

* ⚡ Adjust check in `hasProxyAuth`

* ⚡ Refactor `credentialSelected` from prop to event

* ⚡ Eventify `valueChanged`, `setFocus`, `onBlur`

* ⚡ Eventify `optionSelected`

* ⚡ Add `noDataExpression`

* 🔥 Remove logging

* 🔥 Remove URL from scopes

* ⚡ Disregard expressions for display

* 🎨 Use CSS modules

* 📘 Tigthen interface

* 🐛 Fix generic auth display

* 🐛 Fix generic auth validation

* 📘 Loosen type

* 🚚 Move event params to end

* ⚡ Generalize reference

* ⚡ Refactor generic auth as `credentialsSelect` param

* ⏪ Restore check for `httpHeaderAuth `

* 🚚 Rename `existing` to `predefined`

* Extend metrics for HTTP Request node (#3282)

* ⚡ Extend metrics

* 🧪 Add tests

* ⚡ Update param names

Co-authored-by: Alex Grozav <alex@grozav.com>
@MiloradFilipovic MiloradFilipovic added the ui Enhancement in /editor-ui or /design-system label May 20, 2022
@MiloradFilipovic MiloradFilipovic self-assigned this May 20, 2022
@pemontto
Copy link
Contributor

pemontto commented May 20, 2022

🙌 this will solve so many head scratching moments for new users! E.g. in the Jira node, choosing the Server vs Cloud option before the auth dropdown. Same thing with all the Oauth2 vs API key nodes 🎉.

Assuming this will be configurable by the node developer?

@MiloradFilipovic
Copy link
Contributor Author

🙌 this will solve so many head scratching moments for new users! E.g. in the Jira node, choosing the Server vs Cloud option before the auth dropdown. Same thing with all the Oauth2 vs API key nodes 🎉.

Assuming this will be configurable by the node developer?

I don't think it's currently configurable on the node level, but that's really good suggestion. Thanks, hopefully we'll be able to work on in in soon.

@mutdmour mutdmour self-requested a review May 23, 2022 09:14
Comment on lines +74 to +76
export function getDefaultCredentialsIndex(node: INode): number {
return node.type === WEBHOOK_NODE_TYPE ? 1 : 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this works... but we could do better. we can try to look through the params and credentials dependency and figure out where the credential should set..
because there's exceptions we need to look at here

  • Git node
  • Pipedrive
  • Sentry io

@alexgrozav
Copy link
Member

@MiloradFilipovic Great work on this! I have a few remarks regarding the implementation of it, going forward:

  • The configuration of the credentials index will be done by the node developer, not by the user, as @pemontto said, therefore the nodeCredentialsIndex field will be an internal field, defined only inside the packages/nodes-base node definitions.
  • The index configuration field will not be visible anywhere within the UI itself, it's for internal use only
  • The nodeCredentialsIndex will be available directly via the node configuration in the frontend (no need for a prop, should already be there)
  • If the nodeCredentialsIndex field is missing, default to 0.

@n8n-assistant n8n-assistant bot added community Authored by a community member core Enhancement outside /nodes-base and /editor-ui labels May 23, 2022
@Joffcom Joffcom removed the community Authored by a community member label May 23, 2022
@n8n-assistant n8n-assistant bot added the n8n team Authored by the n8n team label May 23, 2022
Base automatically changed from custom-operations to master May 24, 2022 09:36
@MiloradFilipovic
Copy link
Contributor Author

Closing in favor of #3461
Decided to go the automatic detection route after talking with @mutdmour

@MiloradFilipovic MiloradFilipovic deleted the N8N-3653-customizable-credentials-position branch June 28, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants