fix(flex): respect the UseOneMinuteIntervals effective date - #1696
Merged
Conversation
UseOneMinuteIntervals is a per-site boolean with no effective-from date, so every flex recomputation re-derived a worker's entire history under the site's CURRENT mode. Switching it on recalculated already-closed periods at one-minute precision and changed their balances. On one tenant a mass flag flip rewrote 3,017 rows: 2,267 balances dropped, averaging -9.6 h, worst -93.6 h. Resolve the mode PER ROW instead of per site, preferring the write-time marker, then the new AssignedSite.UseOneMinuteIntervalsFrom (base 10.0.61), then the existing AssignedSiteVersions timeline. The calc forks now use that resolved value; the timeline already computed it but was deliberately display-only. Write-time stamps and DTO passthroughs keep the raw flag: a new registration must record the CURRENT mode. Also seed the chain with a decimal fallback. SumFlexEndInSeconds is 0 on ~97% of rows -- migration 20260108054344 added it with defaultValue 0 and no backfill -- so seeding from it discarded the accumulated balance. This is required for the effective-date fix to work at all: the first post-switch row seeds from the last pre-switch row, which by definition only has decimal columns. Commit 57b5657 added the same fallback for PlanHoursInSeconds and PaiedOutFlexInSeconds and missed the seed; a new 2-arg overload of ApplyNettoFlexChainSecondPrecision now makes it impossible for a call site to forget it. Stamp UseOneMinuteIntervalsFrom on a false->true transition, before the one-way OR from 994c9cd and guarded on null so an ops backfill is not clobbered. The field is ops-only and exposed on no DTO. Gate the previously ungated SumFlexEndInSeconds write in TimePlanningFlexService, which corrupted five-minute sites, and read its seed before the decimal is mutated so the payout delta is not applied twice. ApplyRunningFlexChain now resolves the mode per row and keeps its seconds accumulator in lockstep with the decimal, so a chain spanning a mode boundary carries the balance correctly; previously the seconds accumulator went stale and a one-minute row after a five-minute row chained off garbage. Also fixes a pre-existing CI gap: ScheduleMessageReadTests ran on PRs but not on master, and it is one of only two classes exercising this code path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k
There was a problem hiding this comment.
🟡 Changes recommended
There is a concrete XML documentation defect in PlanRegistrationHelper (invalid/mismatched XML doc tags on the new overload) that can cause build-breaking doc warnings depending on compiler settings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes flex recomputation so historical balances are recalculated under the mode that was in effect when each row was registered, rather than the site’s current UseOneMinuteIntervals flag—preventing retroactive balance changes after enabling one-minute intervals.
Changes:
- Introduces an effective-from date (
UseOneMinuteIntervalsFrom) and per-row mode resolution viaOneMinuteModeTimeline(marker → effective date → audit trail). - Updates flex-chain and pause computations to fork per row and adds seconds↔decimal seed fallbacks to prevent balance collapse at mode boundaries.
- Adds focused in-memory unit tests and wires them into CI shards (plus closes a workflow test-gap on master).
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/TimePlanning.Pn.csproj | Bumps Microting.TimePlanningBase to 10.0.61 to consume the new effective-date column/support. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs | Resolves one-minute mode per row using OneMinuteModeTimeline, fixes seeding via fallback, and applies the running flex chain per-row. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs | Stamps UseOneMinuteIntervalsFrom on false→true transition before the one-way OR. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningPlanningService/TimePlanningPlanningService.cs | Uses per-row mode-at-registration for cascaded recompute chains and the safer seconds-chain overload. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningFlexService/TimePlanningFlexService.cs | Prevents unconditional SumFlexEndInSeconds mutation and updates seconds balance only for one-minute rows. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanRegistrationHelper.cs | Adds seconds↔decimal fallback helpers and a safer overload to prevent incorrect zero-seeding. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/OneMinuteModeTimeline.cs | Adds effective-date override, stamping helper, null-site handling, and a per-row mode resolver. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/RunningFlexChainModeBoundaryTests.cs | New unit tests pinning running flex-chain correctness across a mode boundary. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/OneMinuteIntervalsEffectiveDateTests.cs | New unit tests for effective-date precedence, seed fallback, and stamp guards. |
| .github/workflows/dotnet-core-pr.yml | Adds the new tests to shard f in PR workflow. |
| .github/workflows/dotnet-core-master.yml | Adds the new tests to shard f and ensures ScheduleMessageReadTests run on master. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+526
to
+530
| /// <summary> | ||
| /// Preferred overload: seeds the chain from <paramref name="preTimePlanning"/> | ||
| /// (null when this is the first row) through | ||
| /// <see cref="SumFlexEndSecondsWithFallback"/>, so no call site can | ||
| /// accidentally seed from the raw, usually-zero <c>SumFlexEndInSeconds</c> |
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.
The bug
AssignedSite.UseOneMinuteIntervalsis a per-site boolean with no effective-from date, so every flex recomputation re-derived a worker's entire history under the site's current mode. Switching it on recalculated already-closed periods at one-minute precision and changed their balances.Diagnosed from a live incident: a mass raw
UPDATEflipped the flag on 2026-08-26 between 13:49:24 and 13:53:47, and the recompute that fired at 13:53:47 rewrote 3,017 rows — 2,267 balances dropped, averaging −9.6 h, worst −93.6 h. Across the estate ~30,000 orphaned flex hours are attributable to flag-on sites, 13 tenants, 91 sites.The fix
Resolve the mode per row, not per site. Precedence: the write-time
RegisteredUnderOneMinuteIntervalsmarker → the newAssignedSite.UseOneMinuteIntervalsFrom(base 10.0.61) → the existingAssignedSiteVersionstimeline. The calc forks now use that value.OneMinuteModeTimelinealready computed it — it was deliberately display-only, and that deliberate choice was the bug.Write-time stamps and DTO passthroughs deliberately keep the raw flag: a new registration must record the current mode.
Why not just the derived timeline? It falls back to the last audited save date, and sites flipped by raw
UPDATEleave no audit row. Measured against the known flip: correct for 6 sites, over 3 months early for 50, worst case 499 days. Early is the destructive direction.Seed fallback.
SumFlexEndInSecondsis 0 on ~97% of rows (migration20260108054344added it withdefaultValue: 0and no backfill), so seeding from it discarded the balance. Required for the effective-date fix to work at all — the first post-switch row seeds from the last pre-switch row, which by definition only has decimal columns. Commit57b56579added the identical fallback forPlanHoursInSeconds/PaiedOutFlexInSecondsand missed the seed; a 2-argApplyNettoFlexChainSecondPrecisionoverload now makes it impossible for a call site to forget.Forward stamping.
UpdateAssignedSiterecords the date on a false→true transition, before the one-way OR from994c9cd4and guarded onnullso an ops backfill is not clobbered. Ops-only; exposed on no DTO.Two bugs found along the way: the ungated
SumFlexEndInSecondswrite inTimePlanningFlexService(corrupted five-minute sites, and applied the payout delta twice by reading its seed after the decimal was mutated), andApplyRunningFlexChainleaving its seconds accumulator stale so a one-minute row after a five-minute row chained off garbage.Tests
Two new fixtures, all pure in-memory.
OneMinuteIntervalsEffectiveDateTestscovers the resolution precedence, the seed fallback, and the stamp guards.RunningFlexChainModeBoundaryTestscovers the chain across a mode boundary in both directions, a uniformly-five-minute regression guard (sentinel proves no*InSecondswrites on the common path), and an anchor with a zero seconds column — the collapse mechanism itself.Both added to shard
fin both workflows. Also fixes a pre-existing gap:ScheduleMessageReadTestsran on PRs but not master, and it is one of only two classes exercising this path.Not in this PR
🤖 Generated with Claude Code
https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k