RADAR-base / RADAR-Appserver

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

[FEAT] Process notification requests and cancellations asynchronously #438

Open peyman-mohtashami opened 1 year ago

peyman-mohtashami commented 1 year ago

Is your feature request related to a problem? Please describe. Processing notification requests and cancellations can be time-consuming, particularly when there is a significant delay in waiting. For instance, when we transmit requests from a distant region to the app server, the response time is notably slow.

Describe the solution you'd like To enhance efficiency, we should implement a feature that allows multiple requests and cancellations to be bundled into a single request. This solution will require modifications to be made to the Questionnaire app accordingly.

Describe alternatives you've considered

Priority 3

Difficulty 2

Additional context

yatharthranjan commented 1 year ago

Hi @peyman-mohtashami, thanks for adding the issue.

Can you and @mpgxvii give an overview of what requests bundling is required?

The appserver already has a few endpoints which support this, such as

  1. Adding notifications in bulk - here
  2. Get all notifications for user - here
  3. Remove all notifications for a user - here

I agree these are not async and perhaps we can look into that for quicker responses, but Joris already made a start on that here where async is useful.

mpgxvii commented 1 year ago

Yes I think on the appserver side we already have the batch notification adding, so I think we would just need to implement this on the Questionnaire app side? I think initially we wanted to schedule these individually because we needed to store the notification ids back to each task, but maybe we can process this afterwards instead.

peyman-mohtashami commented 1 year ago

Thanks @yatharthranjan. Are these endpoints used in the Questionnaire app? @mpgxvii, I checked the alpha version of the Questionnaire app and couldn't find usage of these endpoints.

mpgxvii commented 1 year ago

Thanks @yatharthranjan. Are these endpoints used in the Questionnaire app? @mpgxvii, I checked the alpha version of the Questionnaire app and couldn't find usage of these endpoints.

@peyman-mohtashami Yes these are not implemented yet on the questionnaire app side.

peyman-mohtashami commented 1 year ago

Thanks @mpgxvii. So, the only thing that is needed is to add to the Questionnaire app. Could you give me a short docs on it, or point me to the resource files to find the paths?

peyman-mohtashami commented 1 year ago

I think this should be the resource where paths are defined, right? https://github.com/RADAR-base/RADAR-Appserver/blob/25d53a82cade28c66492713101371a0470c349bb/src/main/java/org/radarbase/appserver/controller/FcmNotificationController.java

mpgxvii commented 1 year ago

I think this should be the resource where paths are defined, right? https://github.com/RADAR-base/RADAR-Appserver/blob/25d53a82cade28c66492713101371a0470c349bb/src/main/java/org/radarbase/appserver/controller/FcmNotificationController.java

Yes, that's correct.

Bdegraaf1234 commented 1 year ago

While working on this I encountered some odd behaviour:

The endpoint /projects/{projectId}/users/{subjectId}/messaging/notifications/schedule returns all scheduled notifications for the user.

While doing this we log Scheduling {} messages. From the code here I got the impression that this endpoint schedules multiple notifications, because it calls this function.

I propose we make this endpoint a GET request to avoid confusion, remove the logic involving the schedulerservice (because it doesn't seem to do anything) and rename the function getAllScheduledUserNotifications.

But maybe I am missing something here? If not, let me know and I can get started on a pull-request.

yatharthranjan commented 1 year ago

Hi @Bdegraaf1234 , i can understand the confusion.

There are 2 steps related to the notifications in the appserver -

  1. Just adding the notifications for a user with the correct details such as title, time, etc. These are not triggered by themselves but store valuable information about the notification.
  2. Scheduling the notifications (using Quartz jobs) at the correct time so they are are delivered to the users.

The /schedule endpoint is to do the point 2 above in case the notifications were added previously using point 1 but were not scheduled. This is possible when adding the notifications with schedule=false query parameter. When the schedule=true is used, the schedule step is also done after adding the notification, so an additional request to /schedule endpoint is not required.

Since the /schedule endpoint is adding Quartz triggers and jobs in the database for existing notification entities, hence we use POST instead of GET.

When you are getting Scheduling {} messages it could be because there are no notifications for the user to schedule present in the database or that all of them already are scheduled.

Hope it is clear, but let me know if you need more info.

Bdegraaf1234 commented 1 year ago

Hi @yatharthranjan

That clarifies things indeed, thank you for the explanation. Will leave this as is then!