feat: add bulk edit monitor notifications endpoint#3376
feat: add bulk edit monitor notifications endpoint#3376ashu130698 wants to merge 12 commits intobluewave-labs:developfrom
Conversation
ashu130698
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb2d8b8d3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| action: "add" | "remove" | "set", | ||
| teamId: string | ||
| ): Promise<number> => { | ||
| const objectIds = monitorIds.map((id) => new mongoose.Types.ObjectId(id)); |
There was a problem hiding this comment.
Converting each monitorId with new mongoose.Types.ObjectId(id) will throw when any ID is not a 24-char hex string, and this path currently bubbles into the global error handler as a 500. For requests with one typoed ID, the whole bulk update fails with a server error instead of a 400 validation error, which makes the endpoint brittle for user-provided input.
Useful? React with 👍 / 👎.
| if (!notificationIds || !Array.isArray(notificationIds) || notificationIds.length === 0) { | ||
| throw new AppError({ message: "Notification IDs are required", status: 400 }); |
There was a problem hiding this comment.
set action
The controller requires notificationIds.length > 0 for every action, which makes action: "set" unable to clear notifications from selected monitors. Monitors support having no notifications, so this blocks a valid bulk-edit scenario (replace with an empty set) and forces clients to do extra per-monitor workarounds.
Useful? React with 👍 / 👎.
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
In general, it looks decent.
We do require validation handled by Zod, please see the validation dir to see examples of other validation schemas.
Other than that some nitpicks here and there.
Thanks for the clean PR, looking forward to revisions!
| try { | ||
| const teamId = requireTeamId(req.user?.teamId); | ||
|
|
||
| const { monitorIds, notificationIds, action } = req.body; |
There was a problem hiding this comment.
Body input must be validated, see the validations directory for examples of validation
| if (!monitorIds || !Array.isArray(monitorIds) || monitorIds.length === 0) { | ||
| throw new AppError({ message: "Monitor IDs are required", status: 400 }); | ||
| } | ||
|
|
||
| if (!notificationIds || !Array.isArray(notificationIds)) { | ||
| throw new AppError({ message: "Notification IDs must be an array", status: 400 }); | ||
| } | ||
|
|
||
| if (!action || !["add", "remove", "set"].includes(action)) { | ||
| throw new AppError({ message: "Action must be 'add', 'remove', or 'set'", status: 400 }); | ||
| } |
There was a problem hiding this comment.
These will not be necessary after validation is performed
| await MonitorModel.updateMany({ notifications: notificationId }, { $pull: { notifications: notificationId } }); | ||
| }; | ||
|
|
||
| bulkUpdateNotifications = async ( |
There was a problem hiding this comment.
This can be named updateNotifications, no need to note that it is a bulk operation, the pluralization of Notification already indicates that.
| switch (action) { | ||
| case "add": | ||
| update = { $addToSet: { notifications: { $each: notificationIds } } }; | ||
| break; | ||
| case "remove": | ||
| update = { $pull: { notifications: { $in: notificationIds } } }; | ||
| break; | ||
| case "set": | ||
| update = { $set: { notifications: notificationIds } }; | ||
| break; | ||
| } |
There was a problem hiding this comment.
What happens if we fall through here? Let's be defensive and have a default case
|
Thanks for the review! I've added Joi validation, renamed to updateNotifications, and added a default case in the switch. Let me know if anything else needs changes |
…or uptime monitors
ashu130698
left a comment
ashu130698
left a comment
There was a problem hiding this comment.
Resolved conflicts and added Uptime UI
This PR should be entirely focused on the back end so we can carefully review the implementation. Adding UI should be done in a separate PR where it will be reviewed on its own merit. The larger and more complex PRs are the less likley they are to be reviewed or merged. Thanks! |
…ection for uptime monitors" This reverts commit 27dd565.
|
Thanks for the feedback! I've removed all the frontend UI code from this PR so it's strictly backend. I'll open a separate UI PR once this is merged. |
2 participants
The changes
Added a PATCH endpoint at /api/v1/notifications for bulk updating
notification channels on multiple monitors. Supports 3 actions:
add, remove, and set. Backend only - frontend will follow in a
separate PR.
Fixes #2320
npm run formatin server and client directories.