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

Implement MSC3958: suppress notifications from edits #14960

Merged
merged 7 commits into from
Feb 3, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14960.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Experimental support to suppress notifications from message edits ([MSC3958](https://github.com/matrix-org/matrix-spec-proposals/pull/3958)).
1 change: 1 addition & 0 deletions rust/benches/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ fn bench_eval_message(b: &mut Bencher) {
false,
false,
false,
false,
);

b.iter(|| eval.run(&rules, Some("bob"), Some("person")));
Expand Down
17 changes: 17 additions & 0 deletions rust/src/push/base_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,23 @@ pub const BASE_PREPEND_OVERRIDE_RULES: &[PushRule] = &[PushRule {
}];

pub const BASE_APPEND_OVERRIDE_RULES: &[PushRule] = &[
// We don't want to notify on edits. Not only can this be confusing in real
// time (2 notifications, one message) but it's especially confusing
// if a bridge needs to edit a previously backfilled message.
PushRule {
rule_id: Cow::Borrowed("global/override/.com.beeper.suppress_edits"),
priority_class: 5,
conditions: Cow::Borrowed(&[Condition::Known(KnownCondition::EventMatch(
EventMatchCondition {
key: Cow::Borrowed("content.m.relates_to.rel_type"),
pattern: Some(Cow::Borrowed("m.replace")),
pattern_type: None,
},
))]),
actions: Cow::Borrowed(&[Action::DontNotify]),
Copy link
Member

Choose a reason for hiding this comment

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

given that DontNotify is a noop, and the MSC doesn't mention it, why not an empty list here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect I copied from one of the other rules.

(I really wish we'd just get rid of dont_notify since it doesn't do anything, we seem to hit this often. 😢 )

default: true,
default_enabled: true,
},
PushRule {
rule_id: Cow::Borrowed("global/override/.m.rule.suppress_notices"),
priority_class: 5,
Expand Down
2 changes: 1 addition & 1 deletion rust/src/push/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ fn test_requires_room_version_supports_condition() {
};
let rules = PushRules::new(vec![custom_rule]);
result = evaluator.run(
&FilteredPushRules::py_new(rules, BTreeMap::new(), true, false, true, false),
&FilteredPushRules::py_new(rules, BTreeMap::new(), true, false, true, false, false),
None,
None,
);
Expand Down
8 changes: 8 additions & 0 deletions rust/src/push/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ pub struct FilteredPushRules {
msc3381_polls_enabled: bool,
msc3664_enabled: bool,
msc3952_intentional_mentions: bool,
msc3958_suppress_edits_enabled: bool,
}

#[pymethods]
Expand All @@ -431,6 +432,7 @@ impl FilteredPushRules {
msc3381_polls_enabled: bool,
msc3664_enabled: bool,
msc3952_intentional_mentions: bool,
msc3958_suppress_edits_enabled: bool,
) -> Self {
Self {
push_rules,
Expand All @@ -439,6 +441,7 @@ impl FilteredPushRules {
msc3381_polls_enabled,
msc3664_enabled,
msc3952_intentional_mentions,
msc3958_suppress_edits_enabled,
}
}

Expand Down Expand Up @@ -476,6 +479,11 @@ impl FilteredPushRules {
{
return false;
}
if !self.msc3958_suppress_edits_enabled
&& rule.rule_id == "global/override/.com.beeper.suppress_edits"
{
return false;
}

true
})
Expand Down
1 change: 1 addition & 0 deletions stubs/synapse/synapse_rust/push.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class FilteredPushRules:
msc3381_polls_enabled: bool,
msc3664_enabled: bool,
msc3952_intentional_mentions: bool,
msc3958_suppress_edits_enabled: bool,
): ...
def rules(self) -> Collection[Tuple[PushRule, bool]]: ...

Expand Down
5 changes: 5 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
self.msc3952_intentional_mentions = experimental.get(
"msc3952_intentional_mentions", False
)

# MSC3959: Do not generate notifications for edits.
self.msc3958_supress_edit_notifs = experimental.get(
"msc3958_supress_edit_notifs", False
)
Comment on lines +178 to +180
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a typo: supress -> suppress

Copy link
Member Author

Choose a reason for hiding this comment

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

That's annoying, but I think I'm going to just leave it since it is a flag that will hopefully disappear soon.

1 change: 1 addition & 0 deletions synapse/storage/databases/main/push_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ def _load_rules(
msc3664_enabled=experimental_config.msc3664_enabled,
msc3381_polls_enabled=experimental_config.msc3381_polls_enabled,
msc3952_intentional_mentions=experimental_config.msc3952_intentional_mentions,
msc3958_suppress_edits_enabled=experimental_config.msc3958_supress_edit_notifs,
)

return filtered_rules
Expand Down
42 changes: 41 additions & 1 deletion tests/push/test_bulk_push_rule_evaluator.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

from twisted.test.proto_helpers import MemoryReactor

from synapse.api.constants import EventContentFields
from synapse.api.constants import EventContentFields, RelationTypes
from synapse.api.room_versions import RoomVersions
from synapse.push.bulk_push_rule_evaluator import BulkPushRuleEvaluator
from synapse.rest import admin
Expand Down Expand Up @@ -370,3 +370,43 @@ def test_room_mentions(self) -> None:
},
)
)

@override_config({"experimental_features": {"msc3958_supress_edit_notifs": True}})
def test_suppress_edits(self) -> None:
"""Event edits should not generate notifications."""
clokep marked this conversation as resolved.
Show resolved Hide resolved
bulk_evaluator = BulkPushRuleEvaluator(self.hs)

# Create & persist an event to use as the parent of the relation.
event, context = self.get_success(
self.event_creation_handler.create_event(
self.requester,
{
"type": "m.room.message",
"room_id": self.room_id,
"content": {
"msgtype": "m.text",
"body": "helo",
},
"sender": self.alice,
},
)
)
self.get_success(
self.event_creation_handler.handle_new_client_event(
self.requester, events_and_context=[(event, context)]
)
)

# Room mentions from those without power should not notify.
self.assertFalse(
self._create_and_process(
bulk_evaluator,
{
"body": self.alice,
"m.relates_to": {
"rel_type": RelationTypes.REPLACE,
"event_id": event.event_id,
},
},
)
)