Nv 2405 workflows crud controller #3745

BiswaViraj · 2023-07-07T17:42:45Z

What change does this PR introduce?

This is the base branch for the new workflows controller.

Note: All the stacked PRs have been merged here, so most probably it doesn't need much of a review as each stacked PR was reviewed.

Why was this change needed?

Other information (Screenshots)

Stacked PRs merged into this base branch:

…nd commands as deprecated

linear · 2023-07-07T17:42:47Z

NV-2405 Workflows CRUD Controller

What?

Reusing the current "templates" usecases created a "workflows" controller that will have the exact same functionality of the existing one.

Why? (Context)

To finalize our "workflows" terminology migration from 0.16.0 version.

Definition of Done

A new workflows controller with all existing templates functionality is available
All existing tests are working against it
Old templates controller and methods are marked as "Deprecated"
A deprecation label is visible on our API reference documentation.

github-actions · 2023-07-07T17:43:03Z

Pull reviewers stats

Stats of the last 30 days for novu:

User	Total reviews	Time to review	Total comments
p-fernandez	60	18h 28m	168
LetItRock	21	1d 8h 3m	75
djabarovgeorge	15	2d 9h 51m	53
scopsy	13	47m	3
unicodeveloper	12	7d 8h 42m	123
ainouzgali	12	9h 25m	47
jainpawan21	5	3h 16m	2
davidsoderberg	4	16h 18m	2
BiswaViraj	4	1d 22h 30m	2
peoray	1	27m	1
michaldziuba03	1	10h 53m	1
Cliftonz	1	12h 3m	3

p-fernandez

Sorry I missed some filenames mainly. 🙏🏻

I leave it approved anyway so you can merge it whenever.
Many thanks for doing this tedious work! 🌟

apps/api/src/app/workflows/dto/change-template-status-request.dto.ts

apps/api/src/app/workflows/dto/create-notification-template.request.dto.ts

p-fernandez · 2023-07-07T20:31:32Z

apps/api/src/app/workflows/dto/create-notification-template.request.dto.ts

+import { PreferenceChannels } from '../../shared/dtos/preference-channels';
+import { NotificationStep } from '../../shared/dtos/notification-step';
+
+export class CreateWorkflowRequestDto implements ICreateNotificationTemplateDto {


Suggested change

export class CreateWorkflowRequestDto implements ICreateNotificationTemplateDto {

export class CreateWorkflowRequestDto implements ICreateWorkflowDto {

We would need to rename the implemented interface. You might need to duplicate it to not affect the deprecated API or just change the implemented DTO also for the deprecated API.

apps/api/src/app/workflows/dto/create-notification-template.request.dto.ts

apps/api/src/app/workflows/dto/index.ts

apps/api/src/app/workflows/dto/update-notification-template-request.dto.ts

p-fernandez · 2023-07-07T20:35:04Z

apps/api/src/app/workflows/e2e/get-workflow.e2e.ts

@@ -0,0 +1,32 @@
+import { expect } from 'chai';
+import { NotificationTemplateService, UserSession } from '@novu/testing';


Not for this one but might need to change the DAL service name too in the near future for consistency. Unfortunately schema will might have to remain.

p-fernandez · 2023-07-07T20:35:22Z

apps/api/src/app/workflows/queries/CreateNotificationTemplateQuery.ts

Rename to workflow is needed. 🙏🏻

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

ainouzgali

Great job @BiswaViraj ! Just notice the file names changes Pablo pointed out.

ainouzgali · 2023-07-09T10:07:53Z

apps/api/src/app/workflows/dto/create-notification-template.request.dto.ts

+  @ApiProperty()
+  @IsString()
+  @IsDefined({
+    message: 'Notification group must be provided ',


Suggested change

message: 'Notification group must be provided ',

message: 'Workflow group must be provided ',

We are also changing notification group => workflow group

@ainouzgali Should we also create a new controller for it with usecases renamed to the new one?

I think we should, so we have all outside refactor together. But maybe in a separate pr would feel more tidy

...pp/workflows/usecases/change-workflow-active-status/change-workflow-active-status.usecase.ts

apps/api/src/app/workflows/usecases/create-workflow/create-workflow.usecase.ts

ainouzgali · 2023-07-09T10:20:46Z

apps/api/src/app/workflows/usecases/update-workflow/update-workflow.usecase.ts

+
+  async execute(command: UpdateWorkflowCommand): Promise<NotificationTemplateEntity> {
+    const existingTemplate = await this.notificationTemplateRepository.findById(command.id, command.environmentId);
+    if (!existingTemplate) throw new NotFoundException(`Notification template with id ${command.id} not found`);


Suggested change

if (!existingTemplate) throw new NotFoundException(`Notification template with id ${command.id} not found`);

if (!existingTemplate) throw new NotFoundException(`Workflow with id ${command.id} not found`);

apps/api/src/app/workflows/usecases/update-workflow/update-workflow.usecase.ts

ainouzgali · 2023-07-09T10:23:11Z

apps/api/src/app/workflows/usecases/update-workflow/update-workflow.usecase.ts

+      command.environmentId
+    );
+    if (!notificationTemplateWithStepTemplate) {
+      throw new NotFoundException(`Notification template ${command.id} is not found`);


Suggested change

throw new NotFoundException(`Notification template ${command.id} is not found`);

throw new NotFoundException(`Workflow with id ${command.id} not found`);

ainouzgali · 2023-07-09T10:30:56Z

apps/api/src/app/notification-template/notification-template.controller.ts

@@ -43,7 +43,7 @@ import { CreateNotificationTemplateQuery } from './queries';
 @Controller('/notification-templates')
 @UseInterceptors(ClassSerializerInterceptor)
 @UseGuards(JwtAuthGuard)
-@ApiTags('Workflows')
+@ApiTags('Notification Templates')


Do we want to still show it in our api docs? Because 'Notification Templates' tag should be added to bootstrap. It currently shows Workflows

Maybe we can show it for a few releases and then remove it later

refactor: rename dtos to workflow, and other minor changes

BiswaViraj added 7 commits July 7, 2023 23:09

feat: added new workflows controller

5a3e502

feat: renamed the usecases/commands

9c47c77

refactor: updated test cases for the workflow controller

baf45cc

fix: tests

fb0465f

fix: tests

5a885e1

refactor: marked the old notification-temlates controller, usecases a…

24c841a

…nd commands as deprecated

refactor: update swagger docs for notification templates

77bfab7

github-actions bot added the @novu/api label Jul 7, 2023

BiswaViraj requested review from ainouzgali and p-fernandez July 7, 2023 17:58

p-fernandez approved these changes Jul 7, 2023

View reviewed changes

ainouzgali approved these changes Jul 9, 2023

View reviewed changes

BiswaViraj added 2 commits July 10, 2023 18:47

refactor: rename dtos to workflow, and other minor changes

825daff

fix: typo

e9bdec2

BiswaViraj mentioned this pull request Jul 10, 2023

refactor: rename dtos to workflow, and other minor changes #3761

Merged

BiswaViraj added 3 commits July 11, 2023 14:59

fix: rename file

d0a2b57

fix: rename file

08efdc7

Merge pull request #3761 from novuhq/nv-2556-small-refactors-after-pr

74cea26

refactor: rename dtos to workflow, and other minor changes

github-actions bot added @novu/web @novu/dal @novu/shared labels Jul 11, 2023

Merge branch 'next' into nv-2405-workflows-crud-controller

5f3a309

BiswaViraj added this pull request to the merge queue Jul 11, 2023

Merged via the queue into next with commit 5bbd72a Jul 11, 2023
30 checks passed

BiswaViraj deleted the nv-2405-workflows-crud-controller branch July 11, 2023 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nv 2405 workflows crud controller #3745

Nv 2405 workflows crud controller #3745

BiswaViraj commented Jul 7, 2023 •

edited

linear bot commented Jul 7, 2023

What?

Why? (Context)

Definition of Done

github-actions bot commented Jul 7, 2023

p-fernandez left a comment

p-fernandez Jul 7, 2023

p-fernandez Jul 7, 2023

p-fernandez Jul 7, 2023

ainouzgali left a comment

ainouzgali Jul 9, 2023

BiswaViraj Jul 10, 2023

ainouzgali Jul 10, 2023

ainouzgali Jul 9, 2023

ainouzgali Jul 9, 2023

ainouzgali Jul 9, 2023

BiswaViraj Jul 10, 2023

	export class CreateWorkflowRequestDto implements ICreateNotificationTemplateDto {
	export class CreateWorkflowRequestDto implements ICreateWorkflowDto {

		@@ -0,0 +1,32 @@
		import { expect } from 'chai';
		import { NotificationTemplateService, UserSession } from '@novu/testing';

	message: 'Notification group must be provided ',
	message: 'Workflow group must be provided ',

	if (!existingTemplate) throw new NotFoundException(`Notification template with id ${command.id} not found`);
	if (!existingTemplate) throw new NotFoundException(`Workflow with id ${command.id} not found`);

	throw new NotFoundException(`Notification template ${command.id} is not found`);
	throw new NotFoundException(`Workflow with id ${command.id} not found`);

Nv 2405 workflows crud controller #3745

Nv 2405 workflows crud controller #3745

Conversation

BiswaViraj commented Jul 7, 2023 • edited

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

Stacked PRs merged into this base branch:

linear bot commented Jul 7, 2023

What?

Why? (Context)

Definition of Done

github-actions bot commented Jul 7, 2023

Pull reviewers stats

p-fernandez 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

ainouzgali 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

BiswaViraj commented Jul 7, 2023 •

edited