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, + ); + }); +}