fix: [SDK-4735] route requestPermission fallbackToSettings to settings when permanently denied#2656
Conversation
… via rationale transition
This reverts commit 92e2747.
📊 Diff Coverage ReportDiff Coverage Report (Changed Lines Only)Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff). Changed Files Coverage
Overall (aggregate gate)18/173 touched executable lines covered (10.4% — requires ≥ 80%) Per-file detail (informational; gate is aggregate above):
❌ Coverage Check FailedAggregate coverage on touched lines is 10.4% (minimum 80%). |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Permission-flow logic change with new persisted state and deferred lifecycle handler registration — worth a human look despite no bugs surfaced by automated review.
Extended reasoning...
Overview
The PR fixes two bugs in requestPermission(fallbackToSettings = true) for notification and location permissions: (1) the settings fallback dialog was being torn down by PermissionsActivity.onDestroy immediately after creation, and (2) the permanently-denied recovery path could get permanently stuck when the shouldShowRequestPermissionRationale true → false transition was never witnessed. Touches PermissionsActivity, AlertDialogPrepromptForAndroidSettings, PermissionsViewModel, both NotificationPermissionController / LocationPermissionController, a new PROMPTED_PERMISSION_ preference key, plus an unrelated LocationModule import fix and formatting.
Security risks
No direct security risks — no auth, crypto, or permission-grant logic changes. The change only affects how the SDK routes the user to the system Settings UI when a permission is permanently denied; the OS still owns the actual grant decision. The new preference key is a boolean local flag with no PII.
Level of scrutiny
Medium-to-high. This is critical user-facing runtime-permission flow code, with logic changes (lifecycle handler registration to defer dialog presentation, new persisted state with subtle ordering relative to shouldShowSettings) rather than a mechanical refactor. The deferred-dialog pattern relies on addActivityLifecycleHandler firing onActivityAvailable synchronously when there is a current activity, which is correctly handled by the is PermissionsActivity guard. The recovery-path ordering (PROMPTED flag written after shouldShowSettings reads it) is documented and looks correct.
Other factors
The diff coverage gate is failing (10.4% vs required 80%) because the new Controller paths are exercised by Robolectric tests that JaCoCo doesn't credit — substantive tests were added (3 cases per controller plus 3 ViewModel cases), so this is a tooling artifact rather than a real coverage gap. PR has been manually tested by the author. Author explicitly requested review via @claude review; given the touched surface (permissions) and the non-trivial state machine changes, a human reviewer familiar with the Oct 2025 ViewModel refactor history should sign off.
nan-li
left a comment
nan-li
left a comment
There was a problem hiding this comment.
I repro'd original issue and now it works as expected
2 participants
Description
One Line Summary
Fix
requestPermission(fallbackToSettings = true)so the "open settings" prompt actually appears when a permission is permanently denied.Before, SDK wasn't showing settings prompt:
Details
Motivation
When a notification (or location) permission is permanently denied,
requestPermission(true)is supposed to route the user to the app's settings. Two separate bugs prevented this:Settings dialog created then immediately dismissed (regression). The fallback dialog was parented to the transparent
PermissionsActivity, which finishes immediately after the reject callback. ItsonDestroy()then calledAlertDialogPrepromptForAndroidSettings.dismissCurrentDialog(), tearing down the dialog the instant it was shown. Symptom:showSettings == truein the debugger, but nothing visible on screen. This regressed in the Oct 2025 ViewModel refactor (6fa661dfb), which moved the activity-finish to after the callback and added theonDestroydismiss; the pre-refactor flow finished the activity first and showed the dialog on the resumed host activity.Stuck permanently-denied state.
shouldShowSettingsonly persisted the "resolved denial" preference on the exactshouldShowRequestPermissionRationaletrue -> falsetransition. If a permission was already permanently blocked when OneSignal first observed it (denied via system Settings, in a prior session/version, or process death mid-prompt), that transition is never seen, the pref staysfalse, and the fallback is permanently stuck.Scope
NotificationPermissionController/LocationPermissionController: the fallback dialog is now deferred until a non-PermissionsActivityhost activity is in the foreground, then shown there.PermissionsActivity.onDestroy -> dismissCurrentDialog()removed, along with the now-deadcurrentDialog/dismissCurrentDialogtracking inAlertDialogPrepromptForAndroidSettings.PermissionsViewModel+IPreferencesService: newPROMPTED_PERMISSION_pref records that OneSignal has requested a permission at least once, used byshouldShowSettingsto recover the permanently-blocked state without having witnessed the rationale transition.IModuleimport inLocationModuleand reformats its builder chains.Testing
Unit testing
Added 3 cases to
PermissionsViewModelTestscovering the recovery path: shows settings when permanently blocked and previously prompted, does not show settings on a first denial when never prompted before, and persists the prompted-before flag after a request. Existing permission tests updated/retained.Manual testing
Built the demo app against the local SDK in Android Studio. Verified that, on an install whose notification permission was permanently denied, tapping PROMPT PUSH now surfaces the "open settings" dialog (and it stays visible) rather than nothing.
Affected code checklist
Note: touches notification and location runtime-permission prompting (no dedicated checklist item exists for permissions).
Checklist
Overview
Testing
Final pass
Made with Cursor