zulip/zerver/tests/test_message_edit_notificat...

691 lines
27 KiB
Python
Raw Normal View History

from typing import Any, Dict, Mapping, Union
from unittest import mock
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
from django.utils.timezone import now as timezone_now
from zerver.actions.user_settings import do_change_user_setting
from zerver.actions.user_topics import do_set_user_topic_visibility_policy
from zerver.lib.push_notifications import get_apns_badge_count, get_apns_badge_count_future
from zerver.lib.test_classes import ZulipTestCase
from zerver.lib.test_helpers import mock_queue_publish
from zerver.models import Subscription, UserPresence, UserTopic
from zerver.models.scheduled_jobs import NotificationTriggers
from zerver.models.streams import get_stream
from zerver.tornado.event_queue import maybe_enqueue_notifications
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
class EditMessageSideEffectsTest(ZulipTestCase):
def _assert_update_does_not_notify_anybody(self, message_id: int, content: str) -> None:
url = "/json/messages/" + str(message_id)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
request = dict(
content=content,
)
with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as m:
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
result = self.client_patch(url, request)
self.assert_json_success(result)
self.assertFalse(m.called)
def test_updates_with_pm_mention(self) -> None:
hamlet = self.example_user("hamlet")
cordelia = self.example_user("cordelia")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.login_user(hamlet)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id = self.send_personal_message(
hamlet,
cordelia,
content="no mention",
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
self._assert_update_does_not_notify_anybody(
message_id=message_id,
content="now we mention @**Cordelia, Lear's daughter**",
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
def _login_and_send_original_stream_message(
self, content: str, enable_online_push_notifications: bool = False
) -> int:
"""
Note our conventions here:
Hamlet is our logged in user (and sender).
Cordelia is the receiver we care about.
Scotland is the stream we send messages to.
"""
hamlet = self.example_user("hamlet")
cordelia = self.example_user("cordelia")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
cordelia.enable_online_push_notifications = enable_online_push_notifications
cordelia.save()
self.login_user(hamlet)
self.subscribe(hamlet, "Scotland")
self.subscribe(cordelia, "Scotland")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id = self.send_stream_message(
hamlet,
"Scotland",
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
content=content,
)
return message_id
def _get_queued_data_for_message_update(
self, message_id: int, content: str, expect_short_circuit: bool = False
) -> Dict[str, Any]:
"""
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
This function updates a message with a post to
/json/messages/(message_id).
By using mocks, we are able to capture two pieces of data:
enqueue_kwargs: These are the arguments passed in to
maybe_enqueue_notifications.
queue_messages: These are the messages that
maybe_enqueue_notifications actually
puts on the queue.
Using this helper allows you to construct a test that goes
pretty deep into the missed-messages codepath, without actually
queuing the final messages.
"""
url = "/json/messages/" + str(message_id)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
request = dict(
content=content,
)
with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as m:
with self.captureOnCommitCallbacks(execute=True):
result = self.client_patch(url, request)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
cordelia = self.example_user("cordelia")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
cordelia_calls = [
call_args
for call_args in m.call_args_list
if call_args[1]["user_notifications_data"].user_id == cordelia.id
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
]
if expect_short_circuit:
self.assert_length(cordelia_calls, 0)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
return {}
# Normally we expect maybe_enqueue_notifications to be
# called for Cordelia, so continue on.
self.assert_length(cordelia_calls, 1)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
enqueue_kwargs = cordelia_calls[0][1]
queue_messages = []
def fake_publish(queue_name: str, event: Union[Mapping[str, Any], str], *args: Any) -> None:
queue_messages.append(
dict(
queue_name=queue_name,
event=event,
)
)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
with mock_queue_publish(
"zerver.tornado.event_queue.queue_json_publish", side_effect=fake_publish
) as m:
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
maybe_enqueue_notifications(**enqueue_kwargs)
self.assert_json_success(result)
return dict(
enqueue_kwargs=enqueue_kwargs,
queue_messages=queue_messages,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
def _send_and_update_message(
self,
original_content: str,
updated_content: str,
enable_online_push_notifications: bool = False,
expect_short_circuit: bool = False,
connected_to_zulip: bool = False,
present_on_web: bool = False,
) -> Dict[str, Any]:
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id = self._login_and_send_original_stream_message(
content=original_content,
enable_online_push_notifications=enable_online_push_notifications,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
if present_on_web:
self._make_cordelia_present_on_web()
if connected_to_zulip:
with self._cordelia_connected_to_zulip():
info = self._get_queued_data_for_message_update(
message_id=message_id,
content=updated_content,
expect_short_circuit=expect_short_circuit,
)
else:
info = self._get_queued_data_for_message_update(
message_id=message_id,
content=updated_content,
expect_short_circuit=expect_short_circuit,
)
return dict(
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id=message_id,
info=info,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
def test_updates_with_stream_mention(self) -> None:
original_content = "no mention"
updated_content = "now we mention @**Cordelia, Lear's daughter**"
notification_message_data = self._send_and_update_message(original_content, updated_content)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
cordelia = self.example_user("cordelia")
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
hamlet = self.example_user("hamlet")
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
user_id=cordelia.id,
acting_user_id=hamlet.id,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id=message_id,
notifications: Calculate PMs/mentions settings like other settings. Previously, we checked for the `enable_offline_email_notifications` and `enable_offline_push_notifications` settings (which determine whether the user will receive notifications for PMs and mentions) just before sending notifications. This has a few problem: 1. We do not have access to all the user settings in the notification handlers (`handle_missedmessage_emails` and `handle_push_notifications`), and therefore, we cannot correctly determine whether the notification should be sent. Checks like the following which existed previously, will, for example, incorrectly not send notifications even when stream email notifications are enabled- ``` if not receives_offline_email_notifications(user_profile): return ``` With this commit, we simply do not enqueue notifications if the "offline" settings are disabled, which fixes that bug. Additionally, this also fixes a bug with the "online push notifications" feature, which was, if someone were to: * turn off notifications for PMs and mentions (`enable_offline_push_notifications`) * turn on stream push notifications (`enable_stream_push_notifications`) * turn on "online push" (`enable_online_push_notifications`) then, they would still receive notifications for PMs when online. This isn't how the "online push enabled" feature is supposed to work; it should only act as a wrapper around the other notification settings. The buggy code was this in `handle_push_notifications`: ``` if not ( receives_offline_push_notifications(user_profile) or receives_online_push_notifications(user_profile) ): return // send notifications ``` This commit removes that code, and extends our `notification_data.py` logic to cover this case, along with tests. 2. The name for these settings is slightly misleading. They essentially talk about "what to send notifications for" (PMs and mentions), and not "when to send notifications" (offline). This commit improves this condition by restricting the use of this term only to the database field, and using clearer names everywhere else. This distinction will be important to have non-confusing code when we implement multiple options for notifications in the future as dropdown (never/when offline/when offline or online, etc). 3. We should ideally re-check all notification settings just before the notifications are sent. This is especially important for email notifications, which may be sent after a long time after the message was sent. We will in the future add code to thoroughly re-check settings before sending notifications in a clean manner, but temporarily not re-checking isn't a terrible scenario either.
2021-07-14 15:34:01 +02:00
mention_email_notify=True,
mention_push_notify=True,
already_notified={},
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
queue_messages = info["queue_messages"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.assert_length(queue_messages, 2)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.assertEqual(queue_messages[0]["queue_name"], "missedmessage_mobile_notifications")
mobile_event = queue_messages[0]["event"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.assertEqual(mobile_event["user_profile_id"], cordelia.id)
self.assertEqual(mobile_event["trigger"], NotificationTriggers.MENTION)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.assertEqual(queue_messages[1]["queue_name"], "missedmessage_emails")
email_event = queue_messages[1]["event"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.assertEqual(email_event["user_profile_id"], cordelia.id)
self.assertEqual(email_event["trigger"], NotificationTriggers.MENTION)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def test_second_mention_is_ignored(self) -> None:
original_content = "hello @**Cordelia, Lear's daughter**"
updated_content = "re-mention @**Cordelia, Lear's daughter**"
self._send_and_update_message(original_content, updated_content, expect_short_circuit=True)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def _turn_on_stream_push_for_cordelia(self) -> None:
"""
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
conventions:
Cordelia is the message receiver we care about.
Scotland is our stream.
"""
cordelia = self.example_user("cordelia")
stream = self.subscribe(cordelia, "Scotland")
recipient = stream.recipient
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
cordelia_subscription = Subscription.objects.get(
user_profile_id=cordelia.id,
recipient=recipient,
)
cordelia_subscription.push_notifications = True
cordelia_subscription.save()
def test_updates_with_stream_push_notify(self) -> None:
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self._turn_on_stream_push_for_cordelia()
# Even though Cordelia configured this stream for pushes,
# we short-circuit the logic, assuming the original message
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# also did a push.
original_content = "no mention"
updated_content = "nothing special about updated message"
self._send_and_update_message(original_content, updated_content, expect_short_circuit=True)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def _cordelia_connected_to_zulip(self) -> Any:
"""
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
Right now the easiest way to make Cordelia look
connected to Zulip is to mock the function below.
This is a bit blunt, as it affects other users too,
but we only really look at Cordelia's data, anyway.
"""
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
return mock.patch(
"zerver.tornado.event_queue.receiver_is_off_zulip",
return_value=False,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
def test_stream_push_notify_for_sorta_present_user(self) -> None:
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self._turn_on_stream_push_for_cordelia()
# Simulate Cordelia still has an actively polling client, but
# the lack of presence info should still mark her as offline.
#
# Despite Cordelia being offline, we still short circuit
# offline notifications due to the her stream push setting.
original_content = "no mention"
updated_content = "nothing special about updated message"
self._send_and_update_message(
original_content, updated_content, expect_short_circuit=True, connected_to_zulip=True
)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def _make_cordelia_present_on_web(self) -> None:
cordelia = self.example_user("cordelia")
presence: Rewrite the backend data model. This implements the core of the rewrite described in: For the backend data model for UserPresence to one that supports much more efficient queries and is more correct around handling of multiple clients. The main loss of functionality is that we no longer track which Client sent presence data (so we will no longer be able to say using UserPresence "the user was last online on their desktop 15 minutes ago, but was online with their phone 3 minutes ago"). If we consider that information important for the occasional investigation query, we have can construct that answer data via UserActivity already. It's not worth making Presence much more expensive/complex to support it. For slim_presence clients, this sends the same data format we sent before, albeit with less complexity involved in constructing it. Note that we at present will always send both last_active_time and last_connected_time; we may revisit that in the future. This commit doesn't include the finalizing migration, which drops the UserPresenceOld table. The way to deploy is to start the backfill migration with the server down and then start the server *without* the user_presence queue worker, to let the migration finish without having new data interfering with it. Once the migration is done, the queue worker can be started, leading to the presence data catching up to the current state as the queue worker goes over the queued up events and updating the UserPresence table. Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
2020-06-11 16:03:47 +02:00
now = timezone_now()
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
UserPresence.objects.create(
user_profile_id=cordelia.id,
realm_id=cordelia.realm_id,
presence: Rewrite the backend data model. This implements the core of the rewrite described in: For the backend data model for UserPresence to one that supports much more efficient queries and is more correct around handling of multiple clients. The main loss of functionality is that we no longer track which Client sent presence data (so we will no longer be able to say using UserPresence "the user was last online on their desktop 15 minutes ago, but was online with their phone 3 minutes ago"). If we consider that information important for the occasional investigation query, we have can construct that answer data via UserActivity already. It's not worth making Presence much more expensive/complex to support it. For slim_presence clients, this sends the same data format we sent before, albeit with less complexity involved in constructing it. Note that we at present will always send both last_active_time and last_connected_time; we may revisit that in the future. This commit doesn't include the finalizing migration, which drops the UserPresenceOld table. The way to deploy is to start the backfill migration with the server down and then start the server *without* the user_presence queue worker, to let the migration finish without having new data interfering with it. Once the migration is done, the queue worker can be started, leading to the presence data catching up to the current state as the queue worker goes over the queued up events and updating the UserPresence table. Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
2020-06-11 16:03:47 +02:00
last_connected_time=now,
last_active_time=now,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
def test_stream_push_notify_for_fully_present_user(self) -> None:
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self._turn_on_stream_push_for_cordelia()
# Simulate Cordelia is FULLY present, not just in term of
# browser activity, but also in terms of her client descriptors.
original_content = "no mention"
updated_content = "nothing special about updated message"
self._send_and_update_message(
original_content,
updated_content,
expect_short_circuit=True,
connected_to_zulip=True,
present_on_web=True,
)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def test_online_push_enabled_for_fully_present_mentioned_user(self) -> None:
cordelia = self.example_user("cordelia")
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
hamlet = self.example_user("hamlet")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# Simulate Cordelia is FULLY present, not just in term of
# browser activity, but also in terms of her client descriptors.
original_content = "no mention"
updated_content = "newly mention @**Cordelia, Lear's daughter**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
enable_online_push_notifications=True,
connected_to_zulip=True,
present_on_web=True,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
user_id=cordelia.id,
acting_user_id=hamlet.id,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id=message_id,
notifications: Calculate PMs/mentions settings like other settings. Previously, we checked for the `enable_offline_email_notifications` and `enable_offline_push_notifications` settings (which determine whether the user will receive notifications for PMs and mentions) just before sending notifications. This has a few problem: 1. We do not have access to all the user settings in the notification handlers (`handle_missedmessage_emails` and `handle_push_notifications`), and therefore, we cannot correctly determine whether the notification should be sent. Checks like the following which existed previously, will, for example, incorrectly not send notifications even when stream email notifications are enabled- ``` if not receives_offline_email_notifications(user_profile): return ``` With this commit, we simply do not enqueue notifications if the "offline" settings are disabled, which fixes that bug. Additionally, this also fixes a bug with the "online push notifications" feature, which was, if someone were to: * turn off notifications for PMs and mentions (`enable_offline_push_notifications`) * turn on stream push notifications (`enable_stream_push_notifications`) * turn on "online push" (`enable_online_push_notifications`) then, they would still receive notifications for PMs when online. This isn't how the "online push enabled" feature is supposed to work; it should only act as a wrapper around the other notification settings. The buggy code was this in `handle_push_notifications`: ``` if not ( receives_offline_push_notifications(user_profile) or receives_online_push_notifications(user_profile) ): return // send notifications ``` This commit removes that code, and extends our `notification_data.py` logic to cover this case, along with tests. 2. The name for these settings is slightly misleading. They essentially talk about "what to send notifications for" (PMs and mentions), and not "when to send notifications" (offline). This commit improves this condition by restricting the use of this term only to the database field, and using clearer names everywhere else. This distinction will be important to have non-confusing code when we implement multiple options for notifications in the future as dropdown (never/when offline/when offline or online, etc). 3. We should ideally re-check all notification settings just before the notifications are sent. This is especially important for email notifications, which may be sent after a long time after the message was sent. We will in the future add code to thoroughly re-check settings before sending notifications in a clean manner, but temporarily not re-checking isn't a terrible scenario either.
2021-07-14 15:34:01 +02:00
mention_push_notify=True,
mention_email_notify=True,
online_push_enabled=True,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
idle=False,
already_notified={},
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
queue_messages = info["queue_messages"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
self.assert_length(queue_messages, 1)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def test_online_push_enabled_for_fully_present_boring_user(self) -> None:
cordelia = self.example_user("cordelia")
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
hamlet = self.example_user("hamlet")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# Simulate Cordelia is FULLY present, not just in term of
# browser activity, but also in terms of her client descriptors.
original_content = "no mention"
updated_content = "nothing special about updated message"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
enable_online_push_notifications=True,
connected_to_zulip=True,
present_on_web=True,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
user_id=cordelia.id,
acting_user_id=hamlet.id,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id=message_id,
online_push_enabled=True,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
idle=False,
already_notified={},
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
queue_messages = info["queue_messages"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
notifications: Calculate PMs/mentions settings like other settings. Previously, we checked for the `enable_offline_email_notifications` and `enable_offline_push_notifications` settings (which determine whether the user will receive notifications for PMs and mentions) just before sending notifications. This has a few problem: 1. We do not have access to all the user settings in the notification handlers (`handle_missedmessage_emails` and `handle_push_notifications`), and therefore, we cannot correctly determine whether the notification should be sent. Checks like the following which existed previously, will, for example, incorrectly not send notifications even when stream email notifications are enabled- ``` if not receives_offline_email_notifications(user_profile): return ``` With this commit, we simply do not enqueue notifications if the "offline" settings are disabled, which fixes that bug. Additionally, this also fixes a bug with the "online push notifications" feature, which was, if someone were to: * turn off notifications for PMs and mentions (`enable_offline_push_notifications`) * turn on stream push notifications (`enable_stream_push_notifications`) * turn on "online push" (`enable_online_push_notifications`) then, they would still receive notifications for PMs when online. This isn't how the "online push enabled" feature is supposed to work; it should only act as a wrapper around the other notification settings. The buggy code was this in `handle_push_notifications`: ``` if not ( receives_offline_push_notifications(user_profile) or receives_online_push_notifications(user_profile) ): return // send notifications ``` This commit removes that code, and extends our `notification_data.py` logic to cover this case, along with tests. 2. The name for these settings is slightly misleading. They essentially talk about "what to send notifications for" (PMs and mentions), and not "when to send notifications" (offline). This commit improves this condition by restricting the use of this term only to the database field, and using clearer names everywhere else. This distinction will be important to have non-confusing code when we implement multiple options for notifications in the future as dropdown (never/when offline/when offline or online, etc). 3. We should ideally re-check all notification settings just before the notifications are sent. This is especially important for email notifications, which may be sent after a long time after the message was sent. We will in the future add code to thoroughly re-check settings before sending notifications in a clean manner, but temporarily not re-checking isn't a terrible scenario either.
2021-07-14 15:34:01 +02:00
# Cordelia being present and having `enable_online_push_notifications`
# does not mean we'll send her notifications for messages which she
# wouldn't otherwise have received notifications for.
self.assert_length(queue_messages, 0)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def test_updates_with_stream_mention_of_sorta_present_user(self) -> None:
cordelia = self.example_user("cordelia")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# We will simulate that the user still has an active client,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# but they don't have UserPresence rows, so we will still
# send offline notifications.
original_content = "no mention"
updated_content = "now we mention @**Cordelia, Lear's daughter**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
connected_to_zulip=True,
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
user_id=cordelia.id,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id=message_id,
acting_user_id=self.example_user("hamlet").id,
notifications: Calculate PMs/mentions settings like other settings. Previously, we checked for the `enable_offline_email_notifications` and `enable_offline_push_notifications` settings (which determine whether the user will receive notifications for PMs and mentions) just before sending notifications. This has a few problem: 1. We do not have access to all the user settings in the notification handlers (`handle_missedmessage_emails` and `handle_push_notifications`), and therefore, we cannot correctly determine whether the notification should be sent. Checks like the following which existed previously, will, for example, incorrectly not send notifications even when stream email notifications are enabled- ``` if not receives_offline_email_notifications(user_profile): return ``` With this commit, we simply do not enqueue notifications if the "offline" settings are disabled, which fixes that bug. Additionally, this also fixes a bug with the "online push notifications" feature, which was, if someone were to: * turn off notifications for PMs and mentions (`enable_offline_push_notifications`) * turn on stream push notifications (`enable_stream_push_notifications`) * turn on "online push" (`enable_online_push_notifications`) then, they would still receive notifications for PMs when online. This isn't how the "online push enabled" feature is supposed to work; it should only act as a wrapper around the other notification settings. The buggy code was this in `handle_push_notifications`: ``` if not ( receives_offline_push_notifications(user_profile) or receives_online_push_notifications(user_profile) ): return // send notifications ``` This commit removes that code, and extends our `notification_data.py` logic to cover this case, along with tests. 2. The name for these settings is slightly misleading. They essentially talk about "what to send notifications for" (PMs and mentions), and not "when to send notifications" (offline). This commit improves this condition by restricting the use of this term only to the database field, and using clearer names everywhere else. This distinction will be important to have non-confusing code when we implement multiple options for notifications in the future as dropdown (never/when offline/when offline or online, etc). 3. We should ideally re-check all notification settings just before the notifications are sent. This is especially important for email notifications, which may be sent after a long time after the message was sent. We will in the future add code to thoroughly re-check settings before sending notifications in a clean manner, but temporarily not re-checking isn't a terrible scenario either.
2021-07-14 15:34:01 +02:00
mention_email_notify=True,
mention_push_notify=True,
already_notified={},
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# She will get messages enqueued. (Other tests drill down on the
# actual content of these messages.)
self.assert_length(info["queue_messages"], 2)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
def test_updates_with_topic_wildcard_mention_in_followed_topic(self) -> None:
cordelia = self.example_user("cordelia")
hamlet = self.example_user("hamlet")
self.subscribe(cordelia, "Scotland")
do_change_user_setting(
cordelia, "enable_followed_topic_email_notifications", False, acting_user=None
)
do_change_user_setting(
cordelia, "enable_followed_topic_push_notifications", False, acting_user=None
)
do_change_user_setting(cordelia, "wildcard_mentions_notify", False, acting_user=None)
do_set_user_topic_visibility_policy(
user_profile=cordelia,
stream=get_stream("Scotland", cordelia.realm),
topic_name="test",
visibility_policy=UserTopic.VisibilityPolicy.FOLLOWED,
)
# Only users who either sent or reacted to messages in the topic
# are considered for @topic mention notifications.
self.send_stream_message(cordelia, "Scotland")
# We will simulate that the user still has an active client,
# but they don't have UserPresence rows, so we will still
# send offline notifications.
original_content = "no mention"
updated_content = "now we mention @**topic**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
connected_to_zulip=True,
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
user_id=cordelia.id,
acting_user_id=hamlet.id,
message_id=message_id,
topic_wildcard_mention_in_followed_topic_email_notify=True,
topic_wildcard_mention_in_followed_topic_push_notify=True,
already_notified={},
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
# messages will get enqueued.
self.assert_length(info["queue_messages"], 2)
def test_updates_with_stream_wildcard_mention_in_followed_topic(self) -> None:
cordelia = self.example_user("cordelia")
hamlet = self.example_user("hamlet")
self.subscribe(cordelia, "Scotland")
do_change_user_setting(
cordelia, "enable_followed_topic_email_notifications", False, acting_user=None
)
do_change_user_setting(
cordelia, "enable_followed_topic_push_notifications", False, acting_user=None
)
do_change_user_setting(cordelia, "wildcard_mentions_notify", False, acting_user=None)
do_set_user_topic_visibility_policy(
user_profile=cordelia,
stream=get_stream("Scotland", cordelia.realm),
topic_name="test",
visibility_policy=UserTopic.VisibilityPolicy.FOLLOWED,
)
# We will simulate that the user still has an active client,
# but they don't have UserPresence rows, so we will still
# send offline notifications.
original_content = "no mention"
updated_content = "now we mention @**all**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
connected_to_zulip=True,
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
user_id=cordelia.id,
acting_user_id=hamlet.id,
message_id=message_id,
stream_wildcard_mention_in_followed_topic_email_notify=True,
stream_wildcard_mention_in_followed_topic_push_notify=True,
already_notified={},
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
# messages will get enqueued.
self.assert_length(info["queue_messages"], 2)
def test_updates_with_topic_wildcard_mention(self) -> None:
cordelia = self.example_user("cordelia")
hamlet = self.example_user("hamlet")
# Only users who either sent or reacted to messages in the topic
# are considered for @topic mention notifications.
self.subscribe(cordelia, "Scotland")
self.send_stream_message(cordelia, "Scotland")
# We will simulate that the user still has an active client,
# but they don't have UserPresence rows, so we will still
# send offline notifications.
original_content = "no mention"
updated_content = "now we mention @**topic**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
connected_to_zulip=True,
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
user_id=cordelia.id,
acting_user_id=hamlet.id,
message_id=message_id,
topic_wildcard_mention_email_notify=True,
topic_wildcard_mention_push_notify=True,
already_notified={},
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
# messages will get enqueued.
self.assert_length(info["queue_messages"], 2)
def test_updates_with_stream_wildcard_mention(self) -> None:
cordelia = self.example_user("cordelia")
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
hamlet = self.example_user("hamlet")
# We will simulate that the user still has an active client,
# but they don't have UserPresence rows, so we will still
# send offline notifications.
original_content = "no mention"
updated_content = "now we mention @**all**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
connected_to_zulip=True,
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
user_id=cordelia.id,
acting_user_id=hamlet.id,
message_id=message_id,
stream_wildcard_mention_email_notify=True,
stream_wildcard_mention_push_notify=True,
already_notified={},
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
# She will get messages enqueued.
self.assert_length(info["queue_messages"], 2)
def test_updates_with_upgrade_wildcard_mention(self) -> None:
# If there was a previous wildcard mention delivered to the
# user (because wildcard_mention_notify=True), we don't notify
original_content = "Mention @**all**"
updated_content = "now we mention @**Cordelia, Lear's daughter**"
self._send_and_update_message(
original_content, updated_content, expect_short_circuit=True, connected_to_zulip=True
)
def test_updates_with_upgrade_wildcard_mention_disabled(self) -> None:
# If the user has disabled notifications for wildcard
# mentions, they won't have been notified at first, which
# means they should be notified when the message is edited to
# contain a wildcard mention.
#
# This is a bug that we're not equipped to fix right now.
cordelia = self.example_user("cordelia")
cordelia.wildcard_mentions_notify = False
cordelia.save()
original_content = "Mention @**all**"
updated_content = "now we mention @**Cordelia, Lear's daughter**"
self._send_and_update_message(
original_content, updated_content, expect_short_circuit=True, connected_to_zulip=True
)
def test_updates_with_stream_mention_of_fully_present_user(self) -> None:
cordelia = self.example_user("cordelia")
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
hamlet = self.example_user("hamlet")
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# Simulate Cordelia is FULLY present, not just in term of
# browser activity, but also in terms of her client descriptors.
original_content = "no mention"
updated_content = "now we mention @**Cordelia, Lear's daughter**"
notification_message_data = self._send_and_update_message(
original_content,
updated_content,
connected_to_zulip=True,
present_on_web=True,
)
message_id = notification_message_data["message_id"]
info = notification_message_data["info"]
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
expected_enqueue_kwargs = self.get_maybe_enqueue_notifications_parameters(
maybe_enqueue_notifications: Take in notification_data dataclass. * Modify `maybe_enqueue_notifications` to take in an instance of the dataclass introduced in 951b49c048ba3464e74ad7965da3453fe36d0a96. * The `check_notify` tests tested the "when to notify" logic in a way which involved `maybe_enqueue_notifications`. To simplify things, we've earlier extracted this logic in 8182632d7e9f8490b9b9295e01b5912dcf173fd5. So, we just kill off the `check_notify` test, and keep only those parts which verify the queueing and return value behavior of that funtion. * We retain the the missedmessage_hook and message message_edit_notifications since they are more integration-style. * There's a slightly subtle change with the missedmessage_hook tests. Before this commit, we short-circuited the hook if the sender was muted (5a642cea115be159175d1189f83ba25d2c5c7632). With this commit, we delegate the check to our dataclass methods. So, `maybe_enqueue_notifications` will be called even if the sender was muted, and the test needs to be updated. * In our test helper `get_maybe_enqueue_notifications_parameters` which generates default values for testing `maybe_enqueue_notifications` calls, we keep `message_id`, `sender_id`, and `user_id` as required arguments, so that the tests are super-clear and avoid accidental false positives. * Because `do_update_embedded_data` also sends `update_message` events, we deal with that case with some hacky code for now. See the comment there. This mostly completes the extraction of the "when to notify" logic into our new `notification_data` module.
2021-06-23 14:12:32 +02:00
user_id=cordelia.id,
acting_user_id=hamlet.id,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
message_id=message_id,
notifications: Calculate PMs/mentions settings like other settings. Previously, we checked for the `enable_offline_email_notifications` and `enable_offline_push_notifications` settings (which determine whether the user will receive notifications for PMs and mentions) just before sending notifications. This has a few problem: 1. We do not have access to all the user settings in the notification handlers (`handle_missedmessage_emails` and `handle_push_notifications`), and therefore, we cannot correctly determine whether the notification should be sent. Checks like the following which existed previously, will, for example, incorrectly not send notifications even when stream email notifications are enabled- ``` if not receives_offline_email_notifications(user_profile): return ``` With this commit, we simply do not enqueue notifications if the "offline" settings are disabled, which fixes that bug. Additionally, this also fixes a bug with the "online push notifications" feature, which was, if someone were to: * turn off notifications for PMs and mentions (`enable_offline_push_notifications`) * turn on stream push notifications (`enable_stream_push_notifications`) * turn on "online push" (`enable_online_push_notifications`) then, they would still receive notifications for PMs when online. This isn't how the "online push enabled" feature is supposed to work; it should only act as a wrapper around the other notification settings. The buggy code was this in `handle_push_notifications`: ``` if not ( receives_offline_push_notifications(user_profile) or receives_online_push_notifications(user_profile) ): return // send notifications ``` This commit removes that code, and extends our `notification_data.py` logic to cover this case, along with tests. 2. The name for these settings is slightly misleading. They essentially talk about "what to send notifications for" (PMs and mentions), and not "when to send notifications" (offline). This commit improves this condition by restricting the use of this term only to the database field, and using clearer names everywhere else. This distinction will be important to have non-confusing code when we implement multiple options for notifications in the future as dropdown (never/when offline/when offline or online, etc). 3. We should ideally re-check all notification settings just before the notifications are sent. This is especially important for email notifications, which may be sent after a long time after the message was sent. We will in the future add code to thoroughly re-check settings before sending notifications in a clean manner, but temporarily not re-checking isn't a terrible scenario either.
2021-07-14 15:34:01 +02:00
mention_email_notify=True,
mention_push_notify=True,
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
idle=False,
already_notified={},
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
)
self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs)
Notify offline users about edited stream messages. We now do push notifications and missed message emails for offline users who are subscribed to the stream for a message that has been edited, but we short circuit the offline-notification logic for any user who presumably would have already received a notification on the original message. This effectively boils down to sending notifications to newly mentioned users. The motivating use case here is that you forget to mention somebody in a message, and then you edit the message to mention the person. If they are offline, they will now get pushed notifications and missed message emails, with some minor caveats. We try to mostly use the same techniques here as the send-message code path, and we share common code with the send-message path once we get to the Tornado layer and call maybe_enqueue_notifications. The major places where we differ are in a function called maybe_enqueue_notifications_for_message_update, and the top of that function short circuits a bunch of cases where we can mostly assume that the original message had an offline notification. We can expect a couple changes in the future: * Requirements may change here, and it might make sense to send offline notifications on the update side even in circumstances where the original message had a notification. * We may track more notifications in a DB model, which may simplify our short-circuit logic. In the view/action layer, we already had two separate codepaths for send-message and update-message, but this mostly echoes what the send-message path does in terms of collecting data about recipients.
2017-10-03 16:25:12 +02:00
# Because Cordelia is FULLY present, we don't need to send any offline
# push notifications or message notification emails.
self.assert_length(info["queue_messages"], 0)
@mock.patch("zerver.lib.push_notifications.push_notifications_configured", return_value=True)
def test_clear_notification_when_mention_removed(
self, mock_push_notifications: mock.MagicMock
) -> None:
mentioned_user = self.example_user("iago")
self.assertEqual(get_apns_badge_count(mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(mentioned_user), 0)
message_id = self._login_and_send_original_stream_message(
content="@**Iago**",
)
self.assertEqual(get_apns_badge_count(mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(mentioned_user), 1)
self._get_queued_data_for_message_update(message_id=message_id, content="Removed mention")
self.assertEqual(get_apns_badge_count(mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(mentioned_user), 0)
@mock.patch("zerver.lib.push_notifications.push_notifications_configured", return_value=True)
def test_clear_notification_when_group_mention_removed(
self, mock_push_notifications: mock.MagicMock
) -> None:
group_mentioned_user = self.example_user("cordelia")
self.assertEqual(get_apns_badge_count(group_mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(group_mentioned_user), 0)
message_id = self._login_and_send_original_stream_message(
content="Hello @*hamletcharacters*",
)
self.assertEqual(get_apns_badge_count(group_mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(group_mentioned_user), 1)
self._get_queued_data_for_message_update(
message_id=message_id,
content="Removed group mention",
expect_short_circuit=True,
)
self.assertEqual(get_apns_badge_count(group_mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(group_mentioned_user), 0)
@mock.patch("zerver.lib.push_notifications.push_notifications_configured", return_value=True)
def test_not_clear_notification_when_mention_removed_but_stream_notified(
self, mock_push_notifications: mock.MagicMock
) -> None:
mentioned_user = self.example_user("iago")
mentioned_user.enable_stream_push_notifications = True
mentioned_user.save()
self.assertEqual(get_apns_badge_count(mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(mentioned_user), 0)
message_id = self._login_and_send_original_stream_message(
content="@**Iago**",
)
self.assertEqual(get_apns_badge_count(mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(mentioned_user), 1)
self._get_queued_data_for_message_update(message_id=message_id, content="Removed mention")
self.assertEqual(get_apns_badge_count(mentioned_user), 0)
self.assertEqual(get_apns_badge_count_future(mentioned_user), 1)