Page MenuHomePhabricator

change ios notification structure to make it visible for NotificationService
ClosedPublic

Authored by marcin on May 24 2022, 6:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 2:53 AM
Unknown Object (File)
Sat, Nov 9, 1:42 AM
Unknown Object (File)
Fri, Nov 8, 4:18 AM
Unknown Object (File)
Thu, Nov 7, 1:21 PM
Unknown Object (File)
Thu, Nov 7, 7:14 AM
Unknown Object (File)
Thu, Nov 7, 3:31 AM
Unknown Object (File)
Thu, Nov 7, 2:29 AM
Unknown Object (File)
Thu, Nov 7, 2:27 AM

Details

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

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

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:

  1. Test what happens when an old client (with no Notification Service Extension support) receives this notif, OR
  2. 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

marcin requested review of this revision.EditedJul 15 2022, 12:40 AM

@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.

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.

keyserver/src/push/send.js
495 ↗(On Diff #14510)

Before landing, can you change this to something high, eg. 1000?

This revision is now accepted and ready to land.Jul 17 2022, 7:17 AM

Make codeVersion required parameter. Set threshold for mutableContent flag to high number.

Change codeVersion threshold to real build

Rebase master prior to landing

Final rebase before landing