Skip to content
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

[BUG] Similar notifications get scheduled #456

Closed
mpgxvii opened this issue Mar 26, 2024 · 3 comments
Closed

[BUG] Similar notifications get scheduled #456

mpgxvii opened this issue Mar 26, 2024 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@mpgxvii
Copy link
Member

mpgxvii commented Mar 26, 2024

Describe the bug
Previously, when multiple similar notifications are scheduled, only one of them gets accepted and the app server throws a NotificationAlreadyExistsException for the rest of them. The check for this is below:

Optional<Notification> notification = notificationRepository
.findByUserIdAndSourceIdAndScheduledTimeAndTitleAndBodyAndTypeAndTtlSeconds(
user.getId(),
notificationDto.getSourceId(),
notificationDto.getScheduledTime(),
notificationDto.getTitle(),
notificationDto.getBody(),
notificationDto.getType(),
notificationDto.getTtlSeconds());

Thus when multiple notifications of the same title, body, and timestamp are scheduled, only one of them is sent. However, this might have worked previously because the type property was incorrectly set when sent from the Questionnaire app. However, when the type is set correctly (the type is set as the questionnaire name), even when the notification has the same title, body, and timestamp, but the questionnaire name is unique, this still gets scheduled.

Priority
A value between 1-10 signifying the Priority that you think this bug should be dealt with. 1 denotes highest priority.
6

Difficulty
A value between 1-10 signifying the amount of difficulty/work/time that you think will be involved in fixing this bug. 10 denotes highest difficulty
3

To Reproduce
Schedule multiple questionnaires to be valid at the same time (e.g. all PHQ8, RSES questionnaires at 9am everyday).

Expected behavior
Only one of the same type of notification should be shown (same title, body, and timestamp).

Additional context
Add any other context about the problem here.

@mpgxvii mpgxvii added the bug Something isn't working label Mar 26, 2024
@mpgxvii mpgxvii self-assigned this Mar 26, 2024
@mpgxvii
Copy link
Member Author

mpgxvii commented Mar 26, 2024

@yatharthranjan Can we just remove the check for type here: https://github.com/RADAR-base/RADAR-Appserver/blob/master/src/main/java/org/radarbase/appserver/service/FcmNotificationService.java#L174 and only check title, body, timestamp, ttl?

@yatharthranjan
Copy link
Member

I would have thought that is a required feature - i.e to have multiple notifications for different questionnaires? But i see the point that if the title and body is same, then does it add any value?

I think this is related to the previous conversation about grouping similar notifications into one notification -- RADAR-base/RADAR-Questionnaire#1448

@mpgxvii
Copy link
Member Author

mpgxvii commented Mar 28, 2024

I would have thought that is a required feature - i.e to have multiple notifications for different questionnaires? But i see the point that if the title and body is same, then does it add any value?

I think this is related to the previous conversation about grouping similar notifications into one notification -- RADAR-base/RADAR-Questionnaire#1448

Ah okay I see. Okay yes we can have a separate implementation of this through the protocol as discussed here: RADAR-base/RADAR-Questionnaire#1448.

Closing this issue.

@mpgxvii mpgxvii closed this as completed Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants