feat: add a subscription clock offset to dev settings - #1341
Conversation
Drop the committed journey and add the changelog fragment for the Dev Settings option.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d06618e96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
pwltr
left a comment
There was a problem hiding this comment.
I found two issues with the debug subscription clock that should be addressed before merge:
-
MEDIUM — Reschedule due notifications when the subscription clock changes (
app/src/main/java/to/bitkit/repositories/PaykitSubscriptionNotificationScheduler.kt:39). The scheduler reads the shifted clock, butsynchronize()retains existing WorkManager jobs by name, so changing the offset does not update their original delay. A 31-day jump also moves the newly due period out ofupcomingPeriodsAfter(now), whilePaykitSubscriptionNotificationWorkerchecks the real clock and retries until the real start date. The renewal request can become due without its due notification firing. Please reschedule affected work when the offset changes and use the subscription clock in the worker. This is also raised inline. -
MEDIUM — Avoid publishing a shifted recurrence start (
app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt:631).proposalDatecomes from the offset clock, andbuildSubscriptionProposal()publishes it as bothstartsAtandanchor. If the creator later turns the offset Off, or the payer has no matching offset, acceptance happens before that stored start.PaykitSubscriptionRecurrence.periodsThrough()then returns no first payment period until the future start date. Please keep the simulated time local to subscription scheduling rather than storing a shifted start in the shared proposal. This is also raised inline.
jvsena42
left a comment
There was a problem hiding this comment.
One LOW inline, for debug builds only.
Checked and clean:
- Release gating:
isAvailable = BuildConfig.DEBUG,subscriptionDateis a no-op when unavailable,SubscriptionClockOffsetSync.start()returns early, and the row is hidden.assembleDevReleasealso hasDEBUG=false. @SubscriptionClockis injected only intoPaykitPaymentRequestRepoandPaykitSubscriptionNotificationScheduler. No fee, LN invoice expiry, backup timestamp or one-time request deadline uses it:createPaymentRequest,isPending/isExpired, theupdateRequestexpiry check and incoming parsing all stay on the real clock.- No double pay: recurring request ids derive from the anchor (
billingPeriod.startsAt) on both sides, and paid filtering goes throughpaidPeriodsand the local completed and in-flight sets. The offset only widens which periods are generated. - No paying an expired request: recurring requests have no
expiresAt, and under a shifted clock proposals can only look expired earlier. - No
System.currentTimeMillis(), and the dev-settings strings are hardcoded as allowed. - Merges cleanly with current master. It will need a follow-up against #1401, whose new tests construct
PaykitPaymentRequestRepowithout the newsubscriptionClockparameter.
Not raised: the offset persists after dev mode is switched off, the same as the other dev-only toggles on that screen.
Device gate: n/a — dev-settings-only change with no journey files.
|
Pushed 3441108 (merges clean with master; no conflicts).
@pwltr both MEDIUM items are addressed (scheduler and worker; shifted @jvsena42 on #1401: it constructs Local verification: full |
pwltr
left a comment
There was a problem hiding this comment.
Two notification-scheduling cases introduced by the clock-offset change still need fixes. The earlier recurrence-anchor, acceptance-time, worker-clock, and refresh concerns appear addressed on this head.
…he final one past the end
|
Pushed 719e139 (plus efb11ee, a comment shortened for the line-length check).
@pwltr this answers both new MEDIUM threads. Both new tests in |
|
Device re-run on an emulator with the dev debug build of
|
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 9d06618e9: one LOW inline (debug only), a side effect of the acceptance-time fix.
- The first-period LOW is fixed in b4847f8. Acceptance is stored on real time (:857),
accept()returns the earliest pending period, and the restored-acceptance fallback usesclock.now(). The new test covers reset to Off. - pwltr's four MEDIUMs: published
startsAt/anchorstay on real time, offset changes cancel and requeue work, there is one catch-up per subscription ahead of the global cap, and the final period pastendsAtis kept. - No double pay after reset to Off. A period paid under the offset stays
PROOF_SUBMITTEDwhen real time reaches it, and request ids derive from the real-time anchor. - Release gating holds.
isAvailable = BuildConfig.DEBUG, not dev mode, so a persisted offset is ignored in release builds even with dev settings unlocked.
Device gate: n/a — dev-settings-only change with no journey files.
|
Pushed 0204818: the subscription review sheet and its transition timer now take the acceptance time on real time ( Local verification: full |
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 719e139d4 (0204818): no findings. The review-sheet LOW is fixed.
paymentDueOnAcceptance(now, acceptedAt)is nowrequestsThrough(shifted now, real acceptedAt). That is the same pairaccept()uses: it storesclock.now()and refreshes on the subscription clock. The sheet therefore names the period the swipe pays, and the new test covers it.nextSubscriptionTransitiongets the same realacceptedAt, so the label still refreshes at the right period end.- With the offset Off, the default
acceptedAt = nowleaves behaviour unchanged.
Device gate: n/a — dev-settings-only change with no journey files.
jvsena42
left a comment
There was a problem hiding this comment.
Approved!
OBS: Could havesome AI instructions to improve discoverability by AI
Twin: synonymdev/bitkit-ios#798
Adds a subscription clock offset to Dev Settings so a subscription renewal can be tested on a debug build without waiting a billing period.
Description
Out of Scope
Design
N/A — no design available.
Preview
subscription-clock-offset-second-billing-period.mp4
QA Notes
Journeys
second-billing-period.xml— with a 31-day offset the next period comes due as a Payment Request and the subscription detail lists two paymentssecond-billing-period.xml
Manual Tests
N/A
Automated Checks
SubscriptionClockOffsetTest.kt— clamping, presets and the offset applied to a subscription clockPaykitPaymentRequestRepoSubscriptionTest.kt— a subscription clock offset makes the next billing period due, lists the paid next period in the payment history, keeps the published start and the acceptance time on real time and keeps the first period due when accepting with the offset onPaykitSubscriptionNotificationSchedulerTest.kt— an offset change reschedules queued notifications, notifies only the latest period it made due per subscription and still notifies the final period after a jump past the subscription endSubscriptionsScreenTest.kt— the review sheet names the period accepting pays under a clock offsetPaykitPaymentRequestRepoTest.kt— the repo takes the subscription clock