refactor: use conditions filter #4195

djabarovgeorge · 2023-09-24T10:48:31Z

What change does this PR introduce?

deprecated the use of MessageMatcher usecase and switch to ConditionsFilter usecase

Why was this change needed?

In the past we used MessageMatcher in the worked however today we ConditionsFilter in application-generic, initially made for integration but today the code is pretty much the same and they bear the same responsibility.

IMO there is no reason to have them both.

Other information (Screenshots)

linear · 2023-09-24T10:48:33Z

NV-2868 deprecated the use of MessageMatcher use-case, and switch to ConditionsFilter use-case

What?

deprecated the use of MessageMatcher usecase and switch to ConditionsFilter usecase

Why? (Context)

In the past we used MessageMatcher in the worked however today we ConditionsFilter in application-generic, initially made for integration but today the code is pretty much the same and they bear the same responsibility.

IMO there is no reason to have them both.

Definition of Done

djabarovgeorge · 2023-09-24T10:49:29Z

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

Now we have the execution detail of the filter action.

djabarovgeorge · 2023-09-24T10:59:24Z

.../app/workflows/usecases/create-notification-template/create-notification-template.command.ts

On create workflow we use Partial where we have IMessageFilter with optional isNegated and type therefore I aligned those types to be optional as well.

djabarovgeorge · 2023-09-24T11:01:26Z

apps/worker/src/app/workflow/usecases/message-matcher/message-matcher.usecase.spec.ts

refactored to use Conflict Filter use case, we still need to change this spec name and delete the MessageMatcher use-case.

This spec will remain in the worker project because we still do not run the application generic tests.

djabarovgeorge · 2023-09-24T11:03:27Z

libs/dal/src/repositories/notification-template/notification-template.entity.ts

Same as on the comment above:

On create workflow we use Partial where we have IMessageFilter with optional isNegated and type therefore I aligned those types to be optional as well.

djabarovgeorge · 2023-09-24T11:04:54Z

packages/application-generic/src/usecases/conditions-filter/conditions-filter.command.ts

 export class ConditionsFilterCommand extends EnvironmentWithUserCommand {
  @IsDefined()
  filters: StepFilter[];

  job?: IJob;

  step?: INotificationTemplateStep;
+
+  variables?: IFilterVariables;


We allow the passing of the data if exists. if not, we will fetch it in the use case if needed.

djabarovgeorge · 2023-09-24T11:06:23Z

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts

+    const filters = command.filters?.length
+      ? command.filters
+      : command.step?.filters?.length
+      ? command.step.filters
+      : [];


Not all of the clients have the job entity with filters (like select integrations), therefore we allow passing filters separately with priority.

djabarovgeorge · 2023-09-24T11:07:49Z

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts

@@ -214,9 +244,8 @@ export class ConditionsFilter extends Filter {
    command: ConditionsFilterCommand,
    filterProcessingDetails: FilterProcessingDetails
  ): Promise<boolean> {
-    const subscriber = await this.subscriberRepository.findOne({
-      _id: command.job.subscriberId,


IMO this one is a bug, because Subscriber._id is not equal to job.subscriberId.

djabarovgeorge · 2023-09-24T11:11:41Z

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts

    };
  }

+  public static sumFilters(


copied from message matcher for our use.

djabarovgeorge · 2023-09-24T11:12:36Z

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts

+    const { webhookFilters, otherFilters } = this.splitFilters(filter);
+
+    const matchedOtherFilters = await this.filterAsync(otherFilters, (i) =>
      this.processFilter(variables, i, command, filterProcessingDetails)
    );
+    if (otherFilters.length !== matchedOtherFilters.length) {
+      return false;
+    }
+
+    const matchedWebhookFilters = await this.filterAsync(webhookFilters, (i) =>
+      this.processFilter(variables, i, command, filterProcessingDetails)
+    );
+
+    return matchedWebhookFilters.length === webhookFilters.length;
+  }

-    return filter.children.length === matchedOtherFilters.length;
+  private splitFilters(filter: StepFilter) {
+    const webhookFilters = filter.children.filter(
+      (childFilter) => childFilter.on === 'webhook'
+    );
+
+    const otherFilters = filter.children.filter(
+      (childFilter) => childFilter.on !== 'webhook'
+    );
+
+    return { webhookFilters, otherFilters };


was missing from the message matcher usecase

djabarovgeorge · 2023-09-24T11:13:37Z

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts

+  }
+
+  @Instrument()
+  private async normalizeVariablesData(command: ConditionsFilterCommand) {


here we fetch the variable is they are needed and missing from the command

djabarovgeorge · 2023-09-24T11:14:23Z

packages/application-generic/src/usecases/select-integration/select-integration.usecase.ts

refactor to use the updated command

djabarovgeorge · 2023-09-24T11:14:30Z

packages/application-generic/src/usecases/select-variant/select-variant.usecase.ts

refactor to use the updated command

djabarovgeorge · 2023-09-26T09:40:38Z

apps/worker/src/app/workflow/specs/conditions-filter.usecase.spec.ts

was not sure how to call this folder 😅

davidsoderberg · 2023-09-26T15:46:59Z

apps/worker/src/app/workflow/usecases/message-matcher/message-matcher.usecase.ts

-        if (singleRule) {
-          const result = await this.processFilter(variables, children[0], command, filterProcessingDetails);
-          if (!prefiltering) {
-            await this.createExecutionDetails.execute(


I can not find this detail somewhere in the new code.

Are you referring to the createExecutionDetails execution or if (singleRule) block?

to the createExecutionDetails

we will still log the execution detail in send-message.usecase.ts if shouldRun.passed is false, and we won't print in queue-next-job.usecase.ts the same as it was before because of the flag. or did u miss something?

for the moment i remove the responsibility of the createExecutionDetails execution from the filter, do you think we should return it?

@davidsoderberg

But the one of this type is not used anymore?

Not sure I understood what you mean here

@davidsoderberg i will merge this one for now, please let me know if i missed something here 🙏

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts

…her-and-switch-to-conditions-filter # Conflicts: # apps/worker/src/app/workflow/usecases/message-matcher/message-matcher.usecase.ts

refactor: use conditions filter

bd86ef6

djabarovgeorge marked this pull request as ready for review September 24, 2023 10:48

github-actions bot added @novu/api @novu/dal @novu/worker labels Sep 24, 2023

djabarovgeorge commented Sep 24, 2023

View reviewed changes

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

Copy link

Contributor Author

djabarovgeorge Sep 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have the execution detail of the filter action.

djabarovgeorge commented Sep 24, 2023

View reviewed changes

djabarovgeorge added 2 commits September 24, 2023 14:18

fix: spec build

befcfa9

feat: delete old use case

ab3a32f

djabarovgeorge commented Sep 26, 2023

View reviewed changes

davidsoderberg reviewed Sep 26, 2023

View reviewed changes

ainouzgali reviewed Oct 1, 2023

View reviewed changes

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts Outdated Show resolved Hide resolved

packages/application-generic/src/usecases/conditions-filter/conditions-filter.usecase.ts Outdated Show resolved Hide resolved

Base automatically changed from vatiants-polishing to variants-condition October 2, 2023 08:51

djabarovgeorge added 2 commits October 2, 2023 20:38

feat: update after pr comments

5ca053f

Merge branch 'variants-condition' into NV-2868-deprecated-messagematc…

952fae9

…her-and-switch-to-conditions-filter # Conflicts: # apps/worker/src/app/workflow/usecases/message-matcher/message-matcher.usecase.ts

djabarovgeorge merged commit 34107fa into variants-condition Oct 3, 2023
26 checks passed

djabarovgeorge deleted the NV-2868-deprecated-messagematcher-and-switch-to-conditions-filter branch October 3, 2023 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use conditions filter #4195

refactor: use conditions filter #4195

djabarovgeorge commented Sep 24, 2023

linear bot commented Sep 24, 2023

What?

Why? (Context)

Definition of Done

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 24, 2023

djabarovgeorge Sep 26, 2023

davidsoderberg Sep 26, 2023

djabarovgeorge Sep 27, 2023

davidsoderberg Sep 28, 2023

djabarovgeorge Sep 28, 2023

davidsoderberg Sep 29, 2023

djabarovgeorge Oct 1, 2023

djabarovgeorge Oct 3, 2023

refactor: use conditions filter #4195

refactor: use conditions filter #4195

Conversation

djabarovgeorge commented Sep 24, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Sep 24, 2023

What?

Why? (Context)

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

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