BIDMCDigitalPsychiatry / LAMP-platform

The LAMP Platform (issues and documentation).
https://docs.lamp.digital/
Other
12 stars 10 forks source link

Notifications crossing study #641

Closed lukeoftheshire closed 2 years ago

lukeoftheshire commented 2 years ago

A user was helping conduct QA testing across multiple servers. While logged into the second server, they received a notification to do an activity from a prior study (from which they had logged out) at the time which it should have fired. Since they were logged into a different study, the assigned activity didn't exist in this server and the user got a grey spinning circle.

If an activity fails to load, the participant should probably be returned to the activity page instead of the app basically crashing. Additionally, the notification shouldn't have fired at all. From the logs of the SECOND user, I found this lamp.analytics event:

[{'data': {'device_type': 'iOS',
   'action': 'notification',
   'content': {'aps': {'expiration': 10,
     'badge': 0,
     'push-type': 'alert',
     'sound': 'default',
     'content-available': 1,
     'alert': 'You have a mindLAMP activity waiting for you: Daily Survey..',
     'mutable-content': 1,
     'collapse-id': '403189'},
    'actions': [{'name': 'Open App',
      'page': '/participant/U0787924669/activity/qexqbpmgygcmmngacx3a'}],
    'notificationId': '403189',
    'page': '/participant/U0787924669/activity/qexqbpmgygcmmngacx3a',
    'expiry': 21600000},
   'user_agent': 'NativeCore 2022.4.23; iOS 14.8.1; iPhone iPhone12,3'},
  'sensor': 'lamp.analytics',
  'timestamp': 1652990400687}]

Please note also that not only does this activity NOT exist on this server, this is not the correct user id even, but the old (logged-out) id.

In other words, this is like if I called LAMP.SensorEvents('PARTICIPANT_A') but this activity event was trying to open'/participant/PARTICIPANT_B/activity/qexqbpmgygcmmngacx3a'. The SensorEvent logs for PARTICIPANT_B don't show this notfication.

jijopulikkottil commented 2 years ago
  1. Yes, notification should not fire after logged out. We will cross check with remote (@Linoy339 ) and local notification.
  2. We can prevent from displaying the notification if it belongs to other user. To do this, we can add ParicipantID in push payload, and can verify with logged userid. To do this, we also need permission from apple detailed here . @saranya-sajeev please confirm it is possible with Android also.
  3. " the participant should probably be returned to the activity page instead of the app basically crashing" - Yes, we can make sure it should not crash and returned to the home page. @sarithapillai8

@avaidyam We can start (1) and (3) items. Let us know if we need to implement (2)

avaidyam commented 2 years ago

@jijopulikkottil I think we want to do (2) as well. In addition we should use this to remove notifications instead of showing a "notification expired" message.

saranya-sajeev commented 2 years ago

@jijopulikkottil @avaidyam In android also we can check ParicipantID in payload against logged in user id.

ZCOEngineer commented 2 years ago

iOS and Android will check on this after #643

jijopulikkottil commented 2 years ago

@jijopulikkottil I think we want to do (2) as well. In addition we should use this to remove notifications instead of showing a "notification expired" message.

We can't remove delivered notification by implementing (2).
We can start working on item (2) once the request submit and accepted.

@avaidyam, do we need to fix the following case on front-end:

Since they were logged into a different study, the assigned activity didn't exist in this server and the user got a grey spinning circle.

@Linoy339 please confirm (1)

Linoy339 commented 2 years ago

@jijopulikkottil, @avaidyam We have confirmed that notification are not firing after logged out.

avaidyam commented 2 years ago

We can start working on item (2) once the request submit and accepted.

@jijopulikkottil It looks like mindLAMP is not going to be permitted to apply for that entitlement. ("This entitlement is intended for certain types of apps — such as messaging apps or location sharing apps...") so we will need to ignore item (2) for now.

do we need to fix the following case on front-end:

Yes, we do.

avaidyam commented 2 years ago

@jijopulikkottil @Linoy339 Has this been QA tested and confirmed fixed yet?

jijopulikkottil commented 2 years ago

@avaidyam QA-testing is in-progress. Can confirm tomorrow.

@lukeoftheshire Could you try once more and check whether you are able to reproduce it or not. We tried here using different user under different study but not getting the issue. We think It maybe an edge case.

sarithapillai8 commented 2 years ago

@avaidyam @lukeoftheshire @jijopulikkottil

Since they were logged into a different study, the assigned activity didn't exist in this server and the user got a grey spinning circle.

This is handled and updated in zco-staging. After QA confirms, we can update in dashboard-staging.

jijopulikkottil commented 2 years ago

@avaidyam QA-testing is in-progress. Can confirm tomorrow.

We couldn't reproduce this issue.

jijopulikkottil commented 2 years ago

@avaidyam @lukeoftheshire @jijopulikkottil

Since they were logged into a different study, the assigned activity didn't exist in this server and the user got a grey spinning circle.

This is handled and updated in zco-staging. After QA confirms, we can update in dashboard-staging.

I just tested by programmatically manipulated the url to open when tapping on notification. And got this alert. Simulator Screen Shot - iPhone 13 - 2022-06-09 at 19 50 31

avaidyam commented 2 years ago

@jijopulikkottil Thanks! That's helpful to know. Can we also make sure old activity schedules are not cached by the iOS app and the backend (cc @Linoy339) is able to handle logout events correctly too (i.e. stop sending notifications to a device token once it is logged out)?

jijopulikkottil commented 2 years ago

Yes, on iOS side, we are deleting pending and delivered notifications when logged out. On server side, @Linoy339 also confirmed this (stop sending notifications to a device token once it is logged out).

avaidyam commented 2 years ago

@sarithapillai8 @jijopulikkottil I believe this issue is now resolved so I'm closing the issue. Correct me if it's still pending.

ertjlane commented 1 year ago

This seems to be fixed in staging!