Page MenuHomePhabricator

Update thread unread status from AppDelegate notifications callbacks on iOS
ClosedPublic

Authored by marcin on Jun 7 2022, 7:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 12:27 AM
Unknown Object (File)
Mon, Nov 11, 10:17 PM
Unknown Object (File)
Mon, Nov 11, 9:34 PM
Unknown Object (File)
Mon, Nov 11, 8:35 PM
Unknown Object (File)
Mon, Nov 11, 7:52 PM
Unknown Object (File)
Mon, Nov 11, 7:27 PM
Unknown Object (File)
Mon, Nov 11, 7:26 PM
Unknown Object (File)
Mon, Nov 11, 7:07 PM

Details

Summary

Update thread unread status from AppDelegate callbacks in response to rescinds

Test Plan

Comment-out encryption code in SQLiteQuery executor. Create two clients - A (web), B(mobile and web). Send message from A to B while mobile B is inactive. Open mobile B but do not read the message. Download the database from XCode. Kill mobile B, read message on web B and download SQLite database from XCode. Check that unread status in a thread the message was sent for changed from true to false.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-396
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin requested review of this revision.Jun 7 2022, 7:34 AM
native/ios/Comm/AppDelegate.mm
163 ↗(On Diff #13394)

If you take my advice in D4231, this check will need to be updated to a boolean

169 ↗(On Diff #13394)

So the plan for now is to update this, since it is safe outside of UNNotificationAppExtension? And later when we implement it inside UNNotificationAppExtension, we will do something different I assume (eg. ENG-1218)

native/ios/Comm/AppDelegate.mm
169 ↗(On Diff #13394)

Actually I do not think that keeping this logic here is related to NotificationService at all. To move this logic from here to NotificationService we would need:

  1. To write to SQLite from NotificationService
  2. To be able to process rescinds via notification filtering mechanism.

At this moment we will probably use NotificationService to write to some other storage that will be processed in JS when the app starts. The code above is executed outside of JS, so we would simply overwrite unread status twice. Additionally this will only happen once we get ability to filter notifications.

native/ios/Comm/AppDelegate.mm
169 ↗(On Diff #13394)

Makes sense! Thanks for explaining

Rebase to master after CI change

tomek added inline comments.
native/ios/Comm/AppDelegate.mm
164–165

Do we trust that threadID is always present in the notification? Maybe we should check it?

This revision is now accepted and ready to land.Jun 8 2022, 7:00 AM
native/ios/Comm/AppDelegate.mm
164–165

It is not optional field on the keyserver side. I am not against ensuring existence of threadID key, but then we would need also to ensure that backgroundNotifTypeKey exists since it is put in the notification via the same mechanism as threadID

native/ios/Comm/AppDelegate.mm
164–165

Kind of, but backgroundNotificationTypeKey is compared to e.g. CLEAR so if it isn't present, the behavior would be the same as if it was present but had a different value.

native/ios/Comm/AppDelegate.mm
164–165

Historically we've assumed that we launch keyserver changes based on codeVersion, eg. if my comment here is heeded than we should be okay. That said, it's true that @palys-swm's suggestion would make this safer

Add security check against missing thread ID when handling notification rescind.