Amqp provider based on #3812 #4040
Conversation
|
@p-fernandez Please can you help check this out? |
| @@ -179,4 +180,12 @@ export const smsProviders: IProviderConfig[] = [ | |||
| docReference: 'https://sendchamp.readme.io/reference/api-structure', | |||
| logoFileName: { light: 'sendchamp.svg', dark: 'sendchamp.svg' }, | |||
| }, | |||
| { | |||
| id: SmsProviderIdEnum.AmqpSms, | |||
| displayName: 'Amqp', | |||
There was a problem hiding this comment.
| displayName: 'Amqp', | |
| displayName: 'AMQP', |
For the display name let's keep the acronym in capitals as it is not code syntax.
| @@ -71,6 +71,7 @@ export enum SmsProviderIdEnum { | |||
| AfricasTalking = 'africas-talking', | |||
| Novu = 'novu-sms', | |||
| Sendchamp = 'sendchamp', | |||
| AmqpSms = 'amqp-sms', | |||
There was a problem hiding this comment.
| AmqpSms = 'amqp-sms', | |
| Amqp = 'amqp', |
We don't need to specify it is for the SMS channel. We add the channel to the enum key and value for the cases that a provider can send notifications to more than one channel.
| @@ -0,0 +1 @@ | |||
| test('should trigger amqp-sms library correctly', async () => {}); | |||
There was a problem hiding this comment.
Please add tests for the provider.
| # Novu AmqpSms Provider | ||
|
|
||
| A AmqpSms sms provider library for [@novu/node](https://github.com/novuhq/novu) | ||
|
|
||
| ## Usage | ||
|
|
||
| ```javascript | ||
| FILL IN THE INITIALIZATION USAGE | ||
| ``` |
There was a problem hiding this comment.
| # Novu AmqpSms Provider | |
| A AmqpSms sms provider library for [@novu/node](https://github.com/novuhq/novu) | |
| ## Usage | |
| ```javascript | |
| FILL IN THE INITIALIZATION USAGE | |
| ``` | |
| # Novu AMQP Provider | |
| A AMQP sms provider library for [@novu/node](https://github.com/novuhq/novu) | |
| ## Usage | |
| ```javascript | |
| FILL IN THE INITIALIZATION USAGE |
Please add the usage.
| export const amqpSmsConfig: IConfigCredentials[] = [ | ||
| { | ||
| key: CredentialsKeyEnum.ApiKey, | ||
| displayName: 'Amqp Queue', | ||
| type: 'text', | ||
| required: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
I guess from the perspective of AMQP the field from is not while it is mandatory for us in the SMS credentials configuration. But this raises the question if the channel SMS is the right one or if AMQP should live in a channel of its own, even if AMQP can be applied to manage SMS, because what would be the typical interface of an entity SMS (From number or ID, To number and message) seems not to apply to AMQP.
@scopsy I think this is an important point that we should discuss, if gathering providers by channel should be done by goal or by message entity type.
| @@ -91,6 +91,7 @@ | |||
| "@novu/termii": "^0.18.0", | |||
| "@novu/testing": "^0.18.0", | |||
| "@novu/twilio": "^0.18.0", | |||
| "@novu/amqp-sms": "0.16.3", | |||
There was a problem hiding this comment.
The provider versions are outdated as unfortunately this PR has taken long for us to review. It will need updates.
| channel.sendToQueue(this.queue, Buffer.from(JSON.stringify(options))); | ||
|
|
||
| return { | ||
| id: '1234', |
There was a problem hiding this comment.
This is not right. I would expect the AMQP service called to return an ID in the response and be used here. Same for the date. I would expect AMQP response to deliver a timestamp.
Though checking the documentation of amqplib I find the following.
Also the open channel is not being closed after the execution.
I think this part should be reviewed properly how to rightfully implement. For example, checking AMQP configuration I miss also the assertQueue use in the implementation that is mentioned in their examples.
| ISmsOptions, | ||
| ISmsProvider, | ||
| } from '@novu/stateless'; | ||
| import amqplib from 'amqplib'; |
There was a problem hiding this comment.
What's the reasoning behind choosing amqplib for this implementation and not other libraries? Is because is the mentioned in the examples by RabbitMQ guides?
Also worth to mention this library does not support AMQP 1.0 so this implementation would be targeted to brokers that support AMQP 0.9.1. I ignore what's the depth of the problems for any use to choose any or other protocol.
|
Closing due to inactivity |
What change does this PR introduce?
This PR will add amqp protocol provider
Why was this change needed?
Based on
#3812
Other information (Screenshots)