Details

Reviewers

tomek
ashoat

Commits

rCOMM71e1f1ceda6e: change ios notification structure to make it visible for NotificationService

Summary

If we want the system to launch NotificationService when notification is received it muse have flag mutable-content set to true (https://developer.apple.com/documentation/usernotifications/unnotificationserviceextension?language=objc)

Test Plan

This change was already used to test previous differentials in this stack. The only additional testing needed is to check whether it doesn't break previous versions of the app (no NotificationService).

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.May 24 2022, 6:38 AM

Herald added subscribers: • abosh, atul, • adrian, ashoat. · View Herald TranscriptMay 24 2022, 6:38 AM

marcin added a parent revision: D4063: Use notification service on ios to store notification in sqlite.May 24 2022, 6:38 AM

Harbormaster completed remote builds in B9437: Diff 13073.May 24 2022, 6:44 AM

marcin requested review of this revision.May 24 2022, 6:44 AM

From the docs it looks like this is safe, but we should be careful about modifying all existing notifs. It's probably okay, but if we have any doubts we could hide this behind a codeVersion check

This revision is now accepted and ready to land.May 24 2022, 12:22 PM

Rebase

marcin edited parent revisions, added: D4445: Retrieve messages from temporary message storage in AppDelegate; removed: D4063: Use notification service on ios to store notification in sqlite.Jul 7 2022, 7:20 AM

Harbormaster completed remote builds in B10373: Diff 14281.Jul 7 2022, 8:46 AM

Rebase

Harbormaster completed remote builds in B10525: Diff 14474.Jul 14 2022, 6:10 AM

Just making sure – is there any danger in setting this flag when sending a notif to an old client that doesn't support the Notification Service Extension? It might be smart to either:

Test what happens when an old client (with no Notification Service Extension support) receives this notif, OR
Gate this on codeVersion that supports Notification Service Extension

@ashoat I have already realized this concern and attempted to reach @palys-swm regarding this matter. Now that you also brought this matter into discussion I think it is best to add codeVersion if statement to this diff.

Add codeVersion flag to determine whether notification should be visible for NotificationService

@ashoat I noticed that codeVersion is optional parameter so we need an additional check to satisfy flow constraints. However I noticed that the only place this function (prepareIOSNotification) gets called is sendIOSNotification where codeVersion is specified. Can we then make this parameter obligatory and simplify if statement?

When I opened XCode I found out that the current code version is 136 - this is why I check codeVersion against 136.

Harbormaster completed remote builds in B10545: Diff 14510.Jul 15 2022, 12:49 AM

In D4113#129296, @marcinwasowicz wrote:

@ashoat I noticed that codeVersion is optional parameter so we need an additional check to satisfy flow constraints. However I noticed that the only place this function (prepareIOSNotification) gets called is sendIOSNotification where codeVersion is specified. Can we then make this parameter obligatory and simplify if statement?

Good question – I can't tell why it's optional either! I think it's okay to make it required.

When I opened XCode I found out that the current code version is 136 - this is why I check codeVersion against 136.

I used to do it this way too, but then realized that sometimes we accidentally increment code version with the engineer who is working on it knowing if it is ready. So I think instead it's better to use a high number here, and then submit another diff later when you're ready to launch the feature.