[Story] Show that I have been mentioned in the push notification

[VolkerJunginger]

Story

As a user that has been mentioned I want to see this in the push so that I can react fast.

Figma link: TBD

Acceptance criteria

• show that user has been mentioned in the push

Size estimate

M

Dependencies

MSC

• MSC4028: Push all encrypted events except for muted rooms matrix-org/matrix-spec-proposals#4028
• Support MSC4028 (Push all encrypted events except for muted rooms) matrix-org/synapse#15847
• https://github.com/matrix-org/internal-config/issues/1469

Preconditions

• We need backend support for the new push rule but we can simulate the behavior by setting a similar push rule at the room level.

Open questions

Questions

Beta Give feedback

1. Can we implement MSC4028 in Synapse? -> Yes it is coming
2. What kind of copy should we display for the mention in push notifications?
3. Would the copy be different between the mention of the reading user and the mentions of other users if multiples are contained?
4. We need to call this API to allow the MSC 4028 push rule to be enabled or disabled we will not need it given the new MSC spec
5. The push rule might not even exist on the server so we would need to also have an API that allows us to check if it is present or not (otherwise the Mentions Only option is probably useless to have for encrypted rooms)
6. Could we consider this story done if the feature is not enabled on matrix.org?

Subtasks

Android

Beta Give feedback

1. Mentions: Push notifications display user mentions differently element-x-android#1451
   A-Notifications T-Enhancement X-Needs-Design +3
   
2. Mentions: Add a warning on servers where MSC 4028 is not supported element-x-android#1794
   A-Notifications T-Enhancement X-Needs-Design +3
   

iOS

Beta Give feedback

1. Mentions: Push notifications display user mentions differently element-x-ios#1825
   A-Notifications T-Enhancement X-Needs-Design +3
   
2. Mentions: Add a warning on servers where MSC 4028 is not supported element-x-ios#2078
   A-Notifications T-Enhancement X-Needs-Design +3

Rust

Beta Give feedback

1. Implement MSC3952: Intentional mentions matrix-org/matrix-rust-sdk#2606
   A-Notification A-Room-List A-Timeline X-Needs-Rust Z-Schedule +5
   
2. Notification Item should contain a flag to know if the user has been mentioned matrix-org/matrix-rust-sdk#2834
   +0
3. ffi(bindings): check if MSC 4028 is enabled on the homeserver matrix-org/matrix-rust-sdk#2840
   +0

Design

Beta Give feedback

1. How the mention text is displayed

• QA signoff on completion
• Design signoff on completion
• Product signoff on completion

[manuroe]

Can we implement MSC4028 in Synapse?

@pmaier1 What do you think about this open question we have for this story? This is backend dependency we need to make mentions work in e2e rooms.

[pmaier1]

Yes, we're working on this here.

[Velin92]

What is the expected behaviour in how we display the user mention in the push notifications? What is the expected copy that replaces the user mention link? (This is actually something that we can do at a copy level by just detecting the permalink, however we would only be able to get the user id of the mention data coming from the SDK) Is there any additional copy too?
@manuroe
@VolkerJunginger
@amshakal

EDIT:
Actually we also able to use the original string without the need of the display name, that wrapped the link if the mention was sent as an hyperlink for example Test , this is usually the display name however a mention can also be just the url directly, which means that this solution is not always going to work, so we might still need the display name provided by the SDK

[manuroe]

@giomfo, can you share here the trick to simulate matrix-org/matrix-spec-proposals#4028 while the HS is not yet ready, please?

[giomfo]

To simulate MSC4028, I plugged on an existing button the following code (in the Legacy iOS-client) to add/remove a specific Override push rule.
This rule is added only for the current user, it force the server to push all encrypted event for this account. You just have to define a unique ID (here "encrypted_marcel"):

            if let rule = notificationCenter?.rule(byId: "encrypted_marcel") {
                removePushRule(rule: rule, completion: completion)
                return
            } else {
                notificationCenter?.addOverrideRule(
                    withId: "encrypted_marcel",
                    conditions: [["kind": "event_match", "key": "type", "pattern": "m.room.encrypted"]],
                    notify: true,
                    sound: false,
                    highlight: false
                )
                completion()
                return
            }

In parallel I used a web client to configure some rooms in the Mention&Keywords mode.
Then my mobile client was able to receive all encrypted events and displayed only the mentions for these rooms.

[hughns]

Further to this topic coming up in the weekly BWI drop syncs I have made https://github.com/matrix-org/internal-config/issues/1469 to track the request to enable it on matrix.org. There were various discussions in rooms but hopefully this issue can be authoritative on the formal request.

I asked @manuroe in the EX Leads room but it is probably better to ask here:

1. What status does MSC4028 need to be in for delivery at the end of this month? It is currently a draft MSC that needs some work based on feedback from the implementation that was done in Synapse.
2. What level of testing is actually needed ahead of the delivery? Whilst matrix.org is convenient can these needs be met through some other homeservers (beta.matrix.org, element.io etc.)

CC @giomfo as the MSC author.

[hughns]

One further thought: if it is needed on matrix.org would it be acceptable to make it only apply to a preconfigured list of users? (this is an approach used for some other features).

[giomfo]

Hi @hughns The MSC4028 is in draft because it is waiting for a first implementation on the client side.
Note: the expected (suggested) implementation is different for the Legacy Element mobile and EX. I created a GH issue for Element-iOS, Android and Web to support this new MSC (see the MSC history here). I didn't create GH issues for EX (but #2065 seems to be the right entry point).

I think the implementation in EX should be the minimum to take this MSC out of the draft mode, but the implementation in Legacy mobile clients and EW will be required before this MSC goes in Production.

About testing, element.io may be a better candidate than matrix.org. But the idea of a preconfigured list of users seems a good plan to test on matrix.org too.
If I understand correctly, the new push rule org.matrix.msc4028.encrypted_event (= .m.rule.encrypted_event) would be enabled server side only for this preconfigured list of users

[hughns]

I have made an EMS staging homeserver - https://msc4028-staging.ems.host/ - to aide with folks testing. It should allow registration from anyone with a @element.io email address. When I tried it with Element Web I found that email notifications were enabled by default, which is helpful to see that impact.

@giomfo The following is what I would recommend for testing (and rollout) to support you progressing the MSC:

1. Use https://msc4028-staging.ems.host to test and point any relevant folks at it.
2. Once you have EX client support available for testing and you have tested and confirmed that the email and non-EX client behaviour is okay, then put in request tickets to enable on element.io and beta.matrix.org.
3. After 2) and assuming feasible (check with backend) then enable for a list of users on matrix.org.
4. Once it is in the spec then enable for all users.

How does that sound?

On progressing this generally I made the following notes/observations that might be helpful to you:

• given how this underpins a key part of EX user expectations I suggest you progress the MSC as soon as is practical and engage in the feedback on the MSC. It is the fastest way to get it adopted.
• given that not all homeservers will support this (both today and in future) I suggest that the clients have a decent way to handle this case (otherwise the app will appear to be broken). e.g. the client determines whether the feature is available and then disabled/greys-out the options that are not available

[manuroe]

Thanks @hughns !

I am not sure we need the discoverability for this feature and add complexity to the app for a future "legacy" behavior. EX is a whole with the feature enabled in the feature. We need to make the right behavior when the app will be production ready someone in early 2023. I will double check it with @VolkerJunginger after the October release.

[Velin92]

@manuroe I have recently tested the EMS staging homeserver for MSC 4028 that was provided by @hughns
And when the notifications settings of a room are set to "Mentions only" you correctly receive push notifications only and only when you are mentioned either directly or indirectly though @room.

The only problem is that the msc4028 push rule to allow pushing all the events to encrypted room while having a default value of true, is also going to not be enabled immediately since:

Contrary to the stable push rule, this unstable one should not be enabled by default until the clients are ready to support a minimum this MSC.

So we would need to first call the following API on Element X to allow for this, and we should decide when and how this should be called (if that's what we want to do), and it would be useful to have an API to check if this push rule is present or not (otherwise offering the ability to set Mentions Only in an encrypted room is pretty useless)

Maybe we could call this API when we are also setting the push rule to be Mentions Only?
We might also need an API t make sure that the push rule exists on the homeserver.

[giomfo]

@Velin92 Hi, FYI I just updated the MSC here to have the unstable push rule enabled by default as soon as the server owner decides to support this MSC.

@clokep are you ok to update the synapse implementation (to enable by default the unstable push rule) in order to avoid the workaround suggested by @Velin92 client side?

cc @hughns (to plan an EMS staging homeserver update)

[Velin92]

@Velin92 Hi, FYI I just updated the MSC here to have the unstable push rule enabled by default as soon as the server owner decides to support this MSC.

@clokep are you ok to update the synapse implementation (to enable by default the unstable push rule) in order to avoid the workaround suggested by @Velin92 client side?

cc @hughns (to plan an EMS staging homeserver update)

This would make things a lot of easier, since we would ned just a single API to check if such push rule exists and is set to true to decide to show the Mentions only option in encrypted rooms, without the need of setting the API ourselves.

[Velin92]

@VolkerJunginger @manuroe
Question!
Since with @callumu we are thinking of adding a copy when you are mentioned something like "You have been mention" or "X mentioned you", should this also appear for messages that are replies to messages that contain mentions but where the reply does not?

Because as of right now also replies to mentioning messages are considered as containing the mentions, which means that also replies to mentions will be delivered as notifications for the "Mentions only" setting.

[VolkerJunginger]

I'll try to untangle this:

1. A reply to mention does not mean the user is mentioned again. A mention must always be intentional. A reply to a message that contains a mention is only an intentional mention when the new reply message also contains a "new" mention.

2. The notification should only mention that the user has been mentioned if a mention is an intentional mention. (I mentioned that in 1 😂)

3. I am not 100% sure if we already have a different copy for notifications that contain a reply (User X replied to your message). If we do there is a hierarchy:

    1. Intentional mentions (User mentioned you)
    2. Replies to the users message (user replied to your message)
   

If a user is mentioned in a reply only show that the user has been mentioned.

[Velin92]

I'll try to untangle this:

1. A reply to mention does not mean the user is mentioned again. A mention must always be intentional. A reply to a message that contains a mention is only an intentional mention when the new reply message also contains a "new" mention.
2. The notification should only mention that the user has been mentioned if a mention is an intentional mention. (I mentioned that in 1 😂)
3. I am not 100% sure if we already have a different copy for notifications that contain a reply (User X replied to your message). If we do there is a hierarchy:

    1. Intentional mentions (User mentioned you)
    2. Replies to the users message (user replied to your message)
   

If a user is mentioned in a reply only show that the user has been mentioned.

@VolkerJunginger The weird thing is that for example El-Web still sends replies to a mentioned message with intentional mentions (but this could be a bug actually...)
Also I think we still need to support for backwards compatibility the legacy mention system, but this will definitely also make replies with mentions still be considered a reply, we can ofc drop the legacy support compltely, maybe we should just not do that right now, but a bit later when MSC 4028 and intentional mentions MSC are more common/developed?

Regarding replies, no we don't have yet a custom copy or a way to detect in the notification if is a reply to the user message, we probably need a story/epic/ticket about this.

[jplatte]

The weird thing is that for example El-Web still sends replies to a mentioned message with intentional mentions (but this could be a bug actually...)

That would definitely be a bug, the spec says that those should be filtered out.

Also I think we still need to support for backwards compatibility the legacy mention system, but this will definitely also make replies with mentions still be considered a reply, we can ofc drop the legacy support compltely, maybe we should just not do that right now, but a bit later when MSC 4028 and intentional mentions MSC are more common/developed?

All of this should be handled through push rules transparently (they should contain rules that any event with m.mentions should not trigger the "legacy" rules); this is why set_mentions must be called for every content event if there are no mentions. However, this doesn't actually work in EleWeb despite it sending m.mentions, I haven't had time to look into why.

[Velin92]

The weird thing is that for example El-Web still sends replies to a mentioned message with intentional mentions (but this could be a bug actually...)

That would definitely be a bug, the spec says that those should be filtered out.

Also I think we still need to support for backwards compatibility the legacy mention system, but this will definitely also make replies with mentions still be considered a reply, we can ofc drop the legacy support compltely, maybe we should just not do that right now, but a bit later when MSC 4028 and intentional mentions MSC are more common/developed?

All of this should be handled through push rules transparently (they should contain rules that any event with m.mentions should not trigger the "legacy" rules); this is why set_mentions must be called for every content event if there are no mentions. However, this doesn't actually work in EleWeb despite it sending m.mentions, I haven't had time to look into why.

You think we can safely drop the legacy push rules for mentions rn? Since now the app supports sending intentional mentions.

[jplatte]

No, I don't think we should drop anything. What do you even mean by that? Disabling the rules for each user account that logs in with EX? That would disable them across all clients the user is using, AFAIK.

[Velin92]

No, I don't think we should drop anything. What do you even mean by that? Disabling the rules for each user account that logs in with EX? That would disable them across all clients the user is using, AFAIK.

Right sorry then I think I didn't understand, how would you suggest to transparently bypass the legacy rules in favour of the intentional mentions one?

[jplatte]

You don't have to do anything. It should already work.

[giomfo]

@jplatte @Velin92 About Intentional Mentions spec, see the section "Impact on replies". Don't you think El-Web follows this recommendation?

[Velin92]

You don't have to do anything. It should already work.

@jplatte you are right! just tested it by sending a reply to a message that contains mentions with a new message that does not contain any from El-X to El-X and it is not seen/recognised as an highlighted event, which is in line with the acceptance criteria we defined here, then what I experienced is caused by El-Web adding the intentional mention also for the replies.
Then we don't need to worry about this @VolkerJunginger

@giomfo by reading the spec it says:

Clients should include the sender of the event being replied to as well as any mentioned users in that event (excluding yourself) in the new event's m.mentions property

According to the spec El-Web seems to behave correctly however... I don't really see the point in mentioning all the users of a reply of a message that contains mentions, if the new message does not contain any, contains only a subset or a new set of mentions, also this would definitely clash with the criteria that were defined here.

[jplatte]

Oh, that's the recommendation from the MSC? That is exactly counter to what product ppl. have been saying. But now I remember we also discussed this in the context of Ruma, which only contains a convenience method to set mentions in a reply such that they don't re-mention people originally mentioned. I guess this that was just based on popular opinion then, because I can't find anything about this in https://spec.matrix.org/latest/client-server-api/.

[clokep]

Oh, that's the recommendation from the MSC? That is exactly counter to what product ppl. have been saying.

I think I mentioned this earlier in the thread, but the goal of the MSC was to not change behavior of mentions, but to make them intentional and fix all the buggy behavior around it.

We can change the recommendation but it was purposefully left to follow up work so that we could get that MSC through.

[clokep]

@clokep are you ok to update the synapse implementation (to enable by default the unstable push rule) in order to avoid the workaround suggested by @Velin92 client side?

Yes, but I want to be sure we're actually on the same page about all this behavior, the planning has been too adhoc and I don't think we're all in agreement about how it should work.

[Velin92]

After having a chat with @VolkerJunginger since as of right now probably all the server do not support MSC 4028 and some users might not understand why the "Mentions and keywords only" option does not work at all, we should definitely have a warning shown if the server does not support MSC 4028.

The idea that as soon as this screen is loaded:

We check through the API I exposed here if the homeserver supports MSC 4028 and if not, an alert saying something like "be aware that your homeserver does not support the Mentions only options for ecrypted rooms which may result in the loss of all pushes if enabled" (final copy TBD) is shown.

This should be shown in Advanced Settings -> Notifications -> Group or Direct, as soon as the view is loaded
This should also be shown in Room Details -> Notifications -> Custom Settings on, but only and only if the room is encrypted otherwise we can avoid that

@amshakal @callumu can we have a chat and decide the final copy?
Ofc also the Alert thing is just a suggestion, it can also just be a description/subtitle of the "Mentions Only" option, if the alert is too intrusive.

[giomfo]

@Velin92 I think the last point mentioned just above has been fixed by element-hq/element-x-ios#2081

I would like to converge about another point that you raised here. After our discussion, I agreed with you, and I updated the MSC here to have the unstable push rule enabled by default as soon as the server owner decides to support this MSC. As far as I know, the synapse implementation was not updated to take into account this spec change yet.
Can you confirm that this change would still make sense to you before I renew my demand to the backend team?

[giomfo]

@manuroe @VolkerJunginger: about the MSC4028 test plan suggested by @hughns here, we are currently at the step 1).
Before be able to consider step 2):

• I have to clarify this point with the dev team
• I need to know if the unread notification counts is expected at the room level in EX. Some implementations are required at the client side for that, see the "unread notification count" section in the MSC.
• I will have to manage some potential requests to the backend team (for example, implement MSC4062: Add a new push rule tweak to disable email notification))

The most important to know is: what is the due date of this story?

[hughns]

@giomfo a question came up from the Data Sync Working Group when we were discussing around this topic - the question was: how does this MSC interact with throttling that iOS/Apple (and possibly Android/Firebase) apply to push notifications that do not result in a notification being shown to the user?

For iOS in Pushing background updates to your App it says: (my emphasis)

The system treats background notifications as low priority: you can use them to refresh your app’s content, but the system doesn’t guarantee their delivery. In addition, the system may throttle the delivery of background notifications if the total number becomes excessive. The number of background notifications allowed by the system depends on current conditions, but don’t try to send more than two or three per hour.

(There is also a reference to throttling at https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/CreatingtheNotificationPayload.html#//apple_ref/doc/uid/TP40008194-CH10-SW8)

A quick search hasn't found anything authoritative for Android.

I note that this doesn't invalidate the MSC (which is concerned about a new type of rule irrespective of the notification channel used), but it may significantly limit it's practical use on iOS (and Android).

I don't see any reference to throttling or rate limiting in the MSC itself.

[giomfo]

@hughns the limitation that you found in Apple documentation is only about what they name "Background Update Notifications". The number of this kind of notification is limited because it is used to make run the full application in background. By default a push notification is supposed to make run only the app extension: Notification Service Extension (NSE).
As far as I know, we don't trigger for the moment any background update notification for Element. This kind of notification is out of the MSC scope.

I understand your concerns about the increase in the number of notifications introduced by this MSC. I don't know if there is an actual limitation on notification number at the push provider level (APNs or Firebase). But when I prepared this MSC the main potential issue raised was at the server side, see the section Push sending in the Potential issues list. Several risks have been identified and some alternatives have been suggested. Only a full-scale test can help to sort these risks and handle them. Your suggested test plan is ideal for that.

[giomfo]

@manuroe @VolkerJunginger: about the MSC4028 test plan suggested by @hughns here, we are currently at the step 1). Before be able to consider step 2):

• I have to clarify this point with the dev team
• I need to know if the unread notification counts is expected at the room level in EX. Some implementations are required at the client side for that, see the "unread notification count" section in the MSC.
• I will have to manage some potential requests to the backend team (for example, implement MSC4062: Add a new push rule tweak to disable email notification))

The most important to know is: what is the due date of this story?

About the first point, I synced with @Velin92. We agreed that the unstable push rule should be enabled by default. I filed element-hq/synapse#16846 for that.

@VolkerJunginger @manuroe is there a due date for this story? the test plan will take time