feat(api): refactor of trigger event #2825

p-fernandez · 2023-02-17T11:03:36Z

What change does this PR introduce?

Refactor of trigger event.
Also moves ISubscribersDefine interface to @novu/shared as it was being exported from @novu/node. Felt weird our API had that dependency from a client package.

Why was this change needed?

Trigger event calls ProcessSubscriber that inside was hosting the creation of the notification and the building of the job steps for the jobs of the notification. Felt like a confusing implementation so decided to isolate both use cases into ProcessSubscriber and CreateNotification.

Other information (Screenshots)

linear · 2023-02-17T11:03:39Z

NV-1724 Feature: refactor trigger event

Why? (Context)

Trigger event calls ProcessSubscriber that inside was hosting the creation of the notification and the building of the job steps for the jobs of the notification. Felt like a confusing implementation so decided to isolate both use cases into ProcessSubscriber and CreateNotification.

What?

Refactor.

Definition of Done

We have a clear separation of logic for the subscriber domain operations and the notification domain operations.

The code is easier to understand and any developer can understand the flow of what's happening inside of the trigger of an event.

Trigger event is left unmodified functionality wise.

Existing test suites pass.

p-fernandez · 2023-02-17T11:05:11Z

apps/api/src/app/events/e2e/trigger-event-topic.e2e.ts

@@ -5,11 +5,12 @@ import {
  NotificationTemplateEntity,
  SubscriberEntity,
 } from '@novu/dal';
-import { ISubscribersDefine, ITopic, TriggerRecipients } from '@novu/node';


Didn't make sense to me an interface that is shared in the monorepo to be exported from the Node.js client package.

p-fernandez · 2023-02-17T11:06:57Z

apps/api/src/app/events/usecases/create-notification/create-notification.command.ts

+  overrides: Record<string, Record<string, unknown>>;
+
+  @IsDefined()
+  payload: any; // eslint-disable-line @typescript-eslint/no-explicit-any


I think here we would need to also make shareable the interface ITriggerPayload so we can remove all any set for payload property. But not 100% sure if it will be right interface.

yeah, I saw this couple of times when we use any and add the eslint-disable comment, I agree we should have the type for the payload and we might assign it to Record<string, unknown> to get rid of the eslint comment...

type ITriggerPayload = Record<string, unknown>;

Soon to be fixed.

p-fernandez · 2023-02-17T11:08:55Z

apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts

-    );
-
-    const jobs: Omit<JobEntity, '_id'>[] = [];
+  public async execute(command: ProcessSubscriberCommand): Promise<SubscriberEntity | undefined> {


Now this use case does just one thing, process the subscriber.

p-fernandez · 2023-02-17T11:10:53Z

apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts

+    const template = await this.notificationTemplateRepository.findByTriggerIdentifier(
+      command.environmentId,
+      command.identifier
+    );


Potentially we could send the template from the queued data as during ParseEventRequest we already check the template exists, but I find sensible to retrieve it again in case during the queued time for the event, the template is being somehow deleted.

p-fernandez · 2023-02-17T11:11:23Z

apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts

+    if (!template) {
+      const message = 'Notification template could not be found';
+      Logger.error(message, LOG_CONTEXT);
+      throw new ApiException(message);
+    }


Saving execution time and DB calls if we reach this scenario.

The use-case TriggerEvent is used by the TriggerHandlerQueueService and throwing that error won't work for the case when the trigger queue events are processed, because there is no REST context anymore... so I feel like it should throw some generic runtime error and we should catch and wrap it in other places... wdyt?

Not totally understand the point you explain. ApiException is a generic runtime error. We already throw something similar in the method validateTransactionIdProperty that validates the entry data. And the worker at the end it has its own context, though mostly of the information provided will come from the database. I am happy to discuss this privately if it is easier because I couldn't understand what you mean, sorry. 😓

APIException extends BadRequestException which is HttpException, but we are not in that HTTP context anymore, so I think that this is not an appropriate exception here.

🤦🏻 We have a bigger problem then. We would need to change the definition of ApiException and the use in the monorepo. The are more places where it is used under the queue context. Adding to technical debt.

p-fernandez · 2023-02-17T11:12:23Z

apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts

+    let actorProcessed;
+    if (actor) {
+      actorProcessed = await this.processSubscriber.execute(
+        ProcessSubscriberCommand.create({
+          environmentId,
+          organizationId,
+          userId,
+          subscriber: actor,
+        })
+      );
+    }


Before, when sending the actor, we were searching for it in the DB inside the loop when executing ProcessSubscriber. Therefore we would be searching for the actor number of subscribers times. Not efficient. 😅
Also its processing is the same as any subscriber recipient, so that helped to simplify ProcessSubscriber code.

p-fernandez · 2023-02-17T11:14:20Z

apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts

      );
+
+      // If no subscriber makes no sense to try to create notification
+      if (subscriberProcessed) {


Comment says it all. Before we would try to create a notification even if the subscriber didn't exist in the database.

should we then return even before we enter this for loop?

Do you mean instead to do:

if (!subscriberProcessed) { continue; }

?
RIght now I am surrounding the loop code to execute with the condition so nothing is executed if no subscriber. And the query we have to do it, to double check in case data stored in the queue and database data is not consistent.

nvm, I feel like I didn't notice that the code is surrounded within the same if...

p-fernandez · 2023-02-17T11:18:38Z

apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts

-  @Cached(CacheKeyPrefixEnum.NOTIFICATION_TEMPLATE)
-  private async getNotificationTemplate({ _id, environmentId }: { _id: string; environmentId: string }) {
-    return await this.notificationTemplateRepository.findById(_id, environmentId);


I didn't move this code at all and removed it because we are passing the template all the time to the notification creation so not needed to cache its retrieval as we only need to look for it once.
Before it made sense as we were checking in the loop for every subscriber if the template existed.
CC: @djabarovgeorge

LetItRock

awesome! 🚀 left some small suggestions

LetItRock · 2023-02-17T11:48:00Z

apps/api/src/app/events/usecases/create-notification/create-notification.command.ts

+  overrides: Record<string, Record<string, unknown>>;
+
+  @IsDefined()
+  payload: any; // eslint-disable-line @typescript-eslint/no-explicit-any


yeah, I saw this couple of times when we use any and add the eslint-disable comment, I agree we should have the type for the payload and we might assign it to Record<string, unknown> to get rid of the eslint comment...

type ITriggerPayload = Record<string, unknown>;

LetItRock · 2023-02-17T11:54:21Z

apps/api/src/app/events/usecases/create-notification/create-notification.usecase.ts

+type NotificationJob = Omit<JobEntity, '_id'>;
+
+@Injectable()
+export class CreateNotification {


this one should be called probably CreateNotificationJobs because it's what it returns

LetItRock · 2023-02-17T12:07:30Z

apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts

    subscriberPayload,
-    subscriber: SubscriberEntity | null
-  ) {
+    // TODO: Getting rid of this null would be amazing


after we do a findBySubscriberId we should have a check and throw if null

If we throw we would break the loop. That would mean that some notifications will be processed from the trigger but not all, leading to inconsistency. Unless we set up a rollback for the full trigger, for now I think it is better to leave the current behaviour of sending to the existing subscribers the notifications, and just skip it for non existent subscribers or failed subscribers.
Something worth to bring to tech summit to see what's the best direction we want to have for best fault tolerance. 👍🏻

ahhh ok sorry I was confused about this TODO comment, I think it's not possible to get rid of that null type in this function then because the subscriber is created in the createSubscriberUsecase.execute call when it's null...

LetItRock · 2023-02-17T12:19:54Z

apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts

+    if (!template) {
+      const message = 'Notification template could not be found';
+      Logger.error(message, LOG_CONTEXT);
+      throw new ApiException(message);
+    }


The use-case TriggerEvent is used by the TriggerHandlerQueueService and throwing that error won't work for the case when the trigger queue events are processed, because there is no REST context anymore... so I feel like it should throw some generic runtime error and we should catch and wrap it in other places... wdyt?

LetItRock · 2023-02-17T12:22:37Z

apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts

      );
+
+      // If no subscriber makes no sense to try to create notification
+      if (subscriberProcessed) {


should we then return even before we enter this for loop?

p-fernandez requested review from davidsoderberg, scopsy and djabarovgeorge February 17, 2023 11:03

p-fernandez self-assigned this Feb 17, 2023

github-actions bot added @novu/api @novu/shared providers labels Feb 17, 2023

p-fernandez force-pushed the nv-1724-feature-refactor-trigger-event branch from f0d1e05 to 0334c3e Compare February 17, 2023 11:04

p-fernandez commented Feb 17, 2023

View reviewed changes

p-fernandez force-pushed the nv-1724-feature-refactor-trigger-event branch 2 times, most recently from 478f534 to d5c03b8 Compare February 17, 2023 11:20

github-actions bot removed the providers label Feb 17, 2023

LetItRock approved these changes Feb 17, 2023

View reviewed changes

p-fernandez force-pushed the nv-1724-feature-refactor-trigger-event branch from 6a0e315 to 3fd3df4 Compare February 20, 2023 09:52

p-fernandez added 2 commits February 20, 2023 14:10

feat(api): refactor of trigger event

28b48aa

feat(api): pr suggestions

228b08c

p-fernandez force-pushed the nv-1724-feature-refactor-trigger-event branch from ba733bc to 228b08c Compare February 20, 2023 14:10

p-fernandez added this pull request to the merge queue Feb 20, 2023

Merged via the queue into next with commit 72319a1 Feb 20, 2023

p-fernandez deleted the nv-1724-feature-refactor-trigger-event branch February 20, 2023 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): refactor of trigger event #2825

feat(api): refactor of trigger event #2825

p-fernandez commented Feb 17, 2023

linear bot commented Feb 17, 2023

Why? (Context)

What?

Definition of Done

p-fernandez Feb 17, 2023

p-fernandez Feb 17, 2023

LetItRock Feb 17, 2023

p-fernandez Feb 20, 2023

p-fernandez Feb 17, 2023

p-fernandez Feb 17, 2023

p-fernandez Feb 17, 2023

LetItRock Feb 17, 2023

p-fernandez Feb 20, 2023

LetItRock Feb 20, 2023

p-fernandez Feb 20, 2023

p-fernandez Feb 17, 2023 •

edited

p-fernandez Feb 17, 2023

LetItRock Feb 17, 2023

p-fernandez Feb 20, 2023

LetItRock Feb 20, 2023

p-fernandez Feb 17, 2023 •

edited

LetItRock left a comment

LetItRock Feb 17, 2023

LetItRock Feb 17, 2023

LetItRock Feb 17, 2023

p-fernandez Feb 20, 2023

LetItRock Feb 20, 2023

LetItRock Feb 17, 2023

LetItRock Feb 17, 2023

feat(api): refactor of trigger event #2825

feat(api): refactor of trigger event #2825

Conversation

p-fernandez commented Feb 17, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Feb 17, 2023

Why? (Context)

What?

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-fernandez Feb 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-fernandez Feb 17, 2023 • edited

Choose a reason for hiding this comment

LetItRock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-fernandez Feb 17, 2023 •

edited

p-fernandez Feb 17, 2023 •

edited