feat(api): move the providers decryption for the template out of loop #2887
Conversation
| if (provider) { | ||
| providers.set(channelType, provider); | ||
| } else { | ||
| providers.set(channelType, InAppProviderIdEnum.Novu); |
There was a problem hiding this comment.
I am keeping this for retrocompatibility but can't grasp the need of setting a default provider if no provider for the channel. Is related to our free trial provider? @davidsoderberg
There was a problem hiding this comment.
I am not sure (i do think we need to add the novu integration by default as the in-app channel, that way we can use it more securely and not as a default)
Small note maybe we should add it only if it is in-app channelType. (i am pretty sure it could be otherwise but still)
There was a problem hiding this comment.
Should I add that check? Do not mind if we find it is safe to do. If not, I'd rather leave it for a different PR.
There was a problem hiding this comment.
Tend to agree we can leave it to separate PR it was a small thought I had.
That's why I think we need to treat 'novu' in-app as a regular integration.
p-fernandez
requested review from
LetItRock,
ainouzgali,
BiswaViraj and
djabarovgeorge
| channelType: ChannelTypeEnum | ||
| ): Promise<string> { | ||
| const integrations = await this.getDecryptedIntegrations.execute( | ||
| GetDecryptedIntegrationsCommand.create({ |
There was a problem hiding this comment.
I think we can pass here findOne:true as a param so on the command, that way you wont fetch by 'find'
There was a problem hiding this comment.
Checking GetDecryptedIntegrations code I do not find a big difference between both options as downstream still operates as an array and does all the checks adding the one entity found to an array.
There was a problem hiding this comment.
You are right, the only difference as far as I know will be the query operation find/findOne when 'find' will query all the collection, and 'findOne' will stop after the first find.
|
@p-fernandez Looks awesome and a great catch on the unnecessary compute. I really liked the refactor, but (not related to this PR at all!! 🙂) I am not sure if we need the provider id on the job entity at all, more than that I think it is a bug. The reason is that for chat and push we allow to have multiple integration providers (maybe to other channels in the future as well), more then that and the thing is that we refetch the integration on the send-message-type(sms/email, etc..) (which is good which means that if the integration was updated we get the most updated one). So due to the refetch i am not sure we need it on the job entity at all unless we use it somewhere (i had not a chance to validate) |
Indeed for multiprovider channels this should change and probably the way of fetching the integrations information. If adding the provider to the job is a bug I have not enough information right now to comment. Would need a deeper dig. 🙂 |
It could be a bug or that could be redundant data or none, maybe we should ask on some channel about it or/and create a ticket about it. 🤔 |
As discussed with the team the goal of providerId in the job entity:
|
| @@ -22,6 +22,9 @@ export class CreateNotificationJobsCommand extends EnvironmentWithUserCommand { | |||
| @IsDefined() | |||
| template: NotificationTemplateEntity; | |||
|
|
|||
| @IsDefined() | |||
| templateProviderIds: Map<ChannelTypeEnum, string>; | |||
There was a problem hiding this comment.
there is nothing related to the template, so I feel it should be called channelToProviderIds?
There was a problem hiding this comment.
I do agree might not be the best name but I named it like this because it is a map of the notification template providers in use, but passed in a way we could access them quick. I wanted to make special mention where the information came from, because someone could think why don't we get them from the template object and tempted to move the logic inside again of this use case and we would be in the same problem as we were before.
What change does this PR introduce?
I moved the template providers decryption out of the step generation loop when creating a notification for every subscriber.
Why was this change needed?
I consider we were doing an expensive operation multiple times when we could just do it once for all the template steps. Downstream, the decryption is using synchronous calls to the
cryptomodule (createDecipherivfunctionality), so we were doing (number of subscribers * number of steps) synchronous calls. We can expect they might be resource expensive as being cryptographic functionalities so it seems sensible to only do the minimum needed to achieve our goals. And because for now, the flow is set to just have one template when triggering an event we can safely reuse the providers of said template for the different subscribers that will be passed in thetoproperty.Other information (Screenshots)