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 the dont_notify and coalesce push rule actions. #1501

Merged
merged 3 commits into from
May 3, 2023

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Apr 25, 2023

Per MSC3987, these should both be considered no-ops.

Preview: https://pr1501--matrix-spec-previews.netlify.app

Per MSC3987, these should both be considered no-ops.
Comment on lines +187 to +191
{{% boxes/note %}}
Older versions of the Matrix specification included the `dont_notify` and
`coalesce` actions. These should both be considered no-ops (ignored, not
rejected) if received from a client.
{{% /boxes/note %}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fear is that someone will reject a push rule with actions of ["dont_notify"], which is equivalent to [].

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to evaluate this change for a system where 1.3 and 1.11 servers will co-exists and am wondering: In terms of RFC2119-language, would the "ignoring without rejecting" behavior be a MUST or a SHOULD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably a MUST, old clients might still use dont_notify?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think that would make sense. Contrary to what I wrote in #1782 (comment) this morning this might actually be a case where keyword usage in an info box would be helpful.

@clokep
Copy link
Contributor Author

clokep commented Apr 25, 2023

(This is a draft since the MSC hasn't actually passed FCP yet.)

@clokep clokep marked this pull request as ready for review May 1, 2023 11:24
@clokep clokep requested a review from a team as a code owner May 1, 2023 11:24
@clokep
Copy link
Contributor Author

clokep commented May 1, 2023

(This is a draft since the MSC hasn't actually passed FCP yet.)

FCP has finished, putting up for review.

Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

The change that you've made looks good, but I think we should also be removing the places where the rules are used, e.g. in https://spec.matrix.org/v1.6/client-server-api/#predefined-rules

A grep says that there's 5 more references to dont_notify in content/client-server-api/modules/push.md, and it's also in data/api/client-server/pushrules.yaml (3x) and in data/event-schemas/examples/m.push_rules.yaml (2x)

@clokep clokep requested a review from uhoreg May 3, 2023 11:37
@clokep
Copy link
Contributor Author

clokep commented May 3, 2023

I think we should also be removing the places where the rules are used, e.g. in spec.matrix.org/v1.6/client-server-api/#predefined-rules

Oops, I forgot to do that. Thanks for noticing! 👍

@uhoreg uhoreg merged commit e1dc5f8 into matrix-org:main May 3, 2023
10 checks passed
@clokep clokep deleted the clokep/push-action-clean-up branch May 3, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants