-
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
Conversation
g-saracca
left a comment
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.
…ATAVERSE_IMAGE_TAG to unstable
…m/IQSS/dataverse-client-javascript into 386-update-notifications-use-case
…m filePreview (not needed)
…itation from filePreview
…d by displayOrder bug
Mentioned the possibility of adding testing on cases where it should fail, for example, if a user is using JS Dataverse to delete a notification without being authenticated, or while being authenticated and had lack of permissions. |
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 were previously failing because due a dependency on a dataverse PR. After that PR was merged the "unstable" image should work with this PR.
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: