search

RADAR-base / RADAR-Appserver

General purpose application server for the radar platform currently with capability to schedule push notifications
10 stars 1 forks source link

[BUG] Similar notifications get scheduled #456

Closed mpgxvii closed 3 months ago

mpgxvii commented 3 months ago

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:

https://github.com/RADAR-base/RADAR-Appserver/blob/c8c04392b1a38135e8d47e13734e7841237278ef/src/main/java/org/radarbase/appserver/service/FcmNotificationService.java#L167-L175

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 commented 3 months ago

@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 commented 3 months ago

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 -- https://github.com/RADAR-base/RADAR-Questionnaire/issues/1448

mpgxvii commented 3 months ago

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: https://github.com/RADAR-base/RADAR-Questionnaire/issues/1448.

Closing this issue.