Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions apps/api/src/app/shared/dtos/pagination-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,23 @@ 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';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to a shared type


export interface IPagination {
page: number;
limit: number;
}

// eslint-disable-next-line @typescript-eslint/naming-convention
export function PaginationRequestDto(defaultLimit = 10, maxLimit = 100): Constructor<any> {
export function PaginationRequestDto(defaultLimit = 10, maxLimit = 100): Constructor<IPagination> {
LetItRock marked this conversation as resolved.
Show resolved Hide resolved
class PaginationRequest {
@ApiPropertyOptional({
type: Number,
required: false,
})
@Type(() => Number)
@IsInt()
page?: number = 0;
page = 0;

@ApiPropertyOptional({
type: Number,
Expand All @@ -25,7 +30,7 @@ export function PaginationRequestDto(defaultLimit = 10, maxLimit = 100): Constru
@IsInt()
@Min(1)
@Max(maxLimit)
limit?: number = defaultLimit;
limit = defaultLimit;
}

return PaginationRequest;
Expand Down
33 changes: 33 additions & 0 deletions apps/api/src/app/shared/dtos/pagination-with-filters-request.ts
LetItRock marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { ApiPropertyOptional } from '@nestjs/swagger';
import { IsOptional, IsString } from 'class-validator';

import { Constructor } from '../types';
import { IPagination, PaginationRequestDto } from './pagination-request';

export interface IPaginationWithFilters extends IPagination {
query?: string;
}

// eslint-disable-next-line @typescript-eslint/naming-convention
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;
}
Comment on lines +12 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

1 change: 1 addition & 0 deletions apps/api/src/app/shared/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export type Constructor<I> = new (...args: any[]) => I;
8 changes: 4 additions & 4 deletions apps/api/src/app/subscribers/subscribers.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ export class SubscribersController {
GetSubscribersCommand.create({
organizationId: user.organizationId,
environmentId: user.environmentId,
page: query.page ? Number(query.page) : 0,
limit: query.limit ? Number(query.limit) : 10,
page: query.page,
limit: query.limit,
})
);
}
Expand Down Expand Up @@ -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,
feedId: feedsQuery,
query: { seen: query.seen, read: query.read },
limit: query.limit != null ? parseInt(query.limit) : 10,
limit: query.limit,
payload: query.payload,
});

Expand Down
4 changes: 2 additions & 2 deletions apps/api/src/app/tenant/tenant.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ export class TenantController {
GetTenantsCommand.create({
organizationId: user.organizationId,
environmentId: user.environmentId,
page: query.page ? Number(query.page) : 0,
limit: query.limit ? Number(query.limit) : 10,
page: query.page,
limit: query.limit,
})
);
}
Expand Down
4 changes: 2 additions & 2 deletions apps/api/src/app/widgets/widgets.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,10 @@ export class WidgetsController {
organizationId: subscriberSession._organizationId,
subscriberId: subscriberSession.subscriberId,
environmentId: subscriberSession._environmentId,
page: query.page != null ? parseInt(query.page) : 0,
page: query.page,
feedId: feedsQuery,
query: { seen: query.seen, read: query.read },
limit: query.limit != null ? parseInt(query.limit) : 10,
limit: query.limit,
payload: query.payload,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,8 @@ export class WorkflowOverridesController {
organizationId: user.organizationId,
environmentId: user.environmentId,
userId: user._id,
page: query.page ? query.page : 0,
limit: query.limit ? query.limit : 10,
page: query.page,
limit: query.limit,
})
);
}
Expand Down
8 changes: 6 additions & 2 deletions apps/api/src/app/workflows/dto/workflows-request.dto.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import { PaginationRequestDto } from '../../shared/dtos/pagination-request';
import { PaginationWithFiltersRequestDto } from '../../shared/dtos/pagination-with-filters-request';

export class WorkflowsRequestDto extends PaginationRequestDto(10, 100) {}
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.',
}) {}
104 changes: 104 additions & 0 deletions apps/api/src/app/workflows/e2e/get-notification-templates.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
FilterPartTypeEnum,
StepTypeEnum,
TemplateVariableTypeEnum,
TriggerTypeEnum,
} from '@novu/shared';

describe('Get workflows - /workflows (GET)', async () => {
Expand Down Expand Up @@ -123,4 +124,107 @@ describe('Get workflows - /workflows (GET)', async () => {
expect(page1Limit3Results.pageSize).to.equal(3);
expect(page1Limit3Results.data[2]._id).to.equal(templates[0]._id);
});

it('should paginate and filter workflows based on the name', async () => {
const promises: Promise<NotificationTemplateEntity>[] = [];
const count = 10;
for (let i = 0; i < count; i++) {
promises.push(
notificationTemplateService.createTemplate({
name: `Pagination Test ${i}`,
})
);
}
await Promise.all(promises);

const { body } = await session.testAgent.get(`/v1/workflows?page=0&limit=2&query=Pagination+Test`);

expect(body.data.length).to.equal(2);
expect(body.totalCount).to.equal(count);
expect(body.page).to.equal(0);
expect(body.pageSize).to.equal(2);
for (let i = 0; i < 2; i++) {
expect(body.data[i].name).to.contain('Pagination Test');
}
});

it('should filter workflows based on the name', async () => {
const promises: Promise<NotificationTemplateEntity>[] = [];
const count = 10;
for (let i = 0; i < count; i++) {
promises.push(
notificationTemplateService.createTemplate({
name: `Test Template ${i}`,
})
);
}
await Promise.all(promises);

const { body } = await session.testAgent.get(`/v1/workflows?page=0&limit=100&query=Test+Template`);

expect(body.data.length).to.equal(count);
expect(body.totalCount).to.equal(count);
expect(body.page).to.equal(0);
expect(body.pageSize).to.equal(100);
for (let i = 0; i < count; i++) {
expect(body.data[i].name).to.contain('Test Template');
}
});

it('should filter workflows based on the trigger identifier', async () => {
const promises: Promise<NotificationTemplateEntity>[] = [];
const count = 10;
const triggerIdentifier = 'test-trigger-identifier';
for (let i = 0; i < count; i++) {
promises.push(
notificationTemplateService.createTemplate({
triggers: [{ identifier: `${triggerIdentifier}-${i}`, type: TriggerTypeEnum.EVENT, variables: [] }],
})
);
}
await Promise.all(promises);

const { body } = await session.testAgent.get(`/v1/workflows?page=0&limit=100&query=${triggerIdentifier}`);

expect(body.data.length).to.equal(count);
expect(body.totalCount).to.equal(count);
expect(body.page).to.equal(0);
expect(body.pageSize).to.equal(100);
for (let i = 0; i < count; i++) {
expect(body.data[i].triggers[0].identifier).to.contain(`${triggerIdentifier}`);
}
});

it('should filter workflows based on both the name and trigger identifier', async () => {
const promises: Promise<NotificationTemplateEntity>[] = [];
const count = 10;
for (let i = 0; i < count; i++) {
if (i % 2 === 0) {
promises.push(
notificationTemplateService.createTemplate({
name: Math.random() > 0.5 ? `SMS ${i}` : `sms ${i}`,
})
);
continue;
}

promises.push(
notificationTemplateService.createTemplate({
triggers: [{ identifier: `sms-trigger-${i}`, type: TriggerTypeEnum.EVENT, variables: [] }],
})
);
}
await Promise.all(promises);

const { body } = await session.testAgent.get(`/v1/workflows?page=0&limit=100&query=sms`);
const nameCount = body.data.filter((i) => i.name.toUpperCase().includes('SMS')).length;
const triggerCount = body.data.filter((i) => i.triggers[0].identifier.includes('sms')).length;

expect(body.data.length).to.equal(count);
expect(body.totalCount).to.equal(count);
expect(body.page).to.equal(0);
expect(body.pageSize).to.equal(100);
expect(nameCount).to.equal(5);
expect(triggerCount).to.equal(5);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,16 @@ export class NotificationTemplateController {
@ExternalApiAccessible()
getNotificationTemplates(
@UserSession() user: IJwtPayload,
@Query() query: WorkflowsRequestDto
@Query() queryParams: WorkflowsRequestDto
): Promise<WorkflowsResponseDto> {
return this.getNotificationTemplatesUsecase.execute(
GetNotificationTemplatesCommand.create({
organizationId: user.organizationId,
userId: user._id,
environmentId: user.environmentId,
page: query.page ? query.page : 0,
limit: query.limit ? query.limit : 10,
page: queryParams.page,
limit: queryParams.limit,
query: queryParams.query,
})
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { IsNumber } from 'class-validator';
import { IsNumber, IsOptional, IsString } from 'class-validator';

import { EnvironmentWithUserCommand } from '../../../shared/commands/project.command';

Expand All @@ -13,4 +13,8 @@ export class GetNotificationTemplatesCommand extends EnvironmentWithUserCommand

@IsNumber()
limit: number;

@IsOptional()
@IsString()
query?: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ export class GetNotificationTemplates {
command.organizationId,
command.environmentId,
command.page * command.limit,
command.limit
command.limit,
command.query
);

const workflows = await this.updateHasActiveIntegrationFlag(list, command);
Expand Down
10 changes: 7 additions & 3 deletions apps/api/src/app/workflows/workflow.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,18 @@ export class WorkflowController {
description: `Workflows were previously named notification templates`,
})
@ExternalApiAccessible()
getWorkflows(@UserSession() user: IJwtPayload, @Query() query: WorkflowsRequestDto): Promise<WorkflowsResponseDto> {
getWorkflows(
@UserSession() user: IJwtPayload,
@Query() queryParams: WorkflowsRequestDto
): Promise<WorkflowsResponseDto> {
return this.getWorkflowsUsecase.execute(
GetNotificationTemplatesCommand.create({
organizationId: user.organizationId,
userId: user._id,
environmentId: user.environmentId,
page: query.page ? query.page : 0,
limit: query.limit ? query.limit : 10,
page: queryParams.page,
limit: queryParams.limit,
query: queryParams.query,
})
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,15 +165,31 @@ export class NotificationTemplateRepository extends BaseRepository<
return { totalCount: totalItemsCount, data: this.mapEntities(items) };
}

async getList(organizationId: string, environmentId: string, skip = 0, limit = 10) {
const totalItemsCount = await this.count({ _environmentId: environmentId });
async getList(organizationId: string, environmentId: string, skip = 0, limit = 10, query?: string) {
let searchQuery: FilterQuery<NotificationTemplateDBModel> = {};
if (query) {
searchQuery = {
$or: [
{ name: { $regex: regExpEscape(query), $options: 'i' } },
{ 'triggers.identifier': { $regex: regExpEscape(query), $options: 'i' } },
],
Comment on lines +172 to +175
Copy link
Contributor Author

Choose a reason for hiding this comment

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

allow searching by the name or trigger identifier case-insensitive

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

};
}

const totalItemsCount = await this.count({
_environmentId: environmentId,
...searchQuery,
});

const requestQuery: NotificationTemplateQuery = {
_environmentId: environmentId,
_organizationId: organizationId,
};

const items = await this.MongooseModel.find(requestQuery)
const items = await this.MongooseModel.find({
...requestQuery,
...searchQuery,
})
.sort({ createdAt: -1 })
.skip(skip)
.limit(limit)
Expand Down Expand Up @@ -218,3 +234,7 @@ export class NotificationTemplateRepository extends BaseRepository<
return process.env.BLUEPRINT_CREATOR;
}
}

function regExpEscape(literalString: string): string {
return literalString.replace(/[-[\]{}()*+!<=:?./\\^$|#\s,]/g, '\\$&');
}
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,17 @@ notificationTemplateSchema.index({
'triggers.identifier': 1,
});

notificationTemplateSchema.index({
_environmentId: 1,
name: 1,
});

notificationTemplateSchema.index({
_environmentId: 1,
'triggers.identifier': 1,
name: 1,
});

notificationTemplateSchema.plugin(mongooseDelete, { deletedAt: true, deletedBy: true, overrideMethods: 'all' });

// eslint-disable-next-line @typescript-eslint/naming-convention
Expand Down
Loading