Skip to content
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

remove unused dont_notify and coalesce push actions #643

Closed
richvdh opened this issue Jun 10, 2020 · 8 comments · Fixed by matrix-org/matrix-spec-proposals#3987
Closed
Labels
A-Client-Server Issues affecting the CS API A-Push wart A point where the protocol is inconsistent or inelegant

Comments

@richvdh
Copy link
Member

richvdh commented Jun 10, 2020

cf: https://matrix.org/docs/spec/client_server/r0.6.1#actions. dont_notify is a no-op and its presence causes confusion. coalesce has never been implemented afaik

@richvdh richvdh added the wart A point where the protocol is inconsistent or inelegant label Jun 10, 2020
@richvdh
Copy link
Member Author

richvdh commented Jun 10, 2020

also in the cleaning-up-pushrules arena: #637

@richvdh
Copy link
Member Author

richvdh commented Jun 10, 2020

for reference: these actions both date back to matrix-org/matrix-spec-proposals#10. I think they are relics of an early draft which never got cleaned up.

@turt2live turt2live added the A-Client-Server Issues affecting the CS API label Jun 10, 2020
@ara4n
Copy link
Member

ara4n commented Jun 10, 2020

dont_notify is not a no-op - it's a means of telling the evaluator to stop executing rules when it hits a matching rule with action dont_notify. I guess your point is that you could just leave the actions empty for this situation (given we default to not sending a notif), but I'd be concerned that in reality we have a tonne of these actions in the wild already, and explicitly spelling out that the action expected by matching a rule is not to notify may not be a bad thing.

curl -s 'https://matrix.org/_matrix/client/r0/pushrules/' -H "Authorization: Bearer $at" | grep -c dont_notify
102

@ara4n
Copy link
Member

ara4n commented Jun 10, 2020

the rationale for coalesce's existence is that we had this functionality pre-Matrix in Amdocs UC and it was actually quite useful - beyond a given threshold we'd start saying "206 more messages in #foo" rather than sending 206 pushes in a row. It obviously hasn't been fleshed out here though, but I wonder whether we should at least reserve the keyword for things like this, even if it hasn't been fleshed out.

@turt2live
Copy link
Member

given we gatekeep the spec and require the use of namespaces for custom implementations, do we need to reserve the keyword?

@richvdh
Copy link
Member Author

richvdh commented Jun 11, 2020

dont_notify is not a no-op

Unless I'm misunderstanding how the rules work - in which case, I would be delighted to be corrected - it is absolutely a no-op. Including it in the list has absolutely no effect. It's even valid to have a push rule with both notify and dont_notify (which, because dont_notify has no effect, is the same as notify).

Now, there may be an argument for having a specified no-op, but: it is a no-op.

I guess your point is that you could just leave the actions empty for this situation (given we default to not sending a notif),

That is exactly my point, and I contend that its very existence is a source of confusion, since it leads to the questions of "when do I need to set it? what happens if I don't set it? Is it a no-op or not?"

but I'd be concerned that in reality we have a tonne of these actions in the wild already

Well, I wouldn't propose forbidding its use: just marking it deprecated.

and explicitly spelling out that the action expected by matching a rule is not to notify may not be a bad thing.

I think it would be less of a bad thing if clients had do to it: which is to say that for a rule to be valid, it has to either specify notify or dont_notify. But the fact is that this is not currently enforced, leaving us to support a mix of some rules where dont_notify is explicit and some where it's left implicit.

At the very least, the spec should make clear that it is a no-op. But I think we can go further.

@richvdh richvdh added the A-Push label Jun 12, 2020
@auscompgeek
Copy link
Contributor

dont_notify is used in matrix-org/matrix-spec-proposals#2153.

@richvdh
Copy link
Member Author

richvdh commented Jul 6, 2020

dont_notify is used in matrix-org/matrix-spec-proposals#2153.

yes, but not in a way that makes any material difference here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Client-Server Issues affecting the CS API A-Push wart A point where the protocol is inconsistent or inelegant
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants