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

Silent unexpected notifications #640

Closed
Tracked by #1794
manuroe opened this issue Jun 20, 2023 · 3 comments · Fixed by #1041
Closed
Tracked by #1794

Silent unexpected notifications #640

manuroe opened this issue Jun 20, 2023 · 3 comments · Fixed by #1041
Assignees

Comments

@manuroe
Copy link
Member

manuroe commented Jun 20, 2023

Make sure we do not display notifications for pushed events that do not match a push rule.

User story: element-hq/element-meta#1794.

@jmartinesp
Copy link
Member

jmartinesp commented Jun 29, 2023

I'm new to this part of the app, so I'm not sure if I'm doing something wrong but in the current EAX:

  1. When notifications are set to 'mentions and keywords only', no push notifications are received for messages in encrypted rooms even if I mention the user, use a registered keyword or reply to some of their messages: the actual push notification that triggers app wakeup to process the incoming push are never received at the device. These seem to work fine in non-encrypted rooms. I don't know if this is expected.
  2. When 'mentions and keywords only' is enabled in both encrypted and unencrypted rooms, setting filterByPushRules to true or false in Client.getNotificationItem makes no difference for events that don't mention the user because that code is never reached (I guess it's filtered out in the backend?).
  3. The default notification when we can't load the actual event data should probably be improved. Right now it prints something like: Message {EVENT_ID} in room {ROOM_ID} which is kind of useful for debugging, but seems broken for a end-user.

Given the first point, I'm not sure how I can send an event that the device will receive but will later evaluate using the push rules. My bet was on the 'mentions and keywords only' behaviour, but it seems like it's not working as I expected it to.

@jmartinesp
Copy link
Member

AFAIK push rules are being verified by Client.getNotificationItem(..., filterByPushRules = true). However, I spoke with @Velin92 before (thanks!) and he mentioned this parameter is not reliable right now, as seen in this issue.

I'm wondering what we should do on Android, set it to true and just miss the edge cases? Set it to false and try to somehow filter the notifications on the app (if it's possible at all), or set it to false and just ignore the push rules altogether?

@jmartinesp
Copy link
Member

jmartinesp commented Jun 29, 2023

Also, just to double check, which types of events should we display in the notifications?

  • Messages.
  • Room State changes (all?).
  • Membership changes (all?).

Anything else? I'm asking because right now, with the code we have from the initial implementation, every single type is being displayed, with some placeholder messages:

private fun MessageLikeEventContent.toContent(): String {
    return use {
        when (it) {
            MessageLikeEventContent.CallAnswer -> "CallAnswer"
            MessageLikeEventContent.CallCandidates -> "CallCandidates"
            MessageLikeEventContent.CallHangup -> "CallHangup"
            MessageLikeEventContent.CallInvite -> "CallInvite"
            MessageLikeEventContent.KeyVerificationAccept -> "KeyVerificationAccept"
            MessageLikeEventContent.KeyVerificationCancel -> "KeyVerificationCancel"
            MessageLikeEventContent.KeyVerificationDone -> "KeyVerificationDone"
            MessageLikeEventContent.KeyVerificationKey -> "KeyVerificationKey"
            MessageLikeEventContent.KeyVerificationMac -> "KeyVerificationMac"
            MessageLikeEventContent.KeyVerificationReady -> "KeyVerificationReady"
            MessageLikeEventContent.KeyVerificationStart -> "KeyVerificationStart"
            is MessageLikeEventContent.ReactionContent -> "Reacted to ${it.relatedEventId.take(8)}"
            MessageLikeEventContent.RoomEncrypted -> "RoomEncrypted"
            is MessageLikeEventContent.RoomMessage -> it.messageType.toContent()
            MessageLikeEventContent.RoomRedaction -> "RoomRedaction"
            MessageLikeEventContent.Sticker -> "Sticker"
        }
    }
}   

EDIT: taking a look at iOS' code, it seems like it's only for messages and invites, everything else should use the default notification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants