Skip to content

Runtime notifications permission#18239

Merged
irfano merged 16 commits intoParent-PR-for-updating-targetSdkVersion-to-33-(Android-13)from
issue/17714-notification-runtime-permission
Apr 8, 2023
Merged

Runtime notifications permission#18239
irfano merged 16 commits intoParent-PR-for-updating-targetSdkVersion-to-33-(Android-13)from
issue/17714-notification-runtime-permission

Conversation

@irfano
Copy link
Copy Markdown
Member

@irfano irfano commented Apr 6, 2023 •
edited
Loading

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
Copy Markdown
Contributor

wpmobilebot commented Apr 6, 2023 •
edited
Loading

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
Copy Markdown
Contributor

wpmobilebot commented Apr 6, 2023 •
edited
Loading

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
Copy Markdown
Contributor

ravishanker commented Apr 6, 2023 •
edited by irfano
Loading

@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

ravishanker
ravishanker reviewed Apr 6, 2023
View reviewed changes
WordPress/src/main/res/values/strings.xml Outdated
<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
Copy Markdown
Contributor

@ravishanker ravishanker Apr 6, 2023 •
edited
Loading

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
Copy Markdown
Member Author

@irfano irfano Apr 6, 2023

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

ravishanker
ravishanker approved these changes Apr 6, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

irfano commented Apr 6, 2023 •
edited
Loading

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
Copy Markdown
Contributor

wpmobilebot commented Apr 6, 2023

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

@irfano
Merge branch 'Parent-PR-for-updating-targetSdkVersion-to-33-(Android-…
6cf91df 
…13)' into issue/17714-notification-runtime-permission
@irfano
Copy link
Copy Markdown
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.

@irfano
Merge branch 'Parent-PR-for-updating-targetSdkVersion-to-33-(Android-…
32e2a4e 
…13)' into issue/17714-notification-runtime-permission
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 7, 2023

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

Reviewers

1 more reviewer

@ravishanker ravishanker ravishanker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

[Type] Task

Projects

None yet

Milestone

22.2

Development

Successfully merging this pull request may close these issues.

3 participants

@irfano @wpmobilebot @ravishanker