MM-55036 - Calls: Fix: Receiving in-app notifications for calls notifications while active #7623

cpoile · 2023-10-20T18:10:50Z

Summary

Fix for receiving in-app notifications for calls notifications while active -- we no longer see the overlay when the app is active
~~NOTE: we are testing on the exact message sent by the calls plugin.~~
- This is not elegant, I agree. We could add a new NOTIFICATION_TYPE.CALLS (which would require changes across the products). That would certainly be more "correct." But on the other hand: the current solution won't break because there's no other way for the existing products to accidentally send those exact messages. The only way would be for the product or a plugin to explicitly send them, and if they do that then they could just as easily send a NOTIFICATION_TYPE.CALLS too.
- We are now testing by pushnotification type, for push proxy v5.27.0 +
For iOS we keep the notification's buzz and alert sound. Android doesn't have this ability, so added a ticket to track: https://mattermost.atlassian.net/browse/MM-55059 (/cc @matthewbirtch)

Ticket Link

https://mattermost.atlassian.net/browse/MM-55036

Checklist

Added or updated unit tests (required for all new features)
Has UI changes
Includes text changes and localization file updates
Have tested against the 5 core themes to ensure consistency between them.

Device Information

Android: 13, Pixel 6
Android: 13, Galaxy Tab s7+
iOS: 16.5.1, iPhone 14

Release Note

Calls: Fixed an issue where the in-app "push" notification overlay would appear when the user already received the incoming call notification banner.

enahum

Definitely not elengant but ok for the time being, I would definitely want us to go in the direction of the type of message in the near future, yes is more work but it does keep it cleaner and consistent even if we decide to chsnge the message.

My worry on this and perhaps cause idk the details, is, can the message of the notification be in a language different than english?

cpoile · 2023-10-20T18:40:18Z

@enahum Yes, great point - we have a ticket for i18n to be completed for 1.0, so I've added ticket for typing the calls push notification: https://mattermost.atlassian.net/browse/MM-55060

mattermost-build · 2023-10-20T20:20:12Z

Building app in separate branch.

streamer45 · 2023-10-30T17:08:54Z

which would require changes across the products

Could you help me understand what it would take to do this cleanly?

cpoile · 2023-11-09T19:49:43Z

@streamer45 An update: I'm working on it, but I'm hitting snags. We can chat in our 1:1.

cpoile · 2023-11-10T22:44:17Z

Ok, fixed. With the following PRs, calls notifications will either be sent as message (pre push proxy v5.27.0) or as calls (push proxy v5.27.0 + ).

MM-55060 - Calls: Add PushTypeCalls for calls-specific push notifications mattermost#25405
MM-55060 - Calls: Add PushTypeCalls for calls-specific push notifications mattermost-push-proxy#110
MM-55060 - Calls: Add PushTypeCalls for calls-specific push notifications mattermost-plugin-calls#561

streamer45

Thanks 💯

streamer45 · 2023-11-13T17:53:25Z

app/products/calls/utils.ts

+export function isCallsStartedMessage(message = '') {
+    return (message === 'You\'ve been invited to a call' || callsMessageRegex.test(message));
+}


Can we leave a comment to deprecate this at some point?

Done in 13d1946

cpoile · 2023-11-14T19:10:11Z

Please see: mattermost/mattermost#25405 (comment)
Changes here are from 2203aae
Thanks everyone.

enahum · 2023-11-15T01:58:42Z

app/products/calls/utils.ts

@@ -14,6 +15,8 @@ import type PostModel from '@typings/database/models/servers/post';
 import type {IntlShape} from 'react-intl';
 import type {RTCIceServer} from 'react-native-webrtc';

+const callsMessageRegex = /^\u200b.* is inviting you to a call$/;


we should add a note here not to forget when the plugin starts to support localization

This regex won't need to be changed when we support localization -- when we support localization it will be handled by the sub_type check. The regex will still be needed to match older messages from pre-0.21.0 calls (or pre-change MM server and pre-change proxy servers). When we finally remove that section of the code below, this can be removed (it will show up as dead-code and go will tell us).
Just in case, I added a note to the removal ticket https://mattermost.atlassian.net/browse/MM-55506

streamer45

Good stuff 👍

…ications while active (mattermost#7623) * when active, do not show overlay if this is a calls-started notification * add calls notification type; keep message test for backwards compat * process calls notifications properly * revert NOTIFICATION_TYPE.CALLS; use sub_type for backwards compatibility * add ticket and comment for future refactoring of isCallsStartedMessage

when active, do not show overlay if this is a calls-started notification

872f81e

cpoile added the 2: Dev Review Requires review by a core commiter label Oct 20, 2023

cpoile requested review from streamer45 and enahum October 20, 2023 18:10

mm-cloud-bot added the release-note label Oct 20, 2023

enahum approved these changes Oct 20, 2023

View reviewed changes

matthewbirtch added the Build Apps for PR Build the mobile app for iOS and Android to test label Oct 20, 2023

mattermost-build removed the Build Apps for PR Build the mobile app for iOS and Android to test label Oct 20, 2023

matthewbirtch approved these changes Oct 20, 2023

View reviewed changes

add calls notification type; keep message test for backwards compat

1d704ed

process calls notifications properly

ba4af4e

cpoile force-pushed the MM-55036-fix-calls-in-app-notfication branch from 375a6ce to ba4af4e Compare November 10, 2023 21:08

streamer45 approved these changes Nov 13, 2023

View reviewed changes

streamer45 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 13, 2023

cpoile added 2 commits November 14, 2023 13:26

revert NOTIFICATION_TYPE.CALLS; use sub_type for backwards compatibility

2203aae

add ticket and comment for future refactoring of isCallsStartedMessage

13d1946

cpoile mentioned this pull request Nov 14, 2023

MM-55060 - Add PushSubTypeCalls for calls-specific push notifications mattermost/mattermost-plugin-calls#564

Merged

cpoile requested review from streamer45 and enahum November 14, 2023 19:10

cpoile removed the 4: Reviews Complete All reviewers have approved the pull request label Nov 14, 2023

cpoile added the 2: Dev Review Requires review by a core commiter label Nov 14, 2023

enahum approved these changes Nov 15, 2023

View reviewed changes

streamer45 approved these changes Nov 16, 2023

View reviewed changes

cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 16, 2023

cpoile merged commit 6fb5d85 into main Nov 16, 2023
1 check passed

cpoile deleted the MM-55036-fix-calls-in-app-notfication branch November 16, 2023 14:28

amyblais added this to the v2.12.0 milestone Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-55036 - Calls: Fix: Receiving in-app notifications for calls notifications while active #7623

MM-55036 - Calls: Fix: Receiving in-app notifications for calls notifications while active #7623

cpoile commented Oct 20, 2023 •

edited

Loading

enahum left a comment

cpoile commented Oct 20, 2023

mattermost-build commented Oct 20, 2023

streamer45 commented Oct 30, 2023

cpoile commented Nov 9, 2023

cpoile commented Nov 10, 2023

streamer45 left a comment

streamer45 Nov 13, 2023

cpoile Nov 14, 2023

cpoile commented Nov 14, 2023

enahum Nov 15, 2023

cpoile Nov 15, 2023 •

edited

Loading

streamer45 left a comment

MM-55036 - Calls: Fix: Receiving in-app notifications for calls notifications while active #7623

MM-55036 - Calls: Fix: Receiving in-app notifications for calls notifications while active #7623

Conversation

cpoile commented Oct 20, 2023 • edited Loading

Summary

Ticket Link

Checklist

Device Information

Release Note

enahum left a comment

Choose a reason for hiding this comment

cpoile commented Oct 20, 2023

mattermost-build commented Oct 20, 2023

streamer45 commented Oct 30, 2023

cpoile commented Nov 9, 2023

cpoile commented Nov 10, 2023

streamer45 left a comment

Choose a reason for hiding this comment

streamer45 Nov 13, 2023

Choose a reason for hiding this comment

cpoile Nov 14, 2023

Choose a reason for hiding this comment

cpoile commented Nov 14, 2023

enahum Nov 15, 2023

Choose a reason for hiding this comment

cpoile Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

streamer45 left a comment

Choose a reason for hiding this comment

cpoile commented Oct 20, 2023 •

edited

Loading

cpoile Nov 15, 2023 •

edited

Loading