feat(editor): Simplify NDV by moving authentication details to credentials modal #5067

MiloradFilipovic · 2022-12-30T16:00:35Z

* master: (33 commits) ci: Update Jest setup on the CI to avoid OOM errors (no-changelog) (#5042) fix(editor): Fix displaying of some trigger nodes in the creator panel (#5040) fix(editor): Add sticky note without manual trigger (#5039) fix(editor): Display default missing value in table view as `undefined` (#5038) refactor(editor): Tweak phrasing of `itemMatching()` (no-changelog) (#5037) 📚 Update CHANGELOG.md and main package.json to 0.209.3 🔖 Release n8n@0.209.3 ⬆️ Set n8n-core@0.149.2, n8n-editor-ui@0.175.3, n8n-nodes-base@0.207.2 and n8n-workflow@0.131.2 on n8n 🔖 Release n8n-editor-ui@0.175.3 ⬆️ Set n8n-design-system@0.49.3 and n8n-workflow@0.131.2 on n8n-editor-ui 🔖 Release n8n-design-system@0.49.3 🔖 Release n8n-nodes-base@0.207.2 ⬆️ Set n8n-core@0.149.2 and n8n-workflow@0.131.2 on n8n-nodes-base 🔖 Release n8n-node-dev@0.88.2 ⬆️ Set n8n-core@0.149.2 and n8n-workflow@0.131.2 on n8n-node-dev 🔖 Release n8n-core@0.149.2 ⬆️ Set n8n-workflow@0.131.2 on n8n-core 🔖 Release n8n-workflow@0.131.2 fix(core): Non owner should be permitted to use their own credentials (#5036) fix(editor): Fix for loading executions that are not on the current executions list (#5035) ...

… hiding auth selector UI in that case

…scription. Disabling `Credentials of type not found` error in node

…port for multiple auth fileds

* master: ci: Fix linting on builds (no-changelog) (#5062) refactor: Lint for no interpolation in regular string (#5060) (no-changelog) ci: Fix lint for build (no-changelog) (#5059) perf: Prevent oclif from loading ts-node and typescript (#5047) (no-changelog) fix(editor): Make node title non-editable in executions view (#5046) refactor: Lint for no unneeded backticks (#5057) (no-changelog) fix(core): Fix full manual execution for error trigger as starter of 2+ node workflow (#5055) fix(editor): Support tabbing away from inline expression editor (#5056) refactor(editor): Usage and plans page on Desktop (#5051) 📚 Update CHANGELOG.md and main package.json to 0.209.4 🔖 Release n8n@0.209.4 ⬆️ Set n8n-editor-ui@0.175.4 on n8n 🔖 Release n8n-editor-ui@0.175.4 fix(editor): Usage and plans page on Desktop (#5045) feat(editor): Switch to expression on `=` input (#5044) fix(editor): Fix trigger node type identification on add to canvas (#5043)

… nodes. Pre-populating credential type for HTTP request node when selected from 'app action' menu

… UI in NDV

* master: (53 commits) fix: Remove anonymous ID from tracking calls (#5099) refactor(core): Add more overloads for string-type node parameters (no-changelog) (#5101) fix(Read Binary File Node): Do not crash the execution when the source file does not exist (#5100) fix: Stop OOM crashes in Execution Data pruning (#5095) feat(editor): Introduce proxy completions to expressions (#5075) fix(Google Sheets Node): Fix for auto-range detection 📚 Update CHANGELOG.md and main package.json to 0.210.1 🔖 Release n8n@0.210.1 ⬆️ Set n8n-editor-ui@0.176.1 and n8n-nodes-base@0.208.1 on n8n 🔖 Release n8n-editor-ui@0.176.1 ⬆️ Set n8n-design-system@0.50.1 on n8n-editor-ui 🔖 Release n8n-design-system@0.50.1 🔖 Release n8n-nodes-base@0.208.1 fix: Pass in the correct server reference to external hooks (no-changelog) (#5094) fix(Google Sheets Node): Append or Update fails for numeric values feat: Add user management invite links without SMTP set up (#5084) fix: Remove annonymous ID (no-changelog) (#5093) 📚 Update CHANGELOG.md and main package.json to 0.210.0 🔖 Release n8n@0.210.0 ⬆️ Set n8n-core@0.150.0, n8n-editor-ui@0.176.0, n8n-nodes-base@0.208.0 and n8n-workflow@0.132.0 on n8n ... # Conflicts: # packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

* master: (21 commits) test: Fix default owner test to be able to reach setup page (#5113) refactor: Reduce custom docker image size by about 30% (#5104) fix(editor): Disable data pinning on multiple output node types (#5111) 📚 Update CHANGELOG.md and main package.json to 0.210.2 🔖 Release n8n@0.210.2 ⬆️ Set n8n-core@0.150.1, n8n-editor-ui@0.176.2, n8n-nodes-base@0.208.2 and n8n-workflow@0.132.1 on n8n 🔖 Release n8n-editor-ui@0.176.2 ⬆️ Set n8n-workflow@0.132.1 on n8n-editor-ui 🔖 Release n8n-nodes-base@0.208.2 ⬆️ Set n8n-core@0.150.1 and n8n-workflow@0.132.1 on n8n-nodes-base 🔖 Release n8n-node-dev@0.89.1 ⬆️ Set n8n-core@0.150.1 and n8n-workflow@0.132.1 on n8n-node-dev 🔖 Release n8n-core@0.150.1 ⬆️ Set n8n-workflow@0.132.1 on n8n-core 🔖 Release n8n-workflow@0.132.1 feat: Add source to all View Plans links (no-changelog) (#5097) fix: Do not attempt to save statistics data for unsaved workflows (#5106) fix: Update links for user management and SMTP help (#5109) fix(editor): Omit `pairedItem` from proxy completions (#5098) test: Fix e2e tests for inline expression editor (#5108) ...

…ot selected

* master: 📚 Update CHANGELOG.md and main package.json to 0.212.1 🔖 Release n8n@0.212.1 ⬆️ Set n8n-core@0.151.2, n8n-editor-ui@0.178.1, n8n-nodes-base@0.210.1 and n8n-workflow@0.133.2 on n8n 🔖 Release n8n-editor-ui@0.178.1 ⬆️ Set n8n-workflow@0.133.2 on n8n-editor-ui 🔖 Release n8n-nodes-base@0.210.1 ⬆️ Set n8n-core@0.151.2 and n8n-workflow@0.133.2 on n8n-nodes-base 🔖 Release n8n-node-dev@0.90.2 ⬆️ Set n8n-core@0.151.2 and n8n-workflow@0.133.2 on n8n-node-dev 🔖 Release n8n-core@0.151.2 ⬆️ Set n8n-workflow@0.133.2 on n8n-core 🔖 Release n8n-workflow@0.133.2 ci: Fix typing issues in cli tests (no-changelog) (#5227) fix(editor): Remove infinite loading in not found workflow level execution (#5174) fix: IsWeekend not checking if DateTime (#5221) (no-changelog) fix(core): Fix execute-once incoming data handling (#5211) fix(core): Make pindata with webhook responding on last node manual-only (#5223) fix(core): Fix onWorkflowPostExecute not being called (#5224) fix(core): Fix expression extension misdetection (#5219) fix: Add schema to postgres migrations (hotfix) (#5218)

… authentication fields which can have `none` value

… in credentials modal

cypress/e2e/2-credentials.cy.ts

packages/editor-ui/src/stores/ui.ts

packages/editor-ui/src/constants.ts

packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue

packages/editor-ui/src/Interface.ts

packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue

mutdmour · 2023-01-24T11:12:36Z

packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue

+		activeNodeType(): INodeTypeDescription | null {
+			const activeNode = this.ndvStore.activeNode;
+
+			if (activeNode) {
+				return this.nodeTypesStore.getNodeType(activeNode.type, activeNode.typeVersion);
+			}
+			return null;
+		},
+		nodeAuthOptions(): NodeAuthenticationOption[] {
+			return getNodeAuthOptions(this.activeNodeType);
+		},


would have designed this differently, would have passed credential types from nodecredentials when opening a new credential.. that way this modal is agnostic of what node is open.. just gets a list of cred types to show

maybe the cred types to render can be passed down from credentialedit instead.. avoid this component being aware of activenode

Agree completely. With the this wisdom I would have started it like you are saying here. Discussed it also with Nik yesterday and the decision was to do this rework as as the first thing of P1 project (ticket). So I would suggest leaving is like this for now. Thoughts?

not really related to P1 or this ticket.. it's not a big deal though.. it's up to you here what you think is best..

I would leave it as is for now so we don't block p0 anymore and work on it straight away as part of ADO-239

packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

mutdmour · 2023-01-24T11:48:19Z

packages/editor-ui/src/utils/nodeTypesUtils.ts

+// A field is considered main auth filed if:
+// 1. It is a credential dependency
+// 2. If all of it's possible values are used in credential's display options
+const findAlternativeAuthField = (


what's an example of a node here?

All nodes from this list.

mutdmour · 2023-01-24T11:50:58Z

packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

+								!this.credentialType.__overwrittenProperties?.includes(property.name)
+							) {
+								Vue.set(
+									this.credentialData,


should not we clear credentialdata every time we switch? seems like we are storing data for every auth type.. would not that lead to issues if two auth types share the same prop type?

Isn't this code doing that (reset)? It's setting credential parameters to their default values.

it does not reset if you set a value because of the condition above !this.credentialData.hasOwnProperty(property.name)

Oh, right 🤦‍♂️

packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

packages/editor-ui/src/utils/nodeTypesUtils.ts

packages/editor-ui/src/components/NodeCredentials.vue

…cy path to determine if the field is required or optional

mutdmour · 2023-01-24T21:33:20Z

packages/editor-ui/src/components/CredentialEdit/AuthTypeSelector.vue

@@ -0,0 +1,151 @@
+<template>


nitpick - script first

mutdmour · 2023-01-24T21:38:57Z

packages/editor-ui/src/Interface.ts

+export interface NewCredentialsModal extends IModalState {
 	requiredCredentials?: boolean;


nitpick - still requiredCredential is not a great name here..

Alright, updated.

mutdmour · 2023-01-24T21:39:23Z

packages/editor-ui/src/Interface.ts

@@ -1159,7 +1162,7 @@ export interface UIState {
 	sidebarMenuCollapsed: boolean;
 	modalStack: string[];
 	modals: {
-		[key: string]: IModalState;
+		[key: string]: IModalState | NewCredentialsModal;


types could get more specific

Suggested change

[key: string]: IModalState | NewCredentialsModal;

newModal: NewCredentialsModal;

[key: string]: IModalState;

Agree, added new types and typeguards for this.

mutdmour · 2023-01-24T21:40:13Z

packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

@@ -212,21 +212,14 @@ export default mixins(showMessage, nodeHelpers).extend({
 	},
 	async mounted() {
 		this.requiredCredentials =
-			this.uiStore.modals[CREDENTIAL_EDIT_MODAL_KEY].requiredCredentials === true;
+			(this.uiStore.modals[CREDENTIAL_EDIT_MODAL_KEY] as NewCredentialsModal)


should avoid type conversions (as here).. suggestion above to set the types should fix this..

Fixed by better typing as said.

mutdmour · 2023-01-24T21:41:48Z

packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

@@ -470,6 +461,15 @@ export default mixins(showMessage, nodeHelpers).extend({
 		isSharingAvailable(): boolean {
 			return this.settingsStore.isEnterpriseFeatureEnabled(EnterpriseEditionFeature.Sharing);
 		},
+		getDefaultCredentialTypeName(): string {


nitpick - computed property so does not require get since it's not a function

Agree, updated.

mutdmour · 2023-01-24T21:42:32Z

packages/editor-ui/src/components/CredentialEdit/CredentialEdit.vue

+			if (!this.credentialType) {
+				return;
+			}
+			for (const property of this.credentialType.properties) {


should this check for overrides still as original logic does? as not to break cloud behavior for example...

please make sure this works on cloud with overrides

Yes, adding it back.

…type

* master: fix(editor): Fix save modal appearing after duplicating a workflow (#5247) feat(editor): Adjust Google sign-in button to adhere to the guidelines (#5248) fix(HelpScout Node): Fix tag search not working when getting all conversations (#5239) fix(editor): Do not request workflow data twice when opening a workflow (#5246) fix(editor): Fix the element-ui imports in SettingsLdapView (no-changelog) (#5245) fix(core): Handle missing binary metadata in download urls (#5242) ci: Simplify DB truncate in tests (no-changelog) (#5243) feat(core): Add LDAP support (#3835) fix(core): Upsert credentials and workflows in the import:* commands (#5231) feat(Jira Software Node): Use resource locator component (#5090) ci: Publish n8n docker images to GHCR (#5213) fix(Google Drive Node): Use the correct mimetype on converted downloads (#5240) fix(editor): Prevent workflow execution list infinite no network error (#5230) fix: Extension being too eager and making calls when it shouldn't (#5232) feat(Send Email Node): Overhaul refactor(core): Add support for implicit schema in postgres migrations (#5233) # Conflicts: # packages/editor-ui/src/components/CredentialEdit/CredentialConfig.vue

ivov · 2023-01-26T09:28:59Z

cypress/constants.ts

+export const GMAIL_NODE_NAME = 'Gmail';
+export const TRELLO_NODE_NAME = 'Trello';
+export const NOTION_NODE_NAME = 'Notion';
+export const PIPEDRIVE_NODE_NAME = 'Pipedrive';
+export const HTTP_REQUEST_NODE_NAME = 'HTTP Request';


Can we avoid duplication and import from packages/editor-ui/src/constants.ts?

These constants represent node names as users would type them in search fields so they are used only in tests while constants in editor-ui hold node type names used to identify nodes in code. So my vote is to leave this here.

cypress/e2e/2-credentials.cy.ts

ivov · 2023-01-26T09:33:52Z

cypress/e2e/2-credentials.cy.ts

+		cy.get('body').type('{downArrow}');
+		cy.get('body').type('{enter}');
+		// Select incoming authentication
+		nodeDetailsView.getters.parameterInput('incomingAuthentication').should('exist');


Not super familiar with Cypress, do we need this assertion? I expect if it doesn't, the click step at L145 will fail.

For me it's just a question of stating that at that point that element should be there clearly in the code. But agree, click() has a timeout already built in and will fail if element is not there so it's redundant to have should.exist here.

cypress/e2e/2-credentials.cy.ts

ivov · 2023-01-26T10:50:46Z

packages/editor-ui/src/utils/nodeTypesUtils.ts

+	);
+};
+
+export const getAuthTypeForNodeCredential = (


Same of the comments above apply here and below as well.

packages/editor-ui/src/utils/nodeTypesUtils.ts

packages/editor-ui/src/utils/typeGuards.ts

janober · 2023-01-27T16:20:49Z

Got released with n8n@0.213.0

MiloradFilipovic added 17 commits December 23, 2022 17:17

⚡ Removing authentication parameter from NDV

86f407d

⚡ Added auth type selector to credentials modal

7bc2f39

🔨 Extracting reusable logic to util functions

bd23dd0

⚡ Updating credentials position, adding label for radio buttons

f9e7076

⚡ Using first node credentials for nodes with single auth options and…

a95f873

… hiding auth selector UI in that case

⚡ Fixing credentials modal when opened from credentials page

f7290ce

⚡ Showing all available credentials in NDV credentials dropdown

44f0e63

⚡ Updating node credentials dropdown component to show credentials de…

6f22945

…scription. Disabling `Credentials of type not found` error in node

⚡ Moving auth related fields from NDV to credentials modal. Added sup…

328b1d9

…port for multiple auth fileds

⚡ Moving NDV fields that authentication depends on to credentials modal

e65f6f0

⚡ Keeping old auth/credentials UI in NDV for HTTP Request and Webhook…

302a9d8

… nodes. Pre-populating credential type for HTTP request node when selected from 'app action' menu

💄 Use old label and field position for nodes that use old credentials…

952639f

… UI in NDV

⚡ Implementing more generic way to find node's auth fileds

b42037d

📚 Adding comments on parameter hiding logic

64c8773

⚡ Fixing node auth options logic for multiple auth fields

d994904

MiloradFilipovic self-assigned this Dec 30, 2022

n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 30, 2022

MiloradFilipovic added 10 commits December 31, 2022 11:27

👕 Fixing lint errors

1d7224f

💄 Addressing design review comments

92a85aa

⚡ Not selecting first auth option when opening new credential dialog

32bcd7e

⚡ Using default credentials name and icon if authentication type is n…

f0a6b46

…ot selected

⚡ Updating credential data when auth type is changed

eef1c33

⚡ Setting new credentials type for HTTP Request and Webhook nodes

f17c6c0

⚡ Setting nodes with access when changing auth type

b697faf

👕 Fixing lint error

9186240

MiloradFilipovic added 5 commits January 24, 2023 09:58

⚡ Improving main auth field detection logic so it doesn't account for…

ec2129f

… authentication fields which can have `none` value

⚡ Restoring accidentally deleted not existing credential issue logic

44c8ff8

⚡ Updating other nodes when deleted credentials have been updated

344f271

⚡ Using filtered auth type list to show or hide radio buttons section…

892ab64

… in credentials modal

mutdmour reviewed Jan 24, 2023

View reviewed changes

👕 Addressing lint error

1304b1f

mutdmour reviewed Jan 24, 2023

View reviewed changes

MiloradFilipovic added 7 commits January 24, 2023 14:00

👌 Addressing PR review feedback

44a3e90

👕 Fixing lint issues

cb2e586

⚡ Updating main auth filed detection logic so it checks full dependen…

b8a3c4c

…cy path to determine if the field is required or optional

👌 Addressing the rest of PR feedback

81dcc00

✅ Updating credential tests

f005395

⚡ Resetting credential data on authentication type change

44a5a08

⚡ Created AuthTypeSelector component

9ad8586

MiloradFilipovic requested a review from mutdmour January 24, 2023 18:13

mutdmour reviewed Jan 24, 2023

View reviewed changes

MiloradFilipovic added 2 commits January 25, 2023 11:59

👌 Addressing PR comments

a8eaa84

⚡ Not resetting overwritten credential properties when changing auth …

9f9cd79

…type

MiloradFilipovic requested a review from ivov January 25, 2023 11:41

MiloradFilipovic added 2 commits January 25, 2023 17:17

⚡ Hiding auth selector section if there are no options to show

be860c3

ivov reviewed Jan 26, 2023

View reviewed changes

MiloradFilipovic requested a review from ivov January 26, 2023 12:49

ivov approved these changes Jan 26, 2023

View reviewed changes

MiloradFilipovic merged commit b321c5e into master Jan 27, 2023

MiloradFilipovic deleted the ado-6-ndv-simplify-credentials branch January 27, 2023 08:05

n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Jan 27, 2023

janober removed the Upcoming Release Will be part of the upcoming release label Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(editor): Simplify NDV by moving authentication details to credentials modal #5067

feat(editor): Simplify NDV by moving authentication details to credentials modal #5067

MiloradFilipovic commented Dec 30, 2022 •

edited

Loading

mutdmour Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 25, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 25, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 25, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 25, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 25, 2023

mutdmour Jan 24, 2023

mutdmour Jan 24, 2023

MiloradFilipovic Jan 25, 2023

ivov Jan 26, 2023

MiloradFilipovic Jan 26, 2023

ivov Jan 26, 2023

MiloradFilipovic Jan 26, 2023

ivov Jan 26, 2023

janober commented Jan 27, 2023

		export interface NewCredentialsModal extends IModalState {
		requiredCredentials?: boolean;

	[key: string]: IModalState \| NewCredentialsModal;
	newModal: NewCredentialsModal;
	[key: string]: IModalState;

feat(editor): Simplify NDV by moving authentication details to credentials modal #5067

feat(editor): Simplify NDV by moving authentication details to credentials modal #5067

Conversation

MiloradFilipovic commented Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janober commented Jan 27, 2023

MiloradFilipovic commented Dec 30, 2022 •

edited

Loading