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

Commit

Permalink
Add experimental "dont_push" push action to suppress push for notific…
Browse files Browse the repository at this point in the history
…ations

This is a potential solution to https://github.com/vector-im/riot-web/issues/3374
and element-hq/element-web#5953
as raised by Mozilla at element-hq/element-web#10868.

This lets you define a push rule action which increases the badge count (unread notification)
count on a given room, but doesn't actually send a push for that notification via email or HTTP.
We might want to define this as the default behaviour for group chats in future
to solve https://github.com/vector-im/riot-web/issues/3268 at last.

This is implemented as a string action rather than a tweak because:
 * Other pushers don't care about the tweak, given they won't ever get pushed
 * The DB can store the tweak more efficiently using the existing `notify` table.
 * It avoids breaking the default_notif/highlight_action optimisations.

Clients which generate their own notifs (e.g. desktop notifs from Riot/Web
would need to be aware of the new push action) to uphold it.

An alternative way to do this would be to maintain a `msg_count` alongside
`highlight_count` and `notification_count` in `unread_notifications` in sync responses.
However, doing this by counting the rows in `events` since the `stream_position`
of the user's last read receipt turns out to be painfully slow (~200ms), perhaps
due to the size of the events table.  So instead, we use the highly optimised
existing event_push_actions (and event_push_actions_staging) table to maintain
the counts - using the code paths which already exist for tracking unread
notification counts efficiently.  These queries are typically ~3ms or so.

The biggest issues I see here are:
 * We're slightly repurposing the `notif` field on `event_push_actions` to
   track whether a given action actually sent a `push` or not.  This doesn't
   seem unreasonable, but it's slightly naughty given that previously the
   field explicitly tracked whether `notify` was true for the action (and
   as a result, it was uselessly always set to 1 in the DB).
 * We're going to put more load on the `event_push_actions` table for all the
   random group chats which people had previously muted. In practice i don't
   think there are many of these though.
 * There isn't an MSC for this yet (although this comment could become one).
  • Loading branch information
ara4n committed Sep 18, 2019
1 parent 38fd1f8 commit 2292dc3
Showing 1 changed file with 10 additions and 5 deletions.
15 changes: 10 additions & 5 deletions synapse/storage/event_push_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ def _get_unread_counts_by_receipt_txn(
def _get_unread_counts_by_pos_txn(self, txn, room_id, user_id, stream_ordering):

# First get number of notifications.
# We don't need to put a notif=1 clause as all rows always have
# notif=1
# We ignore the notif column, given we want unread counts irrespective of
# whether the notification actually sent a push or not.
sql = (
"SELECT count(*)"
" FROM event_push_actions ea"
Expand Down Expand Up @@ -223,6 +223,7 @@ def get_after_receipt(txn):
" AND ep.user_id = ?"
" AND ep.stream_ordering > ?"
" AND ep.stream_ordering <= ?"
" AND ep.notif = 1"
" ORDER BY ep.stream_ordering ASC LIMIT ?"
)
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
Expand Down Expand Up @@ -251,6 +252,7 @@ def get_no_receipt(txn):
" AND ep.user_id = ?"
" AND ep.stream_ordering > ?"
" AND ep.stream_ordering <= ?"
" AND ep.notif = 1"
" ORDER BY ep.stream_ordering ASC LIMIT ?"
)
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
Expand Down Expand Up @@ -323,6 +325,7 @@ def get_after_receipt(txn):
" AND ep.user_id = ?"
" AND ep.stream_ordering > ?"
" AND ep.stream_ordering <= ?"
" AND ep.notif = 1"
" ORDER BY ep.stream_ordering DESC LIMIT ?"
)
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
Expand Down Expand Up @@ -351,6 +354,7 @@ def get_no_receipt(txn):
" AND ep.user_id = ?"
" AND ep.stream_ordering > ?"
" AND ep.stream_ordering <= ?"
" AND ep.notif = 1"
" ORDER BY ep.stream_ordering DESC LIMIT ?"
)
args = [user_id, user_id, min_stream_ordering, max_stream_ordering, limit]
Expand Down Expand Up @@ -400,7 +404,7 @@ def get_if_maybe_push_in_range_for_user(self, user_id, min_stream_ordering):
def _get_if_maybe_push_in_range_for_user_txn(txn):
sql = """
SELECT 1 FROM event_push_actions
WHERE user_id = ? AND stream_ordering > ?
WHERE user_id = ? AND stream_ordering > ? AND notif = 1
LIMIT 1
"""

Expand Down Expand Up @@ -429,14 +433,15 @@ def add_push_actions_to_staging(self, event_id, user_id_actions):
return

# This is a helper function for generating the necessary tuple that
# can be used to inert into the `event_push_actions_staging` table.
# can be used to insert into the `event_push_actions_staging` table.
def _gen_entry(user_id, actions):
is_highlight = 1 if _action_has_highlight(actions) else 0
notif = 0 if "dont_push" in actions else 1
return (
event_id, # event_id column
user_id, # user_id column
_serialize_action(actions, is_highlight), # actions column
1, # notif column
notif, # notif column
is_highlight, # highlight column
)

Expand Down

0 comments on commit 2292dc3

Please sign in to comment.