Fix redundant device PATCH, preference/server divergence, and non-UTC scheduled workflows - #8
Merged
Merged
Conversation
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
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
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
Pre-existing formatting drift on main, unrelated to this PR's fixes. Included because CI's dart format check gates the merge.
Member
Author
|
Two formatting commits were added after the initial push, because CI's
CI is green on |
This was referenced Aug 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes three bugs found while porting this SDK to Swift. Each is a separate commit with a regression test.
Fixes #5, fixes #6, fixes #7.
1.
registerDevicePATCHed on every launch (#5)registerDevice()persisted the raw OS permission but compared it against the effective value (osPermission && (savedPreference ?? true)). Any device whose developer had calledsetNotificationEnabled(false)while the OS permission was granted saw those two values differ permanently, so it sent a redundantPATCH update-deviceon every cold start, with a misleadingDevice permission status changedlog line each time.Now compares raw against raw. The developer preference already has its own write path through
setNotificationEnabled, so it does not need to drive this check.The resume-sync behaviour from
[0.3.1](commitc1369b9) is load-bearing here, so its tests indevice_service_permission_resume_scenarios_test.dartanddevice_service_resume_permission_repro_test.dartwere left untouched and all still pass.2. Notification preference could diverge from the server (#6)
setNotificationEnabled()wrote the preference toSharedPreferencesbefore the PATCH. When the PATCH failed the call threw, but local state had already changed — and because the method short-circuits when the local preference already matches, a retry returnedsuccesswithout ever contacting the server. The divergence was sticky: the device kept receiving notifications the user asked to stop, and recovery required toggling to the opposite value and back.Now persists only after the server confirms.
This required amending one existing test.
saves preference locally even if server call failsasserted exactly the behaviour being fixed — it was pinning the bug — so its assertion is inverted rather than a contradictory test being added alongside it. It is nowdoes not save preference locally when the server call fails (regression for #6).3. Scheduled workflows fired at the wrong time outside UTC (#7)
WorkflowExecutionRequest.toJson()usedtoIso8601String()onscheduledFor. Dart appendsZonly whenisUtcis true, so a localDateTime— whatDateTime.now(),DateTime(...)and every date picker produce — serialised with no timezone designator, which the backend reads as UTC. A user at UTC+2 scheduling for 12:00 local had the workflow fire at 14:00 local; west of UTC it fired early. The error scaled with the offset and was silent.Now normalises with
toUtc(), which is a no-op on an already-UTC value so both inputs are correct.createScheduledWorkflowForSubscribersandcreateScheduledWorkflowForSegmentsgained a line noting thatscheduledForis an absolute instant and a localDateTimeis converted.Note this changes the value sent on the wire for callers passing local times. That value was wrong, so it is the fix rather than a behaviour change anyone could have been relying on deliberately — but it is the one change here with an externally visible effect, so worth a deliberate look.
The new test asserts the string ends with
Z, which fails on the old implementation at any offset including UTC+0, so it is not silently passing on a UTC machine.Verification
flutter test— 391 passing, same count before and after.dart analyze— 87 pre-existing info-level lints, zero new, none in the four files touched.Not included
No version bump and no CHANGELOG entry — the release is yours to cut.