fix(export): don't crash on shift slot ids > 289 (out-of-range Options index)#1603
Merged
Merged
Conversation
…s index) GetShiftTime looked up a shift's time by indexing plr.Options, a fixed 288-element list (slots 1-288 -> "00:00".."23:55", with only 289 special-cased to "24:00"). A shift slot id >= 290 therefore indexed past the end of the list and threw IndexOutOfRange, aborting the whole Excel export (FillDataRow). The Pause columns are the worst case: they always pass actualStamp=null and so hit this lookup even when UseOneMinuteIntervals is on. Real data on a customer site had Pause1Id = 388/517, crashing the all-workers report. Compute the time arithmetically instead: minutes = (shift-1)*5, formatted HH:mm. This reproduces the old Options output exactly for the valid 1..288 range, keeps 289 -> "24:00", and extends gracefully past midnight (290 -> "24:05", 313 -> "26:00") with no array index and no crash. Adds the missing test coverage: - unit tests for GetShiftTime across the valid range, 289, out-of-range (290/313), and 0/null (PlanRegistrationHelperTests, shard f); - an export regression test seeding out-of-range Stop1Id/Pause1Id and asserting both single-worker and all-workers GenerateExcelDashboard succeed (DagsoversigtWorksheetExportTests, shard g). Note (separate, pre-existing): the bad Pause1Id values are themselves a data bug, and export temp filenames use second-resolution UtcNow with no uniqueness (concurrent exports can collide) -- both worth separate follow-ups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a production crash in the TimePlanning Excel export caused by out-of-range shift slot IDs (>= 290) by switching GetShiftTime from plr.Options indexing to arithmetic time computation, and adds unit + export regression tests to lock the behavior.
Changes:
- Compute shift slot time as
(shift-1)*5minutes to avoidIndexOutOfRangeExceptionin exports. - Add unit coverage for
GetShiftTime(plr, shift)including cross-midnight/out-of-range slot IDs. - Add an end-to-end export regression test that seeds out-of-range
Stop1Id/Pause1Idand validates both export overloads succeed and render"26:00".
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs |
Replaces plr.Options indexing with arithmetic formatting for shift slot IDs to prevent export crashes. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs |
Adds unit test cases covering valid, boundary, and out-of-range slot IDs for the 2-arg GetShiftTime. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs |
Adds regression test ensuring both export paths handle cross-midnight/out-of-range shift IDs without throwing and with correct output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2795
to
+2796
| var minutes = (shift.Value - 1) * 5; | ||
| return $"{minutes / 60:00}:{minutes % 60:00}"; |
Comment on lines
+274
to
+276
| [Test] | ||
| public async Task Export_WithCrossMidnightShiftSlotId_DoesNotThrow() | ||
| { |
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
The all-workers (and single-worker) timeplanning Excel export threw and aborted:
Root cause
GetShiftTimeresolved a shift's time by indexingplr.Options— a fixed 288-element list (slots 1–288 → "00:00".."23:55", only289special-cased to "24:00"). A shift slot id ≥ 290 indexes past the end →IndexOutOfRange. The Pause columns are the worst case: they always passactualStamp=null, so they hit this lookup even whenUseOneMinuteIntervalsis on.Confirmed against real data: a customer site had
Pause1Id = 388and517(both > 288) — exactly the crash.The fix
Compute the time arithmetically —
minutes = (shift-1)*5, formattedHH:mm— instead of indexingOptions. This reproduces the old output exactly for the valid 1–288 range, keeps289 → "24:00", and extends gracefully past midnight (290 → "24:05",313 → "26:00"). No array index, no crash. (Drop-in; the now-unusedplrparam is kept to avoid touching ~15 call sites — a separate cleanup.)Tests (the coverage gap)
GetShiftTimeacross the valid range,289, out-of-range (290/313),0/null—PlanRegistrationHelperTests(shard f).Stop1Id=313+Pause1Id=295and asserts both export overloads succeed and render "26:00" —DagsoversigtWorksheetExportTests(shard g). Verified it fails without the fix.Separate follow-ups (not in this PR)
Pause1Idvalues (≈32–43 h) are a data/encoding bug upstream — the existingif (Start1Id > 289)workaround doesn't cover Pause or shifts 2–5, nor run at export time.UtcNowwith no uniqueness → concurrent exports can collide on the same path.🤖 Generated with Claude Code