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.

Sign up for GitHub

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

Jump to bottom

Don't show the notification permission request as the first screen #3350

Merged
merged 4 commits into from
Dec 13, 2024
Merged

Don't show the notification permission request as the first screen #3350

merged 4 commits into from
Dec 13, 2024

Conversation

mebarbosa
Copy link
Contributor

@mebarbosa mebarbosa commented Dec 11, 2024 •
edited
Loading

Description

  • This is the first PR to improve the notification permission request. Here, I am proposing to request the notification permission when is actually needed. This PR removes the notification permission request from the first screen and move it to when the user tries to enable the notification for a podcast.
  • I did not change the notification message the we already have in the app.
  • ⚠️ I am going to handle the Warm before using data notification in another PR

Fixes #3360

Testing Instructions

Android 15 - Notification ON

  1. Have a fresh install
  2. Open the app and ensure you don't see the notification request in Log in page ✅
  3. Go to some podcast
  4. Follow this podcast
  5. Tap to enable the notification
  6. Ensure you see the notification permission
  7. Allow it
  8. Ensure you see the toast with notification ON ✅

Android 15 - Notification OFF

  1. Have a fresh install
  2. Open the app and ensure you don't see the notification request in Log in page ✅
  3. Go to some podcast
  4. Follow this podcast
  5. Tap to enable the notification
  6. Ensure you see the notification permission ✅
  7. Do not Allow it
  8. Ensure don't see the toast with notification on
  9. Tap to enable the notification again
  10. Ensure you see the warning about not having notification permission ✅

Android 11

  1. Have a fresh install
  2. Go to some podcast
  3. Follow this podcast
  4. Tap to enable the notification
  5. Ensure you see the toast with notification ON ✅
  6. Disable the podcast notification
  7. Ensure you see the toast with notification OFF ✅

Screenshots or Screencast

Android 15 - Notification On

android.15.notification.on.webm

Android 15 - Notification Off

android.15.notification.off.webm

Android 11

android.11.webm

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@mebarbosa mebarbosa added [Type] Enhancement Improve an existing feature. [Area] Notification labels Dec 11, 2024
@mebarbosa mebarbosa added this to the 7.80 milestone Dec 11, 2024
@mebarbosa mebarbosa requested a review from a team as a code owner December 11, 2024 12:11
@mebarbosa mebarbosa requested review from MiSikora and removed request for a team December 11, 2024 12:11
@mebarbosa mebarbosa linked an issue Dec 11, 2024 that may be closed by this pull request
Don't show the notification permission request as the first screen #2698
Open
1 task
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 11, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
App Name 📱 Mobile
Build TypedebugProd
Commitcb605f4
Direct Downloadpocketcasts-app-prototype-build-pr3350-cb605f4.apk
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name 🚗 Automotive
Build TypedebugProd
Commitcb605f4
Direct Downloadpocketcasts-automotive-prototype-build-pr3350-cb605f4.apk
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name ⌚ Wear
Build TypedebugProd
Commitcb605f4
Direct Downloadpocketcasts-wear-prototype-build-pr3350-cb605f4.apk

MiSikora
MiSikora reviewed Dec 12, 2024
View reviewed changes
Copy link
Contributor

@MiSikora MiSikora left a comment

Choose a reason for hiding this comment

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

I think this doesn't cover everything. Right now, for example, I can do the following:

1.Follow a podcast.
2. Go to the Profile tab.
3. Go to the Settings.
4. Go to the Notifications.
5. Enable Notify me.

The app will show as if notifications are enabled but the permission still isn't granted.

...ces/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/NotificationPermissionHelper.kt
Comment on lines +16 to +17
onPermissionGranted: () -> Unit = {},
onPermissionHandlingNotRequired: () -> Unit = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you ever anticipate to these two callbacks do different things? I think for a user perspective it doesn't matter whether if they granted the permission or if it was already granted by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I put just one callback, the onPermissionGranted one. But I noticed that in MainActivity we were not doing anything for the case we don't need to require permission when call checkForNotificationPermission, so I decided to do this to avoid any potential issue

@mebarbosa
Copy link
Contributor Author

I think this doesn't cover everything. Right now, for example, I can do the following:

1.Follow a podcast. 2. Go to the Profile tab. 3. Go to the Settings. 4. Go to the Notifications. 5. Enable Notify me.

The app will show as if notifications are enabled but the permission still isn't granted.

@MiSikora I am preparing another PR to handle this other Notification screen scenario

@mebarbosa mebarbosa requested a review from MiSikora December 12, 2024 12:27
@mebarbosa mebarbosa mentioned this pull request Dec 12, 2024
Merged
6 tasks
@mebarbosa
Copy link
Contributor Author

@MiSikora here is the PR with the scenario you found: #3357

@MiSikora
Copy link
Contributor

Before I review, could you clarify the plan here? Are you planning to go screen by screen and then merge everything into one PR to main?

I’m not opposed to this approach, but I’d like to understand the scope and effort involved. There are other scenarios beyond the one I mentioned in this review that might need consideration. For example, navigating to a podcast, accessing its settings, and toggling the Notifications switch should also trigger the notifications permission.

There may be additional scenarios as well, so I think it would be helpful to outline them before starting the work. This would allow us to get a rough estimate of the effort involved. Right now there is only a mention of the Warn before using data notification.

@mebarbosa
Copy link
Contributor Author

Before I review, could you clarify the plan here? Are you planning to go screen by screen and then merge everything into one PR to main?

I’m not opposed to this approach, but I’d like to understand the scope and effort involved. There are other scenarios beyond the one I mentioned in this review that might need consideration. For example, navigating to a podcast, accessing its settings, and toggling the Notifications switch should also trigger the notifications permission.

There may be additional scenarios as well, so I think it would be helpful to outline them before starting the work. This would allow us to get a rough estimate of the effort involved. Right now there is only a mention of the Warn before using data notification.

@MiSikora My plan is to split this into different PRs to make testing easier. While working on this, I found the tests a bit annoying because I had to account for different Android versions and scenarios, such as granting or not granting permissions. And that was just one scenario. I imagine that having more scenarios could make it even harder for testing.

I followed your suggestion I created subtask for the parent and I updated with a comment: #2698 (comment)

@mebarbosa mebarbosa linked an issue Dec 13, 2024 that may be closed by this pull request
[Notification Permission Request] Remove notification permission request from first screen #3360
Open
@mebarbosa mebarbosa enabled auto-merge (squash) December 13, 2024 12:20
@mebarbosa mebarbosa merged commit 3fb03a0 into task/notification-permission Dec 13, 2024
17 checks passed
@mebarbosa mebarbosa deleted the task/update-notification-permission-request branch December 13, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiSikora MiSikora MiSikora approved these changes

Assignees
No one assigned
Labels
[Area] Notification [Type] Enhancement Improve an existing feature.
Projects
None yet
Milestone
7.80
3 participants
@mebarbosa @wpmobilebot @MiSikora