fix(api): test email send with the novu provider issue fix #2876
Conversation
NV-1747 Sentry - POST /v1/events/test/email raises error when using Novu Provider
When using the Novu Provider to send the test email with that endpoint the API app raises an error: |
| @@ -64,4 +64,4 @@ FF_IS_TOPIC_NOTIFICATION_ENABLED=true | |||
| STORE_NOTIFICATION_CONTENT=true | |||
|
|
|||
| MAX_NOVU_INTEGRATION_MAIL_REQUESTS=300 | |||
| NOVU_EMAIL_INTEGRATION_API_KEY= | |||
| NOVU_EMAIL_INTEGRATION_API_KEY=test | |||
There was a problem hiding this comment.
for the e2e tests to run the logic that calculates the Novu provider limits
| this.analyticsService.track('Test Email Sent - [Events]', command.userId, { | ||
| _organization: command.organizationId, | ||
| _environment: command.environmentId, | ||
| channel: ChannelTypeEnum.EMAIL, | ||
| providerId, | ||
| }); |
There was a problem hiding this comment.
added a new segment analytics event - please note it's sent to analytics after the email is sent
| @@ -54,7 +56,7 @@ export class GetNovuIntegration { | |||
| providerId: CalculateLimitNovuIntegration.getProviderId(command.channelType), | |||
| ...limit, | |||
| }); | |||
| throw new Error(`Limit for Novus ${command.channelType.toLowerCase()} provider was reached.`); | |||
| throw new ApiException(`Limit for Novus ${command.channelType.toLowerCase()} provider was reached.`); | |||
There was a problem hiding this comment.
when the Novu provider limit is reached we should throw the ApiException not the general Error
| ) { | ||
| const { providerId } = integration; | ||
| const mailHandler = mailFactory.getHandler( | ||
| { ...integration, providerId: GetNovuIntegration.mapProviders(ChannelTypeEnum.EMAIL, providerId) }, |
There was a problem hiding this comment.
this line actually fixes the original issue
| @@ -12,13 +15,15 @@ import { TestSendMessageCommand } from './send-message.command'; | |||
| import { ApiException } from '../../../shared/exceptions/api.exception'; | |||
| import { CompileEmailTemplate } from '../../../content-templates/usecases/compile-email-template/compile-email-template.usecase'; | |||
| import { CompileEmailTemplateCommand } from '../../../content-templates/usecases/compile-email-template/compile-email-template.command'; | |||
| import { GetNovuIntegration } from '../../../integrations/usecases/get-novu-integration'; | |||
|
|
|||
| @Injectable() | |||
| export class SendTestEmail { | |||
There was a problem hiding this comment.
I am sorry for this but I had renamed the class and file name in a PR that just got merged yesterday. I can help you with the right changes if needed.
There was a problem hiding this comment.
hmm I don't see any issues with that, just updated with the next branch, but thanks!
There was a problem hiding this comment.
I saw the conflicted file in the pipeline and I wrote that comment. Good wasn't too hard to fix!
| this.analyticsService.track('Test Email Sent - [Events]', command.userId, { | ||
| _organization: command.organizationId, | ||
| _environment: command.environmentId, | ||
| channel: ChannelTypeEnum.EMAIL, | ||
| providerId, | ||
| }); |
| const mailHandler = mailFactory.getHandler( | ||
| { ...integration, providerId: GetNovuIntegration.mapProviders(ChannelTypeEnum.EMAIL, providerId) }, | ||
| mailData.from |
There was a problem hiding this comment.
I would move all this bit of code inside the try so in case of misconfiguration for the MailHandler or parsing error in the providers mapping we would still get signalled that the error was Unexpected provider error. Easier to identify at debugging as downstream we have no error identification.
| try { | ||
| await sendTestEmail(requestDto); | ||
| } catch (e) { | ||
| expect(e.response.status).to.equal(400); |
There was a problem hiding this comment.
I don't think that 400 Bad Request is the right code when hitting the limit. Find either 409 Conflict or 422 Unprocessable Entity more adequate, as any of them indicate that the request is valid and understood by the server but resource status doesn't allow to proceed. Rather than confuse the client stating the request in wrongly formatted somehow.
Anyway, this is inherited from existing functionality but might be worth to improve it. Feel free to ignore it for this PR and we can address it in a different one.
There was a problem hiding this comment.
I do agree, I'll update it ;)
| @@ -54,7 +55,7 @@ export class GetNovuIntegration { | |||
| providerId: CalculateLimitNovuIntegration.getProviderId(command.channelType), | |||
| ...limit, | |||
| }); | |||
| throw new Error(`Limit for Novus ${command.channelType.toLowerCase()} provider was reached.`); | |||
| throw new ConflictException(`Limit for Novus ${command.channelType.toLowerCase()} provider was reached.`); | |||
There was a problem hiding this comment.
| throw new ConflictException(`Limit for Novus ${command.channelType.toLowerCase()} provider was reached.`); | |
| throw new ConflictException(`Limit for Novu's ${command.channelType.toLowerCase()} provider ${providerId} was reached.`); |
Just for full context maybe. Feel free to ignore.
What change does this PR introduce?
When using the test email send with a Novu provider the API app raises an error.
https://novu-r9.sentry.io/issues/3908824474/?project=6248811&query=is%3Aunresolved&referrer=issue-stream
Why was this change needed?
Fixes the issue.
Other information (Screenshots)