Handle notification permission in BaseActivity#6440
Handle notification permission in BaseActivity#6440rajvirsingh2 wants to merge 11 commits intowikimedia:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates notification permission handling to provide user-facing guidance when push notification permission is denied, and adds a utility method to deep-link into the app’s notification settings.
Changes:
- Add snackbar messaging in
BaseActivityfor both “deny” and “don’t ask again” notification-permission outcomes. - Introduce a
DeviceUtilhelper to open the app’s notification settings screen. - Add new string resources for the snackbar message and action labels.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| app/src/main/res/values/strings.xml | Adds new user-facing strings for notification permission denial/rationale snackbars. |
| app/src/main/java/org/wikipedia/util/DeviceUtil.kt | Adds a helper to navigate the user to notification/app settings. |
| app/src/main/java/org/wikipedia/activity/BaseActivity.kt | Implements snackbar UX when POST_NOTIFICATIONS permission is denied. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun openNotificationSettings(context: Context){ | ||
| val intent= if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { | ||
| putExtra(Settings.EXTRA_APP_PACKAGE,context.packageName) | ||
| } | ||
| }else{ | ||
| Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { | ||
| data = android.net.Uri.fromParts("package", context.packageName, null) | ||
| } | ||
| } | ||
| context.startActivity(intent as? Intent) |
There was a problem hiding this comment.
openNotificationSettings() uses ACTION_APPLICATION_DETAILS_SETTINGS for pre-O devices, which opens the generic app details screen rather than the app’s notification settings. There’s already a pre-O approach in NotificationSettingsPreferenceLoader using the android.settings.APP_NOTIFICATION_SETTINGS action with app_package/app_uid extras; consider using that here (and only falling back to app details if the intent can’t be resolved).
| fun openNotificationSettings(context: Context){ | |
| val intent= if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | |
| Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { | |
| putExtra(Settings.EXTRA_APP_PACKAGE,context.packageName) | |
| } | |
| }else{ | |
| Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { | |
| data = android.net.Uri.fromParts("package", context.packageName, null) | |
| } | |
| } | |
| context.startActivity(intent as? Intent) | |
| fun openNotificationSettings(context: Context) { | |
| val intent = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | |
| Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { | |
| putExtra(Settings.EXTRA_APP_PACKAGE, context.packageName) | |
| } | |
| } else { | |
| // Try the dedicated app notification settings screen on pre-O, if available, | |
| // and fall back to the generic app details screen otherwise. | |
| Intent("android.settings.APP_NOTIFICATION_SETTINGS").apply { | |
| putExtra("app_package", context.packageName) | |
| putExtra("app_uid", context.applicationInfo.uid) | |
| }.takeIf { it.resolveActivity(context.packageManager) != null } | |
| ?: Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { | |
| data = android.net.Uri.fromParts("package", context.packageName, null) | |
| } | |
| } | |
| context.startActivity(intent) |
| data = android.net.Uri.fromParts("package", context.packageName, null) | ||
| } | ||
| } | ||
| context.startActivity(intent as? Intent) |
There was a problem hiding this comment.
context.startActivity(intent as? Intent) is unnecessary and makes the call nullable; if that cast were ever to fail it would pass null and crash at runtime. Use context.startActivity(intent) and either (a) require an Activity parameter, or (b) add FLAG_ACTIVITY_NEW_TASK when context isn’t an Activity, since Context can be an application context.
| context.startActivity(intent as? Intent) | |
| if (context !is Activity) { | |
| intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | |
| } | |
| context.startActivity(intent) |
| if(!granted){ | ||
| if(Build.VERSION.SDK_INT>= Build.VERSION_CODES.O){ | ||
| if(ActivityCompat.shouldShowRequestPermissionRationale(this,android.Manifest.permission.POST_NOTIFICATIONS)){ | ||
| //User denied once->Show Snackbar | ||
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_rationale)) | ||
| .setAction(R.string.notification_permission_rationale_action){ | ||
| DeviceUtil.openNotificationSettings(this) | ||
| } | ||
| .show() | ||
| }else{ | ||
| //User denied (with "Don't Ask Again") | ||
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_denied)) | ||
| .setAction(R.string.app_settings){ |
There was a problem hiding this comment.
The permission being handled here is POST_NOTIFICATIONS, which only exists as a runtime permission on API 33+ (TIRAMISU). The SDK_INT >= O gate is misleading and could mask future changes (e.g., if the launcher were ever invoked on 26–32). Consider gating this logic with Build.VERSION_CODES.TIRAMISU (or removing the version check entirely, since the launcher is already only triggered on 33+ in NotificationPresenter.maybeRequestPermission).
| if(!granted){ | |
| if(Build.VERSION.SDK_INT>= Build.VERSION_CODES.O){ | |
| if(ActivityCompat.shouldShowRequestPermissionRationale(this,android.Manifest.permission.POST_NOTIFICATIONS)){ | |
| //User denied once->Show Snackbar | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_rationale)) | |
| .setAction(R.string.notification_permission_rationale_action){ | |
| DeviceUtil.openNotificationSettings(this) | |
| } | |
| .show() | |
| }else{ | |
| //User denied (with "Don't Ask Again") | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_denied)) | |
| .setAction(R.string.app_settings){ | |
| if (!granted) { | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | |
| if (ActivityCompat.shouldShowRequestPermissionRationale(this, android.Manifest.permission.POST_NOTIFICATIONS)) { | |
| //User denied once->Show Snackbar | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_rationale)) | |
| .setAction(R.string.notification_permission_rationale_action) { | |
| DeviceUtil.openNotificationSettings(this) | |
| } | |
| .show() | |
| } else { | |
| //User denied (with "Don't Ask Again") | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_denied)) | |
| .setAction(R.string.app_settings) { |
| <string name="notifications_all_types_text">All types</string> | ||
| <string name="notifications_menu_user_talk_page">%s\'s talk page</string> | ||
| <string name="notifications_offline_disable_message">The function is not available while offline.</string> | ||
| <string name="notification_permission_rationale">Enable Notification to stay updated</string> |
There was a problem hiding this comment.
User-facing copy: Enable Notification to stay updated is grammatically incorrect (singular/plural) and reads like a title rather than a sentence. Consider adjusting to something like “Enable notifications to stay up to date.” (and match capitalization/punctuation style used by nearby strings).
| <string name="notification_permission_rationale">Enable Notification to stay updated</string> | |
| <string name="notification_permission_rationale">Enable notifications to stay up to date.</string> |
| fun openNotificationSettings(context: Context){ | ||
| val intent= if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { |
There was a problem hiding this comment.
PR description mentions adding openAppNotificationSettings(), but the code introduces openNotificationSettings(). Consider aligning the method name (or updating the PR description) to avoid confusion for reviewers and future grep/search.
| fun openNotificationSettings(context: Context){ | ||
| val intent= if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| Intent(Settings.ACTION_APP_NOTIFICATION_SETTINGS).apply { | ||
| putExtra(Settings.EXTRA_APP_PACKAGE,context.packageName) | ||
| } | ||
| }else{ | ||
| Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS).apply { | ||
| data = android.net.Uri.fromParts("package", context.packageName, null) | ||
| } | ||
| } |
There was a problem hiding this comment.
Formatting: this block deviates from standard Kotlin formatting (missing spaces after fun params / before {, around = and after commas, and } else { style). Since the project runs ktlint as part of check, this is likely to fail CI; please run ktlint/auto-format on this file.
| if(!granted){ | ||
| if(Build.VERSION.SDK_INT>= Build.VERSION_CODES.O){ | ||
| if(ActivityCompat.shouldShowRequestPermissionRationale(this,android.Manifest.permission.POST_NOTIFICATIONS)){ | ||
| //User denied once->Show Snackbar | ||
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_rationale)) | ||
| .setAction(R.string.notification_permission_rationale_action){ | ||
| DeviceUtil.openNotificationSettings(this) | ||
| } | ||
| .show() | ||
| }else{ | ||
| //User denied (with "Don't Ask Again") | ||
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_denied)) | ||
| .setAction(R.string.app_settings){ |
There was a problem hiding this comment.
Formatting: the added if/else block is missing spaces around operators and after commas (e.g., if(!granted), SDK_INT>=, this,android.Manifest...). This repository enforces Kotlin style via ktlint, so this may fail ./gradlew check; please auto-format these lines.
| if(!granted){ | |
| if(Build.VERSION.SDK_INT>= Build.VERSION_CODES.O){ | |
| if(ActivityCompat.shouldShowRequestPermissionRationale(this,android.Manifest.permission.POST_NOTIFICATIONS)){ | |
| //User denied once->Show Snackbar | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_rationale)) | |
| .setAction(R.string.notification_permission_rationale_action){ | |
| DeviceUtil.openNotificationSettings(this) | |
| } | |
| .show() | |
| }else{ | |
| //User denied (with "Don't Ask Again") | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_denied)) | |
| .setAction(R.string.app_settings){ | |
| if (!granted) { | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | |
| if (ActivityCompat.shouldShowRequestPermissionRationale(this, android.Manifest.permission.POST_NOTIFICATIONS)) { | |
| //User denied once->Show Snackbar | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_rationale)) | |
| .setAction(R.string.notification_permission_rationale_action) { | |
| DeviceUtil.openNotificationSettings(this) | |
| } | |
| .show() | |
| } else { | |
| //User denied (with "Don't Ask Again") | |
| FeedbackUtil.makeSnackbar(this, getString(R.string.notification_permission_denied)) | |
| .setAction(R.string.app_settings) { |
What does this do?
Completes the TODO in BaseActivity to show user-facing messages when notification permission is denied. Adds openAppNotificationSettings() to DeviceUtil to support navigating the user directly to the app's notification settings.
Why is this needed?
Previously, when a user denied the notification permission, nothing happened and the denial was silently ignored. This change improves the user experience by giving clear, actionable feedback in both denial scenarios.
Phabricator:
https://phabricator.wikimedia.org/T...