First cut of a module to automatically change push rule actions when new users register

[babolivier]

This work is happening in the context of the Tchap mainlining. See https://github.com/matrix-org/matrix-dinsic/issues/856 for more information about why this specific module is needed.

The mypy failure is expected since matrix-org/synapse#12406 hasn't been released in a stable version of Synapse yet. It passes locally when using that PR's branch.

Ideally the module should check for invalid push rules at startup but implementing a push rules parser sounds like a pretty big job, and I'm not sure it'd be worth it.

[reivilibre]

I don't have access to the issue you linked for context, but I wonder whether we should be using the 'server-default rules' mechanism built in to push rules, perhaps exposing it as a config option? (as grim as config options can be).

[babolivier]

Removing the request for review because this is not actually the right way to resolve this issue.

Also, @reivilibre:

the 'server-default rules' mechanism built in to push rules

What are you referring to by this?

If you're suggesting allowing the config to change the server's default push rules, this sounds like a bad idea to me because

1. we don't want existing rules to change under the feet of users who already exist
2. Synapse's default push rules are defined by the spec
3. the push rules code in Synapse is very non-trivial to hack into

[babolivier]

I've updated the module to change the push rule's actions instead of creating a new one, which is a better approach to the problem this module is trying to solve.

[reivilibre]

If you're suggesting allowing the config to change the server's default push rules, this sounds like a bad idea to me because

1. we don't want existing rules to change under the feet of users who already exist

This is fair enough but I wasn't sure whether this was a requirement or not without context.

2. Synapse's default push rules are defined by the spec

The spec defines a minimal set of default rules, but specifies that homeservers are able to specify others, so I don't think this point is pertinent here.

3. the push rules code in Synapse is very non-trivial to hack into

Adding server-default push rules doesn't seem that difficult, though. It's not as though you need to hack into the way they're evaluated :).

[babolivier]

This is fair enough but I wasn't sure whether this was a requirement or not without context.

Yeah, I was under the impression that everyone in the Synapse team had access to that repo, but that's not the case :/
I've asked for its access to be broadened.

but specifies that homeservers are able to specify others

I've missed that. In this case, it might not be too bad (but in general my experience with adding "simple" things to the push rules management code of Synapse has been that it's never as simple as you initially believe it to be).
However, the goal of this work is to mainline matrix-org/synapse-dinsic#60, which is about modifying existing push rules rather than creating new ones (hence the change of editing actions vs creating new rules), and it feels to me like more work to do that with additional server-side rules (instead of automatically modifying existing ones).

(also actually ignore what I said earlier, we don't really care about changing this under the feet of existing users because it shouldn't be an actual change in UX)

[babolivier]

Another issue with using configurable server-side rules is that if you change them they're not automatically sent to clients of existing users, despite the new rules applying to them as well. So the notification rules would get out of sync between the clients and the server until the clients hit the "clear cache and reload" (or equivalent) button.

[squahtx]

Looks good, assuming matrix-org/synapse#12406 lands

[squahtx]

lgtm!