notifications: Simplify how triggers are passed around.

This removes the utterly unnecessary `triggers` dict (which always was
a dict with exactly one value True) in favor of a single field,
'trigger'.

Inspired by Kunal Gupta's work in #6659.
This commit is contained in:
Tim Abbott
2017-10-18 21:37:35 -07:00
parent 735b49e505
commit e98ca0714b
4 changed files with 29 additions and 95 deletions

View File

@@ -388,14 +388,14 @@ def get_alert_from_message(message):
Determine what alert string to display based on the missed messages. Determine what alert string to display based on the missed messages.
""" """
sender_str = message.sender.full_name sender_str = message.sender.full_name
if message.recipient.type == Recipient.HUDDLE and message.triggers['private_message']: if message.recipient.type == Recipient.HUDDLE and message.trigger == 'private_message':
return "New private group message from %s" % (sender_str,) return "New private group message from %s" % (sender_str,)
elif message.recipient.type == Recipient.PERSONAL and message.triggers['private_message']: elif message.recipient.type == Recipient.PERSONAL and message.trigger == 'private_message':
return "New private message from %s" % (sender_str,) return "New private message from %s" % (sender_str,)
elif message.recipient.type == Recipient.STREAM and message.triggers['mentioned']: elif message.recipient.type == Recipient.STREAM and message.trigger == 'mentioned':
return "New mention from %s" % (sender_str,) return "New mention from %s" % (sender_str,)
elif (message.recipient.type == Recipient.STREAM and elif (message.recipient.type == Recipient.STREAM and
(message.triggers['stream_push_notify'] and message.stream_name)): (message.trigger == 'stream_push_notify' and message.stream_name)):
return "New stream message from %s in %s" % (sender_str, message.stream_name,) return "New stream message from %s in %s" % (sender_str, message.stream_name,)
else: else:
return "New Zulip mentions and private messages from %s" % (sender_str,) return "New Zulip mentions and private messages from %s" % (sender_str,)
@@ -503,12 +503,7 @@ def handle_push_notification(user_profile_id, missed_message):
umessage = UserMessage.objects.get(user_profile=user_profile, umessage = UserMessage.objects.get(user_profile=user_profile,
message__id=missed_message['message_id']) message__id=missed_message['message_id'])
message = umessage.message message = umessage.message
triggers = missed_message['triggers'] message.trigger = missed_message['trigger']
message.triggers = {
'private_message': triggers['private_message'],
'mentioned': triggers['mentioned'],
'stream_push_notify': triggers['stream_push_notify'],
}
message.stream_name = missed_message.get('stream_name', None) message.stream_name = missed_message.get('stream_name', None)
if umessage.flags.read: if umessage.flags.read:

View File

@@ -184,21 +184,13 @@ class EditMessageSideEffectsTest(ZulipTestCase):
mobile_event = queue_messages[0]['event'] mobile_event = queue_messages[0]['event']
self.assertEqual(mobile_event['user_profile_id'], cordelia.id) self.assertEqual(mobile_event['user_profile_id'], cordelia.id)
self.assertEqual(mobile_event['triggers'], dict( self.assertEqual(mobile_event['trigger'], 'mentioned')
stream_push_notify=False,
private_message=False,
mentioned=True,
))
self.assertEqual(queue_messages[1]['queue_name'], 'missedmessage_emails') self.assertEqual(queue_messages[1]['queue_name'], 'missedmessage_emails')
email_event = queue_messages[1]['event'] email_event = queue_messages[1]['event']
self.assertEqual(email_event['user_profile_id'], cordelia.id) self.assertEqual(email_event['user_profile_id'], cordelia.id)
self.assertEqual(email_event['triggers'], dict( self.assertEqual(email_event['trigger'], 'mentioned')
stream_push_notify=False,
private_message=False,
mentioned=True,
))
def test_second_mention_is_ignored(self): def test_second_mention_is_ignored(self):
# type: () -> None # type: () -> None

View File

@@ -342,11 +342,7 @@ class HandlePushNotificationTest(PushNotificationTest):
missed_message = { missed_message = {
'message_id': message.id, 'message_id': message.id,
'triggers': { 'trigger': 'private_message',
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
},
} }
with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=''), \ with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=''), \
mock.patch('zerver.lib.push_notifications.requests.request', mock.patch('zerver.lib.push_notifications.requests.request',
@@ -401,11 +397,7 @@ class HandlePushNotificationTest(PushNotificationTest):
missed_message = { missed_message = {
'user_profile_id': self.user_profile.id, 'user_profile_id': self.user_profile.id,
'message_id': message.id, 'message_id': message.id,
'triggers': { 'trigger': 'private_message',
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
},
} }
with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=''), \ with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=''), \
mock.patch('zerver.lib.push_notifications.requests.request', mock.patch('zerver.lib.push_notifications.requests.request',
@@ -452,11 +444,7 @@ class HandlePushNotificationTest(PushNotificationTest):
missed_message = { missed_message = {
'message_id': message.id, 'message_id': message.id,
'triggers': { 'trigger': 'private_message',
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
},
} }
apn.handle_push_notification(user_profile.id, missed_message) apn.handle_push_notification(user_profile.id, missed_message)
@@ -471,11 +459,7 @@ class HandlePushNotificationTest(PushNotificationTest):
missed_message = { missed_message = {
'message_id': message.id, 'message_id': message.id,
'triggers': { 'trigger': 'private_message',
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
},
} }
with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=True), \ with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=True), \
mock.patch('zerver.lib.push_notifications.get_apns_payload', mock.patch('zerver.lib.push_notifications.get_apns_payload',
@@ -514,11 +498,7 @@ class HandlePushNotificationTest(PushNotificationTest):
missed_message = { missed_message = {
'message_id': message.id, 'message_id': message.id,
'triggers': { 'trigger': 'private_message',
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
},
} }
with mock.patch('zerver.lib.push_notifications.get_apns_payload', with mock.patch('zerver.lib.push_notifications.get_apns_payload',
return_value={'apns': True}), \ return_value={'apns': True}), \
@@ -629,44 +609,28 @@ class TestGetAlertFromMessage(PushNotificationTest):
def test_get_alert_from_private_group_message(self): def test_get_alert_from_private_group_message(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.HUDDLE) message = self.get_message(Recipient.HUDDLE)
message.triggers = { message.trigger = 'private_message'
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
}
alert = apn.get_alert_from_message(message) alert = apn.get_alert_from_message(message)
self.assertEqual(alert, "New private group message from King Hamlet") self.assertEqual(alert, "New private group message from King Hamlet")
def test_get_alert_from_private_message(self): def test_get_alert_from_private_message(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.PERSONAL) message = self.get_message(Recipient.PERSONAL)
message.triggers = { message.trigger = 'private_message'
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
}
alert = apn.get_alert_from_message(message) alert = apn.get_alert_from_message(message)
self.assertEqual(alert, "New private message from King Hamlet") self.assertEqual(alert, "New private message from King Hamlet")
def test_get_alert_from_mention(self): def test_get_alert_from_mention(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.STREAM) message = self.get_message(Recipient.STREAM)
message.triggers = { message.trigger = 'mentioned'
'private_message': False,
'mentioned': True,
'stream_push_notify': False,
}
alert = apn.get_alert_from_message(message) alert = apn.get_alert_from_message(message)
self.assertEqual(alert, "New mention from King Hamlet") self.assertEqual(alert, "New mention from King Hamlet")
def test_get_alert_from_stream_message(self): def test_get_alert_from_stream_message(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.STREAM) message = self.get_message(Recipient.STREAM)
message.triggers = { message.trigger = 'stream_push_notify'
'private_message': False,
'mentioned': False,
'stream_push_notify': True,
}
message.stream_name = 'Denmark' message.stream_name = 'Denmark'
alert = apn.get_alert_from_message(message) alert = apn.get_alert_from_message(message)
self.assertEqual(alert, "New stream message from King Hamlet in Denmark") self.assertEqual(alert, "New stream message from King Hamlet in Denmark")
@@ -674,11 +638,7 @@ class TestGetAlertFromMessage(PushNotificationTest):
def test_get_alert_from_other_message(self): def test_get_alert_from_other_message(self):
# type: () -> None # type: () -> None
message = self.get_message(0) message = self.get_message(0)
message.triggers = { message.trigger = 'stream_push_notify'
'private_message': False,
'mentioned': False,
'stream_push_notify': False,
}
alert = apn.get_alert_from_message(message) alert = apn.get_alert_from_message(message)
alert = apn.get_alert_from_message(self.get_message(0)) alert = apn.get_alert_from_message(self.get_message(0))
self.assertEqual(alert, self.assertEqual(alert,
@@ -689,11 +649,7 @@ class TestGetAPNsPayload(PushNotificationTest):
def test_get_apns_payload(self): def test_get_apns_payload(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.HUDDLE) message = self.get_message(Recipient.HUDDLE)
message.triggers = { message.trigger = 'private_message'
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
}
payload = apn.get_apns_payload(message) payload = apn.get_apns_payload(message)
expected = { expected = {
'alert': { 'alert': {
@@ -717,11 +673,7 @@ class TestGetGCMPayload(PushNotificationTest):
message.content = 'a' * 210 message.content = 'a' * 210
message.rendered_content = 'a' * 210 message.rendered_content = 'a' * 210
message.save() message.save()
message.triggers = { message.trigger = 'mentioned'
'private_message': False,
'mentioned': True,
'stream_push_notify': False,
}
user_profile = self.example_user('hamlet') user_profile = self.example_user('hamlet')
payload = apn.get_gcm_payload(user_profile, message) payload = apn.get_gcm_payload(user_profile, message)
@@ -745,11 +697,7 @@ class TestGetGCMPayload(PushNotificationTest):
def test_get_gcm_payload_personal(self): def test_get_gcm_payload_personal(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.PERSONAL, 1) message = self.get_message(Recipient.PERSONAL, 1)
message.triggers = { message.trigger = 'private_message'
'private_message': True,
'mentioned': False,
'stream_push_notify': False,
}
user_profile = self.example_user('hamlet') user_profile = self.example_user('hamlet')
payload = apn.get_gcm_payload(user_profile, message) payload = apn.get_gcm_payload(user_profile, message)
expected = { expected = {
@@ -770,11 +718,7 @@ class TestGetGCMPayload(PushNotificationTest):
def test_get_gcm_payload_stream_notifications(self): def test_get_gcm_payload_stream_notifications(self):
# type: () -> None # type: () -> None
message = self.get_message(Recipient.STREAM, 1) message = self.get_message(Recipient.STREAM, 1)
message.triggers = { message.trigger = 'stream_push_notify'
'private_message': False,
'mentioned': False,
'stream_push_notify': True,
}
message.stream_name = 'Denmark' message.stream_name = 'Denmark'
user_profile = self.example_user('hamlet') user_profile = self.example_user('hamlet')
payload = apn.get_gcm_payload(user_profile, message) payload = apn.get_gcm_payload(user_profile, message)

View File

@@ -702,11 +702,14 @@ def maybe_enqueue_notifications(user_profile_id, message_id, private_message,
if (idle or always_push_notify) and (private_message or mentioned or stream_push_notify): if (idle or always_push_notify) and (private_message or mentioned or stream_push_notify):
notice = build_offline_notification(user_profile_id, message_id) notice = build_offline_notification(user_profile_id, message_id)
notice['triggers'] = { if private_message:
'private_message': private_message, notice['trigger'] = 'private_message'
'mentioned': mentioned, elif mentioned:
'stream_push_notify': stream_push_notify, notice['trigger'] = 'mentioned'
} elif stream_push_notify:
notice['trigger'] = 'stream_push_notify'
else:
raise AssertionError("Unknown notification trigger!")
notice['stream_name'] = stream_name notice['stream_name'] = stream_name
if not already_notified.get("push_notified"): if not already_notified.get("push_notified"):
queue_json_publish("missedmessage_mobile_notifications", notice, lambda notice: None) queue_json_publish("missedmessage_mobile_notifications", notice, lambda notice: None)