When a bunch of messages with active notifications are all read at
once -- e.g. by the user choosing to mark all messages, or all in a
stream, as read, or just scrolling quickly through a PM conversation
-- there can be a large batch of this information to convey. Doing it
in a single GCM/FCM message is better for server congestion, and for
the device's battery.
The corresponding client-side logic is in zulip/zulip-mobile#3343 .
Existing clients today only understand one message ID at a time; so
accommodate them by sending individual GCM/FCM messages up to an
arbitrary threshold, with the rest only as a batch.
Also add an explicit test for this logic. The existing tests
that happen to cause this function to run don't exercise the
last condition, so without a new test `--coverage` complains.
Since da8f4bc0e back in August, this control flow has caused
`flags.active_mobile_push_notification` to be cleared if we don't send
these `remove` messages at all, and if we send them directly to GCM...
but not if we send them via the Zulip notification bouncer.
As a result, on a server configured to send `remove` notification-messages
via the bouncer, we accumulate "active" messages and never clear them.
If the user then does `mark_all_as_read`, we end up sending a `remove`
for each of those messages again, and all in one giant burst. We've
seen puzzling bursts of hundreds of removals pass through the bouncer
since turning on removals on chat.zulip.org; it's likely many of them
are caused by this bug.
This issue was made more acute with f4478aad5, which unconditionally
enabled removals.
Test added by tabbott.
This is the only server-side change required for the FCM migration!
Optionally, at some point in the future we might choose to migrate
to the new ("v1") API which FCM also offers. Nothing revolutionary
but there are some nice things about it:
https://firebase.google.com/docs/cloud-messaging/migrate-v1
The client-side fix to make these not a problem was in release
16.2.96, of 2018-08-22. We've been sending them from the
development community server chat.zulip.org since 2018-11-29.
We started forcing clients to upgrade with commit fb7bfbe9a,
deployed 2018-12-05 to zulipchat.com.
(The mobile app unconditionally makes a request to a route on
zulipchat.com to check for this kind of forced upgrade, so that
applies to mobile users of any Zulip server.)
So at this point it's long past safe for us to unconditionally
send these. Hardwire the old `SEND_REMOVE_PUSH_NOTIFICATIONS`
setting to True, and simplify it out.
If we make a practice on the Zulip server of always explicitly setting
the desired priority, then when an old server doesn't set the priority
we can reasonably have the bouncer make a guess.
That is, this allows a Zulip server to now set the `priority`; but if
it doesn't, we use upstream's default value, which has the same effect
as we've always previously had by not setting it at all.
But when this is deployed to the push notifications bouncer server, it
does allow another server to set priority when pushing notifications
through the bouncer.
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.
If a user deletes message between when it triggered a potential push
notification for another user, and when that notification was actually
sent, we'd end up with a situation where the `Message` table didn't
have an entry for the requested message ID.
The clean fix for this is to still throw an exception in the event
that the message doesn't exist at all, but if it exists in
ArchivedMessage, don't throw a user-facing exception.
This seems to happen when Apple is having a partial outage on some of
their APNS shards; it should be treated like other networking errors
connecting to APNS (with an automatic retry).
This prevents the warning about push notifications not being
registered for from being printed in development environment startup
by default. In development, that's the expected state, and we don't
need to spam up the output with that notice.
The previous content made it sound like we were actually sending a
push notification, which could be confusing/alarming in some cases
(see e.g. 9c224ccdd3). Instead, we make
clear that we're sending it to all clients (which one might correctly
suspect is vacuous in the development environment).
While it could make sense to print these logging statements at WARN
level on server startup, it doesn't make sense to do so on every
message (though it perhaps did make sense to do so before more recent
changes added good ways to discover you forgot to configure push
notifications).
Instead, we now just do a WARN log on queue processor startup, and
then at DEBUG level for individual messages.
Fixes#10894.
Fixes#10745.
Use get_display_recipient to get stream names, and remove the
references to message.stream_name in push_notifications.py which were
added in 97571a203, as the actual stream names were being retrived
only for Message objects associated with public streams.
Also, rename get_alert_from_message to get_gcm_alert.
With the implementation of the and get_apns_alert_title and
get_apns_alert_subtitle, the logic within get_alert_from_message
is only relevant to the GCM payload, so we adjust the name
accordingly.
Progresses #9949.
Resolves https://github.com/zulip/zulip-mobile/issues/1316.
The string that is returned from get_alert_from_message is
dependent upon the same message that is passed into get_apns_payload
and get_gcm_payload. The contents of those payloads that are tested via
TestGetAPNsPayload and TestGetGCMPayload, which makes the tests for
get_alert_from_message redundant.
Also, simplify the logic by removing the last elif conditional.
The APNS client libraries (especially the hyper.http20 one) were
determined via profiling to take significant time during the import
process, so we move them to be lazily imported in order to optimize
the overall Zulip import process. This save up to about 100ms in
import time.
These libraries are only used in certain Django processes inside
zulipchat.com, and so are unnecessary both in development as well as
for self-hosted Zulip servers.
Now that we allow multiple users to have registered the same token, we
need to configure calls to unregister tokens to only query the
targeted user_id.
We conveniently were already passing the `user_id` into the push
notification bouncer for the remove API, so no migration for older
Zulip servers is required.
Previously, Zulip did not correctly handle the case of a mobile device
being registered with a push device token being registered for
multiple accounts on the same server (which is a common case on
zulipchat.com). This was because our database `unique` and
`unique_together` indexes incorrectly enforced the token being unique
on a given server, rather than unique for a given user_id.
We fix this gap, and at the same time remove unnecessary (and
incorrectly racey) logic deleting and recreating the tokens in the
appropriate tables.
There's still an open mobile app bug causing repeated re-registrations
in a loop, but this should fix the fact that the relevant mobile bug
causes the server to 500.
Follow-up work that may be of value includes:
* Removing `ios_app_id`, which may not have much purpose.
* Renaming `last_updated` to `data_created`, since that's what it is now.
But none of those are critical to solving the actual bug here.
Fixes#8841.
This uses the recently introduced active_mobile_push_notification
flag; messages that have had a mobile push notification sent will have
a removal push notification sent as soon as they are marked as read.
Note that this feature is behind a setting,
SEND_REMOVE_PUSH_NOTIFICATIONS, since the notification format is not
supported by the mobile apps yet, and we want to give a grace period
before we start sending notifications that appear as (null) to
clients. But the tracking logic to maintain the set of message IDs
with an active push notification runs unconditionally.
This is designed with at-least-once semantics; so mobile clients need
to handle the possibility that they receive duplicat requests to
remove a push notification.
We reuse the existing missedmessage_mobile_notifications queue
processor for the work, to avoid materially impacting the latency of
marking messages as read.
Fixes#7459, though we'll need to open a follow-up issue for
using these data on iOS.
This adds a new function called handle_remove_push_notification in
zerver/lib/push_notifications.py which requires user_profile id and
the message id which has to be removed in the function.
For now, the function only supports GCM (and is mostly there for
prototyping).
The payload which is being delivered needs to contain the narrow
information and the content of the message.
This should make it much simpler for the mobile apps to line up the
data from server_settings against the data in the notifications.
Addresses part of #10094.
The only changes visible at the AST level, checked using
https://github.com/asottile/astpretty, are
zerver/lib/test_fixtures.py:
'\x1b\\[(1|0)m' ↦ '\\x1b\\[(1|0)m'
'\\[[X| ]\\] (\\d+_.+)\n' ↦ '\\[[X| ]\\] (\\d+_.+)\\n'
which is fine because re treats '\\x1b' and '\\n' the same way as
'\x1b' and '\n'.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
We've had this sort of logic for GCM for a long time; it's worth
adding for APNS as well.
Writing this is a bit of a reminder that I'm not a fan of how our unit
tests for push notifications work.
Greg Price
Anders Kaseorg
Tim Abbott
ishanrai05
Steve Howell
Jack Zhang
Kunal Gupta