-
Notifications
You must be signed in to change notification settings - Fork 10
Update GetAllNotificationsByUser use case #387
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving a comment about how to handle the query params but approving 👍🏼
src/notifications/infra/repositories/NotificationsRepository.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the GetAllNotificationsByUser use case to support filtering unread notifications and pagination parameters (limit and offset). The return type has been changed from a simple array to a NotificationSubset object that includes both the notifications array and a total count.
Key changes:
- Modified GetAllNotificationsByUser to accept onlyUnread, limit, and offset parameters
- Changed return type from Notification[] to NotificationSubset across the codebase
- Fixed additionalInfo field type from string to Record<string, unknown>
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/notifications/domain/models/NotificationSubset.ts | New model interface containing notifications array and totalNotificationCount |
| src/notifications/domain/useCases/GetAllNotificationsByUser.ts | Added pagination and filtering parameters to the use case |
| src/notifications/domain/repositories/INotificationsRepository.ts | Updated interface signature to match new parameters and return type |
| src/notifications/infra/repositories/NotificationsRepository.ts | Implemented query parameter building and response transformation for new features |
| src/notifications/domain/models/Notification.ts | Changed additionalInfo type from string to Record<string, unknown> |
| src/notifications/infra/transformers/NotificationPayload.ts | Updated payload interface to match Notification model change |
| test/integration/notifications/NotificationsRepository.test.ts | Updated tests to handle NotificationSubset return type and added tests for new filtering/pagination features |
| test/functional/notifications/GetAllNotificationsByUser.test.ts | Updated functional tests for new return type and parameters |
| test/functional/notifications/DeleteNotification.test.ts | Updated to work with NotificationSubset structure |
| test/integration/collections/CollectionsRepository.test.ts | Changed test file ID to avoid conflicts |
| test/environment/.env | Updated Docker image configuration for testing |
| CHANGELOG.md | Documented new features and fixes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
What this PR does / why we need it:
Updates the GetAllNotificationsByUser use case to allow results to return only unread messages. Also adds parameters for pagination (limit and offset).
Which issue(s) this PR closes:
Related Dataverse PRs:
Special notes for your reviewer:
The tests are currently failing because it is depending on a dataverse PR that hasn't been merged yet, which is slightly behind develop. When that PR is merged, the .env can be updated to use 'unstable'.
Suggestions on how to test this:
review code and tests. Note that this has a waiting tag. Should not be merged until the related Dataverse PR is merged.
NOTE: before merging, revert the .env file to use the 'unstable' dataverse image
Is there a release notes or changelog update needed for this change?:
Yes, changelog updated.
Additional documentation: