Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MM-58292] Suppress push notifications to call thread if receiver is in call #807

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

streamer45
Copy link
Collaborator

Summary

PR adds some logic to suppress mobile push notifications for replies to a call thread if the receiver is in that call. This is to mimic (see note) the behavior of the web/desktop client-side hook.

Note: I haven't found an easy way to check for mentions, so we'd be disabling any type of notification, which I find reasonable for the call thread, given the user should have the app open somewhere.

Ticket Link

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

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Jul 4, 2024
@streamer45 streamer45 added this to the v0.29.0 / MM 9.11 milestone Jul 4, 2024
@streamer45 streamer45 requested a review from cpoile July 4, 2024 15:27
@streamer45 streamer45 self-assigned this Jul 4, 2024
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a solid solution, notifications have been such a pain. Thanks for fixing it.

@@ -215,3 +215,34 @@ func (s *Store) GetCallSessionsCount(callID string, opts GetCallSessionOpts) (in

return count, nil
}

func (s *Store) IsUserInCall(userID, callID string, opts GetCallSessionOpts) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the performance impact of this approach? I guess we don't really have any other option, and it's not an event that will happen too often, but just curious.

Copy link
Collaborator Author

@streamer45 streamer45 Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpoile I'd expect this to be extremely efficient, given that callID is an index and the sessions table is very small. Plus the query doesn't need to return any structured data. The only further optimization would be an index on calls_sessions.userid, but given the premise (small table + filter on call), it's not worth it since a scan will be the best option anyway. To put it in comparison, I think it could be more costly the RPC hook call itself than the DB query.

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jul 8, 2024
@streamer45 streamer45 merged commit 45318db into main Jul 8, 2024
18 checks passed
@streamer45 streamer45 deleted the MM-58292 branch July 8, 2024 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants