From 96c3de9775c3aa44e137f6393a0b4dcb7524b9b1 Mon Sep 17 00:00:00 2001 From: Mohanned Binmiskeen Date: Tue, 21 Jul 2026 21:19:48 +0300 Subject: [PATCH] fix: sync notification permission changes on app resume The resume-time reconciliation (checkAndHandlePermissionStatusChange) saved the new OS permission status before registerDevice compared against it, so registerDevice saw no change and skipped the server update. On devices that do not kill the app on a permission toggle (e.g. Samsung), this left the device stuck at its previous pushNotificationEnabled value in PushFire. Stop pre-writing the last-known status; registerDevice now detects the change, PATCHes, and persists the status only after a successful sync (so a failed sync retries on the next resume). Developer opt-out via setNotificationEnabled(false) still survives an OS re-grant. Adds regression coverage for the resume vs cold-start paths and the reported Samsung scenarios. --- CHANGELOG.md | 5 + lib/src/services/device_service.dart | 11 +- pubspec.yaml | 2 +- ...vice_permission_resume_scenarios_test.dart | 165 ++++++++++++++++++ ..._service_resume_permission_repro_test.dart | 125 +++++++++++++ 5 files changed, 305 insertions(+), 3 deletions(-) create mode 100644 test/services/device_service_permission_resume_scenarios_test.dart create mode 100644 test/services/device_service_resume_permission_repro_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index f7b8801..2ba3604 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## [0.3.1] + +### Fixed +- **Notification permission changes now sync to PushFire when the app is resumed, not just on cold start.** When the OS notification permission changed while the app stayed alive (common on some OEMs, e.g. Samsung, which do not kill the app on a settings toggle), the resume-time reconciliation saved the new permission status *before* `registerDevice` compared against it, so `registerDevice` saw "no change" and skipped the server update — leaving the device stuck at its previous `pushNotificationEnabled` value in PushFire (a re-grant kept showing as denied). The saved status is no longer written up front; `registerDevice` now detects the change and PATCHes, and persists the status only after a successful sync (so a failed sync is retried on the next resume instead of being lost). Developer opt-out via `setNotificationEnabled(false)` still survives an OS re-grant. + ## [0.3.0] ### Added diff --git a/lib/src/services/device_service.dart b/lib/src/services/device_service.dart index deff9ff..87e2788 100644 --- a/lib/src/services/device_service.dart +++ b/lib/src/services/device_service.dart @@ -648,8 +648,12 @@ class DeviceService { return null; } - // OS permission changed — save the new OS status - await _savePermissionStatus(currentOsPermission); + // OS permission changed. Do NOT persist the new status here: + // registerDevice() compares the incoming value against the saved + // last-known status to decide whether to PATCH, and saves it itself only + // after a successful sync. Writing it up front made registerDevice() see + // "no change" and silently skip the server update (and also suppressed + // retries, since a failed sync would still have advanced the status). if (!currentOsPermission) { // OS permission was revoked — always PATCH server false @@ -666,6 +670,9 @@ class DeviceService { final device = await registerDevice(); return device; } else { + // Developer opted out — don't touch the server, but acknowledge the + // OS change so it isn't re-detected on every resume. + await _savePermissionStatus(currentOsPermission); PushFireLogger.info( 'OS notification permission re-granted but preference is disabled - not restoring'); return null; diff --git a/pubspec.yaml b/pubspec.yaml index 532dfe0..8da72cb 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -3,7 +3,7 @@ description: A lightweight push notification tracking SDK for Firebase. homepage: https://github.com/FlywheelStudio/pushfire_sdk/blob/main/README.md repository: https://github.com/FlywheelStudio/pushfire_sdk -version: 0.3.0 +version: 0.3.1 environment: sdk: '>=3.3.1 <4.0.0' diff --git a/test/services/device_service_permission_resume_scenarios_test.dart b/test/services/device_service_permission_resume_scenarios_test.dart new file mode 100644 index 0000000..ebf15be --- /dev/null +++ b/test/services/device_service_permission_resume_scenarios_test.dart @@ -0,0 +1,165 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:pushfire_sdk/src/api/pushfire_api_client.dart'; +import 'package:pushfire_sdk/src/config/pushfire_config.dart'; +import 'package:pushfire_sdk/src/services/device_service.dart'; + +/// End-to-end-ish coverage of the exact scenarios reported on a Samsung device +/// (SM-A546E) that does NOT kill the app when the notification permission is +/// toggled in system settings, so the SDK reconciles via the resume path +/// (checkAndHandlePermissionStatusChange), not a cold start. +/// +/// Each test asserts what the server (PushFire) should end up being told. +class RecordingApiClient extends PushFireApiClient { + final List> postCalls = []; + final List> patchCalls = []; + + RecordingApiClient() + : super(const PushFireConfig(apiKey: 'test', baseUrl: 'http://test/')); + + @override + Future> post( + String endpoint, Map data) async { + postCalls.add({'endpoint': endpoint, 'data': data}); + return {'id': 'device-1'}; + } + + @override + Future> patch( + String endpoint, Map data) async { + patchCalls.add({'endpoint': endpoint, 'data': data}); + return {'success': true}; + } +} + +class PlatformState { + bool osPermission; + PlatformState(this.osPermission); +} + +const _deviceInfo = { + 'os': 'android', + 'osVersion': '16', + 'language': 'en', + 'manufacturer': 'samsung', + 'model': 'SM-A546E', + 'appVersion': '1.0.0', +}; + +DeviceService buildService(RecordingApiClient api, PlatformState state) { + return DeviceService( + api, + const PushFireConfig(apiKey: 'test', baseUrl: 'http://test/'), + isPushNotificationEnabledOverride: () async => state.osPermission, + getDeviceInfoOverride: () async => _deviceInfo, + getFcmTokenOverride: () async => 'fcm-token-1', + ); +} + +/// The value of pushNotificationEnabled in the most recent PATCH, or null if +/// no PATCH was sent. +bool? lastPatchedEnabled(RecordingApiClient api) { + if (api.patchCalls.isEmpty) return null; + return api.patchCalls.last['data']['data']['pushNotificationEnabled'] as bool?; +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUp(() => SharedPreferences.setMockInitialValues({})); + + test('Case 1 — granted, then REVOKED in settings, app resumed -> server denied', + () async { + final api = RecordingApiClient(); + final state = PlatformState(true); + final service = buildService(api, state); + + await service.registerDevice(); // first launch: enabled + api.patchCalls.clear(); + + // User turns notifications OFF in settings; app is resumed (not killed). + state.osPermission = false; + await service.checkAndHandlePermissionStatusChange(); + + expect(api.patchCalls, hasLength(1)); + expect(lastPatchedEnabled(api), false); + }); + + test('Case 2 — denied, then RE-GRANTED in settings, app resumed -> restored', + () async { + final api = RecordingApiClient(); + final state = PlatformState(true); + final service = buildService(api, state); + + await service.registerDevice(); // enabled + state.osPermission = false; // revoke + await service.checkAndHandlePermissionStatusChange(); + api.patchCalls.clear(); + + // User turns notifications back ON; app resumed. + state.osPermission = true; + await service.checkAndHandlePermissionStatusChange(); + + // This is the case that used to stay "denied" in PushFire. + expect(api.patchCalls, hasLength(1)); + expect(lastPatchedEnabled(api), true); + }); + + test( + 'Case 3 — developer disabled via setNotificationEnabled(false): OS ' + 're-grant does NOT restore', () async { + final api = RecordingApiClient(); + final state = PlatformState(true); + final service = buildService(api, state); + + await service.registerDevice(); // enabled + await service.setNotificationEnabled(false); // developer opt-out + api.patchCalls.clear(); + + // OS toggled off then back on while the app is alive. + state.osPermission = false; + await service.checkAndHandlePermissionStatusChange(); + state.osPermission = true; + await service.checkAndHandlePermissionStatusChange(); + + // Server must never be flipped back to enabled against the opt-out. + final anyEnabledPatch = api.patchCalls.any((c) => + c['data']['data']['pushNotificationEnabled'] == true); + expect(anyEnabledPatch, isFalse, + reason: 'developer opt-out must survive an OS re-grant'); + }); + + test('Case 4 — app resumed with NO permission change -> no server call', + () async { + final api = RecordingApiClient(); + final state = PlatformState(true); + final service = buildService(api, state); + + await service.registerDevice(); // enabled + api.patchCalls.clear(); + api.postCalls.clear(); + + // Resume, permission unchanged. + await service.checkAndHandlePermissionStatusChange(); + + expect(api.patchCalls, isEmpty); + expect(api.postCalls, isEmpty); + }); + + test('Case 5 — resuming again after a synced revoke -> no duplicate PATCH', + () async { + final api = RecordingApiClient(); + final state = PlatformState(true); + final service = buildService(api, state); + + await service.registerDevice(); // enabled + state.osPermission = false; + await service.checkAndHandlePermissionStatusChange(); // syncs -> denied + api.patchCalls.clear(); + + // App resumed a second time, still denied, nothing changed. + await service.checkAndHandlePermissionStatusChange(); + + expect(api.patchCalls, isEmpty); + }); +} diff --git a/test/services/device_service_resume_permission_repro_test.dart b/test/services/device_service_resume_permission_repro_test.dart new file mode 100644 index 0000000..72d47c9 --- /dev/null +++ b/test/services/device_service_resume_permission_repro_test.dart @@ -0,0 +1,125 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:pushfire_sdk/src/api/pushfire_api_client.dart'; +import 'package:pushfire_sdk/src/config/pushfire_config.dart'; +import 'package:pushfire_sdk/src/services/device_service.dart'; + +/// Reproduction for the permission-change sync bug. +/// +/// Symptom (reported on a Samsung device that does NOT kill the app when the +/// user toggles the notification permission in system settings): after the +/// permission changes and the app is brought back to the foreground, PushFire +/// still shows the old value. On a Pixel/emulator the OS kills the app on the +/// permission change, so the working cold-start path runs instead and it looks +/// fine — hence the intermittency. +/// +/// These two tests isolate the difference: the SAME permission change is pushed +/// through the resume path vs the cold-start path. +class RecordingApiClient extends PushFireApiClient { + final List> postCalls = []; + final List> patchCalls = []; + + RecordingApiClient() + : super(const PushFireConfig(apiKey: 'test', baseUrl: 'http://test/')); + + @override + Future> post( + String endpoint, Map data) async { + postCalls.add({'endpoint': endpoint, 'data': data}); + return {'id': 'device-1'}; + } + + @override + Future> patch( + String endpoint, Map data) async { + patchCalls.add({'endpoint': endpoint, 'data': data}); + return {'success': true}; + } +} + +class PlatformState { + bool osPermission; + PlatformState(this.osPermission); +} + +const _deviceInfo = { + 'os': 'android', + 'osVersion': '16', + 'language': 'en', + 'manufacturer': 'samsung', + 'model': 'SM-A546E', + 'appVersion': '1.0.0', +}; + +DeviceService buildService(RecordingApiClient api, PlatformState state) { + return DeviceService( + api, + const PushFireConfig(apiKey: 'test', baseUrl: 'http://test/'), + isPushNotificationEnabledOverride: () async => state.osPermission, + getDeviceInfoOverride: () async => _deviceInfo, + getFcmTokenOverride: () async => 'fcm-token-1', + ); +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUp(() => SharedPreferences.setMockInitialValues({})); + + test('CONTROL (cold-start path): OS revoke while app dead -> PATCH false', + () async { + final api = RecordingApiClient(); + final state = PlatformState(true); // permission granted at first launch + final service = buildService(api, state); + + // First launch: registers with enabled = true. + await service.registerDevice(); + api.postCalls.clear(); + api.patchCalls.clear(); + + // Permission revoked while the app was terminated; app relaunches and + // auto-register runs registerDevice() directly (the init/cold-start path). + state.osPermission = false; + await service.registerDevice(); + + // Cold-start path correctly syncs the server to disabled. + expect(api.patchCalls, hasLength(1)); + expect(api.patchCalls.first['endpoint'], 'update-device'); + expect( + api.patchCalls.first['data']['data']['pushNotificationEnabled'], + false, + ); + }); + + test( + 'BUG (resume path): OS revoke while app alive -> server should be PATCHed ' + 'false but is NOT', () async { + final api = RecordingApiClient(); + final state = PlatformState(true); // permission granted at first launch + final service = buildService(api, state); + + // First launch: registers with enabled = true. + await service.registerDevice(); + api.postCalls.clear(); + api.patchCalls.clear(); + + // User toggles the permission OFF in system settings. On a device that does + // not kill the app, the app is merely resumed, so the SDK reconciles via + // checkAndHandlePermissionStatusChange() (the same entry point used by the + // lifecycle observer and syncNotificationPermission()). + state.osPermission = false; + await service.checkAndHandlePermissionStatusChange(); + + // EXPECTED: the server is told the device is now disabled. + // ACTUAL (bug): no PATCH is sent, so PushFire keeps showing enabled. + expect( + api.patchCalls, + hasLength(1), + reason: 'resume path should PATCH update-device with the new value', + ); + expect( + api.patchCalls.first['data']['data']['pushNotificationEnabled'], + false, + ); + }); +}