-
Notifications
You must be signed in to change notification settings - Fork 364
Add experimental support for MSC3768: Push rule action for in-app notifications #18720
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
base: develop
Are you sure you want to change the base?
Add experimental support for MSC3768: Push rule action for in-app notifications #18720
Conversation
848e221
to
5f921e4
Compare
5f921e4
to
948514a
Compare
…ifications Signed-off-by: Johannes Marbach <[email protected]>
948514a
to
389a870
Compare
synapse/handlers/push_rules.py
Outdated
@@ -154,6 +154,9 @@ def check_actions(actions: List[Union[str, JsonDict]]) -> None: | |||
# ignored (resulting in no action from the pusher). | |||
if a in ["notify", "dont_notify", "coalesce"]: | |||
pass | |||
# In-app only notification as per MSC3768 | |||
elif a == "org.matrix.msc3768.notify_in_app": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"org.matrix.msc3768.notify_in_app"
should be a constant; along with the others but we should at-least do good with this new one for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added one for org.matrix.msc3768.notify_in_app
in 4215344. I can also add constants for the other three if you're ok with the PR growing a bit? Looks like it'd be 30 to 40 occurrences to replace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also add constants for the other three if you're ok with the PR growing a bit? Looks like it'd be 30 to 40 occurrences to replace.
I'd be fine with it but for the sake of others trying to follow along on this PR we should do it separately if you're keen.
@@ -174,6 +174,9 @@ pub enum Action { | |||
Notify, | |||
SetTweak(SetTweak), | |||
|
|||
// In-app only notification as per MSC3768 | |||
NotifyInApp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle this at this spot given "org.matrix.msc3768.notify_in_app"
shouldn't result in push notifications?
synapse/rust/src/push/evaluator.rs
Lines 227 to 229 in 458e641
// Filter out "dont_notify" and "coalesce" actions, as we don't store them | |
// (since they result in no action by the pushers). | |
.filter(|a| **a != Action::DontNotify && **a != Action::Coalesce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think this should stay as it is. The doc comment for run
says:
/// Returns the set of actions, if any, that match (filtering out any
///dont_notify
andcoalesce
actions).
dont_notify
and coalesce
are probably removed because they are literally equivalent to an empty actions array. If we also remove notify_in_app
here, I think that would lead to the event not causing any notification at all?
Co-authored-by: Eric Eastwood <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
Co-authored-by: Eric Eastwood <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
Co-authored-by: Eric Eastwood <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
@@ -0,0 +1 @@ | |||
Add experimental support for MSC3768 (push rule action for in-app notifications). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this discussion on the MSC that this might be better suited as a responsibility of the client or OS, should this PR/MSC move forward? (seems like you agree with the points)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that suppressing pushes on the server could be a MAY. It's relatively simple and saves at least some processing cost. So generally, I'd still like this to go ahead. However, for the sake of the MSC, we could also leave it in draft if you'd rather not merge it. Draft PRs are usually sufficient for the process, especially when they received reviewed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If leaving the PR unmerged is viable to you, that works for me. I'm not really convinced by the concept at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean on the Synapse side or in general?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the current reasoning in the MSC, I'm not seeing the value of this proposal yet. The Synapse implementation appears technically sound, but I'm open to either:
- Not merging this PR if you agree, or
- Gathering additional perspectives to help us decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see. No, I'm totally fine with not merging this. Just thought you might have feedback on the MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this back to draft since it seems like functionally we're done. Thanks a lot for your review. 🙏
@@ -0,0 +1 @@ | |||
Add experimental support for MSC3768 (push rule action for in-app notifications). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If leaving the PR unmerged is viable to you, that works for me. I'm not really convinced by the concept at the moment.
else: | ||
pass # The spec doesn't forbid introducing custom actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous discussion, #18720 (comment)
If we're doing this behavior and can justify it, we should just do it all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would basically make the whole function obsolete then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like mostly. I think we could still validate known rules like set_tweak
🤷 although an unknown variant should also pass by according to the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. 🤔
Well given that we won't merge this at the moment, I'm going to leave this unaddressed for now.
Signed-off-by: Johannes Marbach <[email protected]>
Signed-off-by: Johannes Marbach <[email protected]>
This adds experimental support for matrix-org/matrix-spec-proposals#3768.
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.