From b509cb68819ae1ef8ae178893c9047961f7a5aa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Wed, 10 Jun 2026 04:21:06 +0200 Subject: [PATCH] fix(export): don't crash on shift slot ids > 289 (out-of-range Options 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 --- .../DagsoversigtWorksheetExportTests.cs | 105 +++++++++++++++++- .../PlanRegistrationHelperTests.cs | 34 ++++++ .../TimePlanningWorkingHoursService.cs | 11 +- 3 files changed, 145 insertions(+), 5 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs index d70d16af..5af5c652 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs @@ -263,10 +263,111 @@ await SeedSiteWithTwoDays(siteUid: 9902, employeeNo: "2", AssertRowDateAndEmployee(rowsByIndex[3], workbookPart, date15, "2"); } + // ------------------------------------------------------------------ + // 5. Regression: a cross-midnight / out-of-range shift slot id (> 289) + // must not crash the export. Production bug: Stop1Id = 313 (= 02:00 + // next day) made GetShiftTime index past the 288-entry plr.Options + // array and throw IndexOutOfRange. The all-workers path was the one + // that crashed in production, so both overloads are covered. + // ------------------------------------------------------------------ + + [Test] + public async Task Export_WithCrossMidnightShiftSlotId_DoesNotThrow() + { + // Start1Id = 265 -> (265-1)*5 = 1320 min -> 22:00. + // Stop1Id = 313 -> (313-1)*5 = 1560 min -> 26:00 (= 02:00 next day), + // the > 289 case that used to overflow plr.Options and throw. + // Pause1Id = 295 -> (295-1)*5 = 1470 min -> 24:30; Pause always goes + // through the crashing 2-arg GetShiftTime path (actualStamp + // is always null for pause), so it exercises the fix too. + await SeedSiteAndPlanRegistration( + siteUid: 9810, + employeeNo: "1", + date: new DateTime(2026, 5, 15), + useOneMinuteIntervals: false, + start1Id: 265, stop1Id: 313, pause1Id: 295); + + // --- Single-worker overload --- + var singleResult = await _service.GenerateExcelDashboard( + new TimePlanningWorkingHoursRequestModel + { + SiteId = 9810, + DateFrom = new DateTime(2026, 5, 15), + DateTo = new DateTime(2026, 5, 15), + }); + + Assert.That(singleResult.Success, Is.True, singleResult.Message); + Assert.That(singleResult.Model, Is.Not.Null); + Assert.That(singleResult.Model!.Length, Is.GreaterThan(0)); + + // Confirm not just "no throw" but correct arithmetic output: the + // Shift1 Stop cell for slot 313 renders "26:00" on the Dashboard sheet. + var (_, shift1Stop) = ReadDashboardShift1Cells(singleResult.Model!); + Assert.That(shift1Stop, Is.EqualTo("26:00"), + "Out-of-range slot 313 must render arithmetically as 26:00, not throw"); + + // Release the single-worker file handle before invoking the all-workers + // overload. Both exports write to /tmp/results/{yyyyMMdd_HHmmss}_.xlsx and + // return a still-open FileStream; calling them back-to-back inside the same + // second would otherwise collide on the identical filename and fail with + // an IOException unrelated to the slot-id regression under test. + await singleResult.Model!.DisposeAsync(); + + // --- All-workers overload (the path that crashed in production) --- + var allResult = await _service.GenerateExcelDashboard( + new TimePlanningWorkingHoursReportForAllWorkersRequestModel + { + DateFrom = new DateTime(2026, 5, 15), + DateTo = new DateTime(2026, 5, 15), + }); + + Assert.That(allResult.Success, Is.True, allResult.Message); + Assert.That(allResult.Model, Is.Not.Null); + Assert.That(allResult.Model!.Length, Is.GreaterThan(0)); + + // The all-workers workbook has no "Dashboard" sheet; the positional + // FillDataRow layout lives on the per-site sheet, named after the site + // ("Site 9810"). Same 0-indexed columns: 7=Shift1Start, 8=Shift1Stop. + var (_, allShift1Stop) = ReadDashboardShift1Cells(allResult.Model!, "Site 9810"); + Assert.That(allShift1Stop, Is.EqualTo("26:00"), + "All-workers path (the one that crashed in production) must also render slot 313 as 26:00"); + + await allResult.Model!.DisposeAsync(); + } + // ------------------------------------------------------------------ // Helpers // ------------------------------------------------------------------ + /// + /// Opens the xlsx stream and returns the (Shift1Start, Shift1Stop) cell text + /// for the first populated data row of the positional "Dashboard" sheet. + /// Column layout from FillDataRow (0-indexed): 7=Shift1Start, 8=Shift1Stop. + /// + private static (string Start, string Stop) ReadDashboardShift1Cells(Stream xlsx, string sheetName = "Dashboard") + { + xlsx.Position = 0; + using var doc = SpreadsheetDocument.Open(xlsx, false); + var workbookPart = doc.WorkbookPart!; + var dashboardSheet = workbookPart.Workbook.Descendants() + .First(s => s.Name == sheetName); + var dashboardPart = (WorksheetPart)workbookPart.GetPartById(dashboardSheet.Id!); + var rows = dashboardPart.Worksheet.Descendants().ToList(); + foreach (var row in rows.Where(r => r.RowIndex == null || r.RowIndex! > 1U)) + { + var cells = row.Elements().ToList(); + if (cells.Count < 9) continue; + var shift1Start = CellText(cells[7], workbookPart); + var shift1Stop = CellText(cells[8], workbookPart); + if (!string.IsNullOrEmpty(shift1Start) || !string.IsNullOrEmpty(shift1Stop)) + { + return (shift1Start, shift1Stop); + } + } + return ("", ""); + } + + private static void AssertRowDateAndEmployee(Row row, WorkbookPart wb, double expectedOaDate, string expectedEmployeeNo) { var employeeCell = row.Elements().Single(c => @@ -297,7 +398,7 @@ private static string CellText(Cell c, WorkbookPart wb) /// private async Task SeedSiteAndPlanRegistration( int siteUid, string employeeNo, DateTime date, bool useOneMinuteIntervals, - int start1Id, int stop1Id) + int start1Id, int stop1Id, int pause1Id = 0) { var core = await GetCore(); var sdkDb = core.DbContextHelper.GetDbContext(); @@ -355,7 +456,7 @@ private async Task SeedSiteAndPlanRegistration( Date = date, Start1Id = start1Id, Stop1Id = stop1Id, - Pause1Id = 0, + Pause1Id = pause1Id, PlanText = "", CommentOffice = "", CommentOfficeAll = "", diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs index 5169f4f0..70e1fc5f 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs @@ -836,6 +836,40 @@ public void GetShiftTime_FlagOnWithActualStamp_ReturnsHHmm() Assert.That(flagOff, Is.EqualTo("08:00"), "Flag-off must use legacy 5-min Options[96] = \"08:00\""); } + /// + /// Production regression (Excel export crash): the 2-arg + /// GetShiftTime(plr, shift) must compute the 5-minute time-of-day + /// arithmetically rather than indexing the fixed 288-entry + /// plr.Options list. Slot ids >= 290 (cross-midnight / night + /// shifts, or mis-encoded device values) previously threw + /// IndexOutOfRange and aborted the whole export + /// (FillDataRow → GetShiftTime). Slot s encodes (s-1)*5 + /// minutes; the don't-wrap convention keeps 289 → "24:00" and extends + /// past midnight: 290 → "24:05", 313 → "26:00". + /// + [TestCase(1, "00:00")] + [TestCase(91, "07:30")] + [TestCase(288, "23:55")] + [TestCase(289, "24:00")] + [TestCase(290, "24:05")] // crash case before the fix + [TestCase(313, "26:00")] // 02:00 next-day cross-midnight + [TestCase(0, "")] + [TestCase(null, "")] + public void GetShiftTime_2Arg_ComputesTimeAndHandlesOutOfRangeSlots(int? shift, string expected) + { + var plr = new PlanRegistration(); + // PlanRegistration's parameterless constructor populates Options with 288 + // 5-minute strings "00:00".."23:55"; the fix no longer indexes them. + + var service = (TimePlanningWorkingHoursService) + System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject( + typeof(TimePlanningWorkingHoursService)); + + var result = service.GetShiftTime(plr, shift); + + Assert.That(result, Is.EqualTo(expected), $"Slot {shift} must map to {expected}"); + } + /// /// Phase 4 contract: the Excel dashboard export /// (GenerateExcelDashboard) emits HH:mm string cells diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs index 4969eaab..31b47d78 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs @@ -2784,11 +2784,16 @@ private Cell CreateWeekNumberCell(DateTime dateValue) internal string GetShiftTime(PlanRegistration plr, int? shift) { - if (shift == 289) + if (shift is null or <= 0) { - return "24:00"; + return ""; } - return shift > 0 ? plr.Options[(int)shift - 1] : ""; + // A shift slot id encodes a 5-minute time-of-day: slot s -> (s-1)*5 minutes. + // Computed arithmetically instead of indexing the fixed 288-entry plr.Options, + // so cross-midnight / out-of-range slot ids (>= 290) don't overflow: + // 288 -> 23:55, 289 -> 24:00, 290 -> 24:05, 313 -> 26:00 (don't-wrap convention). + var minutes = (shift.Value - 1) * 5; + return $"{minutes / 60:00}:{minutes % 60:00}"; } ///