This differential solves the issue: https://linear.app/comm/issue/ENG-7773/badgeonly-notifs-on-ios-show-encrypted-notif-for-staff-members#comment-ac7ea6b6.
Details
Details
- Build iOS app.
- Make a thread badge only.
- Sent notif to this thread.
- Ensure that badge is updated and no "ENCRYPTED" notifs are displayed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-7773
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
An alternative solution to this problem is to set badgeOnly property on a notif payload. However I am of opinion that if we can solve something without putting additional data to notification payload then it should be a preferred solution. However if reviewers disagree I can proceed with this solution.
Comment Actions
I think the changes are good here, but I feel like the code comments can be improved, and I'd like to review the updated code comments before this is landed
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 257–258 ↗ | (On Diff #39213) | You're deleting a comment elsewhere that explains why we use a separate notif for the badge count instead of just modifying the remote notif we received. Can you add that explanation here? |
| 563 ↗ | (On Diff #39213) | Is this function useful anymore? I think we can get rid of it and make the check inline |
| 566–569 ↗ | (On Diff #39213) | This comment was extremely helpful for debugging this exact issue. Can you find a way to keep it, and in particular the part explaining that we set BODY to ENCRYPTED for internal builds, and that we need to filter the notif to avoid displaying that to the user? |
| 645 ↗ | (On Diff #39213) | Why would a local badge-only notif need to be created, as opposed to modifying the remote notif we receive? |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | Sorry for the confusion. This comment is inaccurate. For notifs that don't have body in payload (rescinds and badgeonly) we costruct new notif locally in this code but we pass it to contentHandler so it is not a local notif but a remote notif. I will delete this comment. |
Comment Actions
- Remove needsSilentBadgeUpdate function and replace it with inline checks.
- Bring back useful comment.
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | I'm still confused here, to be honest. If we construct the new notif locally, how is it a remote notif and not a local notif? |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | Even though it is an object instantiated in the NSE code we pass it to contentHandler given to the NSE byt the OS so the OS treats it as a remote notif. |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | Okay, I think I was using the terms wrong. Let me ask my question again. Why would a new remote badge-only notif need to be created, as opposed to modifying the remote notif we receive? |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | Initially badge only notif/rescind has body set to "ENCRYPTED". I recall that when I was introducing this code I tried first to modify body of received object directly and I found that:
However creating new notif object without touching its body property worked as desired. Signal does it too: https://github.com/signalapp/Signal-iOS/blob/main/SignalNSE/NotificationService.swift#L80-L83. I guess that Apple initially sets this field to some flag that prevents system from displaying any visual content. |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) |
Isn't that what we want, though? An empty notif that only updates badge count? |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | I meant visible empty notif bubble. |
| native/ios/NotificationService/NotificationService.mm | ||
|---|---|---|
| 645 ↗ | (On Diff #39213) | Okay, got it. It took a lot of clarification but I think I understand now. Next time hopefully there can be less confusion if we use less ambiguous language |