Fix push notifications for soft-deactivated users.

Previously, these push notification events were being generated, but
then ignored in handle_push_notification because there was no
user_message object.

zerver/lib/push_notifications.py

@@ -26,6 +26,7 @@ import ujson
 from zerver.decorator import statsd_increment
 from zerver.lib.avatar import absolute_avatar_url
 from zerver.lib.exceptions import ErrorCode, JsonableError
+from zerver.lib.message import access_message
 from zerver.lib.queue import retry_event
 from zerver.lib.timestamp import datetime_to_timestamp, timestamp_to_datetime
 from zerver.lib.utils import generate_random_token
@@ -502,21 +503,28 @@ def handle_push_notification(user_profile_id, missed_message):
             receives_online_notifications(user_profile)):
         return
 
-    try:
+    user_profile = get_user_profile_by_id(user_profile_id)
-        umessage = UserMessage.objects.get(user_profile=user_profile,
+    (message, user_message) = access_message(user_profile, missed_message['message_id'])
-                                           message__id=missed_message['message_id'])
+    if user_message is not None:
-    except UserMessage.DoesNotExist:
+        # If ther user has read the message already, don't push-notify.
-        logging.error("Could not find UserMessage with message_id %s and user_id %s" % (
+        #
-            missed_message['message_id'], user_profile_id))
+        # TODO: It feels like this is already handled when things are
-        return
+        # put in the queue; maybe we should centralize this logic with
+        # the `zerver/tornado/event_queue.py` logic?
+        if user_message.flags.read:
+            return
+    else:
+        # Users should only be getting push notifications into this
+        # queue for messages they haven't received if they're
+        # long-term idle; anything else is likely a bug.
+        if not user_profile.long_term_idle:
+            logging.error("Could not find UserMessage with message_id %s and user_id %s" % (
+                missed_message['message_id'], user_profile_id))
+            return
 
-    message = umessage.message
     message.trigger = missed_message['trigger']
     message.stream_name = missed_message.get('stream_name', None)
 
-    if umessage.flags.read:
-        return
-
     apns_payload = get_apns_payload(message)
     gcm_payload = get_gcm_payload(user_profile, message)
     logging.info("Sending push notification to user %s" % (user_profile_id,))

zerver/tests/test_push_notifications.py

@@ -27,6 +27,7 @@ from zerver.models import (
     Recipient,
     Stream,
 )
+from zerver.lib.soft_deactivation import do_soft_deactivate_users
 from zerver.lib import push_notifications as apn
 from zerver.lib.push_notifications import get_mobile_push_content, \
     DeviceToken, PushNotificationBouncerException, get_apns_client
@@ -518,12 +519,63 @@ class HandlePushNotificationTest(PushNotificationTest):
 
     def test_user_message_does_not_exist(self):
         # type: () -> None
-        missed_message = {'message_id': 100}
+        """This simulates a condition that should only be an error if the user is
+        not long-term idle; we fake it, though, in the sense that the user should
+        not have received the message in the first place"""
+        self.make_stream('public_stream')
+        message_id = self.send_stream_message("iago@zulip.com", "public_stream", "test")
+        missed_message = {'message_id': message_id}
         with mock.patch('logging.error') as mock_logger:
             apn.handle_push_notification(self.user_profile.id, missed_message)
             mock_logger.assert_called_with("Could not find UserMessage with "
-                                           "message_id 100 and user_id %s" %
+                                           "message_id %s and user_id %s" %
-                                           (self.user_profile.id,))
+                                           (message_id, self.user_profile.id,))
+
+    def test_user_message_soft_deactivated(self):
+        # type: () -> None
+        """This simulates a condition that should only be an error if the user is
+        not long-term idle; we fake it, though, in the sense that the user should
+        not have received the message in the first place"""
+        self.make_stream('public_stream')
+        self.subscribe(self.user_profile, 'public_stream')
+        do_soft_deactivate_users([self.user_profile])
+
+        message_id = self.send_stream_message("iago@zulip.com", "public_stream", "test")
+        missed_message = {
+            'message_id': message_id,
+            'trigger': 'stream_push_notify',
+        }
+
+        for token in [u'dddd']:
+            PushDeviceToken.objects.create(
+                kind=PushDeviceToken.GCM,
+                token=apn.hex_to_b64(token),
+                user=self.user_profile)
+
+        android_devices = list(
+            PushDeviceToken.objects.filter(user=self.user_profile,
+                                           kind=PushDeviceToken.GCM))
+
+        apple_devices = list(
+            PushDeviceToken.objects.filter(user=self.user_profile,
+                                           kind=PushDeviceToken.APNS))
+
+        with mock.patch('zerver.lib.push_notifications.get_apns_payload',
+                        return_value={'apns': True}), \
+                mock.patch('zerver.lib.push_notifications.get_gcm_payload',
+                           return_value={'gcm': True}), \
+                mock.patch('zerver.lib.push_notifications'
+                           '.send_apple_push_notification') as mock_send_apple, \
+                mock.patch('zerver.lib.push_notifications'
+                           '.send_android_push_notification') as mock_send_android, \
+                mock.patch('logging.error') as mock_logger:
+            apn.handle_push_notification(self.user_profile.id, missed_message)
+            mock_logger.assert_not_called()
+            mock_send_apple.assert_called_with(self.user_profile.id,
+                                               apple_devices,
+                                               {'apns': True})
+            mock_send_android.assert_called_with(android_devices,
+                                                 {'gcm': True})
 
 class TestAPNs(PushNotificationTest):
     def devices(self):