-
Notifications
You must be signed in to change notification settings - Fork 10
Use Cases for Get Notifications and Delete Notifications #335
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
Use Cases for Get Notifications and Delete Notifications #335
Conversation
ekraffmiller
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.
looks good! I just have some comments about naming.
|
Hi Ellen @ekraffmiller , thanks for review! I made changes based on suggestion. |
Hi @ChengShi-1, yes that sounds good 👍 |
|
@ekraffmiller right, we need this. I just create an issue to dataverse repo for it. IQSS/dataverse#11650 |
| import { Notification } from '../models/Notification' | ||
|
|
||
| export interface INotificationsRepository { | ||
| getAllNotificationsByUser(): Promise<Notification[]> |
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.
A pagination feature would be nice:)
|
Waiting for version of the notifications API, IQSS/dataverse#11673 |
…s-and-delete-notifications
ekraffmiller
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.
Hi @ChengShi-1 it looks good! just some comments about naming conventions, and a question about the integration test.
| import { UseCase } from '../../../core/domain/useCases/UseCase' | ||
| import { INotificationsRepository } from '../repositories/INotificationsRepository' | ||
|
|
||
| export class GetUnreadCount implements UseCase<number> { |
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.
to follow the naming convention we have in other use cases, change to GetUnreadNotificationsCount?
| expect(assignRoleNotification?.type).toBe(NotificationType.ASSIGNROLE) | ||
| expect(assignRoleNotification?.sentTimestamp).toBeDefined() | ||
| expect(assignRoleNotification?.displayAsRead).toBeDefined() | ||
| expect(assignRoleNotification?.dataverseDisplayName).toBeDefined() |
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 getting flaky behavior when running this locally - the test fails even though it succeeds on Github. Can you try to reproduce that? If needed, you could avoid relying on the existence of a notification by creating one before running the test.
FAIL test/integration/notifications/NotificationsRepository.test.ts (9.565 s)
RUNS test/integration/files/DirectUpload.test.ts
RUNS test/integration/notifications/NotificationsRepository.test.ts
RUNS test/integration/datasets/DatasetsRepository.test.ts
RUNS test/integration/collections/CollectionsRepository.test.ts
RUNS test/integration/files/FilesRepository.test.ts
● NotificationsRepository › should find notification with ASSIGNROLE type
expect(received).toBeDefined()
Received: undefined
101 | expect(assignRoleNotification?.sentTimestamp).toBeDefined()
102 | expect(assignRoleNotification?.displayAsRead).toBeDefined()
> 103 | expect(assignRoleNotification?.dataverseDisplayName).toBeDefined()
| ^
104 |
105 | expect(assignRoleNotification?.roleAssignments).toBeDefined()
106 | expect(assignRoleNotification?.roleAssignments?.length).toBeGreaterThan(0)
at test/integration/notifications/NotificationsRepository.test.ts:103:58
at fulfilled (test/integration/notifications/NotificationsRepository.test.ts:5:58)
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.
Thanks for catching this! The notifications.find(...) gave a deleted object there as the 1st eligible element,
assignRoleNotification { id: 139, type: 'ASSIGNROLE', displayAsRead: false, sentTimestamp: '2025-08-22T13:28:34Z', objectDeleted: true }
so I add a line to filter out deleted objects there
ekraffmiller
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.
looks good, approved!
|
tests passing, merging |
What this PR does / why we need it:
Inspired by 2025 Q3
Referring to native api
Get All Notifications by User
Delete Notification by User
Set displayAsRead
https://github.com/IQSS/dataverse/pull/11664/files
Which issue(s) this PR closes:
Related Dataverse PRs:
IQSS/dataverse#11696
Special notes for your reviewer:
Test env. variables should be changed back after IQSS/dataverse#11696 is merged
Suggestions on how to test this:
Is there a release notes update needed for this change?:
Additional documentation: