Add support for an @online/@here notification that doesn't send push/email notifications #2183

Open
timabbott opened this Issue Nov 2, 2016 · 7 comments

Projects

None yet

2 participants

@timabbott
Member

Currently, Zulip supports @all and @everyone, which both end up sending complete notifications for a message: desktop notifications for those present, adding the item to the user's "mentions" tab, push notifying mobile recipients, and emailing offline recipients.

It would be nice to add a similar feature with aliases @here/@online that just sends desktop notifications to online users who are subscribed; it'd be a lot more lightweight (no need to worry about emailing/buzzing the phone of everyone who's asleep) while still being a useful way to notify people affected by something (e.g. "@online this Zulip server is going down for 5 minutes of maintenance".)

@timabbott timabbott changed the title from Add support for an @here notification that doesn't send push/email notifications to Add support for an @online/@here notification that doesn't send push/email notifications Nov 15, 2016
@paxapy
Contributor
paxapy commented Nov 15, 2016 edited

should we show warning about group notification that prevent message to submitting like we do for @alland @everyone?
also, is there a easy way to tell if person is online?

@timabbott
Member

I think we might as well use that warning here as well; there's an open PR for making a significant change for that logic (#2264), so it's worth some care to avoid merge conflicting (might just make sense to add this piece later).

If you look at the logic in process_message_event in zerver/lib/event_queue.py, that's the place where we trigger offline notifications for users who are offline when a message is sent. We just want to change that logic to ignore these @online style mentions. The second code path is missedmessage_hook, where the backend discovers the user was in fact already offline at the time the process_message_event code had run; that should similarly not include the user in message_ids_to_notify.append.

It'd be great to have tests for this behavior in both of those code paths. The second code path is probably somewhat harder to trigger...

@paxapy
Contributor
paxapy commented Nov 15, 2016

okay, thanks for details.
I've looked to mentioned PR and found that there is not direct relation with this, because it uses current_stream.subscribers.num_items() as a counter of people that will be notified
and we here talking about only of those who online.
btw, I can continue on this after that change will be merged

@timabbott
Member
timabbott commented Nov 15, 2016 edited

Hmm, yeah, I guess maybe it's simplest to just not do the prompt for now since I'm not sure it's worth the effort of doing the data plumbing for number of online subscribers; we can just plan to do it as a follow-up project.

@paxapy
Contributor
paxapy commented Nov 16, 2016 edited

well, with frontend it's pretty clear, but on backend I still have bunch of questions
it looks like in bugdown we setting this attribute to the message instance:
https://github.com/zulip/zulip/blob/master/zerver/lib/bugdown/__init__.py#L890
which is used to set UserMessage flag later:
https://github.com/zulip/zulip/blob/master/zerver/lib/actions.py#L828
but then, in notification process, we only looks for "mentioned" flag:
https://github.com/zulip/zulip/blob/master/zerver/lib/event_queue.py#L723
so, i'm not sure these @all mentions working at all, or I get wrong somewhere..
anyways, I want your suggestions about better way to implement our new behavior.
should I add new online_mentioned flag, or just set corresponding attribute to the message instance and later pass it to event handler?

@timabbott
Member

I think a new online_mentioned flag is probably the right answer, while the effect for now is ephemeral, I can imagine wanting to use that distinction in future features as well.

@timabbott
Member

The attribute on the message object is how we pass data back from bugdown to its caller; python-markdown doesn't support returning additional stuff so we instead modify the message object to return things like that.

@timabbott timabbott added this to the Zulip roadmap milestone Nov 18, 2016
@TigorC TigorC added a commit to TigorC/zulip that referenced this issue Nov 29, 2016
@TigorC TigorC Added support for an @online/@here notification
Closes #2183
ba5ad5c
@TigorC TigorC added a commit to TigorC/zulip that referenced this issue Dec 13, 2016
@TigorC TigorC Added support for an @online/@here notification
Closes #2183
cf71852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment