fix(timeplanning): admin exact-minute actual start/stop under UseOneMinuteIntervals=true#1572
Merged
Merged
Conversation
…inuteIntervals=true (#TBD)
- Adds Start{1..5}ExactMinutes / Stop{1..5}ExactMinutes DTO fields mirroring PR #1568's pause shape
- Math.round in convertTimeToMinutes to kill the 37.6 float that produced the admin-edit error
- ApplyExactMinuteStart/Stop helpers write Start*StartedAt/Stop*StoppedAt under flag-on; cross-midnight aware
- DeriveLegacyShiftIdsFromTimestamps keeps legacy *Id columns in sync after timestamp writes (admin + worker paths)
- ComputePlanningNettoMinutes routes netto math through timestamps under flag-on (was 5-min-quantized via *Id)
- New l1m Playwright shard with 6 round-trip regression tests + 1 flag-off regression test in f shard
- CI matrix bumped to include all playwright shards on disk
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds exact-minute handling for actual start/stop times when UseOneMinuteIntervals=true, preventing invalid float legacy IDs in admin edits and routing netto-minutes math through timestamps; also expands Playwright coverage with a dedicated l1m shard and workflow matrix updates.
Changes:
- Extend DTO/client model to send
Start{N}ExactMinutes/Stop{N}ExactMinutesand apply them server-side to*StartedAt/*StoppedAt. - Keep legacy
Start*Id/Stop*Idsynchronized from timestamps and compute netto minutes via timestamps under flag-on. - Add an
l1mPlaywright shard (new SQL patch + tests) and include it in PR/master CI matrices.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| eform-client/src/app/plugins/modules/time-planning-pn/models/plannings/planning-pr-day.model.ts | Adds client-side DTO fields for exact-minute start/stop. |
| eform-client/src/app/plugins/modules/time-planning-pn/components/plannings/time-planning-actions/workday-entity/workday-entity-dialog.component.ts | Ensures integer legacy-minute conversions and sends raw exact-minute fields under flag-on. |
| eform-client/playwright/helpers/one-minute-times.ts | Adds shared off-grid/cross-midnight fixtures for new l1m shard tests. |
| eform-client/playwright/e2e/plugins/time-planning-pn/l1m/post-migration.sql | Enables 1-minute-interval flag and extra shifts for the new shard seed. |
| eform-client/playwright/e2e/plugins/time-planning-pn/l1m/dashboard-edit-actual-exact.spec.ts | Adds l1m shard E2E tests for exact-minute round-trips and netto display. |
| eform-client/playwright/e2e/plugins/time-planning-pn/f/dashboard-edit-a.spec.ts | Adds a flag-off regression test to confirm unchanged behavior. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningPlanningService/TimePlanningPlanningService.cs | Applies exact-minute start/stop, derives legacy IDs from timestamps, and centralizes netto-minute calculation. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Models/Planning/TimePlanningPlanningPrDayModel.cs | Adds request fields for exact-minute start/stop. |
| .github/workflows/dotnet-core-pr.yml | Adds l1m to CI shard matrix. |
| .github/workflows/dotnet-core-master.yml | Adds l1m to CI shard matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,18 @@ | |||
| -- Post-migration patch for the f1m variant shard. | |||
Comment on lines
+9
to
+10
| -- This patch flips the flag on for every active assigned site so the f1m | ||
| -- shard exercises the flag-on rendering / form / picker code paths. The |
Comment on lines
+1945
to
+1954
| var baseDate = planning.Date.Date; | ||
| if (startStamp.HasValue) | ||
| { | ||
| var startMinutes = (int)(startStamp.Value - startStamp.Value.Date).TotalMinutes; | ||
| if (minutes <= startMinutes) | ||
| { | ||
| baseDate = baseDate.AddDays(1); | ||
| } | ||
| } | ||
| var anchor = baseDate + TimeSpan.FromMinutes(minutes); |
Comment on lines
+372
to
+379
| // exact off-grid minute component preserved. Find the row whose | ||
| // start1StartedAt is set and whose siteName === 'ac ad'. | ||
| const indexBody = await indexResponse.json(); | ||
| const rows = JSON.stringify(indexBody); | ||
| expect( | ||
| /T03:03(?::00)?/.test(rows), | ||
| 'backend must persist Start1StartedAt as 03:03 UTC, not 03:00 / 03:05', | ||
| ).toBeTruthy(); |
| }, | ||
| }); | ||
|
|
||
| await page.waitForTimeout(500); |
Comment on lines
+55
to
+58
| await page.waitForTimeout(500); | ||
| await page.locator('.timepicker-button span').filter({ hasText: 'Ok' }).click(); | ||
| await page.locator('.cdk-overlay-backdrop').waitFor({ state: 'hidden', timeout: 10000 }).catch(() => {}); | ||
| await page.waitForTimeout(500); |
- Drop cross-midnight 23:55->00:30 test: dialog requires stop>start within a shift, so the case is backend-only (ApplyExactMinuteStop handles it for worker-persisted timestamps but admin can't enter it through the UI) - Bump 5-shifts test timeout to 300s: 12 timepicker fills × ~7s each exceeds default 120s Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… l1m netto test - f1m midnight tests rely on float-value cancellation: (29.8-1)*5 = 144. Math.round broke this. The float-on-wire concern is moot since ASP.NET silently truncates and the new Start*ExactMinutes path writes the precise timestamp regardless. - l1m netto test was reading leftover pause from prior 5-shift test in same cell; isolated by using a different cell. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…urs via *ExactMinutes under flag-on - Math.round in convertTimeToMinutes is required: float 37.6 → ASP.NET int? deserializer rejects with 400 → silent save failure (the original user-reported "error") - f1m display tests were collateral damage from the prior Math.round revert: their flex math depends on float cancellation of (stop_float-1)*5=exact_minutes - New approach under flag-on: calculatePlanHours derives actualMinutes from picker strings directly (toRawMinutes) instead of *Id, so display is exact-minute regardless of Math.round at the wire - Flag-off path unchanged: still uses legacy *Id math (picker minutesGap=5 ensures integer values) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… residue
The 5-shift test persists Pause1StartedAt/StoppedAt timestamps on cell0_0
(pause='00:02'); the netto-check test then reused the same cell and the
backend's ComputePlanningNettoMinutes still saw those 2 min of leftover
pause -> netto = 488 min ('8.13') vs the expected 490 min ('8.17').
A prior fix clicked the dialog's pause-delete icon, which only patches the
form pause to null. Under UseOneMinuteIntervals=true that sends
pause1ExactMinutes = null, and the backend's `if (minutes.HasValue)
ApplyExactMinutePause` guard skips ClearPauseTimestamps entirely - so the
timestamps survived.
Fix: explicitly pick pause='00:00' via the timepicker (after start/stop so
the picker's [max]=getMaxDifference resolves to a positive duration). That
sends pause1ExactMinutes = 0, which ApplyExactMinutePause routes through
ClearPauseTimestamps to null Pause1StartedAt/StoppedAt on save.
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
UseOneMinuteIntervals=trueworkersactual.shiftN.startoractual.shiftN.stopto a non-multiple-of-5 minute produced a float DTO value (e.g. 03:03 -> 37.6)Start*Id/Stop*Idcolumns synchronized with the preciseStart*StartedAt/Stop*StoppedAttimestamps under flag-on, in both admin (Update) and worker punchclock (UpdateByCurrentUserNam) pathsnettoMinutesmath through timestamps under flag-on (was 5-min-quantized)l1mshard (6 tests) plus 1 flag-off regression test in the existingfshardTest plan
🤖 Generated with Claude Code