feat(api): search workflows by name or trigger identifier #5268

LetItRock · 2024-03-06T23:54:31Z

What change does this PR introduce?

Allow searching workflows by name or trigger identifier.

Why was this change needed?

Other information (Screenshots)

linear · 2024-03-06T23:54:34Z

NV-3229 🔍 Search workflows by trigger identifier or name

netlify · 2024-03-06T23:54:54Z

❌ Deploy Preview for dev-web-novu failed.

Name	Link
🔨 Latest commit	`e0c7b5b`
🔍 Latest deploy log	https://app.netlify.com/sites/dev-web-novu/deploys/65eb15dc4ee3fa0008b7bca0

LetItRock · 2024-03-06T23:55:13Z

apps/api/src/app/shared/dtos/pagination-request.ts

@@ -2,10 +2,15 @@ import { ApiPropertyOptional } from '@nestjs/swagger';
 import { Type } from 'class-transformer';
 import { IsInt, Max, Min } from 'class-validator';

-export type Constructor<I> = new (...args: any[]) => I;
+import { Constructor } from '../types';


moved to a shared type

LetItRock · 2024-03-06T23:56:20Z

apps/api/src/app/shared/dtos/pagination-with-filters-request.ts

+export function PaginationWithFiltersRequestDto({
+  defaultLimit = 10,
+  maxLimit = 100,
+  queryDescription,
+}: {
+  defaultLimit: number;
+  maxLimit: number;
+  queryDescription: string;
+}): Constructor<IPaginationWithFilters> {
+  class PaginationWithFiltersRequest extends PaginationRequestDto(defaultLimit, maxLimit) {
+    @ApiPropertyOptional({
+      type: String,
+      required: false,
+      description: `A query string to filter the results. ${queryDescription}`,
+    })
+    @IsOptional()
+    @IsString()
+    query?: string;
+  }
+
+  return PaginationWithFiltersRequest;
+}


extended the PaginationRequestDto class with a query field, allowing also to pass a different description

LetItRock · 2024-03-06T23:59:39Z

apps/api/src/app/subscribers/subscribers.controller.ts

@@ -485,10 +485,10 @@ export class SubscribersController {
      organizationId: user.organizationId,
      environmentId: user.environmentId,
      subscriberId: subscriberId,
-      page: query.page != null ? parseInt(query.page) : 0,
+      page: query.page != null ? parseInt(query.page as any) : 0,


When I typed the pagination interface to the correct field type, these and a few more places started to raise the TS error that the number is not assignable to a string. I didn't want to change it, like by removing parseInt because of the risk of breaking something.

are we certain that PaginationRequestDto paras are validated in the transport dto phase, if we do then I think it should be saved to remove the parsing because if it is not number @ISINT() will throw an exception.

yes, just checked it's covered with query params to dto transformation :) so the fields are integers

LetItRock · 2024-03-06T23:59:58Z

apps/api/src/app/workflows/dto/workflows-request.dto.ts

+export class WorkflowsRequestDto extends PaginationWithFiltersRequestDto({
+  defaultLimit: 10,
+  maxLimit: 100,
+  queryDescription: 'It allows filtering based on either the name or trigger identifier of the workflow items.',
+}) {}


this is how that new interface is used

LetItRock · 2024-03-07T00:01:09Z

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

+        $or: [
+          { name: { $regex: regExpEscape(query), $options: 'i' } },
+          { 'triggers.identifier': { $regex: regExpEscape(query), $options: 'i' } },
+        ],


allow searching by the name or trigger identifier case-insensitive

Sorry, i missed this part of the search implementation.
if we want to search by both name and identifier in the same query (by OR operator), as far as I know, we need to make sure we have an index for both (all cases) meaning:

user query by name, index _environmentId.name

user query by name and identifier, index _environmentId.identifier.name (covers _environmentId.identifier and _environmentId.identifier.name )

This is because if we have only _environmentId.identifier.name we could not use it for queries by name because we would be missing the identifier in the index.

Ok, I do understand what you mean. I will add it, thanks! 🙌

I see that we also have _organizationId.triggers.identifier, but I couldn't find any references in the code where we use _organizationId and identifier. Do we need it?

Also, in Atlas _environmentId_1_triggers.identifier_1 means that we just have to extend it right?

The only places i found that depend on _organizationId in their query are blueprint-related which are located in:

NotificationTemplateRepository.findBlueprintByTriggerIdentifier

NotificationTemplateRepository.getBlueprintList
we could easily refactor the code to use environmentId instead, however for now I would keep the index so we won't scan the collections until we refactor the code.

Thats right we could extend _environmentId_1_triggers.identifier_1 to _environmentId_1_triggers.identifier_1_name

djabarovgeorge

Amazing work, left couple of comment for your review :)

apps/api/src/app/shared/dtos/pagination-request.ts

apps/api/src/app/shared/dtos/pagination-with-filters-request.ts

djabarovgeorge · 2024-03-07T12:05:24Z

apps/api/src/app/subscribers/subscribers.controller.ts

@@ -485,10 +485,10 @@ export class SubscribersController {
      organizationId: user.organizationId,
      environmentId: user.environmentId,
      subscriberId: subscriberId,
-      page: query.page != null ? parseInt(query.page) : 0,
+      page: query.page != null ? parseInt(query.page as any) : 0,


are we certain that PaginationRequestDto paras are validated in the transport dto phase, if we do then I think it should be saved to remove the parsing because if it is not number @ISINT() will throw an exception.

djabarovgeorge · 2024-03-07T12:21:25Z

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

+        $or: [
+          { name: { $regex: regExpEscape(query), $options: 'i' } },
+          { 'triggers.identifier': { $regex: regExpEscape(query), $options: 'i' } },
+        ],


Sorry, i missed this part of the search implementation.
if we want to search by both name and identifier in the same query (by OR operator), as far as I know, we need to make sure we have an index for both (all cases) meaning:

user query by name, index _environmentId.name

user query by name and identifier, index _environmentId.identifier.name (covers _environmentId.identifier and _environmentId.identifier.name )

This is because if we have only _environmentId.identifier.name we could not use it for queries by name because we would be missing the identifier in the index.

djabarovgeorge

💪

LetItRock requested a review from djabarovgeorge March 6, 2024 23:54

LetItRock self-assigned this Mar 6, 2024

github-actions bot added @novu/api @novu/dal labels Mar 6, 2024

feat(api): search workflows by name or trigger identifier

f8d7c7f

LetItRock force-pushed the nv-3229-api-search-workflows branch from b50b9ae to f8d7c7f Compare March 7, 2024 00:05

LetItRock commented Mar 7, 2024

View reviewed changes

LetItRock changed the title ~~feat(api): search workflows by name or trigger identifier~~ [DRAFT] feat(api): search workflows by name or trigger identifier Mar 7, 2024

djabarovgeorge reviewed Mar 7, 2024

View reviewed changes

LetItRock requested a review from djabarovgeorge March 8, 2024 13:29

LetItRock added a commit that referenced this pull request Mar 8, 2024

chore(api,dal): added suggestions from the pr #5268

d52e108

chore(api,dal): added suggestions from the pr #5268

e0c7b5b

LetItRock force-pushed the nv-3229-api-search-workflows branch from d52e108 to e0c7b5b Compare March 8, 2024 13:42

djabarovgeorge approved these changes Mar 11, 2024

View reviewed changes

LetItRock changed the title ~~[DRAFT] feat(api): search workflows by name or trigger identifier~~ feat(api): search workflows by name or trigger identifier Mar 11, 2024

LetItRock merged commit 8cc0f69 into next Mar 11, 2024
25 of 31 checks passed

LetItRock deleted the nv-3229-api-search-workflows branch March 11, 2024 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): search workflows by name or trigger identifier #5268

feat(api): search workflows by name or trigger identifier #5268

LetItRock commented Mar 6, 2024

linear bot commented Mar 6, 2024

netlify bot commented Mar 6, 2024 •

edited

Loading

LetItRock Mar 6, 2024

LetItRock Mar 6, 2024

LetItRock Mar 6, 2024

djabarovgeorge Mar 7, 2024

LetItRock Mar 8, 2024

LetItRock Mar 6, 2024

LetItRock Mar 7, 2024

djabarovgeorge Mar 7, 2024

LetItRock Mar 8, 2024

djabarovgeorge Mar 10, 2024

djabarovgeorge left a comment

djabarovgeorge Mar 7, 2024

djabarovgeorge Mar 7, 2024

djabarovgeorge left a comment

feat(api): search workflows by name or trigger identifier #5268

feat(api): search workflows by name or trigger identifier #5268

Conversation

LetItRock commented Mar 6, 2024

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Mar 6, 2024

netlify bot commented Mar 6, 2024 • edited Loading

❌ Deploy Preview for dev-web-novu failed.

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

djabarovgeorge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djabarovgeorge left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 6, 2024 •

edited

Loading