Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add a default .m.rule.tombstone push rule #4867

Merged
merged 3 commits into from
Apr 29, 2019
Merged

Conversation

turt2live
Copy link
Member

@turt2live turt2live requested a review from a team March 15, 2019 20:27
@codecov
Copy link

codecov bot commented Mar 16, 2019

Codecov Report

Merging #4867 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #4867   +/-   ##
========================================
  Coverage    59.57%   59.57%           
========================================
  Files          326      326           
  Lines        33921    33921           
  Branches      5597     5597           
========================================
  Hits         20209    20209           
  Misses       12298    12298           
  Partials      1414     1414

1 similar comment
@codecov
Copy link

codecov bot commented Mar 16, 2019

Codecov Report

Merging #4867 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #4867   +/-   ##
========================================
  Coverage    59.57%   59.57%           
========================================
  Files          326      326           
  Lines        33921    33921           
  Branches      5597     5597           
========================================
  Hits         20209    20209           
  Misses       12298    12298           
  Partials      1414     1414

@turt2live turt2live removed the request for review from a team March 18, 2019 06:13
@turt2live turt2live requested a review from a team April 8, 2019 17:29
@turt2live
Copy link
Member Author

The MSC has cleared FCP, which means this is ready for actual review.

@erikjohnston
Copy link
Member

This won't cause clients to be told about this new push rule down sync, but OTOH presumably we don't want to bother spending the time wiring in that update functionality now?

@turt2live
Copy link
Member Author

I didn't realize this didn't push it down sync, but I also think that's fine. The set of clients which use push rules is very small, and I think they can easily create their own default if needed. Is it difficult to try and get it through sync?

@erikjohnston
Copy link
Member

TBH I'm not sure how easy it would be to do off the top of my head. Possibly a case of adding it to the push_rules_stream table for all the users ever? That might take a while though

@turt2live
Copy link
Member Author

I'll give it a try and see how badly I can break things

@turt2live turt2live self-assigned this Apr 9, 2019
@turt2live
Copy link
Member Author

I'm not sure how possible it is to do the background update (at least at my level). I can probably stumble my way through getting the update to work, however I don't have a good metric for progress or point of continuance when processing users.

Theory was to figure out all the users which need the push rule and call self.store.set_push_rule_enabled over the set, which causes the push_rules_stream to work and for the values to be updated in the push_rules_enabled table.

For progress I wanted to do some sort of lookup for who has push notifications enabled and who doesn't have an entry for .m.rule.tombstone in the table - the goal was to avoid updating push rules for the millions of bridge users which don't need it. However, trying to select who has the master rule disabled (because it is backwards) yields interesting results:

t2lsynapse=# select * from push_rules_enable where rule_id = 'global/override/.m.rule.master' and user_name = '@travis:t2l.io';
 id | user_name | rule_id | enabled
----+-----------+---------+---------
(0 rows)

t2lsynapse=# select count(*) from push_rules_enable where user_name = '@travis:t2l.io';
 count
-------
   593
(1 row)

I do have push notifications working though, and looking at another homeserver's database I can see that sometimes the push rule is there and sometimes not :(

So in summary: I think it's fine to expect clients to make their own defaults, or resync, or something for this rule because writing this background update looks painful. In practice I don't think there's very many clients which actually use push rules.

@turt2live turt2live removed their assignment Apr 10, 2019
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@turt2live turt2live merged commit 8c5b1e3 into develop Apr 29, 2019
@turt2live turt2live deleted the travis/tombstone-notif branch April 29, 2019 21:40
anoadragon453 added a commit that referenced this pull request Apr 30, 2019
* develop: (34 commits)
  Add a default .m.rule.tombstone push rule (#4867)
  Fix infinite loop in presence handler
  changelog
  more logging improvements
  remove extraneous exception logging
  Clarify logging when PDU signature checking fails
  Changelog
  Add --no-pep-517 to README instructions
  set PIP_USE_PEP517 = False for tests
  Fix handling of SYNAPSE_NO_TLS in docker image (#5005)
  Config option for verifying federation certificates (MSC 1711) (#4967)
  Remove log error for .well-known/matrix/client (#4972)
  Prevent "producer not unregistered" message (#5009)
  add gpg key fingerprint
  Don't crash on lack of expiry templates
  Update debian install docs for new key and repo (#5074)
  Add management endpoints for account validity
  Send out emails with links to extend an account's validity period
  Make sure we're not registering the same 3pid twice
  Newsfile
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants