fix(timeplanning): overview pause aggregation honors sub-slot stamp pairs#1575
Merged
Conversation
…airs
The admin "Oversigt timer" overview's per-day cell showed `Samlet pause:
00:00` for a worker whose real pause was 3 minutes (edit dialog showed
the correct `Pause: 00:03`). Data was on the row — only the OVERVIEW
aggregation was wrong.
`PlanRegistrationHelper.AggregatePauseMinutes` flag-on branch summed
only 5 main pause stamp pairs (`Pause1..Pause5`StartedAt/StoppedAt).
The edit dialog's `computeExactPauseMinutes` (workday-entity-dialog.
component.ts:1080) sums 31 sub-slot pairs total: Pause1, Pause10..Pause19,
Pause100..Pause102 for shift 1 (14 pairs); symmetric for shift 2;
single slot for shifts 3/4/5. Pauses stored in sub-slots (e.g. Pause10)
were silently missed by the backend aggregation.
Extend the flag-on branch to iterate the same 31 pairs the frontend
uses, in the same order. Add legacy `(Pause{N}Id*5)-5` fallback (over
the 5 main slots only — sub-slots have no Id field) for older flag-on
rows that carry ticks but no stamps. Flag-off branch is unchanged
(extracted into a private helper for reuse from both branches).
The previous spec assumed `Pause{N}ExactMinutes` was an entity column
to read; it isn't — it's a request-only DTO field for the admin edit
dialog. Confirmed by reading the actual entity in
Microting.TimePlanningBase. Pivoted to mirroring the frontend's
31-pair aggregation, which is what the edit dialog already does.
Tests:
- 7 new flag-on cases covering sub-slot pauses (Pause10 alone,
Pause1+Pause11, Pause2+Pause20, Pause3), legacy fallback when no
stamps populated, empty-everything zero, and flag-off parity.
- 1 pre-existing test renamed and re-asserted: was
`_NullStampsContributeZero` (Pause1Id=4 → expected 0), now
`_NoStampsFallsBackToLegacyTicks` (Pause1Id=4 → expects 15 via the
legacy fallback). Legitimate business-logic change per the
`feedback_dont_edit_existing_tests.md` exception.
100/100 PlanRegistrationHelper tests pass; build clean (0 errors).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a bug in the admin "Oversigt timer" overview where pauses recorded in pause sub-slots (e.g. Pause10, Pause11, …) were not counted in AggregatePauseMinutes's flag-on branch, causing Samlet pause to display 00:00 while the per-row edit dialog correctly showed the real value. The aggregator is extended to walk the same 31 stamp pairs the frontend computeExactPauseMinutes iterates, with a legacy-tick fallback when no stamps are present.
Changes:
- Replace the 5-slot stamp sum in the flag-on branch with a 31-pair scan over all main and sub-slot pause stamps (shifts 1–5).
- Add a legacy
(Pause{N}Id*5)−5fallback (5 main slots) for flag-on rows with no stamps, and extract the legacy tick computation into a shared private helper used by both branches. - Add 7 new unit tests covering sub-slot aggregation, multi-slot summing per shift, the legacy fallback, all-empty input, and the flag-off path remaining stamp-agnostic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanRegistrationHelper.cs |
Extends AggregatePauseMinutes to iterate all 31 pause stamp pairs via a new GetAllPauseStampPairs enumerator and refactors the legacy tick path into LegacyTickMinutesAcrossMainSlots. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs |
Renames the previous "null stamps → 0" test to reflect the new fallback semantics and adds tests for sub-slot pauses, multi-slot summing, single-slot shifts 3–5, the legacy fallback, an all-empty case, and flag-off path stability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+508
to
+543
| // Shift 1: Pause1, Pause10..Pause19, Pause100..Pause102 (14 pairs) | ||
| yield return (pr.Pause1StartedAt, pr.Pause1StoppedAt); | ||
| yield return (pr.Pause10StartedAt, pr.Pause10StoppedAt); | ||
| yield return (pr.Pause11StartedAt, pr.Pause11StoppedAt); | ||
| yield return (pr.Pause12StartedAt, pr.Pause12StoppedAt); | ||
| yield return (pr.Pause13StartedAt, pr.Pause13StoppedAt); | ||
| yield return (pr.Pause14StartedAt, pr.Pause14StoppedAt); | ||
| yield return (pr.Pause15StartedAt, pr.Pause15StoppedAt); | ||
| yield return (pr.Pause16StartedAt, pr.Pause16StoppedAt); | ||
| yield return (pr.Pause17StartedAt, pr.Pause17StoppedAt); | ||
| yield return (pr.Pause18StartedAt, pr.Pause18StoppedAt); | ||
| yield return (pr.Pause19StartedAt, pr.Pause19StoppedAt); | ||
| yield return (pr.Pause100StartedAt, pr.Pause100StoppedAt); | ||
| yield return (pr.Pause101StartedAt, pr.Pause101StoppedAt); | ||
| yield return (pr.Pause102StartedAt, pr.Pause102StoppedAt); | ||
|
|
||
| // Shift 2: Pause2, Pause20..Pause29, Pause200..Pause202 (14 pairs) | ||
| yield return (pr.Pause2StartedAt, pr.Pause2StoppedAt); | ||
| yield return (pr.Pause20StartedAt, pr.Pause20StoppedAt); | ||
| yield return (pr.Pause21StartedAt, pr.Pause21StoppedAt); | ||
| yield return (pr.Pause22StartedAt, pr.Pause22StoppedAt); | ||
| yield return (pr.Pause23StartedAt, pr.Pause23StoppedAt); | ||
| yield return (pr.Pause24StartedAt, pr.Pause24StoppedAt); | ||
| yield return (pr.Pause25StartedAt, pr.Pause25StoppedAt); | ||
| yield return (pr.Pause26StartedAt, pr.Pause26StoppedAt); | ||
| yield return (pr.Pause27StartedAt, pr.Pause27StoppedAt); | ||
| yield return (pr.Pause28StartedAt, pr.Pause28StoppedAt); | ||
| yield return (pr.Pause29StartedAt, pr.Pause29StoppedAt); | ||
| yield return (pr.Pause200StartedAt, pr.Pause200StoppedAt); | ||
| yield return (pr.Pause201StartedAt, pr.Pause201StoppedAt); | ||
| yield return (pr.Pause202StartedAt, pr.Pause202StoppedAt); | ||
|
|
||
| // Shifts 3/4/5: single slot each | ||
| yield return (pr.Pause3StartedAt, pr.Pause3StoppedAt); | ||
| yield return (pr.Pause4StartedAt, pr.Pause4StoppedAt); | ||
| yield return (pr.Pause5StartedAt, pr.Pause5StoppedAt); |
Member
Author
There was a problem hiding this comment.
Verified. GetPauseIntervals (line 2052) does enumerate the same 31 pairs. Replaced both with a shared EnumeratePauseStampPairs(pr) private enumerator that returns the raw nullable pairs. GetPauseIntervals now consumes it and applies its existing filter (non-null + positive duration). AggregatePauseMinutes consumes the same source directly so it can also distinguish populated-zero-duration pauses from no-stamp-at-all. Fixed in 43da9f2.
Comment on lines
+485
to
+490
| if (totalSeconds == 0) | ||
| { | ||
| // Fallback: older flag-on rows without stamp pairs may still carry | ||
| // legacy 5-minute-tick IDs in the 5 main pause slots. | ||
| return LegacyTickMinutesAcrossMainSlots(pr); | ||
| } |
Member
Author
There was a problem hiding this comment.
Correct edge case. If a worker stamped a pause that resolves to zero duration (start == stop, or stop < start), the stamps were observed but totalSeconds == 0 triggers the legacy-tick fallback, returning a nonzero Pause{N}Id-derived value instead of the intended 0. Fixed in 43da9f2: the helper now tracks a separate hasAnyStamp flag (any non-null endpoint) and falls back only when no stamp was observed at all. Two regression tests added: ZeroDurationStampDoesNotFallBackToLegacy and NegativeDurationStampDoesNotFallBackToLegacy.
…erved stamps Addresses Copilot review on PR #1575: 1. The new GetAllPauseStampPairs duplicated GetPauseIntervals' enumeration of the same 31 pause stamp pairs — exactly the kind of drift the PR was fixing. Replaced both with a shared private enumerator `EnumeratePauseStampPairs(pr)`. GetPauseIntervals now consumes it and applies the existing filter (non-null endpoints, positive duration); AggregatePauseMinutes consumes it and tracks both the duration sum and a hasAnyStamp flag. 2. The fallback to legacy ticks was gated on totalSeconds == 0, which would incorrectly trigger for populated-but-zero-duration stamp pairs (StartedAt == StoppedAt, or StoppedAt < StartedAt). Now gated on hasAnyStamp — if a worker stamped a pause, the intended value is the stamped duration (even if 0), not the legacy Pause{N}Id fallback. Two new tests: - ZeroDurationStampDoesNotFallBackToLegacy (start == stop + Pause1Id=4 → expects 0, not 15) - NegativeDurationStampDoesNotFallBackToLegacy (stop < start + Pause1Id=4 → expects 0, not 15) 102/102 PlanRegistrationHelper tests pass; build clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two layers of regression tests on top of the helper-level unit
tests already in PlanRegistrationHelperTests.cs.
1. Service-level (TimePlanningPlanningService.Index)
- Index_OneMinuteInterval_WithSubSlotPauseStamps_AggregatesCorrectly
seeds an AssignedSite with UseOneMinuteIntervals=true and a
PlanRegistration whose 3-minute pause lives in Pause10*StartedAt/
Pause10*StoppedAt. Calls Index, asserts the response's per-day
PauseMinutes == 3. Locks the call-site contract: if someone
refactors Index and stops invoking AggregatePauseMinutes, this
fails even though the helper-level tests pass.
- Index_LegacyFiveMinuteFlag_WithSubSlotPauseStamps_PauseMinutesIsZero
same seeding with the flag OFF — asserts PauseMinutes == 0 (legacy
5-min-tick path ignores stamps). Proves the flag actually flips
the aggregation path through the service.
2. Angular Playwright e2e (b1m shard, skipped pending fixture work)
- dashboard-pause-aggregate.spec.ts mirrors the existing skipped
pattern in dashboard-edit-multishift.spec.ts. Documents the
assertion shape for when the DB fixture for sub-slot pause
stamps lands in the b1m shard's seed. Until then it's a TODO
with the expected locator + assertion inline.
Build clean; 6/6 PlanningServiceMultiShiftTests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
The admin "Oversigt timer" overview's per-day cell showed
Samlet pause: 00:00for a worker whose real pause was 3 minutes (the edit dialog showed the correctPause: 00:03). Data was on the row — only the OVERVIEW aggregation was wrong.PlanRegistrationHelper.AggregatePauseMinutes's flag-on branch summed only 5 main pause stamp pairs (Pause1..Pause5StartedAt/StoppedAt). The edit dialog'scomputeExactPauseMinutes(workday-entity-dialog.component.ts:1080) sums 31 sub-slot pairs total — Shift 1:Pause1,Pause10..Pause19,Pause100..Pause102(14 pairs); Shift 2: symmetric (14 pairs); Shifts 3/4/5: 1 pair each. Pauses stored in sub-slots (e.g.Pause10) were silently missed by the backend overview aggregation.This extends the flag-on branch to iterate the same 31 pairs in the same order. Legacy
(Pause{N}Id*5)-5fallback added for older flag-on rows that carry ticks but no stamps (over the 5 main slots only — sub-slots have noIdfield). Flag-off branch is unchanged (extracted into a private helper for reuse from both branches).Why the original spec was wrong
Initial diagnosis assumed
Pause{N}ExactMinuteswas an entity column the backend could read; it isn't — it's a request-only DTO field for the admin edit dialog. The implementation agent caught this on first attempt and stopped. Re-diagnosis: edit dialog correctness comes from iterating 31 stamp pairs in JS; backend was iterating 5. Fix is to match.Test plan
dotnet test --filter PlanRegistrationHelper— 100/100 pass (7 new tests + 1 legitimate test rename).dotnet build— 0 errors.devinstall.sh— synced to host workspace.Samlet pause: 00:03instead of00:00.Out of scope (follow-up if needed)
Pause10) rather thanPause1. The fix is resilient to either.(int)(totalSeconds / 60)floor) and frontend dialog (Math.round(totalSeconds / 60)). Pre-existing in the file; not introduced by this fix. Worth revisiting if customers report new view divergence.🤖 Generated with Claude Code