-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
fix(Wait Node): Authentication fix #10236
Conversation
@@ -294,6 +302,29 @@ export class Wait extends Webhook { | |||
default: 'timeInterval', | |||
description: 'Determines the waiting mode to use before the workflow continues', | |||
}, | |||
{ |
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.
there seems to be a bit of duplication between this and the next property. is this because the auth options are more limited here?
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.
yes, we probably should avoid sharing specialized node's properties between different node types as this creates some coupling
@@ -140,8 +140,15 @@ const checkResponseModeConfiguration = (context: IWebhookFunctions) => { | |||
} | |||
}; | |||
|
|||
const authProperty = (nodeType: string) => { | |||
if (nodeType === 'n8n-nodes-base.wait') { | |||
return 'incomingAuthentication'; |
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.
since this is defined in the node already, could we somehow pick it from there instead ?
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.
We should clean this up at a later point.
const nodeVersion = context.getNode().typeVersion; | ||
export async function formWebhook( | ||
context: IWebhookFunctions, | ||
authProperty = FORM_TRIGGER_AUTHENTICATION_PROPERTY, |
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.
This is why I brought up this conversation. If we switch to using class inheritance (instead of the custom style we use), we wouldn't need to pass data around like this, and we could use this.authPropertyName
, like we do in Webhook and Wait nodes.
displayName: 'Authentication', | ||
name: 'incomingAuthentication', | ||
type: 'options', | ||
options: [ | ||
{ | ||
name: 'Basic Auth', | ||
value: 'basicAuth', | ||
}, | ||
{ | ||
name: 'None', | ||
value: 'none', | ||
}, | ||
], | ||
default: 'none', |
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.
we are still duplicating a lot, and the style of this block isn't consistent with the one after this.
The issue with duplication like this is that every time we need to update something in one place, we need to update it in all the copies. And we don't always do that, so we end up with slightly different versions of the same code that later someone else has to clean up.
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 would argue that here it is justifiable as Wait node basically include both Form and Webhook node, and though they have similar properties they are completely different entity, trying too mach reuse code we are tying them together so changes in Webhook would require Form change, which is not what we want
Ideally we want some library of common properties and helpers to setup them, like returnAllOrLimit and helpers to populate options and fixedCollection
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.
What we need is a better way to describe properties, that's type-safe, and allows code reuse without being unintuitive.
Right now we are battling between duplicating code or making code unreadable, and neither is great.
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
✅ All Cypress E2E specs passed |
Got released with |
Summary
Authentication for Wait node
Hint to warn about specific of manual execution
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/NODE-1536/wait-node-authentication