Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues around soft deactivation and push notifications #10397

Open
timabbott opened this issue Aug 22, 2018 · 3 comments
Open

Fix issues around soft deactivation and push notifications #10397

timabbott opened this issue Aug 22, 2018 · 3 comments
Labels
area: mobile Mobile push notifications; features motivated by mobile; issues requiring changes in mobile code. area: notifications (messages)

Comments

@timabbott
Copy link
Sponsor Member

timabbott commented Aug 22, 2018

Today I discovered that soft-deactivated users don't get mobile push notifications for messages on private streams that they have configured to send push notifications. This is pretty poor, since the whole purpose of this feature is to help these users learn about things happening in that stream. (I think email notifications may still work).

Here's the problem:

  • 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
  • We create those UserMessage rows lazily for soft-deactivated users, so they might not exist (yet) until the user comes back.

The outcome is access_message throwing an exception in handle_push_notification. There are 2 ways one could imagine fixing this:
(1) Ensuring do_send_messages creates a UserMessage row when a user has configured a stream to send mobile push notifications or emails, so that our notifications code can always assume a UserMessage row is present. This should completely cover the notifications use case where this originally came up, since the whole soft-deactivation model is that it's always OK for UserMessage rows to have been created before soft-reactivation.
(2) Changing access_message to do something fancy for soft-deactivated users, effectively calling a function wrapping filter_by_subscription_history to create just that one UserMessage row on-demand if appropriate. This would ensure that all of our API endpoints grant the user access to the message. We might need to also do something similar in validate_attachment_request; I'm not sure.

I think we definitely want to do (1), because that helps simplify the understanding required for our notifications code. It is possible we also want to do (2); but let's hold off on thinking about that until we've implemented (1); I think for performance reasons, (1) is worth doing regardless of whether we also do (2).

For (1), I think it would suffice to change this block in get_recipient_info to exclude stream_push_user_ids and stream_email_user_ids from the list.

    long_term_idle_user_ids = get_ids_for(                                                                                                  
        lambda r: r['long_term_idle']                                                                                                       
    )                                                                                                                                       

See https://zulip.readthedocs.io/en/latest/subsystems/sending-messages.html#soft-deactivation for background.

@showell @adnrs96 FYI.

@timabbott timabbott added bug priority: high area: notifications (messages) area: mobile Mobile push notifications; features motivated by mobile; issues requiring changes in mobile code. labels Aug 22, 2018
@shubham-padia
Copy link
Member

@zulipbot claim

shubham-padia added a commit to shubham-padia/zulip that referenced this issue Sep 8, 2018
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`.
shubham-padia added a commit to shubham-padia/zulip that referenced this issue Sep 11, 2018
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: Ensure that userMessage row is created for
stream_push_user_ids and stream_email_user_ids in create_user_messages.
@zulipbot
Copy link
Member

Hello @shubham-padia, you claimed this issue to work on it, but this issue and any referenced pull requests haven't been updated for 10 days. Are you still working on this issue?

If so, please update this issue by leaving a comment on this issue to let me know that you're still working on it. Otherwise, I'll automatically remove you from this issue in 4 days.

If you've decided to work on something else, simply comment @zulipbot abandon so that someone else can claim it and continue from where you left off.

Thank you for your valuable contributions to Zulip!

timabbott pushed a commit that referenced this issue Sep 21, 2018
Fixes the urgent part of #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: Ensure that userMessage row is created for
stream_push_user_ids and stream_email_user_ids in create_user_messages.
@timabbott
Copy link
Sponsor Member Author

I merged #10491, which covers the urgent/proximal issue here. I'm leaving the remaining issue open for us to think about whether we also want to modify access_message to handle similar issues automatically in the future; there is a very real performance tradeoff to be mindful of there.

dipu989 pushed a commit to dipu989/zulip that referenced this issue Sep 23, 2018
Fixes the urgent 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: Ensure that userMessage row is created for
stream_push_user_ids and stream_email_user_ids in create_user_messages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: mobile Mobile push notifications; features motivated by mobile; issues requiring changes in mobile code. area: notifications (messages)
Projects
None yet
Development

No branches or pull requests

3 participants