Page MenuHomePhabricator

Implement displaying error message notification for cases that might cause empty notification on Android
ClosedPublic

Authored by marcin on Aug 4 2023, 7:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 11, 3:43 AM
Unknown Object (File)
Mon, Nov 11, 12:31 AM
Unknown Object (File)
Sun, Nov 3, 7:01 AM
Unknown Object (File)
Sun, Oct 27, 11:11 PM
Unknown Object (File)
Fri, Oct 18, 9:27 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Unknown Object (File)
Fri, Oct 18, 3:06 AM
Subscribers
None

Details

Summary

Investigation of https://linear.app/comm/issue/ENG-4525/empty-notification-on-android hasn't led to reliable reproduction yet. Nevertheless there are potential explanations of how this bug might have occurred. This differential introduces changes that will result in error message being displayed as notification in case a path leading to empty notification occurs. Changes in this differential are based on the algorithm described here: https://linear.app/comm/issue/ENG-4525/empty-notification-on-android#comment-2aae9fa4

  1. One thing to note is that our current knowledge suggests that none of the reasons examined by the algorithm linked above might be the actual cause for the issue. It is possible that the OS simply displays group summary instead of the notification group. If that is the case we should see debugging data instead of empty notifications.
  2. We don't have a theory as to why the OS replaces group with the content of the summary but perhaps this debugging info will help us discover some pattern.
  3. Eventually if we don't have an explanation after landing this differential but we confirm that the OS indeed sometimes just replaces the group with the summary the best thing to do is to populate the summary with informative and user-friendly content that can be released to public users.
IMPORTANT: This diff is intented to be staff-release-only change to help us diagnose the issue. Once we can explain the bug a proper solution will be implemented.
Test Plan
  1. Comment notificationManager.cancel(threadID, threadID.hashCode()); line.
  2. Send some notifs, then some rescinds
  3. Ensure that notification summary with title and body is present but there are no notif ids in the body.

Then keeping those changes:

  1. Comment removeNotificationFromGroupSummary(threadID, rescindID, notification);
  2. Again send some notifs then some rescinds.
  3. Ensure that notification summary with title and body is present and there are notifs ids in the summary body.

Remember to make comm::StaffUtils::isStaffRelease method return true for the testing above.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Add some data to group summary notification in case my theory about invalid rescinds is wrong.

marcin requested review of this revision.Aug 4 2023, 7:51 AM

Accepting for a staff-only release

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
213 ↗(On Diff #29541)

I believe this is the same condition (following the early return on line 210), but I think my suggestion is more readable

214 ↗(On Diff #29541)

How does this get displayed to the user? Can you share a screenshot? I'm worried that it's too much text, and that it might not be possible to view it all

This revision is now accepted and ready to land.Aug 4 2023, 8:40 AM
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
214 ↗(On Diff #29541)

It is an expandable notification so the user has to tap a downward arrow to see this large content. I will upload some screenshots.

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
214 ↗(On Diff #29541)

Before expansion:

Screenshot 2023-08-04 at 17.58.28.png (783×434 px, 128 KB)

After expansion:

Screenshot 2023-08-04 at 17.58.35.png (646×367 px, 68 KB)

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
229 ↗(On Diff #29629)

Actually we can't query for notification body when it is returned in NotificationManager#getActiveNotifications. We have to keep a bundle attached to notification with all not-rescinded notif ids and construct notification body with it.

243 ↗(On Diff #29629)

Temporarily set this to false, so that if a tester sees summary notif on a locked screen or when the app is killed and accidentally taps the notif they can still recover from this by not unlocking the phone or quickly killing the app. We don't want accidental tapping to destroy evidence for this issue.

See comment here... I'm wondering if we can just implement a final solution now instead of more testing. It feels like we maybe fully understand what's causing the issue now, no?

ashoat requested changes to this revision.Aug 15 2023, 8:42 AM

Context in ENG-4525, but basically after talking with @marcin we decided to try to fix the issue that this diff is trying to detect, rather than waiting for it to be detected.

If our fix doesn't work, we may come back to this diff. If we do, I'd like to rebase it on D8803 so that it can be landed to master.

This revision now requires changes to proceed.Aug 15 2023, 8:42 AM

Refactor using isStaffRelease utility.

ashoat added 1 blocking reviewer(s): tomek.

Looks good, but would be great if @tomek could take a look, as he's more familiar with our Android notif code

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
231 ↗(On Diff #31199)

Do we really want to make this change in production? Should we consider gating it on isStaffRelease?

marcin added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
231 ↗(On Diff #31199)

This is my oversight. We want it false for staff to avoid accidental evidence removal. However we don't want it for public users. I will update the diff shortly.

Set auto cancel property to false only for staff releases

Move isStaffRelease checks to better places

tomek requested changes to this revision.Sep 19 2023, 4:51 AM
tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
45 ↗(On Diff #31206)

Do we need this empty space between keys?

237–238 ↗(On Diff #31206)

Might be simplified

243 ↗(On Diff #31206)

Do we have to assign the result? I guess the builder is mutable

366–367 ↗(On Diff #31206)

As in the previous comment - do we have to assign?

405–407 ↗(On Diff #31206)

Why do we break here? If we want the first occurrence of an item matching conditions, maybe we can use e.g. filter / findFirst?

409 ↗(On Diff #31206)

Shouldn't we add in a loop?

433–444 ↗(On Diff #31206)

This isn't too efficient - we're basically iterating linearly twice. But it isn't a big issue either.

This revision now requires changes to proceed.Sep 19 2023, 4:51 AM
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
405–407 ↗(On Diff #31206)

We want to find the group summary notification for the particular threadID and there is at most one groups summary notification for one threadID so we break once we found it.
I will try to replace with findFirst.

409 ↗(On Diff #31206)

The answer to the comment above explains this. We want one group summary notification for the particular threadID so once we found it we add notificationID to the list associated with this groupSummaryNotification.

  1. Take advantage of builder mutability.
  2. Use streams instead of iteration.
  3. Don't iterate twice over notifications id list when removing a notification id

THos should cover all comments from Tomek last review

marcin edited the test plan for this revision. (Show Details)
tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
251–252 ↗(On Diff #31261)
388–392 ↗(On Diff #31261)

It seems to be equivalent, not sure if more readable

407 ↗(On Diff #31261)

Is it a good idea to mutate the result of otif.getNotification().extras.getStringArrayList(GROUP_NOTIF_IDS_KEY)

427 ↗(On Diff #31261)

Is it safe to mutate it? Should we make e.g. a copy or filter and collect?

This revision is now accepted and ready to land.Sep 19 2023, 6:58 AM
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
407 ↗(On Diff #31261)

extras is a bundle (https://developer.android.com/reference/android/os/Bundle#getStringArrayList(java.lang.String)) which does not store values a map but rather serialises values upon storing them and deserialises them upon retrieval. That said mutating this array is not reflected in extras before we call putStringArrayList with new, mutated data

427 ↗(On Diff #31261)

Same as in the comment above.

Some last changes and rebase before landing

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
388–392 ↗(On Diff #31261)

Personally I prefer additional if statement. One liners actually take more thought to understand.