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

Add button to test mobile push notifications #5796

Closed
alya opened this issue Nov 22, 2023 · 9 comments · Fixed by #5799
Closed

Add button to test mobile push notifications #5796

alya opened this issue Nov 22, 2023 · 9 comments · Fixed by #5799
Labels
a-notifications server release goal Things we should try to coordinate with a major Zulip Server release.

Comments

@alya
Copy link
Collaborator

alya commented Nov 22, 2023

The server now supports test push notifications. We should make it convenient to trigger such a notification from the mobile app.

  • Button: "Send a test notification"
  • Placement: Notifications settings panel. I think it would make sense to put it at the top, because this button wouldn't test anything about the settings that can be configured in the rest of the notifications panel.
  • Notification:
    • Heading: Zulip test notification
    • Text: Congratulations! Zulip push notifications are working for [organization name].
@alya alya added a-notifications server release goal Things we should try to coordinate with a major Zulip Server release. labels Nov 22, 2023
@chrisbobbe
Copy link
Contributor

Placement: Notifications settings panel. I think it would make sense to put it at the top, because this button wouldn't test anything about the settings that can be configured in the rest of the notifications panel.

We do have a shaded section for things that are specific to the current account, as this button would be. If we put it in that section, it would reinforce that the button is only about one specific account. You could be logged into other accounts in the app, and you won't get the same behavior from this button as you would with a different account. (A different account usually means a different server, and you could have some servers where notifications are busted and some where they aren't.) That's helpful to be clear about.

That's also an advantage of putting the "Troubleshooting" button in that section. That button's job is similar, so it might be good to put them together, either in or out of the section.

What do you think?

Current Proposed

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Nov 28, 2023

To make one assumption explicit: I assume we'll want this button to disappear, along with everything else, when Zulip's notifications are disabled in system settings. That's to help focus the user on fixing that before expecting any notifications to work:

@chrisbobbe
Copy link
Contributor

Also, is there a reason not to also add a test-notification button on the notification troubleshooting screen?

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Nov 28, 2023

Notification:

  • Heading: Zulip test notification
  • Text: Congratulations! Zulip push notifications are working for [organization name].

For iOS, the heading and text are defined in the push bouncer: https://github.com/zulip/zulip/blob/78440033dc7b7f0dbf323215def7d9c784f04ad4/zerver/lib/push_notifications.py#L1283-L1290

            "title": _("Test notification"),
            "body": _("This is a test notification from {realm_uri}.").format(realm_uri=realm_uri),

so we would need to change it there.

I slightly prefer the more neutral "this is a test notification" text. Saying that "notifications are working" doesn't seem helpful if you're trying the button because your notifications have been severely delayed, whether or not the test notification itself arrives on time. You could also have an issue with how notifications are presented (the sound/banner/vibration/etc.), or just want to remind yourself how they're presented, and to me a neutral-sounding message seems appropriate for those cases too.

@alya
Copy link
Collaborator Author

alya commented Nov 28, 2023

Also, is there a reason not to also add a test-notification button on the notification troubleshooting screen?

Are you thinking both that screen and the general notifications screen? That seems reasonable to me...

@alya
Copy link
Collaborator Author

alya commented Nov 28, 2023

I slightly prefer the more neutral "this is a test notification" text.

Could you start a thread on CZO to discuss the text? I was thinking of a different use case, which is an IT person who just set up a Zulip server; I guess we should consider both.

@alya
Copy link
Collaborator Author

alya commented Nov 28, 2023

I'm fine with the proposed placement.

@alya
Copy link
Collaborator Author

alya commented Nov 28, 2023

Thanks for framing up all the questions!

@chrisbobbe
Copy link
Contributor

Could you start a thread on CZO to discuss the text? I was thinking of a different use case, which is an IT person who just set up a Zulip server; I guess we should consider both.

Sure! Here: https://chat.zulip.org/#narrow/stream/378-api-design/topic/.2323997.20Endpoint.20for.20test.20notification/near/1690482

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 2, 2023
Tested on the office Android device, and on my iPhone.

For the iOS testing, I set up a dev server to talk directly to
Apple's APNs "sandbox" server, with our documented instructions:
  https://github.com/zulip/zulip-mobile/blob/main/docs/howto/push-notifications.md#testing-client-side-changes-on-ios

(I already had the relevant certificate, so I didn't need to go
through the steps with a certificate signing request.)

Fixes: zulip#5796
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-notifications server release goal Things we should try to coordinate with a major Zulip Server release.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants