MSC1930: Add a push rule for m.room.tombstone events

[turt2live]

Rendered

This has a bug. See #2223

[erikjohnston]

This is an interesting UX question. We probably do want to ensure that people get told loudly about room upgrades, but I don't know whether we want to cause everyone's pocket to vibrate. I.e. should we push this in real time or only tell the user loudly when they open the app.

[turt2live]

The running theory is that people would @room anyways, so might as well mirror that. Agreed though that we probably don't need the notify action - the highlight is probably fine.

[richvdh]

seems generally plausible to me, though I'm certainly not an expert in this area

[richvdh]

I'm not really familiar with how notification rules work, but why is it an override rule?

[turt2live]

It probably could be an underride, however I'm not really sure what the difference is either. I basically copy/pasted @room notifications here, which uses an override rule.

[jryans]

I guess I would have expected a default rule to be the lowest priority (underride), but you are right that the default rules are a mix of override and underride definitions for some reason. Who defined the override / underride system originally?

[richvdh]

@dbkr I believe...

[dbkr]

The difference between underride and override is where they come in the precedence in relation to content and room rules: underrides are lower precedence, overrides are higher. So, override rules will apply even if the user has turned off notifications for the room (in riot this is the 'mentions only' setting). Getting a notification for a room that's been closed that you've put on 'mentions only' sounds correct to me, so I think override is the right place for this.

[uhoreg]

I may have missed this in the original room upgrade discussion, but is there any reason users shouldn't be automatically joined to the new room, either by the server or by the client (or both), rather than waiting for the user to manually join it?

[turt2live]

I honestly don't recall the exact reasoning. One reason may be that the new room might be an unsupported room version, making the join fail. The other reason would probably be invite only rooms being difficult to process (despite our recommendations to not upgrade them unless you need to, so far).

[richvdh]

https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/1501-room-version-upgrades.md#have-servers-auto-join-their-users-on-upgrade discusses this, and there is some background at https://github.com/matrix-org/matrix-doc/pull/1502/files/4750b297b35f64a72a155066f4161e46d483824a#r213277100 (sorry about the lack of threading on that PR).

In short: because it was going to be inherently unreliable, I thought it was better to leave it out of the initial implementation and have users do the join (and deal with any failures) manually.

It might be a good idea to revisit that behaviour now we know a bit more about how upgrades work in practice. I don't see any good reason not to attempt an auto-join.

[uhoreg]

FWIW, I'd imagine that this proposal is still valuable even if we do auto-join at some point; presumably, client that auto-join could just ignore the bing.

[turt2live]

or mark it as read and then there's no problems

[turt2live]

have started the auto-join discussion over at https://github.com/matrix-org/matrix-doc/issues/1941

[turt2live]

There doesn't seem to be major opposition to this, and the question about where this fits seems to be solved. Hopefully people will raise concerns if they feel as though I'm jumping the gun here...

@mscbot fcp merge

[mscbot]

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

• @KitsuneRal
• @anoadragon453
• @ara4n
• @dbkr
• @erikjohnston
• @richvdh
• @turt2live
• @uhoreg

Concerns:

• should we actually fix the problem instead Membership should be transferred when upgrading a room matrix-spec#455 resolved by MSC1930: Add a push rule for m.room.tombstone events #1930 (comment)

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[ara4n]

personally, this feels like a bit of a hack around the fact we don’t autojoin users. any server which is capble of pushing its users about the need to rejoin should also be able to join on their behalf (including reaccepting invites).

if we are trying for the best UX here, i suspect we need to understand matrix-org/matrix-spec#455 a little more first.

[anoadragon453]

I echo Matthew's thoughts and think we should think about whether a proper solution is going to be that much more work than adding a push rule, and if not, work on that instead.

I am worried that the longer we delay this though the more people become confused when rooms are upgraded, leading to some potentially ugly experiences.

@mscbot concern should we actually fix the problem instead matrix-org/matrix-spec#455

[turt2live]

I think we still need this regardless of autojoining users, as per the thread at #1930 (comment)

[anoadragon453]

If we're confident that auto-join and bings can be handled cleanly on the client-side together then I don't see any reason not to add bings now.

@mscbot resolve should we actually fix the problem instead matrix-org/matrix-spec#455

[ara4n]

i just discussed this through with @richvdh and @neilisfragile to try to gauge whether it's going to be more work to implement and maintain the push rule than it is to solve matrix-org/matrix-spec#455.

I see that there's already a synapse PR for this at matrix-org/synapse#4867 - but I think we need to consider how painful it will be to change this behaviour in future. I believe the way synapse works is to clone the default pushrules for each user whenever they first set a push rule, so you end up with the defaults hardcoded into everyone's push settings. Changing the default thus requires a background job in synapse to go and fix up everyone's customised push settings from under them, which is a bit of an inconvenience.

It's a tough call to decide whether we should spend the braintime now implementing this (and then turning the volume down on it in future) versus just getting on and solving matrix-org/matrix-spec#455 - especially if we end up solving matrix-org/matrix-spec#455 in a few weeks anyway, as then the effort here will be for a very shortlived feature.

I think my hunch is still in favour of just going and doing matrix-org/matrix-spec#455 - what do others think?

[turt2live]

It entirely depends on what the scope of matrix-org/matrix-spec#455 is: if we can guarantee that members will join the new room, private or otherwise, then this isn't needed. I do strongly believe that we need this push rule though to handle the case of an upgrade happening and the server not being able to deal with it.

[turt2live]

As for muting this rule in the future: I wouldn't expect us to do that. I would expect us to not trigger it if the server is capable of handling the intent, or otherwise marking the room as read for the user so it can't trigger. That's different from turning down the default and would involve some amount of spec discussion, but I think it'd be easier and more reliable to go the route of respecting the intent of the notification rather than altering it.

[ara4n]

I do strongly believe that we need this push rule though to handle the case of an upgrade happening and the server not being able to deal with it.

Good point.

As for muting this rule in the future: I wouldn't expect us to do that. I would expect us to not trigger it if the server is capable of handling the intent

Also good point.

/me checks the box

[mscbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[mscbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[turt2live]

our process is to merge the PR (not the MSC) when it clears FCP, so doing that

[turt2live]

This also has an implementation at matrix-org/synapse#4867 - needs spec

[turt2live]

Spec PR: #2020

[turt2live]

merged 🎉