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

Add experimental "dont_push" push action to suppress push for notifications #6061

Closed
wants to merge 2 commits into from

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Sep 19, 2019

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 column.
  • 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).

…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).
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

other than that, I think this seems reasonable.

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

This is probably more erik's territory but just to note that this might need some care to make sure the db does a sensible thing as this table is generally massive.

@eternaleye
Copy link

Could the notification here, rather than incrementing the notification count, increment a named counter?

Benefits:

  • Not overloading the meaning of the notification counter
    • Can still keep track of how many "true" notifications there are on the room
  • Allows for users to have separate counters for various purposes
  • Still uses the efficient engine

@ara4n
Copy link
Member Author

ara4n commented Sep 24, 2019

Having spoken this through with @erikjohnston, conclusions are:

  • It would be nicer if we separated out a missed-notif count from a unread-msg count properly
  • But doing so ends up being pretty invasive
  • So this is okay for now.
  • We might want to rename the notif column in EPA to be push, given that's what it now describes, but this isn't a blocker
  • I should write an MSC for this.
  • We might want to namespace dont_push as im.vector.dont_push and only de-namespace it once the MSC is approved.

Separately, I'm trying to figure out really what the UX should be for badges in Riot in general, and whether this is really going to help that much.

@richvdh
Copy link
Member

richvdh commented Dec 4, 2019

this doesn't seem to be going anywhere, so I'm going to close it for now at least.

@richvdh richvdh closed this Dec 4, 2019
@richvdh
Copy link
Member

richvdh commented Apr 5, 2022

for reference, this got replaced by msc2625 and element-hq/element-web#7673, which themselves later got backed out and replaced.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants