Skip to content

Commit

Permalink
push_notifs: Gracefully handle exception when server cant push.
Browse files Browse the repository at this point in the history
The problem was that earlier this was just an uncaught JsonableError,
leading to a full traceback getting spammed to the admins.
The prior commit introduced a clear .code for this error on the bouncer
side, meaning the self-hosted server can now detect that and handle it
nicely, by just logging.error about it and also take the opportunity to
adjust the realm.push_notifications_... flags.

(cherry picked from commit e8018a7)
  • Loading branch information
mateuszmandera authored and timabbott committed Feb 16, 2024
1 parent 0a1905e commit d977dfe
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 1 deletion.
20 changes: 19 additions & 1 deletion zerver/lib/push_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,20 @@ def send_notifications_to_bouncer(
"apple_devices": [device.token for device in apple_devices],
}
# Calls zilencer.views.remote_server_notify_push
response_data = send_json_to_push_bouncer("POST", "push/notify", post_data)

try:
response_data = send_json_to_push_bouncer("POST", "push/notify", post_data)
except PushNotificationsDisallowedByBouncerError as e:
logger.warning("Bouncer refused to send push notification: %s", e.reason)
do_set_realm_property(
user_profile.realm,
"push_notifications_enabled",
False,
acting_user=None,
)
do_set_push_notifications_enabled_end_timestamp(user_profile.realm, None, acting_user=None)
return

assert isinstance(response_data["total_android_devices"], int)
assert isinstance(response_data["total_apple_devices"], int)

Expand Down Expand Up @@ -1504,3 +1517,8 @@ def __init__(self) -> None:
@override
def msg_format() -> str:
return _("Device not recognized by the push bouncer")


class PushNotificationsDisallowedByBouncerError(Exception):
def __init__(self, reason: str) -> None:
self.reason = reason
4 changes: 4 additions & 0 deletions zerver/lib/remote_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ def send_to_push_bouncer(
raise PushNotificationBouncerError(
_("Push notifications bouncer error: {error}").format(error=msg)
)
elif "code" in result_dict and result_dict["code"] == "PUSH_NOTIFICATIONS_DISALLOWED":
from zerver.lib.push_notifications import PushNotificationsDisallowedByBouncerError

raise PushNotificationsDisallowedByBouncerError(reason=msg)
elif (
endpoint == "push/test_notification"
and "code" in result_dict
Expand Down
74 changes: 74 additions & 0 deletions zerver/tests/test_push_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -2679,6 +2679,80 @@ def test_end_to_end(self) -> None:
),
)

@override_settings(PUSH_NOTIFICATION_BOUNCER_URL="https://push.zulip.org.example.com")
@responses.activate
def test_end_to_end_failure_due_to_no_plan(self) -> None:
self.add_mock_response()

self.setup_apns_tokens()
self.setup_gcm_tokens()

self.server.last_api_feature_level = 237
self.server.save()

realm = self.user_profile.realm
realm.push_notifications_enabled = True
realm.save()

message = self.get_message(
Recipient.PERSONAL,
type_id=self.personal_recipient_user.id,
realm_id=self.personal_recipient_user.realm_id,
)
UserMessage.objects.create(
user_profile=self.user_profile,
message=message,
)

missed_message = {
"message_id": message.id,
"trigger": NotificationTriggers.DIRECT_MESSAGE,
}
with mock.patch(
"corporate.lib.stripe.RemoteServerBillingSession.current_count_for_billed_licenses",
return_value=100,
) as mock_current_count, self.assertLogs(
"zerver.lib.push_notifications", level="INFO"
) as pn_logger, self.assertLogs(
"zilencer.views", level="INFO"
):
handle_push_notification(self.user_profile.id, missed_message)

self.assertEqual(
pn_logger.output,
[
f"INFO:zerver.lib.push_notifications:Sending push notifications to mobile clients for user {self.user_profile.id}",
"WARNING:zerver.lib.push_notifications:Bouncer refused to send push notification: Your plan doesn't allow sending push notifications. Reason provided by the server: No plan many users",
],
)
realm.refresh_from_db()
self.assertEqual(realm.push_notifications_enabled, False)
self.assertEqual(realm.push_notifications_enabled_end_timestamp, None)

# Now verify the flag will correctly get flipped back if the server stops
# rejecting our notification.

# This will put us within the allowed number of users to use push notifications
# for free, so the server will accept our next request.
mock_current_count.return_value = 5

new_message_id = self.send_personal_message(
self.example_user("othello"), self.user_profile
)
new_missed_message = {
"message_id": new_message_id,
"trigger": NotificationTriggers.DIRECT_MESSAGE,
}

handle_push_notification(self.user_profile.id, new_missed_message)
self.assertIn(
f"Sent mobile push notifications for user {self.user_profile.id}",
pn_logger.output[-1],
)
realm.refresh_from_db()
self.assertEqual(realm.push_notifications_enabled, True)
self.assertEqual(realm.push_notifications_enabled_end_timestamp, None)

@override_settings(PUSH_NOTIFICATION_BOUNCER_URL="https://push.zulip.org.example.com")
@responses.activate
def test_unregistered_client(self) -> None:
Expand Down

0 comments on commit d977dfe

Please sign in to comment.