🐛 CreateNotificationJobCommand fix - update templateProviderIds Map to Record. #3614
Conversation
djabarovgeorge
changed the title
|
@djabarovgeorge do we have any way to create a test case for this value? I think it's the second time it's doing problems :( |
Do you mean that |
|
@djabarovgeorge yes, it caused some issue previously with undefined values for providerId when the job is created. It was fixed later, but now seems there is another issue in that area. |
| @@ -241,14 +242,14 @@ export class TriggerEvent { | |||
| channelType | |||
| ); | |||
| if (provider) { | |||
| providers.set(channelType, provider); | |||
| providers[channelType] = provider; | |||
There was a problem hiding this comment.
Isn't this to cause problems for example in the email channel with the Novu provider plus the email provider a user might enable?
There was a problem hiding this comment.
With the current logic, I think that this.getProviderId will return not novu integration if exists. and then set it
There was a problem hiding this comment.
and won't return the novu email integration?
There was a problem hiding this comment.
It will first of all return, not Novu integration if exist, and if not it will return Novu's integration.
packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| const channel = STEP_TYPE_TO_CHANNEL_TYPE.get( | ||
| step.template.type | ||
| ) as ChannelTypeEnum; |
There was a problem hiding this comment.
I feel we should change the STEP_TYPE_TO_CHANNEL_TYPE to object as we now need to cast the value always
There was a problem hiding this comment.
Looks like the cast was not needed at all so i removed it.
| @@ -241,14 +242,14 @@ export class TriggerEvent { | |||
| channelType | |||
| ); | |||
| if (provider) { | |||
| providers.set(channelType, provider); | |||
| providers[channelType] = provider; | |||
There was a problem hiding this comment.
and won't return the novu email integration?
| @@ -196,6 +196,7 @@ export class SendMessageInApp extends SendMessageBase { | |||
| transactionId: command.transactionId, | |||
| content: this.storeContent() ? content : null, | |||
| payload: messagePayload, | |||
| providerId: integration.providerId, | |||
There was a problem hiding this comment.
funny thing, we did not save the provider on the message, which also means that the previous query of oldMessage always failed.
| @@ -257,7 +258,7 @@ export class SendMessageInApp extends SendMessageBase { | |||
| CreateExecutionDetailsCommand.create({ | |||
| ...CreateExecutionDetailsCommand.getDetailsFromJob(command.job), | |||
| messageId: message._id, | |||
| providerId: InAppProviderIdEnum.Novu, | |||
| providerId: integration.providerId, | |||
There was a problem hiding this comment.
added dynamic provider id
| @@ -296,7 +297,7 @@ export class SendMessageInApp extends SendMessageBase { | |||
| CreateExecutionDetailsCommand.create({ | |||
| ...CreateExecutionDetailsCommand.getDetailsFromJob(command.job), | |||
| messageId: message._id, | |||
| providerId: InAppProviderIdEnum.Novu, | |||
| providerId: integration.providerId, | |||
There was a problem hiding this comment.
was not sure where to put it so i left it on the trigger e2e.
What change does this PR introduce?
Changed the templateProviderIds Map to Record.
Why was this change needed?
Looks like the
plainToInstance(from 'class-transformer') in the BaseCommand is mapping Map values to 'undefined'.Other information (Screenshots)
typestack/class-transformer#1256