Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support for channel notifications #2501

Merged
merged 13 commits into from Oct 11, 2017

Conversation

Projects
None yet
4 participants
Member

dbkr commented Oct 5, 2017 edited

Add condition type to check the sender's power level and add a base
rule using it for @channel notifications.

Thought: should we add a layer of indirection here so a channel can configure the minimum PL for sending a channel bing?

dbkr added some commits Oct 5, 2017

Support for channel notifications
Add condition type to check the sender's power level and add a base
rule using it for @channel notifications.
synapse/push/baserules.py
+ {
+ 'kind': 'event_match',
+ 'key': 'content.body',
+ 'pattern': '*@channel*',
@ara4n

ara4n Oct 5, 2017

Owner

let's call it @room

dbkr added some commits Oct 5, 2017

synapse/push/bulk_push_rule_evaluator.py
+ )
+ auth_events = yield self.store.get_events(auth_events_ids)
+ auth_events = {
+ (e.type, e.state_key): e for e in auth_events.values()
@erikjohnston

erikjohnston Oct 5, 2017 edited

Owner

you want .itervalues() here (which returns an iterator rather than constructing a new list)

@erikjohnston

erikjohnston Oct 5, 2017

Owner

I'd probably also move this in to the else, and explicitly do:

pl_event_key = (EventTypes.PowerLevels, "", )
pl_event_id = context.prev_state_ids.get((EventTypes.PowerLevels, "",))
if pl_event_id:
    pl_event = yield self.store.get_event(pl_event_id)
    auth_events = { pl_event_key: pl_event }
else:
    ...
defer.returnValue(get_user_power_level(event.sender, auth_events))

to avoid some allocations

@dbkr

dbkr Oct 10, 2017

Member

is it worth s/values/itervalues/ in the place I copied it from and all the other places that copied it from that too? :)

@erikjohnston

erikjohnston Oct 10, 2017

Owner

Almost certainly, so long as its obvious how its used

Owner

ara4n commented Oct 5, 2017

i'd avoid overcomplicating it with the layer of indirection to set a powerlevel for channel bing. if folks want that they can just surely add a custom push rule in the client (or in their particular server). conceptually this looks fine to me, but needs input from @erikjohnston on the state wrangling.

synapse/push/bulk_push_rule_evaluator.py
@@ -109,6 +111,23 @@ def _get_rules_for_room(self, room_id):
)
@defer.inlineCallbacks
+ def _get_sender_power_level(self, event, context):
+ pl_event_key = (EventTypes.PowerLevels, "", )
+ if pl_event_key in context.prev_state_ids:
@erikjohnston

erikjohnston Oct 5, 2017

Owner

It's faster to do:

pl_event = context.prev_state_ids.get((EventTypes.PowerLevels, "",))
if pl_event:
   ...
synapse/push/push_rule_evaluator.py
@@ -1,5 +1,6 @@
# -*- coding: utf-8 -*-
# Copyright 2015, 2016 OpenMarket Ltd
+# Copyright 2015 New Vector Ltd

dbkr added some commits Oct 10, 2017

There was already a constant for this
also update copyright
Use notification levels in power_levels
Rather than making the condition directly require a specific power
level. This way the level require to notify a room can be configured
per room.

@erikjohnston erikjohnston merged commit dfbf734 into develop Oct 11, 2017

6 of 8 checks passed

Sytest Dendron (Commit) Build #2746 origin/dbkr/channel_notifications failed in 2 min 42 sec
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #3579 origin/dbkr/channel_notifications succeeded in 3 min 18 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #3676 origin/dbkr/channel_notifications succeeded in 2 min 51 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dbkr dbkr deleted the dbkr/channel_notifications branch Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment