feat: add actor access to notification templates #3724
Conversation
richardbutler
changed the title
There was a problem hiding this comment.
@richardbutler thank you so much! You did amazing work. Sorry for my delay. I left you some questions and suggestions.
The main things for me is the reason for saving the actor on the notification. I don't see why you needed it except for populating for the feed, which could be done from the job.
Another thing, is that I would like to take the approach of adding the actor subscriberId to the job entity instead of creating a new caching function.
Also, some tests will need to be added - but let's first concentrate on the implementation :)
Let me know if you have any questions or need any assistance⭐️
| ...(actor && | ||
| actor.type !== ActorTypeEnum.NONE && { | ||
| actor, | ||
| _actorId: command.job?._actorId, |
There was a problem hiding this comment.
I don't see why we need it on the message.. I feel it could be treated like the extra information we pass to the compile payload - like we do for the subscriber/branding information
| @@ -65,8 +79,11 @@ export class SendMessageSms extends SendMessageBase { | |||
| const smsChannel: NotificationStepEntity = command.step; | |||
| if (!smsChannel.template) throw new PlatformException(`Unexpected error: SMS template is missing`); | |||
|
|
|||
| const { actor } = smsChannel.template; | |||
There was a problem hiding this comment.
Other than in app, there will not be an actor property on the template. It is passed from the editor, and only in app has that option
| _templateId: command._templateId, | ||
| _messageTemplateId: chatChannel.template?._id, | ||
| channel: ChannelTypeEnum.CHAT, | ||
| transactionId: command.transactionId, | ||
| chatWebhookUrl: chatWebhookUrl, | ||
| content: this.storeContent() ? content : null, | ||
| providerId: subscriberChannel.providerId, | ||
| _jobId: command.jobId, |
| @@ -146,6 +157,7 @@ export class SendMessageInApp extends SendMessageBase { | |||
| _notificationId: command.notificationId, | |||
| _environmentId: command.environmentId, | |||
| _subscriberId: command._subscriberId, | |||
| _actorId: command.job._actorId, | |||
There was a problem hiding this comment.
not needed in searching for an old message
| ...(command.actorSubscriber && { | ||
| _actorId: command.actorSubscriber._id, | ||
| }), |
There was a problem hiding this comment.
command.actorSubscriber._id will have the same value as what was before command.actor?._id
| const buildSubscriberIdentifierKey = ({ | ||
| _id, | ||
| _environmentId, | ||
| }: { | ||
| _id: string; | ||
| _environmentId: string; | ||
| }): string => | ||
| buildCommonKey({ | ||
| type: CacheKeyTypeEnum.ENTITY, | ||
| keyEntity: CacheKeyPrefixEnum.SUBSCRIBER, | ||
| environmentId: _environmentId, | ||
| identifierPrefix: IdentifierPrefixEnum.ID, | ||
| identifier: _id, | ||
| }); | ||
|
|
There was a problem hiding this comment.
I understand why you took this approach :) But adding another caching function is not preferred for this particular issue. Handling cache is delicate, we would need to add invalidation at places, and also it will not use the already cached data we have by the subscriberId. So, as it is done only for the actor (the only place its cached by _id) it will probably not have significant affect. Yet, using the one with subscriberId will have so much affect, as we know the actor has already been fetched once in trigger-event.
Let's go the other way, and add the external subscriberId to the job entity. I think we should call it actorSubscriberId.
it will be the equivalent of :
_id -> _actorId and subscriberId -> actorSubscriberId
Let me know if that makes sense to you
| ...(actor && | ||
| actorProcessed && { actor, actorSubscriber: actorProcessed }), |
There was a problem hiding this comment.
Here, revert back please to:
...(actor && actorProcessed && { actor: actorProcessed }),
The actorProcessed already has the subscriberId (which is the only thing in the actor object)
| ...(command.actorSubscriber && { _actorId: command.actorSubscriber._id }), | ||
| ...(command.actor && { actor: command.actor }), |
There was a problem hiding this comment.
Let's remove that fro notification
| ...(actor && | ||
| actor.type !== ActorTypeEnum.NONE && { | ||
| actor, | ||
| _actorId: command.job?._actorId, | ||
| }), |
There was a problem hiding this comment.
No need to be saved on message
| @@ -90,6 +99,7 @@ export class SendMessageEmail extends SendMessageBase { | |||
| if (!emailChannel.template) throw new PlatformException('Email channel template not found'); | |||
|
|
|||
| const email = command.payload.email || subscriber.email; | |||
| const { actor } = emailChannel.template; | |||
| @@ -109,7 +111,9 @@ export class CreateNotificationJobs { | |||
| type: step.template.type, | |||
| providerId: providerId, | |||
| expireAt: notification.expireAt, | |||
| ...(command.actor && { _actorId: command.actor?._id }), | |||
| ...(command.actorSubscriber && { | |||
There was a problem hiding this comment.
My intention is , just to have an example, is to be used like this in the job:
_actorId: command.actor._id,
actorSubscriberId: command.actor.subscriberId
|
Closing as stale for now 🙏 |
What change does this PR introduce?
When using the Novu console, a user may access the actor using
actor.-scoped variables, e.g.actor.firstName. Before this change, this information was not available to the template.actoris also added to the System Variables whitelist and treated as a special case, similar tosubscriber.Why was this change needed?
Closes #3583
Other information
I'm not 100% sure I've managed to successfully run all tests related to this change as the development workflow is a little confusing and I wasn't able to setup/run all test scripts successfully.