From 7696582d4657db9eb2a23acb3317243244c8c349 Mon Sep 17 00:00:00 2001 From: Mohanned Binmiskeen Date: Mon, 3 Aug 2026 11:33:25 +0200 Subject: [PATCH 1/5] fix: compare raw OS permission in registerDevice change check registerDevice() persisted the raw OS permission via _savePermissionStatus(osPermission) but compared the saved value against device.pushNotificationEnabled, which is the effective value (osPermission && preference). Any device with OS permission granted and the developer preference set to false via setNotificationEnabled(false) would PATCH on every launch, forever, since the effective value would never match the stored raw value. Compare like for like: lastPermissionStatus against osPermission. Fixes #5 --- lib/src/services/device_service.dart | 5 +++- ..._service_notification_preference_test.dart | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/lib/src/services/device_service.dart b/lib/src/services/device_service.dart index 87e2788..708f471 100644 --- a/lib/src/services/device_service.dart +++ b/lib/src/services/device_service.dart @@ -86,8 +86,11 @@ class DeviceService { if (existingDeviceId != null && lastFcmToken == fcmToken) { // Device already registered with same FCM token // Check if permission status changed - if so, update device + // Compare like for like: lastPermissionStatus is the raw OS + // permission (see _savePermissionStatus below), so it must be + // compared against the raw osPermission, not the effective value. if (lastPermissionStatus != null && - lastPermissionStatus != device.pushNotificationEnabled) { + lastPermissionStatus != osPermission) { PushFireLogger.info( 'Device permission status changed - updating device with ID: $existingDeviceId'); registeredDevice = diff --git a/test/services/device_service_notification_preference_test.dart b/test/services/device_service_notification_preference_test.dart index 051f932..a06dfa5 100644 --- a/test/services/device_service_notification_preference_test.dart +++ b/test/services/device_service_notification_preference_test.dart @@ -456,6 +456,32 @@ void main() { expect(prefs.getBool('pushfire_notification_preference'), true); }); + test( + 'does not PATCH again when OS permission is unchanged and preference ' + 'is off (regression for #5)', () async { + // Regression test for issue #5: registerDevice() must compare the + // raw OS permission against the raw last-saved OS permission + // (lastPermissionStatus), not against the effective value + // (osPermission && preference). Comparing raw against effective meant + // a device with OS permission granted but the developer preference + // set to false would PATCH on every single launch, forever. + final apiClient = FakeApiClient(); + final service = createTestService(apiClient: apiClient); + + // First launch: registers with OS permission granted (default + // preference true), then the developer turns notifications off. + await service.registerDevice(); + await service.setNotificationEnabled(false); + apiClient.patchCalls.clear(); + + // Second launch: same OS permission (granted), same FCM token, same + // stored device id, preference still off. Nothing has changed, so no + // PATCH should be sent. + await service.registerDevice(); + + expect(apiClient.patchCalls, isEmpty); + }); + test('does not overwrite existing preference on re-registration', () async { // Pre-set preference to false (developer opted out) SharedPreferences.setMockInitialValues({ From 9d9bae1b4e065375f24204e9658189e1f7209026 Mon Sep 17 00:00:00 2001 From: Mohanned Binmiskeen Date: Mon, 3 Aug 2026 11:33:44 +0200 Subject: [PATCH 2/5] fix: persist notification preference only after PATCH succeeds setNotificationEnabled() wrote the preference to SharedPreferences before the server PATCH. If the PATCH threw, local state diverged from the server permanently: the next call would read the already-changed local preference, see it match the requested value, and short-circuit with success without ever contacting the server again. Move _saveNotificationPreference() to after _updateDevice() succeeds. The pre-existing test "saves preference locally even if server call fails" asserted exactly this bug's behavior; it has been rewritten to assert the preference is left unchanged when the PATCH throws. Fixes #6 --- lib/src/services/device_service.dart | 9 ++++-- ..._service_notification_preference_test.dart | 29 ++++++++++++------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/lib/src/services/device_service.dart b/lib/src/services/device_service.dart index 708f471..c8718c0 100644 --- a/lib/src/services/device_service.dart +++ b/lib/src/services/device_service.dart @@ -530,9 +530,6 @@ class DeviceService { 'Cannot set notification preference - no device registered'); } - // Save preference locally first - await _saveNotificationPreference(enabled); - // PATCH server final prefs = await SharedPreferences.getInstance(); final fcmToken = prefs.getString(_fcmTokenKey); @@ -556,6 +553,12 @@ class DeviceService { await _updateDevice(device); + // Persist locally only after the server confirms. If the PATCH + // throws, local state must not diverge from the server, otherwise a + // retry would short-circuit above and report success without ever + // contacting the server again. + await _saveNotificationPreference(enabled); + PushFireLogger.info('Notification preference updated to $enabled'); return SetNotificationResult.success; } on PushFireException { diff --git a/test/services/device_service_notification_preference_test.dart b/test/services/device_service_notification_preference_test.dart index a06dfa5..d264201 100644 --- a/test/services/device_service_notification_preference_test.dart +++ b/test/services/device_service_notification_preference_test.dart @@ -182,24 +182,31 @@ void main() { ); }); - test('saves preference locally even if server call fails', () async { + test( + 'does not save preference locally when the server call fails ' + '(regression for #6)', () async { + // Regression test for issue #6: persisting the preference before the + // PATCH meant a failed request left local state saying "changed" + // while the server still had the old value — and because + // setNotificationEnabled short-circuits when the local preference + // already matches the requested value, a retry would report success + // without ever contacting the server again. The preference must only + // be persisted after the PATCH succeeds. final apiClient = FakeApiClient(); final service = createTestService(apiClient: apiClient); - await service.registerDevice(); + await service.registerDevice(); // saves default preference true apiClient.shouldThrowOnPatch = true; - // Should throw but preference should be saved locally - try { - await service.setNotificationEnabled(false); - fail('Expected exception'); - } on PushFireException { - // Expected - } + // Should throw and leave the local preference unchanged + await expectLater( + () => service.setNotificationEnabled(false), + throwsA(isA()), + ); - // Verify preference was saved locally + // Verify preference was NOT saved locally final prefs = await SharedPreferences.getInstance(); - expect(prefs.getBool('pushfire_notification_preference'), false); + expect(prefs.getBool('pushfire_notification_preference'), true); }); }); From aca5b3f5a3cedbc540ef5ae68b36d3c995d28c37 Mon Sep 17 00:00:00 2001 From: Mohanned Binmiskeen Date: Mon, 3 Aug 2026 11:33:56 +0200 Subject: [PATCH 3/5] fix: serialize scheduledFor as an absolute UTC instant WorkflowExecutionRequest.toJson() called scheduledFor.toIso8601String() directly. Dart only appends the Z designator when isUtc is true, so a local DateTime (what DateTime.now(), DateTime(...), and date pickers produce) serialized without a timezone designator. The backend reads that as UTC, so scheduled workflows fired off by the caller's local offset. Convert to UTC before serializing. toUtc() is a no-op on an already-UTC value, so this is correct for both local and UTC inputs. Also documents on createScheduledWorkflowForSubscribers/Segments (in WorkflowService and the PushFireSDK pass-throughs) that scheduledFor is an absolute instant and a local DateTime is converted to UTC. Fixes #7 --- lib/pushfire_sdk.dart | 4 +++ lib/src/models/workflow_execution.dart | 6 ++++- lib/src/services/workflow_service.dart | 4 +++ test/models/workflow_execution_test.dart | 32 ++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 1 deletion(-) diff --git a/lib/pushfire_sdk.dart b/lib/pushfire_sdk.dart index 29974ab..1545166 100644 --- a/lib/pushfire_sdk.dart +++ b/lib/pushfire_sdk.dart @@ -236,6 +236,8 @@ class PushFireSDK { /// Create a scheduled workflow execution for subscribers /// + /// [scheduledFor] is an absolute instant; a local DateTime is converted to UTC before being sent. + /// /// Returns empty map on web. Future> createScheduledWorkflowForSubscribers({ required String workflowId, @@ -252,6 +254,8 @@ class PushFireSDK { /// Create a scheduled workflow execution for segments /// + /// [scheduledFor] is an absolute instant; a local DateTime is converted to UTC before being sent. + /// /// Returns empty map on web. Future> createScheduledWorkflowForSegments({ required String workflowId, diff --git a/lib/src/models/workflow_execution.dart b/lib/src/models/workflow_execution.dart index 836b37d..3164eb8 100644 --- a/lib/src/models/workflow_execution.dart +++ b/lib/src/models/workflow_execution.dart @@ -139,7 +139,11 @@ class WorkflowExecutionRequest { }; if (scheduledFor != null) { - data['scheduledFor'] = scheduledFor!.toIso8601String(); + // Normalize to UTC so the wire value always carries a `Z` designator. + // toIso8601String() omits the designator for local DateTimes (isUtc + // == false), which the backend then reads as UTC. toUtc() is a no-op + // on an already-UTC value, so this is correct for both inputs. + data['scheduledFor'] = scheduledFor!.toUtc().toIso8601String(); } return { diff --git a/lib/src/services/workflow_service.dart b/lib/src/services/workflow_service.dart index 456f125..5b86169 100644 --- a/lib/src/services/workflow_service.dart +++ b/lib/src/services/workflow_service.dart @@ -78,6 +78,8 @@ class WorkflowService { } /// Create a scheduled workflow execution for subscribers + /// + /// [scheduledFor] is an absolute instant; a local DateTime is converted to UTC before being sent. Future> createScheduledWorkflowForSubscribers({ required String workflowId, required List subscriberIds, @@ -97,6 +99,8 @@ class WorkflowService { } /// Create a scheduled workflow execution for segments + /// + /// [scheduledFor] is an absolute instant; a local DateTime is converted to UTC before being sent. Future> createScheduledWorkflowForSegments({ required String workflowId, required List segmentIds, diff --git a/test/models/workflow_execution_test.dart b/test/models/workflow_execution_test.dart index a4a6b95..a9cd4af 100644 --- a/test/models/workflow_execution_test.dart +++ b/test/models/workflow_execution_test.dart @@ -572,6 +572,38 @@ void main() { expect(data['scheduledFor'], '2026-06-15T10:30:00.000Z'); }); + test( + 'serializes local DateTime scheduledFor as UTC with a Z designator', + () { + // Regression test for issue #7: a local (non-UTC) DateTime must be + // converted to UTC before serialization, otherwise the emitted + // string has no timezone designator and the backend misreads it as + // UTC, firing the workflow off by the caller's offset. + // + // Constructing via DateTime(...) (not DateTime.utc(...)) always + // produces a local DateTime (isUtc == false) regardless of the + // machine's timezone, so this assertion is timezone-independent: + // on the buggy implementation (toIso8601String() without toUtc()) + // the string never ends in 'Z' for a local DateTime, at any offset. + final localScheduledDate = DateTime(2026, 6, 15, 10, 30); + final request = WorkflowExecutionRequest( + workflowId: validUuid1, + type: WorkflowExecutionType.scheduled, + scheduledFor: localScheduledDate, + target: makeTarget(), + ); + + final json = request.toJson(); + final data = json['data'] as Map; + + expect(data['scheduledFor'], isA()); + expect(data['scheduledFor'], endsWith('Z')); + expect( + data['scheduledFor'], + localScheduledDate.toUtc().toIso8601String(), + ); + }); + test('serializes target with correct structure', () { final request = WorkflowExecutionRequest( workflowId: validUuid1, From 87e6663d0154f22ca8190a84fa5912df353be7ce Mon Sep 17 00:00:00 2001 From: Mohanned Binmiskeen Date: Mon, 3 Aug 2026 11:37:52 +0200 Subject: [PATCH 4/5] style: apply dart format to the new workflow execution test --- test/models/workflow_execution_test.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/models/workflow_execution_test.dart b/test/models/workflow_execution_test.dart index a9cd4af..72c2fa0 100644 --- a/test/models/workflow_execution_test.dart +++ b/test/models/workflow_execution_test.dart @@ -572,8 +572,7 @@ void main() { expect(data['scheduledFor'], '2026-06-15T10:30:00.000Z'); }); - test( - 'serializes local DateTime scheduledFor as UTC with a Z designator', + test('serializes local DateTime scheduledFor as UTC with a Z designator', () { // Regression test for issue #7: a local (non-UTC) DateTime must be // converted to UTC before serialization, otherwise the emitted From b0d5d86eb6728d50e5cd4e06a0d8b56e7b2900d7 Mon Sep 17 00:00:00 2001 From: Mohanned Binmiskeen Date: Mon, 3 Aug 2026 11:37:53 +0200 Subject: [PATCH 5/5] style: apply dart format to resume scenarios test Pre-existing formatting drift on main, unrelated to this PR's fixes. Included because CI's dart format check gates the merge. --- ...evice_service_permission_resume_scenarios_test.dart | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/services/device_service_permission_resume_scenarios_test.dart b/test/services/device_service_permission_resume_scenarios_test.dart index ebf15be..06e181f 100644 --- a/test/services/device_service_permission_resume_scenarios_test.dart +++ b/test/services/device_service_permission_resume_scenarios_test.dart @@ -60,7 +60,8 @@ DeviceService buildService(RecordingApiClient api, PlatformState state) { /// no PATCH was sent. bool? lastPatchedEnabled(RecordingApiClient api) { if (api.patchCalls.isEmpty) return null; - return api.patchCalls.last['data']['data']['pushNotificationEnabled'] as bool?; + return api.patchCalls.last['data']['data']['pushNotificationEnabled'] + as bool?; } void main() { @@ -68,7 +69,8 @@ void main() { setUp(() => SharedPreferences.setMockInitialValues({})); - test('Case 1 — granted, then REVOKED in settings, app resumed -> server denied', + test( + 'Case 1 — granted, then REVOKED in settings, app resumed -> server denied', () async { final api = RecordingApiClient(); final state = PlatformState(true); @@ -123,8 +125,8 @@ void main() { 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); + final anyEnabledPatch = api.patchCalls + .any((c) => c['data']['data']['pushNotificationEnabled'] == true); expect(anyEnabledPatch, isFalse, reason: 'developer opt-out must survive an OS re-grant'); });