Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Incorrect notications from intentional mentions which include @room? #16460

Open
clokep opened this issue Oct 10, 2023 · 3 comments
Open

Incorrect notications from intentional mentions which include @room? #16460

clokep opened this issue Oct 10, 2023 · 3 comments
Labels
X-Needs-Info This issue is blocked awaiting information from the reporter

Comments

@clokep
Copy link
Member

clokep commented Oct 10, 2023

Support for intentional mentions was apparently stabilized in #15520 / Synapse v1.86.0rc1, right? Yet, when I just sent a message with @room in a code block, which Element Web correctly translated to m.mentions being set but without the room flag, people still got notified.

The spec says that the push rule for @room should only be enabled if the event does not have an m.mentions property.

Originally posted by @jplatte in #15661 (comment)

@clokep
Copy link
Member Author

clokep commented Oct 10, 2023

Looking at the code it doesn't seem like Synapse would generate this notification:

// For backwards-compatibility the legacy mention rules are disabled
// if the event contains the 'm.mentions' property.
if self.has_mentions
&& (rule_id == "global/override/.m.rule.contains_display_name"
|| rule_id == "global/content/.m.rule.contains_user_name"
|| rule_id == "global/override/.m.rule.roomnotif")
{
continue;
}

Are the people getting notified on updated clients that understand intentional mentions? Is the notification coming from the server or from the client?

@jplatte
Copy link
Contributor

jplatte commented Oct 10, 2023

Thanks. I really just wrote that comment to get a quick "this should work - probably an EleWeb issue" or "oh yeah we didn't actually implement that part of the spec yet". Based on what you wrote above it's the former.

@clokep
Copy link
Member Author

clokep commented Oct 10, 2023

My guess would be either:

  1. Someone is running an old Element Web (or other client that doesn't implement the new rules);
  2. A bug in a client

It definitely could be a bug in Synapse, but we do also test this behavior:

# The legacy push rule should not mention if the mentions field exists.
self.assertFalse(
self._create_and_process(
bulk_evaluator,
{
"body": "@room",
"msgtype": "m.text",
EventContentFields.MENTIONS: {},
},
)
)

@clokep clokep added the X-Needs-Info This issue is blocked awaiting information from the reporter label Oct 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Needs-Info This issue is blocked awaiting information from the reporter
Projects
None yet
Development

No branches or pull requests

2 participants