paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-13 10:26:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

notifications: Optimize push notifications code path in tests.

Browse Source 
This checks if push_notification_enabled() is set to false in
handle_push_notification and adds an early return statement.

This is a significant performance optimization for our unit tests
because the push notifications code path does a number of database
queries, and this migration means we don't end up doing those queries
the hundreds of times we send PMs or mentions in our tests where we're
not trying to test the push notifications functionality.

This should also have a small message sending scalability improvement
for any Zulip servers without push notifications enabled.

Tweaked by tabbott to fix a few small issues.

Fixes #10895.
This commit is contained in:
ishanrai05
2018-12-11 11:35:40 +05:30
committed by Tim Abbott
parent a63eae48cc
commit 4105fb683b
4 changed files with 39 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
zerver/lib/push_notifications.py
View File

@@ -649,6 +649,8 @@ def handle_push_notification(user_profile_id: int, missed_message: Dict[str, Any
missed_message is the event received by the missed_message is the event received by the
zerver.worker.queue_processors.PushNotificationWorker.consume function. zerver.worker.queue_processors.PushNotificationWorker.consume function.
""" """
if not push_notifications_enabled():
return
user_profile = get_user_profile_by_id(user_profile_id) user_profile = get_user_profile_by_id(user_profile_id)
if not (receives_offline_push_notifications(user_profile) or if not (receives_offline_push_notifications(user_profile) or
receives_online_notifications(user_profile)): receives_online_notifications(user_profile)):

32
zerver/tests/test_push_notifications.py
View File

@@ -450,7 +450,8 @@ class HandlePushNotificationTest(PushNotificationTest):
"trigger:%s event:" "trigger:%s event:"
"push_notification" % (self.user_profile.id,)) "push_notification" % (self.user_profile.id,))
def test_disabled_notifications(self) -> None: @mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True)
def test_disabled_notifications(self, mock_push_notifications: mock.MagicMock) -> None:
user_profile = self.example_user('hamlet') user_profile = self.example_user('hamlet')
user_profile.enable_online_email_notifications = False user_profile.enable_online_email_notifications = False
user_profile.enable_online_push_notifications = False user_profile.enable_online_push_notifications = False
@@ -459,8 +460,10 @@ class HandlePushNotificationTest(PushNotificationTest):
user_profile.enable_stream_push_notifications = False user_profile.enable_stream_push_notifications = False
user_profile.save() user_profile.save()
apn.handle_push_notification(user_profile.id, {}) apn.handle_push_notification(user_profile.id, {})
mock_push_notifications.assert_called()
def test_read_message(self) -> None: @mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True)
def test_read_message(self, mock_push_notifications: mock.MagicMock) -> None:
user_profile = self.example_user('hamlet') user_profile = self.example_user('hamlet')
message = self.get_message(Recipient.PERSONAL, type_id=1) message = self.get_message(Recipient.PERSONAL, type_id=1)
UserMessage.objects.create( UserMessage.objects.create(
@@ -474,6 +477,7 @@ class HandlePushNotificationTest(PushNotificationTest):
'trigger': 'private_message', 'trigger': 'private_message',
} }
apn.handle_push_notification(user_profile.id, missed_message) apn.handle_push_notification(user_profile.id, missed_message)
mock_push_notifications.assert_called_once()
def test_deleted_message(self) -> None: def test_deleted_message(self) -> None:
"""Simulates the race where message is deleted before handlingx push notifications""" """Simulates the race where message is deleted before handlingx push notifications"""
@@ -492,8 +496,10 @@ class HandlePushNotificationTest(PushNotificationTest):
do_delete_message(user_profile, message) do_delete_message(user_profile, message)
with mock.patch('zerver.lib.push_notifications.uses_notification_bouncer') as mock_check, \ with mock.patch('zerver.lib.push_notifications.uses_notification_bouncer') as mock_check, \
mock.patch('logging.error') as mock_logging_error: mock.patch('logging.error') as mock_logging_error, \
mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True) as mock_push_notifications:
apn.handle_push_notification(user_profile.id, missed_message) apn.handle_push_notification(user_profile.id, missed_message)
mock_push_notifications.assert_called_once()
# Check we didn't proceed through and didn't log anything. # Check we didn't proceed through and didn't log anything.
mock_check.assert_not_called() mock_check.assert_not_called()
mock_logging_error.assert_not_called() mock_logging_error.assert_not_called()
@@ -516,8 +522,10 @@ class HandlePushNotificationTest(PushNotificationTest):
# This should log an error # This should log an error
with mock.patch('zerver.lib.push_notifications.uses_notification_bouncer') as mock_check, \ with mock.patch('zerver.lib.push_notifications.uses_notification_bouncer') as mock_check, \
mock.patch('logging.error') as mock_logging_error: mock.patch('logging.error') as mock_logging_error, \
mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True) as mock_push_notifications:
apn.handle_push_notification(user_profile.id, missed_message) apn.handle_push_notification(user_profile.id, missed_message)
mock_push_notifications.assert_called_once()
# Check we didn't proceed through. # Check we didn't proceed through.
mock_check.assert_not_called() mock_check.assert_not_called()
mock_logging_error.assert_called_once() mock_logging_error.assert_called_once()
@@ -579,7 +587,8 @@ class HandlePushNotificationTest(PushNotificationTest):
mock.patch('zerver.lib.push_notifications' mock.patch('zerver.lib.push_notifications'
'.send_apple_push_notification') as mock_send_apple, \ '.send_apple_push_notification') as mock_send_apple, \
mock.patch('zerver.lib.push_notifications' mock.patch('zerver.lib.push_notifications'
'.send_android_push_notification') as mock_send_android: '.send_android_push_notification') as mock_send_android, \
mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True) as mock_push_notifications:
apn.handle_push_notification(self.user_profile.id, missed_message) apn.handle_push_notification(self.user_profile.id, missed_message)
mock_send_apple.assert_called_with(self.user_profile.id, mock_send_apple.assert_called_with(self.user_profile.id,
@@ -587,6 +596,7 @@ class HandlePushNotificationTest(PushNotificationTest):
{'apns': True}) {'apns': True})
mock_send_android.assert_called_with(android_devices, mock_send_android.assert_called_with(android_devices,
{'gcm': True}) {'gcm': True})
mock_push_notifications.assert_called_once()
@override_settings(SEND_REMOVE_PUSH_NOTIFICATIONS=True) @override_settings(SEND_REMOVE_PUSH_NOTIFICATIONS=True)
def test_send_remove_notifications_to_bouncer(self) -> None: def test_send_remove_notifications_to_bouncer(self) -> None:
@@ -643,11 +653,13 @@ class HandlePushNotificationTest(PushNotificationTest):
self.make_stream('public_stream') self.make_stream('public_stream')
message_id = self.send_stream_message("iago@zulip.com", "public_stream", "test") message_id = self.send_stream_message("iago@zulip.com", "public_stream", "test")
missed_message = {'message_id': message_id} missed_message = {'message_id': message_id}
with mock.patch('zerver.lib.push_notifications.logger.error') as mock_logger: with mock.patch('zerver.lib.push_notifications.logger.error') as mock_logger, \
mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True) as mock_push_notifications:
apn.handle_push_notification(self.user_profile.id, missed_message) apn.handle_push_notification(self.user_profile.id, missed_message)
mock_logger.assert_called_with("Could not find UserMessage with " mock_logger.assert_called_with("Could not find UserMessage with "
"message_id %s and user_id %s" % "message_id %s and user_id %s" %
(message_id, self.user_profile.id,)) (message_id, self.user_profile.id,))
mock_push_notifications.assert_called_once()
def test_user_message_soft_deactivated(self) -> None: def test_user_message_soft_deactivated(self) -> None:
"""This simulates a condition that should only be an error if the user is """This simulates a condition that should only be an error if the user is
@@ -685,7 +697,8 @@ class HandlePushNotificationTest(PushNotificationTest):
'.send_apple_push_notification') as mock_send_apple, \ '.send_apple_push_notification') as mock_send_apple, \
mock.patch('zerver.lib.push_notifications' mock.patch('zerver.lib.push_notifications'
'.send_android_push_notification') as mock_send_android, \ '.send_android_push_notification') as mock_send_android, \
mock.patch('zerver.lib.push_notifications.logger.error') as mock_logger: mock.patch('zerver.lib.push_notifications.logger.error') as mock_logger, \
mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True) as mock_push_notifications:
apn.handle_push_notification(self.user_profile.id, missed_message) apn.handle_push_notification(self.user_profile.id, missed_message)
mock_logger.assert_not_called() mock_logger.assert_not_called()
mock_send_apple.assert_called_with(self.user_profile.id, mock_send_apple.assert_called_with(self.user_profile.id,
@@ -693,6 +706,7 @@ class HandlePushNotificationTest(PushNotificationTest):
{'apns': True}) {'apns': True})
mock_send_android.assert_called_with(android_devices, mock_send_android.assert_called_with(android_devices,
{'gcm': True}) {'gcm': True})
mock_push_notifications.assert_called_once()
class TestAPNs(PushNotificationTest): class TestAPNs(PushNotificationTest):
def devices(self) -> List[DeviceToken]: def devices(self) -> List[DeviceToken]:
@@ -845,7 +859,8 @@ class TestGetAPNsPayload(PushNotificationTest):
} }
self.assertDictEqual(payload, expected) self.assertDictEqual(payload, expected)
def test_get_apns_payload_huddle_message(self) -> None: @mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value = True)
def test_get_apns_payload_huddle_message(self, mock_push_notifications: mock.MagicMock) -> None:
user_profile = self.example_user("othello") user_profile = self.example_user("othello")
message_id = self.send_huddle_message( message_id = self.send_huddle_message(
self.sender.email, self.sender.email,
@@ -878,6 +893,7 @@ class TestGetAPNsPayload(PushNotificationTest):
} }
} }
self.assertDictEqual(payload, expected) self.assertDictEqual(payload, expected)
mock_push_notifications.assert_called()
def test_get_apns_payload_stream_message(self): def test_get_apns_payload_stream_message(self):
# type: () -> None # type: () -> None

2
zerver/tests/test_signup.py
View File

@@ -462,7 +462,7 @@ class LoginTest(ZulipTestCase):
with queries_captured() as queries: with queries_captured() as queries:
self.register(self.nonreg_email('test'), "test") self.register(self.nonreg_email('test'), "test")
# Ensure the number of queries we make is not O(streams) # Ensure the number of queries we make is not O(streams)
self.assert_length(queries, 79) self.assert_length(queries, 73)
user_profile = self.nonreg_user('test') user_profile = self.nonreg_user('test')
self.assertEqual(get_session_dict_user(self.client.session), user_profile.id) self.assertEqual(get_session_dict_user(self.client.session), user_profile.id)
self.assertFalse(user_profile.enable_stream_desktop_notifications) self.assertFalse(user_profile.enable_stream_desktop_notifications)

18
zerver/tests/test_unread.py
View File

@@ -208,11 +208,14 @@ class PointerTest(ZulipTestCase):
class UnreadCountTests(ZulipTestCase): class UnreadCountTests(ZulipTestCase):
def setUp(self) -> None: def setUp(self) -> None:
self.unread_msg_ids = [ with mock.patch('zerver.lib.push_notifications.push_notifications_enabled',
self.send_personal_message( return_value = True) as mock_push_notifications_enabled:
self.example_email("iago"), self.example_email("hamlet"), "hello"), self.unread_msg_ids = [
self.send_personal_message( self.send_personal_message(
self.example_email("iago"), self.example_email("hamlet"), "hello2")] self.example_email("iago"), self.example_email("hamlet"), "hello"),
self.send_personal_message(
self.example_email("iago"), self.example_email("hamlet"), "hello2")]
mock_push_notifications_enabled.assert_called()
# Sending a new message results in unread UserMessages being created # Sending a new message results in unread UserMessages being created
def test_new_message(self) -> None: def test_new_message(self) -> None:
@@ -482,7 +485,9 @@ class PushNotificationMarkReadFlowsTest(ZulipTestCase):
"message_id").values_list("message_id", flat=True)) "message_id").values_list("message_id", flat=True))
@override_settings(SEND_REMOVE_PUSH_NOTIFICATIONS=True) @override_settings(SEND_REMOVE_PUSH_NOTIFICATIONS=True)
def test_track_active_mobile_push_notifications(self) -> None: @mock.patch('zerver.lib.push_notifications.push_notifications_enabled', return_value=True)
def test_track_active_mobile_push_notifications(self, mock_push_notifications: mock.MagicMock) -> None:
mock_push_notifications.return_value = True
self.login(self.example_email("hamlet")) self.login(self.example_email("hamlet"))
user_profile = self.example_user('hamlet') user_profile = self.example_user('hamlet')
stream = self.subscribe(user_profile, "test_stream") stream = self.subscribe(user_profile, "test_stream")
@@ -530,3 +535,4 @@ class PushNotificationMarkReadFlowsTest(ZulipTestCase):
result = self.client_post("/json/mark_all_as_read", {}) result = self.client_post("/json/mark_all_as_read", {})
self.assertEqual(self.get_mobile_push_notification_ids(user_profile), self.assertEqual(self.get_mobile_push_notification_ids(user_profile),
[]) [])
mock_push_notifications.assert_called()