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

Runtime notifications permission #18239

Conversation

irfano
Copy link
Member

@irfano irfano commented Apr 6, 2023

Fixes #17714
This PR adds runtime notifications permission support.

Notifications screen
This adds a yellow warning box to the notifications screen. It's visible when the notification permission is not allowed.

  • If you tap the "x" button, it's dismissed, and you can't see it unless you allow the permission and disable it again.
  • FIX button opens the system's permission dialog. If you deny the permission several times, the FIX button opens the permission bottom sheet that navigates you to the device settings.

Blogging reminders
This adds a notification permission step to the blogging reminders flow. If the permission is denied and the app can't show the system's native permission dialog, the "Go to Settings → Notifications → App Settings, and turn WordPress on to be notified immediately." message will be displayed. Otherwise, it will be invisible.
Screenshot 2023-04-07 at 01 33 11

Design: YSbDG4YKoDDkjEZSfBrIlN-fi-1300_13590

To test:
Test both on the API 33 device and a lower version device. The behavior shouldn't change on older devices.

Notifications screen - Allow

  1. Launch the JP app.
  2. Ensure you don't allow the notification permission.
  3. Navigate to Notifications.
  4. Notice the yellow warning box.
  5. Tap Fix.
  6. Ensure it opens the system's permission dialog.
  7. Allow permission.
  8. Ensure the warning has been lost and the permission is allowed on system settings.

Notifications screen - Deny

  1. Launch the JP app.
  2. Ensure you don't allow the notification permission.
  3. Navigate to Notifications.
  4. Notice the yellow warning box.
  5. Tap Fix and deny the native permission dialog.
  6. Tap Fix again.
  7. Notice the permission bottom sheet appears.
  8. Tap the green button. Notice it sends you to the permission settings screen.

Blogging reminders screen - Allow

  1. Launch the JP app.
  2. Ensure that notification permission is not allowed.
  3. Navigate to My Site → Site Settings.
  4. Tap Reminders.
  5. Choose some days and tap Update.
  6. Notice the native permission dialog.
  7. Allow it and test the positive case.

Blogging reminders screen - Deny

  1. Launch the JP app.
  2. Ensure that notification permission is not allowed.
  3. Navigate to My Site → Site Settings.
  4. Tap Reminders.
  5. Choose some days and tap Update.
  6. Deny the native permission dialog.
  7. It should open permission bottom sheet.
  8. Tap the "Turn on notifications" button.
  9. Ensure it navigates you to the permission settings screen.
  10. Allow the notification permission and go back to the app.
  11. Ensure the permission screen is passed and the next screen on the flow is opened.

Regression Notes

  1. Potential unintended areas of impact
    Setting blogging Reminders.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Tested manually.

  3. What automated tests I added (or what prevented me from doing so)
    Updated current tests.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@irfano irfano added this to the 22.2 milestone Apr 6, 2023
@irfano irfano requested a review from ravishanker April 6, 2023 04:14
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 6, 2023

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr18239-d26cfbb
Commitd26cfbb
Direct Downloadwordpress-prototype-build-pr18239-d26cfbb.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 6, 2023

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr18239-d26cfbb
Commitd26cfbb
Direct Downloadjetpack-prototype-build-pr18239-d26cfbb.apk
Note: Google Login is not supported on these builds.

@ravishanker
Copy link
Contributor

ravishanker commented Apr 6, 2023

@irfano - Great work!

  • Notifications Screen - Allow: Suggest that entire banner is made clickable. Fix word and touch target is obscure. Not sure if it needs that x. When banner is dismissed it disappears, they may not know how to enable notifications just in case!
  • Notifications Screen - Deny: Didn't have to deny several time to see the Bottom sheet
Banner Bottomsheet Popup
Screenshot_20230406_174700 Screenshot_20230406_174952 Screenshot_20230406_175142

<string name="notifications_permission_dismiss_content_description">Dismiss notification permission warning.</string>
<string name="notifications_permission_off_desc">Push notifications are turned off.</string>
<string name="notifications_permission_bottom_sheet_title">Push notifications are turned off</string>
<string name="notifications_permission_bottom_sheet_description_1">You’ll need to open the app to see notifications.</string>
Copy link
Contributor

@ravishanker ravishanker Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing escape \ character before ' in You'll. Also, same below L:4071

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've learned that and ' are different. 😄 I copied strings from Figma, which uses , that's why the IDEA didn't show an error. But I've changed to '. They both are okay but standard ' is more popular in our strings.

Done: 6ee77da

Copy link
Contributor

@ravishanker ravishanker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, it's done! 🚀

@irfano
Copy link
Member Author

irfano commented Apr 6, 2023

Suggest that entire banner is made clickable. Fix word and touch target is obscure.

That makes sense 👍🏻 I'll make the change. Done: 63431aa

Not sure if it needs that x. When banner is dismissed it disappears, they may not know how to enable notifications just in case!

Some users may be aware that notifications are turned off and just want to close the banner. But as a side effect, other users who are unaware of it may tap "X" by mistake and lose the benefit of the banner.
Maybe we can remove the "X" button and hide the banner if permission is always denied. Wdyt?

@wpmobilebot
Copy link
Contributor

Found 1 violations:

The PR caused the following dependency changes:

-\--- org.wordpress:utils:{strictly 3.5.0} -> 3.5.0
+\--- org.wordpress:utils:{strictly trunk-3c3d298621fda50b5cbf6b981b78920b80fa82b4} -> trunk-3c3d298621fda50b5cbf6b981b78920b80fa82b4

Please review and act accordingly

…13)' into issue/17714-notification-runtime-permission
@irfano
Copy link
Member Author

irfano commented Apr 6, 2023

Didn't have to deny several time to see the Bottom sheet

I think it changes that how many times you can deny the system's native permission dialog until we can't show it anymore. Because I can see and deny the system's native permission dialog twice after the clean install.
I've also made a change to the blogging reminders flow. af470ad See the updated description.

…13)' into issue/17714-notification-runtime-permission
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@irfano irfano merged commit e1ff390 into Parent-PR-for-updating-targetSdkVersion-to-33-(Android-13) Apr 8, 2023
@irfano irfano deleted the issue/17714-notification-runtime-permission branch April 8, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants