[CAP-311] FIX: (BE) Notification Service MarkAllRead Hotfix#203
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the markAllAsRead functionality to filter notifications based on the user's role, ensuring that only relevant notifications are marked as read. The change adds role-based filtering to match the pattern used in other notification methods like findAll and getCount.
Key changes:
- Added
roleparameter to themarkAllAsReadmethod in both the controller and service - Implemented WHERE clause filtering to fetch only notifications relevant to the user's role
- Updated JSDoc documentation to reflect the new parameter
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/src/modules/notifications/notifications.service.ts | Added role parameter and role-based filtering logic to markAllAsRead method |
| backend/src/modules/notifications/notifications.controller.ts | Extracted role from user metadata and passed it to the service method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request updates the notifications system to ensure that when marking all notifications as read, it considers both the user's ID and their role. The main change is that notifications relevant to a user's role are now included, improving accuracy and relevance for users with different roles.
Notifications filtering improvements:
markAllAsReadmethod inNotificationsServicenow accepts bothuserIdandrole, and filters notifications to include those either directly addressed to the user or relevant to their role.markAllAsReadinNotificationsControllernow passes bothuser_idandroleto the service, ensuring role-based notification filtering.Documentation and method signature updates:
markAllAsReadto include the newroleparameter.