paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-05 06:23:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

notifications: Fix double-sending in missedmessage_hook.

Browse Source 
While the missedmessage_hook logic originally did a reasonably good
job of avoiding double-sending notifications, there was a corner case
it didn't handle, namely a user who had been presence-idle when a
message was sent and became also event-queue-idle as well within the
next 10 minutes.  For those users, they got a notification at message
send time, and the missedmessage_hook would deliver it a second time.

We fix this by just checking the conveniently available push_notified
and email_notified variables that indicate whether the message already
had a notification triggered.

Fixes #7031.
This commit is contained in:
Tim Abbott
2017-10-17 21:54:03 -07:00
parent 2648d595c1
commit 513b6d624f
2 changed files with 57 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

50
zerver/tests/test_event_queue.py
View File

@@ -47,7 +47,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=False,
mentioned=False, stream_push_notify=False, stream_name=None,
always_push_notify=False, idle=True)
always_push_notify=False, idle=True, already_notified={})
self.assertTrue(email_notice is None)
self.assertTrue(mobile_notice is None)
@@ -55,15 +55,37 @@ class MissedMessageNotificationsTest(ZulipTestCase):
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=True,
mentioned=False, stream_push_notify=False, stream_name=None,
always_push_notify=False, idle=True)
always_push_notify=False, idle=True, already_notified={})
self.assertTrue(email_notice is not None)
self.assertTrue(mobile_notice is not None)
# Private message won't double-send either notice if we've
# already sent notices before.
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=True,
mentioned=False, stream_push_notify=False, stream_name=None,
always_push_notify=False, idle=True, already_notified={
'push_notified': True,
'email_notified': False,
})
self.assertTrue(email_notice is not None)
self.assertTrue(mobile_notice is None)
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=True,
mentioned=False, stream_push_notify=False, stream_name=None,
always_push_notify=False, idle=True, already_notified={
'push_notified': False,
'email_notified': True,
})
self.assertTrue(email_notice is None)
self.assertTrue(mobile_notice is not None)
# Mention sends a notice
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=False,
mentioned=True, stream_push_notify=False, stream_name=None,
always_push_notify=False, idle=True)
always_push_notify=False, idle=True, already_notified={})
self.assertTrue(email_notice is not None)
self.assertTrue(mobile_notice is not None)
@@ -71,7 +93,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=False,
mentioned=False, stream_push_notify=True, stream_name="Denmark",
always_push_notify=False, idle=True)
always_push_notify=False, idle=True, already_notified={})
self.assertTrue(email_notice is None)
self.assertTrue(mobile_notice is not None)
@@ -79,7 +101,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=True,
mentioned=False, stream_push_notify=False, stream_name=None,
always_push_notify=False, idle=False)
always_push_notify=False, idle=False, already_notified={})
self.assertTrue(email_notice is None)
self.assertTrue(mobile_notice is None)
@@ -87,7 +109,7 @@ class MissedMessageNotificationsTest(ZulipTestCase):
email_notice, mobile_notice = self.check_will_notify(
user_profile.id, message_id, private_message=True,
mentioned=False, stream_push_notify=False, stream_name=None,
always_push_notify=True, idle=False)
always_push_notify=True, idle=False, already_notified={})
self.assertTrue(email_notice is None)
self.assertTrue(mobile_notice is not None)
@@ -130,7 +152,9 @@ class MissedMessageNotificationsTest(ZulipTestCase):
mock_enqueue.assert_called_once()
args_list = mock_enqueue.call_args_list[0][0]
self.assertEqual(args_list, (user_profile.id, msg_id, False, False, False, "Denmark", False, True))
self.assertEqual(args_list, (user_profile.id, msg_id, False, False, False,
"Denmark", False, True,
{'email_notified': False, 'push_notified': False}))
# Clear the event queue, before repeating with a private message
client_descriptor.event_queue.pop()
@@ -141,7 +165,9 @@ class MissedMessageNotificationsTest(ZulipTestCase):
mock_enqueue.assert_called_once()
args_list = mock_enqueue.call_args_list[0][0]
self.assertEqual(args_list, (user_profile.id, msg_id, True, False, False, None, False, True))
self.assertEqual(args_list, (user_profile.id, msg_id, True, False,
False, None, False, True,
{'email_notified': True, 'push_notified': True}))
# Clear the event queue, now repeat with a mention
client_descriptor.event_queue.pop()
@@ -154,7 +180,9 @@ class MissedMessageNotificationsTest(ZulipTestCase):
mock_enqueue.assert_called_once()
args_list = mock_enqueue.call_args_list[0][0]
self.assertEqual(args_list, (user_profile.id, msg_id, False, True, False, "Denmark", False, True))
self.assertEqual(args_list, (user_profile.id, msg_id, False, True,
False, "Denmark", False, True,
{'email_notified': True, 'push_notified': True}))
# Clear the event queue, now repeat with stream message with stream_push_notify
stream = get_stream("Denmark", user_profile.realm)
@@ -172,7 +200,9 @@ class MissedMessageNotificationsTest(ZulipTestCase):
mock_enqueue.assert_called_once()
args_list = mock_enqueue.call_args_list[0][0]
self.assertEqual(args_list, (user_profile.id, msg_id, False, False, True, "Denmark", False, True))
self.assertEqual(args_list, (user_profile.id, msg_id, False, False,
True, "Denmark", False, True,
{'email_notified': False, 'push_notified': False}))
# Clean up the state
sub.push_notifications = True

17
zerver/tornado/event_queue.py
View File

@@ -673,8 +673,14 @@ def missedmessage_hook(user_profile_id, client, last_for_client):
idle = True
message_id = event['message']['id']
# Pass on the information on whether a push or email notification was already sent.
already_notified = dict(
push_notified = event.get("push_notified", False),
email_notified = event.get("email_notified", False),
)
maybe_enqueue_notifications(user_profile_id, message_id, private_message, mentioned,
stream_push_notify, stream_name, always_push_notify, idle)
stream_push_notify, stream_name, always_push_notify, idle,
already_notified)
def receiver_is_off_zulip(user_profile_id):
# type: (int) -> bool
@@ -687,8 +693,8 @@ def receiver_is_off_zulip(user_profile_id):
def maybe_enqueue_notifications(user_profile_id, message_id, private_message,
mentioned, stream_push_notify, stream_name,
always_push_notify, idle):
# type: (int, int, bool, bool, bool, Optional[str], bool, bool) -> Dict[str, bool]
always_push_notify, idle, already_notified):
# type: (int, int, bool, bool, bool, Optional[str], bool, bool, Dict[str, bool]) -> Dict[str, bool]
"""This function has a complete unit test suite in
`test_enqueue_notifications` that should be expanded as we add
more features here."""
@@ -702,6 +708,7 @@ def maybe_enqueue_notifications(user_profile_id, message_id, private_message,
'stream_push_notify': stream_push_notify,
}
notice['stream_name'] = stream_name
if not already_notified.get("push_notified"):
queue_json_publish("missedmessage_mobile_notifications", notice, lambda notice: None)
notified['push_notified'] = True
@@ -712,6 +719,7 @@ def maybe_enqueue_notifications(user_profile_id, message_id, private_message,
if idle and (private_message or mentioned):
# We require RabbitMQ to do this, as we can't call the email handler
# from the Tornado process. So if there's no rabbitmq support do nothing
if not already_notified.get("email_notified"):
queue_json_publish("missedmessage_emails", notice, lambda notice: None)
notified['email_notified'] = True
@@ -763,7 +771,7 @@ def process_message_event(event_template, users):
stream_name = event_template.get('stream_name')
result = maybe_enqueue_notifications(user_profile_id, message_id, private_message,
mentioned, stream_push_notify, stream_name,
always_push_notify, idle)
always_push_notify, idle, {})
result['stream_push_notify'] = stream_push_notify
extra_user_data[user_profile_id] = result
@@ -909,6 +917,7 @@ def maybe_enqueue_notifications_for_message_update(user_profile_id,
stream_name=stream_name,
always_push_notify=always_push_notify,
idle=idle,
already_notified={},
)
def process_notification(notice):