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

Support for channel notifications #2501

Merged
merged 13 commits into from
Oct 11, 2017
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Oct 5, 2017

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 2 commits October 5, 2017 12:39
Add condition type to check the sender's power level and add a base
rule using it for @channel notifications.
{
'kind': 'event_match',
'key': 'content.body',
'pattern': '*@channel*',
Copy link
Member

Choose a reason for hiding this comment

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

let's call it @room

)
auth_events = yield self.store.get_events(auth_events_ids)
auth_events = {
(e.type, e.state_key): e for e in auth_events.values()
Copy link
Member

@erikjohnston erikjohnston Oct 5, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Almost certainly, so long as its obvious how its used

@ara4n
Copy link
Member

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.

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

Choose a reason for hiding this comment

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

It's faster to do:

pl_event = context.prev_state_ids.get((EventTypes.PowerLevels, "",))
if pl_event:
   ...

@@ -1,5 +1,6 @@
# -*- coding: utf-8 -*-
# Copyright 2015, 2016 OpenMarket Ltd
# Copyright 2015 New Vector Ltd
Copy link
Member

Choose a reason for hiding this comment

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

2015?

@erikjohnston erikjohnston merged commit dfbf734 into develop Oct 11, 2017
@dbkr dbkr deleted the dbkr/channel_notifications branch October 17, 2017 12:49
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