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 all 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:
"""Under the default push rules, event edits should not generate notifications."""
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,
},
},
)
)