Add notification group endpoints #3274
Conversation
peoray
changed the title
|
@p-fernandez please review :) |
|
I'm glad someone is working on this again! I'm no able to review your PR because I'm not familiar enough with the codebase, but I'm sure someone from the Novu team will eventually takes care of it |
.../notification-groups/usecases/update-notification-group/update-notification-group.usecase.ts
Outdated
Show resolved
Hide resolved
.../notification-groups/usecases/update-notification-group/update-notification-group.usecase.ts
Outdated
Show resolved
Hide resolved
| const result = await this.notificationGroupRepository.findOne({ | ||
| _environmentId: environmentId, | ||
| _id: id, | ||
| }); | ||
|
|
||
| if (!result) { | ||
| throw new NotFoundException(`Notification group with id ${id} not found`); | ||
| } |
There was a problem hiding this comment.
Instead of doing this you can check inside the NotificationGroupRepository if the response from the update method returns that an entry has been changed and manage the logic accordingly.
Also it feels a bit weird that you check again if the group exists after updating it when you have already checked it before updating it. 🤔
There was a problem hiding this comment.
I need to return an object of the update code as per the NotificationGroupResponseDto and the update method return an object of matched and modified
apps/api/src/app/notification-groups/e2e/patch-notification-group.e2e.ts
Outdated
Show resolved
Hide resolved
apps/api/src/app/notification-groups/e2e/patch-notification-group.e2e.ts
Outdated
Show resolved
Hide resolved
apps/api/src/app/notification-groups/e2e/get-notification-group.e2e.ts
Outdated
Show resolved
Hide resolved
apps/api/src/app/notification-groups/e2e/delete-notification-group.e2e.ts
Outdated
Show resolved
Hide resolved
| @ApiProperty({ | ||
| description: 'The status enum for the performed action', | ||
| enum: ['deleted'], | ||
| }) | ||
| @IsString() | ||
| @IsDefined() | ||
| status: string; |
There was a problem hiding this comment.
What's the reason behind adding this status to the API response of a call that from the method we know it is a delete call? Did you have something in mind that in the future we could benefit from having a status?
There was a problem hiding this comment.
I got this from the issue, @scopsy mentioned it somewhere. Also, we have similar in the remove-subscribe usecase
There was a problem hiding this comment.
I am not particularly fan of returning 200 status for successful delete calls as I consider it complicates consumer's logic besides not adding extra useful information. As it is an implementation detail requested I will discuss it in the core team. 🙂
There was a problem hiding this comment.
Alright then.
Unrelated, but I admired your eye for detail and thorough reviews, especially for the PRs I have raised so far. It helps me to understand the codebase better and overall, a skill I want to emulate.
What change does this PR introduce?
Related to #1321
Why was this change needed?
Other information (Screenshots)