MM-53924 - Mobile push notifications #503

cpoile · 2023-08-25T21:42:17Z

Summary

Added mobile push notifications
Push notification is sent for all call started posts in DMs and GMs, and if the user receives a notification for regular channels then we pretty-fy that one too. (i.e., we currently do not send out a regular channel push notification for a call start message unless the user has push notifications set for all activity in the channel)

DM Channels:

iOS	Android condensed	Android expanded

GM Channels:

iOS	Android condensed	Android expanded

Regular Channels (only if the user receives the notification naturally):

iOS	Android condensed	Android expanded

You can see that Android does it's own processing of the message on the user's device, so this might look different based on the user's make & model. (See the inline code comment below for an example of this.)

Ticket Link

cpoile · 2023-08-25T21:45:11Z

server/push_notifications.go

+
+func buildPushNotificationMessage(senderName string) string {
+	// TODO: translations https://mattermost.atlassian.net/browse/MM-54256
+	return fmt.Sprintf("\u200b%s is inviting you to a call", senderName)


This was kind of funny. For a long time I thought the push notification was being rewritten somewhere because what should have been Sender Name is inviting you to a call was turning into Sender Name: is inviting you to a call in condensed, and

Sender Name is inviting you to a call

in expanded view.
It turns out that (at least my) Android device does its own processing, and if it notices the prefix of the notification is the same as the sender name, it elides it. Thanks, Android. Super helpful.

codecov-commenter · 2023-08-26T15:22:36Z

Codecov Report

Patch coverage has no change and project coverage change: -0.08% ⚠️

Comparison is base (9fdbb65) 5.50% compared to head (2164499) 5.43%.

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #503      +/-   ##
========================================
- Coverage   5.50%   5.43%   -0.08%     
========================================
  Files         23      24       +1     
  Lines       4265    4379     +114     
========================================
+ Hits         235     238       +3     
- Misses      4012    4124     +112     
+ Partials      18      17       -1

Files Changed	Coverage Δ
server/bot_api.go	`0.00% <0.00%> (ø)`
server/plugin.go	`0.00% <0.00%> (ø)`
server/push_notifications.go	`0.00% <0.00%> (ø)`
server/store.go	`0.00% <ø> (ø)`
server/utils.go	`26.85% <0.00%> (-8.52%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

streamer45

Nice! Gave this a first quick pass and left some minor comments.

server/plugin.go

server/push_notifications.go

server/utils.go

# Conflicts: # go.mod # go.sum # server/plugin.go # server/utils.go

matthewbirtch · 2023-08-29T19:14:36Z

@cpoile for the Android expanded notification state, do we have any control over the Okay button? Wondering if it should be 'Join call'? 'Okay' doesn't seem particularly useful and I'm not sure what it would do

streamer45

Looks good 👍

server/push_notifications.go

# Conflicts: # go.mod # go.sum

cpoile · 2023-09-05T18:41:13Z

@matthewbirtch Making a custom button will take some native work, so I think we've got to prioritize it as new effort. Sound good?

matthewbirtch · 2023-09-05T20:07:24Z

@matthewbirtch Making a custom button will take some native work, so I think we've got to prioritize it as new effort. Sound good?

no problem, can we just hide the Okay button at all?

cpoile · 2023-09-06T14:00:42Z

@matthewbirtch Making a custom button will take some native work, so I think we've got to prioritize it as new effort. Sound good?

no problem, can we just hide the Okay button at all?

@matthewbirtch Edit: I'm looking into it, maybe it is in our native code...

cpoile · 2023-09-06T21:11:52Z

@matthewbirtch Ok, I think I figured out Android at least. It's our own native code that gives the Okay button. Will be fixed once mattermost/mattermost-push-proxy#106 and mattermost/mattermost-mobile#7534 roll out.
We don't have that problem with iOS -- there won't be a way to reply to our Calls notifications. You have to tap the notification, which will bring you to the channel.

matthewbirtch · 2023-09-06T21:13:54Z

@matthewbirtch Ok, I think I figured out Android at least. It's our own native code that gives the Okay button. Will be fixed once mattermost/mattermost-push-proxy#106 and mattermost/mattermost-mobile#7534 roll out. We don't have that problem with iOS -- there won't be a way to reply to our Calls notifications. You have to tap the notification, which will bring you to the channel.

Okay, thanks for investigating @cpoile. I think I'm good to approve this then

mobile push notifications

7306301

cpoile added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Aug 25, 2023

cpoile requested review from streamer45 and matthewbirtch August 25, 2023 21:42

cpoile commented Aug 25, 2023

View reviewed changes

use provided channelType

d8118b5

cpoile added 3 commits August 26, 2023 15:47

honour PushNotificationContents setting

60a8641

linter

def7124

linter

b8ffb05

streamer45 reviewed Aug 28, 2023

View reviewed changes

cpoile added 3 commits August 29, 2023 09:34

PR comments

456da6b

Merge branch 'main' into MM-53924-mobile-push-notifications

d13311e

# Conflicts: # go.mod # go.sum # server/plugin.go # server/utils.go

go mod conflicts

fb5567b

import order

2e40044

streamer45 self-requested a review August 30, 2023 19:49

streamer45 approved these changes Aug 31, 2023

View reviewed changes

server/push_notifications.go Show resolved Hide resolved

server/push_notifications.go Outdated Show resolved Hide resolved

streamer45 removed the 2: Dev Review Requires review by a core committer label Aug 31, 2023

streamer45 added this to the v0.19.0 - MM 9.0 milestone Aug 31, 2023

cpoile added 3 commits September 5, 2023 14:23

log get user error, just in case

3ef2012

Merge branch 'main' into MM-53924-mobile-push-notifications

c226c04

# Conflicts: # go.mod # go.sum

merge conflict

2164499

remove CategoryCanReply for the future

5b414d2

matthewbirtch approved these changes Sep 6, 2023

View reviewed changes

matthewbirtch removed the 1: UX Review Requires review by ux label Sep 6, 2023

cpoile merged commit eaf3383 into main Sep 6, 2023

cpoile deleted the MM-53924-mobile-push-notifications branch September 6, 2023 21:20

cpoile added the 3: Reviews Complete All reviewers have approved the pull request label Sep 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-53924 - Mobile push notifications #503

MM-53924 - Mobile push notifications #503

cpoile commented Aug 25, 2023

cpoile Aug 25, 2023

codecov-commenter commented Aug 26, 2023 •

edited

Loading

streamer45 left a comment

matthewbirtch commented Aug 29, 2023

streamer45 left a comment

cpoile commented Sep 5, 2023

matthewbirtch commented Sep 5, 2023

cpoile commented Sep 6, 2023 •

edited

Loading

cpoile commented Sep 6, 2023

matthewbirtch commented Sep 6, 2023

MM-53924 - Mobile push notifications #503

MM-53924 - Mobile push notifications #503

Conversation

cpoile commented Aug 25, 2023

Summary

Ticket Link

cpoile Aug 25, 2023

Choose a reason for hiding this comment

codecov-commenter commented Aug 26, 2023 • edited Loading

Codecov Report

streamer45 left a comment

Choose a reason for hiding this comment

matthewbirtch commented Aug 29, 2023

streamer45 left a comment

Choose a reason for hiding this comment

cpoile commented Sep 5, 2023

matthewbirtch commented Sep 5, 2023

cpoile commented Sep 6, 2023 • edited Loading

cpoile commented Sep 6, 2023

matthewbirtch commented Sep 6, 2023

codecov-commenter commented Aug 26, 2023 •

edited

Loading

cpoile commented Sep 6, 2023 •

edited

Loading