Page MenuHomePhabricator

As an existing editor I need a way to opt into notifications
Closed, DuplicatePublic

Description

Why are we doing this?

As we begin to support contribution notifications through the iOS app, we need a place for contributors to opt into and out of specific notification types.

Feature job story

As an existing editor I need a way to opt into and out of notifications on my iOS device.

Designs

Figma: https://www.figma.com/file/cedgOU5CyOR0UVqtjDOvzE/iOS-Notifications?node-id=962%3A1899

Logged outLogged in - SettingsLogged in - Settings > Push Notifications (off)Logged in - Settings > Push Notifications (on)Logged in - Settings > Subscriptions
image.png (812×375 px, 38 KB)
image.png (812×375 px, 36 KB)
image.png (812×375 px, 23 KB)
image.png (1×375 px, 97 KB)
image.png (1×375 px, 96 KB)

Information architecture

  • No settings related to notifications are shown when a user is logged out
  • On log in there are two notifications related settings
    • Type subscription (shared with web) is available under Settings > Account > Notification subscriptions
    • Push notifications subscriptions are available under Settings > Push notifications

Error states

For error states please see https://phabricator.wikimedia.org/T288773

Development Notes

  • If notification types are supposed to be toggled OFF by default, then upon login we MUST make a call to options API toggle them all off on the server.
  • Handle logout state on this screen. if they want notifications, they have to be logged in.
  • Handle "unsuccessfully registered" state on this screen. It could be OS deviceToken generation failed, or PI subscription failed.
  • Handle "user denied permissions" state on this screen, with an option to go into settings (if not already).
  • Upon load, fetch read options API for the user's primary app language and set current settings (https://www.mediawiki.org/wiki/API:Userinfo)
  • Upon first toggle, ask user for permissions if user hasn't authorized yet.
  • Then upon toggle of any, set updates via API (https://www.mediawiki.org/wiki/API:Options) for user's primary language. If there was a failure in setting it, show an error and reset toggles to what it was before.
  • If user's primary language changes, update the users new primary language settings with the old primary language settings. This will be behind the scenes, if there was an error with this, the read API the next time this screen is loaded will automatically show the server state.
  • There's an explore feed card that asks for remote notification permissions, maybe remove this entirely or only if they are logged out.
  • Disable current news notification code for now, there is a separate task (https://phabricator.wikimedia.org/T287307) to add back in and test that it still works.
  • Heavily test and optimize this screen for accessibility.
API Notes

To read notification preferences: https://www.mediawiki.org/wiki/API:Userinfo
To update notification preferences: https://www.mediawiki.org/wiki/API:Options

Notification Type > API associated value for reading and updating

Talk page message > echo-subscriptions-push-edit-user-talk
Thanks > echo-subscriptions-push-edit-thank
Talk page subscription > echo-subscriptions-push-dt-subscription
Translations > echo-subscriptions-push-cx
Mention > echo-subscriptions-push-mention
Failed mention > echo-subscriptions-push-mention-failure
Successful mention > echo-subscriptions-push-mention-success
Page link > echo-subscriptions-push-article-linked
Connection with Wikidata > echo-subscriptions-push-wikibase-action
Failed login attempts > echo-subscriptions-push-login-fail
Login from an unfamiliar device > echo-subscriptions-push-login-success
Page review > echo-subscriptions-push-page-review
User rights change > echo-subscriptions-push-user-rights
Edit revert > echo-subscriptions-push-reverted
Email from other user > echo-subscriptions-push-emailuser
Edit milestone > echo-subscriptions-push-thank-you-edit

Event Timeline

cmadeo created this task.
cmadeo added a subscriber: Dmantena.

Hi @cmadeo, we had a few questions / notes from engineering sync:

  1. Trending current events is a different paradigm from echo notifications in that it's constructed from the explore feed data, kicked off locally and isn't tied to user data at all. I suggest we don't do this for client simplicity. It does sound like the backend team is considering these types of notifications eventually, so that would be good to wait until that is set up since it would flow through the same system and keep things simple on our end (see here).
  2. You already pointed out, the "learn more" content will need to be updated since it is all very much based on their personal account and stored on our server. Does Android have any copy we can lean on? If not I'm happy to help write something up once we're further in development.
  3. There's a few things that need to happen (in this order) before the user even has the possibility of receiving notifications:
    • We tell the OS that we would like to register for remote notifications. It returns to us a device token if OS registration was successful, or this step could fail.
    • We need to take that device token and register it with our server with another call. This step could also succeed or fail.
    • We need to ask the user permissions for receiving push notifications. This is the typical OS Apple permissions alert view. Once they have denied permission once (or reached into Settings and turned it off from there), it won't present that alert view again, instead we can detect that they have denied permission and add a link to kick them out to the Settings and modify permissions from there. We handle this in the current version of the app already, you'll notice the UI changes to just a single "Turn on Notifications" option that sends them to the Settings app.

The first two bullet points will happen behind the scenes, so we don't need any UI for it. I'm still investigating on when each will happen (upon launch or login, and deregistering on logout). But the point is they could fail and leave the user in a state where they can't move onto the permissions prompt and toggling types. We may want an error state on this Notification Settings screen to inform them that something went wrong.

And we'll want handling on this screen for these permission states: 1. The user never having allowed or denied permission, which will prompt the permissions prompt 2. The user having already denied permission, so link out to Settings app. I think the way we do it in the current app could work (ask permission upon first toggle, swap out all UI with a link to Settings if denied), but I wanted to mention it in case you want to rethink it.

Two more things:

  • Because these notifications are tied to their account, they should be logged in. So we may want to have a logged-out state or hide this path from the Settings root view if they are logged out.
  • We also might need to rethink our Explore feed card that asks for permissions, due to the additional failure cases. Perhaps we just hide this card if registrations have failed or they are logged out?

Thanks!

  1. Trending current events is a different paradigm from echo notifications in that it's constructed from the explore feed data, kicked off locally and isn't tied to user data at all. I suggest we don't do this for client simplicity. It does sound like the backend team is considering these types of notifications eventually, so that would be good to wait until that is set up since it would flow through the same system and keep things simple on our end (see here).

Thanks @Tsevener, it looks like we're already supporting turning push notifications on and off for this notification type, is this something we need to remove?

  1. You already pointed out, the "learn more" content will need to be updated since it is all very much based on their personal account and stored on our server. Does Android have any copy we can lean on? If not I'm happy to help write something up once we're further in development.

Hmm! I'm not sure but I'll take a look or can ask folks on Android.

  1. There's a few things that need to happen (in this order) before the user even has the possibility of receiving notifications:
    • We tell the OS that we would like to register for remote notifications. It returns to us a device token if OS registration was successful, or this step could fail.
    • We need to take that device token and register it with our server with another call. This step could also succeed or fail.
    • We need to ask the user permissions for receiving push notifications. This is the typical OS Apple permissions alert view. Once they have denied permission once (or reached into Settings and turned it off from there), it won't present that alert view again, instead we can detect that they have denied permission and add a link to kick them out to the Settings and modify permissions from there. We handle this in the current version of the app already, you'll notice the UI changes to just a single "Turn on Notifications" option that sends them to the Settings app.

The first two bullet points will happen behind the scenes, so we don't need any UI for it. I'm still investigating on when each will happen (upon launch or login, and deregistering on logout). But the point is they could fail and leave the user in a state where they can't move onto the permissions prompt and toggling types. We may want an error state on this Notification Settings screen to inform them that something went wrong.

Sounds good, happy to design an error state for this, to clarify...would it be appropriate to use a similar error state to something that kicks users back to system settings (in the case where they have denied notification permissions in the past)?

And we'll want handling on this screen for these permission states: 1. The user never having allowed or denied permission, which will prompt the permissions prompt 2. The user having already denied permission, so link out to Settings app. I think the way we do it in the current app could work (ask permission upon first toggle, swap out all UI with a link to Settings if denied), but I wanted to mention it in case you want to rethink it.

This sounds good to me!

Two more things:

  • Because these notifications are tied to their account, they should be logged in. So we may want to have a logged-out state or hide this path from the Settings root view if they are logged out.

Good point!

  • We also might need to rethink our Explore feed card that asks for permissions, due to the additional failure cases. Perhaps we just hide this card if registrations have failed or they are logged out?

Sounds good, happy to re-think that (or remove it)

it looks like we're already supporting turning push notifications on and off for this notification type, is this something we need to remove?

Yep that's my thinking, remove until PI supports it on their side since it's called out as a future possibility in their documentation. But we can discuss this more as a team, maybe it's something we can look into hooking back up at the end of push support development. The echo notifications API only shows account-related notifications, not this news notification. So this would be the one notification that doesn't show in the user's app notification center, which could feel odd.

Sounds good, happy to design an error state for this, to clarify...would it be appropriate to use a similar error state to something that kicks users back to system settings (in the case where they have denied notification permissions in the past)?

Yeah, it could be, though we wouldn't want to send them to system settings since there's nothing they could do to fix it. We would just have to try registering for the user again later behind-the-scenes. Maybe just some label that's like "There was an issue in setting up push notifications. Please try again later.", and hide the notifications type toggles.

@Tsevener sounds good on both fronts! I'll get working on an error state for setting up push notifications!

@cmadeo I'm not sure if it's appropriate to move this back into Needs Design or Blocked, but I think there may be some edge case states that need to be designed based on your last comment and our discussion just now in engineering sync. I'll revisit and add details tomorrow, but just wanted to let you know why it's back in your column. Thanks!

@cmadeo In engineering sync, we came up with four "walls" that the user has to get through in order to begin receiving push notifications. I thought thinking of it in this way would helpful with the alternative UIs we may need to show on this page and possibly others.

  1. User must me logged in.
  2. Then, user must grant app permissions. This could also be in a "denied" state, where we need to show the user a button to go to OS Settings.
  3. At this point once permissions are granted, behind the scenes we need to send in the device token to our servers for push registration/subscription. This could succeed or fail. If it fails the user just needs some sort of UI to try again later. (This is the state UI we still need for this screen).
  4. User needs to toggle ON notification types, because the plan was to have all of these OFF by default

For step 4, we were wondering if it is necessary to require the user to do this. Can we remove the "all types must be toggled off by default" requirement? It seems like one more step to go through for a user that has already granted us permissions, plus it's an extra step for us to send a call to turn these off on the server upon first launch. So whenever a user starts with version 6.9 (assuming they make it through walls 1-3), these toggles will just reflect whatever the server currently has on it (could be on, could be off, could be mixed).

@Tsevener this is super helpful and I agree that if we have the user's permission to receive notifications it would be okay to remove the "all types must be toggled off by default" requirement

@Tsevener do you need anything else on this ticket?

Design needs:

  • Design should remove 'trending and current events' from the mock
  • Logged out state
  • Error state for notification subscription issue 'please try again later'
cmadeo updated the task description. (Show Details)