Skip to content

Commit

Permalink
notifications: Fix soft-deactivated users don't get push notifications.
Browse files Browse the repository at this point in the history
Fixes part of zulip#10397.

It was discovered that soft-deactivated users don't get mobile push
notifications for messages on private streams that they have configured
to send push notifications.

Reason: `handle_push_notification` calls `access_message`, and that
logic assumes that a user who is a recipient of a message has an
associated UserMessage row. Those UserMessage rows are created
lazily for soft-deactivated users, so they might not exist (yet)
until the user comes back.

Solution: Removing stream_push_user_ids and stream_email_user_ids
from long_term_idle_user_ids ensures that a UserMessage row is
created in `create_user_messages`.
  • Loading branch information
shubham-padia committed Sep 8, 2018
1 parent 169de2f commit f79c3b1
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 3 deletions.
14 changes: 13 additions & 1 deletion zerver/lib/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1058,9 +1058,21 @@ def is_service_bot(row: Dict[str, Any]) -> bool:
lambda r: not is_service_bot(r)
)

# Issue `#10397`: It was discovered that soft-deactivated users
# don't get mobile push notifications for messages on private
# streams that they have configured to send push notifications.
# Reason: `handle_push_notification` calls `access_message`, and
# that logic assumes that a user who is a recipient of a message
# has an associated UserMessage row. Those UserMessage rows are
# created lazily for soft-deactivated users, so they might not
# exist (yet) until the user comes back.
# Solution: Removing stream_push_user_ids and stream_email_user_ids
# from long_term_idle_user_ids ensures that a UserMessage row is
# created in `create_user_messages`.
long_term_idle_user_ids = get_ids_for(
lambda r: r['long_term_idle']
)
) - stream_push_user_ids \
- stream_email_user_ids

# These two bot data structures need to filter from the full set
# of users who either are receiving the message or might have been
Expand Down
21 changes: 19 additions & 2 deletions zerver/tests/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from zerver.lib.stream_topic import StreamTopicTarget
from zerver.lib.users import user_ids_to_users, access_user_by_id, \
get_accounts_for_email
from zerver.lib.soft_deactivation import do_soft_deactivate_user

from django.conf import settings

Expand Down Expand Up @@ -599,8 +600,9 @@ def test_clear_scheduled_jobs(self) -> None:
class RecipientInfoTest(ZulipTestCase):
def test_stream_recipient_info(self) -> None:
hamlet = self.example_user('hamlet')
cordelia = self.example_user('cordelia')
othello = self.example_user('othello')
cordelia = self.example_user('cordelia')
do_soft_deactivate_user(cordelia)

realm = hamlet.realm

Expand Down Expand Up @@ -636,7 +638,7 @@ def test_stream_recipient_info(self) -> None:
stream_push_user_ids={hamlet.id},
stream_email_user_ids=set(),
um_eligible_user_ids=all_user_ids,
long_term_idle_user_ids=set(),
long_term_idle_user_ids={cordelia.id},
default_bot_user_ids=set(),
service_bot_tuples=[],
)
Expand All @@ -659,6 +661,21 @@ def test_stream_recipient_info(self) -> None:

self.assertEqual(info['stream_push_user_ids'], set())

# Adding Cordelia to stream_push_user_ids removes her from
# long_term_idle_user_ids.
sub = get_subscription(stream_name, cordelia)
sub.push_notifications = True
sub.save()

info = get_recipient_info(
recipient=recipient,
sender_id=hamlet.id,
stream_topic=stream_topic,
)

self.assertEqual(info['stream_push_user_ids'], {cordelia.id})
self.assertEqual(info['long_term_idle_user_ids'], set())

# Add a service bot.
service_bot = do_create_user(
email='service-bot@zulip.com',
Expand Down

0 comments on commit f79c3b1

Please sign in to comment.