feat: Add Push Webhook provider #3266
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
providers/push-webhook/src/lib/push-webhook.provider.spec.ts
Dismissed
Show dismissed
Hide dismissed
|
@p-fernandez do you think this provider can be released with the next release? |
|
@scopsy can you check this one out as well? |
| @@ -41,4 +41,6 @@ export class CredentialsDto { | |||
| ignoreTls?: boolean; | |||
| @ApiPropertyOptional() | |||
| tlsOptions?: Record<string, unknown>; | |||
| @ApiPropertyOptional() | |||
| baseUrl?: string; | |||
There was a problem hiding this comment.
I find baseUrl could be a bit confusing. And as it is a specific credential property added for this feature what do you think of naming it pushWebhookUrl? I was going to suggest webhookUrl in case we wanted in the future something similar with the other channels, but not sure if that's a direction we will want to take.
@scopsy your thoughts about this?
There was a problem hiding this comment.
Yeah i used the same as the other webhook proposal. I can change it to whatever you prefer :)
There was a problem hiding this comment.
@p-fernandez iirc, I think it was listed as a credential property somewhere but not totally implemented. (this is why i used this property naming but could rename it to whatever you want btw)
There was a problem hiding this comment.
I changed to the already existing webhookUrl
There was a problem hiding this comment.
Also I realised that baseUrl is used for other purposes for the providers (for example when the provider needs to set a certain URL depending on the account / API key). So I think this change is a good one. 👍🏻
Co-authored-by: Pablo Fernández <pablo@novu.co>
| @@ -41,4 +41,6 @@ export class CredentialsDto { | |||
| ignoreTls?: boolean; | |||
| @ApiPropertyOptional() | |||
| tlsOptions?: Record<string, unknown>; | |||
| @ApiPropertyOptional() | |||
| baseUrl?: string; | |||
There was a problem hiding this comment.
Also I realised that baseUrl is used for other purposes for the providers (for example when the provider needs to set a certain URL depending on the account / API key). So I think this change is a good one. 👍🏻
packages/application-generic/src/factories/push/handlers/push-webhook.handler.ts
Outdated
Show resolved
Hide resolved
…webhook.handler.ts Co-authored-by: Pablo Fernández <pablo@novu.co>
|
@p-fernandez good catch! Thanks :) |
libs/shared/src/dto/integration/construct-integration.interface.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Pablo Fernández <pablo@novu.co>
libs/shared/src/consts/providers/credentials/provider-credentials.ts
Outdated
Show resolved
Hide resolved
|
Is this for Push only? If so, is there a reason we aren't doing it for the other channels? Thank you! |
There is work ongoing for Email webhook provider here: #3244 |
|
@p-fernandez Thank you! |
What change does this PR introduce?
Added Webhooks as a push provider.
Why was this change needed?
Closed #3265
Other information (Screenshots)
Added tests as well