From 76db2b5a37d1741dd0137b49386fb1250bb8985d Mon Sep 17 00:00:00 2001 From: Arthur Bassi Date: Fri, 17 Jul 2026 14:46:31 -0300 Subject: [PATCH] fix(work-orders): address PR14 phase-2 review feedback --- .../Board/WorkOrderBoardConstants.cs | 11 -- .../Helpers/WorkOrderBoardMutationRules.cs | 15 +- .../WorkOrderBoardUpdateService.cs | 51 ++++- .../WorkOrderBoardMutationRulesTests.cs | 10 + .../WorkOrderBoardUpdateServiceTests.cs | 178 ++++++++++++++++++ 5 files changed, 244 insertions(+), 21 deletions(-) delete mode 100644 SeaHaven.Services/Board/WorkOrderBoardConstants.cs diff --git a/SeaHaven.Services/Board/WorkOrderBoardConstants.cs b/SeaHaven.Services/Board/WorkOrderBoardConstants.cs deleted file mode 100644 index 78d1ab5..0000000 --- a/SeaHaven.Services/Board/WorkOrderBoardConstants.cs +++ /dev/null @@ -1,11 +0,0 @@ -namespace SeaHaven.Services.Board -{ - /// - /// Shared board constants for Phase 2+ features. Not referenced in Phase 1. - /// - public static class WorkOrderBoardConstants - { - public const int MaxWindowDays = 90; - public const int ClientSideThreshold = 300; - } -} diff --git a/SeaHaven.Services/Helpers/WorkOrderBoardMutationRules.cs b/SeaHaven.Services/Helpers/WorkOrderBoardMutationRules.cs index 0f2be7e..456ff91 100644 --- a/SeaHaven.Services/Helpers/WorkOrderBoardMutationRules.cs +++ b/SeaHaven.Services/Helpers/WorkOrderBoardMutationRules.cs @@ -4,22 +4,25 @@ namespace SeaHaven.Services.Helpers { public static class WorkOrderBoardMutationRules { - private static readonly HashSet ReadOnlyStatuses = new() - { - LifecycleStatus.Canceled, - LifecycleStatus.Completed - }; + private static readonly HashSet ReadOnlyStatuses = + LifecycleStatusSets.Terminal.ToHashSet(); public static bool IsReadOnly(LifecycleStatus? status) => ReadOnlyStatuses.Contains(status); public static bool ShouldBlockStatusChangeWhenPastDue(string field, bool isPastDue) => field.Equals(WorkOrderBoardFieldNames.LifecycleStatus, StringComparison.OrdinalIgnoreCase) && isPastDue; + /// + /// SHOC rule: Incomplete + specific scheduled date + assignee → Scheduled. + /// Week-only targets do not auto-schedule (same as ). + /// public static bool ShouldAutoSchedule( LifecycleStatus? status, DateTime? scheduledDate, - string? assignTo) + string? assignTo, + bool? scheduleWeekOnly = null) => status == LifecycleStatus.Incomplete + && scheduleWeekOnly != true && scheduledDate.HasValue && !string.IsNullOrWhiteSpace(assignTo); diff --git a/SeaHaven.Services/Implementation/WorkOrderBoardUpdateService.cs b/SeaHaven.Services/Implementation/WorkOrderBoardUpdateService.cs index 1eb1ce7..c1409b6 100644 --- a/SeaHaven.Services/Implementation/WorkOrderBoardUpdateService.cs +++ b/SeaHaven.Services/Implementation/WorkOrderBoardUpdateService.cs @@ -30,6 +30,13 @@ namespace SeaHaven.Services.Implementation WorkOrderBoardPatchRequestDto request, string? actorId) { + // Intermediate SaveChanges (e.g. new dispatch identity) must stay atomic with the final patch save. + await using var transaction = _context.Database.IsRelational() + ? await _context.Database.BeginTransactionAsync() + : null; + + try + { if (string.IsNullOrWhiteSpace(request.Field)) throw new WorkOrderBoardValidationException("InvalidField", "Field is required."); @@ -94,12 +101,16 @@ namespace SeaHaven.Services.Implementation if (changes.Count == 0 && !_context.ChangeTracker.HasChanges()) { var unchanged = await LoadBoardRowAsync(workOrderId); + if (transaction is not null) + await transaction.CommitAsync(); return unchanged ?? throw new WorkOrderBoardValidationException("NotFound", "Work order not found."); } if (changes.Count == 0) { await _context.SaveChangesAsync(); + if (transaction is not null) + await transaction.CommitAsync(); var persisted = await LoadBoardRowAsync(workOrderId); return persisted ?? throw new WorkOrderBoardValidationException("NotFound", "Work order not found."); } @@ -127,13 +138,29 @@ namespace SeaHaven.Services.Implementation } catch (DbUpdateConcurrencyException) { + if (transaction is not null) + await transaction.RollbackAsync(); _context.ChangeTracker.Clear(); var currentState = await LoadBoardRowAsync(workOrderId); throw new WorkOrderBoardConcurrencyException(currentState); } + if (transaction is not null) + await transaction.CommitAsync(); + var row = await LoadBoardRowAsync(workOrderId); return row ?? throw new WorkOrderBoardValidationException("NotFound", "Work order not found."); + } + catch (WorkOrderBoardConcurrencyException) + { + throw; + } + catch + { + if (transaction is not null) + await transaction.RollbackAsync(); + throw; + } } private async Task> ApplyFieldMutationAsync( @@ -154,7 +181,7 @@ namespace SeaHaven.Services.Implementation WorkOrderBoardFieldNames.ScheduledDate => ApplyScheduledDate(workOrder, value, auditField), WorkOrderBoardFieldNames.TargetWeek => new List { ApplyTargetWeek(workOrder, value, auditField) }, WorkOrderBoardFieldNames.ScheduleWeekOnly => new List { ApplyBoolField(value, auditField, v => workOrder.ScheduleWeekOnly = v, () => workOrder.ScheduleWeekOnly) }, - WorkOrderBoardFieldNames.VendorId => new List { ApplyVendorId(dispatch!, value, auditField) }, + WorkOrderBoardFieldNames.VendorId => new List { await ApplyVendorIdAsync(dispatch!, value, auditField) }, WorkOrderBoardFieldNames.ApptDate => new List { ApplyApptDate(dispatch!, value, auditField) }, WorkOrderBoardFieldNames.ApptTime => new List { ApplyApptTime(workOrder, dispatch!, value, auditField) }, WorkOrderBoardFieldNames.DocStatus => new List { ApplyDocStatus(workOrder, value, auditField) }, @@ -188,6 +215,8 @@ namespace SeaHaven.Services.Implementation if (!vendorIdForCreate.HasValue || vendorIdForCreate.Value <= 0) throw new WorkOrderBoardValidationException("DispatchRequired", "A primary dispatch is required. Set vendorId first or provide primaryDispatchId."); + await EnsureVendorExistsAsync(vendorIdForCreate.Value); + var created = new Dispatch { WorkOrderId = workOrder.Id, @@ -196,10 +225,18 @@ namespace SeaHaven.Services.Implementation CreatedDate = DateTime.UtcNow }; _context.Dispatches.Add(created); - workOrder.PrimaryDispatch = created; + await _context.SaveChangesAsync(); + workOrder.PrimaryDispatchId = created.Id; return created; } + private async Task EnsureVendorExistsAsync(int vendorId) + { + var vendorExists = await _context.Vendors.AnyAsync(v => v.Id == vendorId && v.IsDeleted != true); + if (!vendorExists) + throw new WorkOrderBoardValidationException("VendorNotFound", "vendorId does not exist."); + } + private async Task ApplyWoNumber(WorkOrder workOrder, string? value, string auditField) { if (!WorkOrderNumberNormalizer.TryNormalize(value, out var normalized, out var error)) @@ -384,12 +421,14 @@ namespace SeaHaven.Services.Implementation return FieldChange.ForField(auditField, old, parsed.ToString()); } - private static FieldChange ApplyVendorId(Dispatch dispatch, string? value, string auditField) + private async Task ApplyVendorIdAsync(Dispatch dispatch, string? value, string auditField) { var vendorId = ParseVendorIdHint(value); if (!vendorId.HasValue) throw new WorkOrderBoardValidationException("InvalidValue", "vendorId must be a positive integer."); + await EnsureVendorExistsAsync(vendorId.Value); + var old = dispatch.VendorId.ToString(); if (dispatch.VendorId == vendorId.Value) return FieldChange.Unchanged(auditField, ResolveDispatchIdForAudit(dispatch)); @@ -461,7 +500,11 @@ namespace SeaHaven.Services.Implementation private static List ApplyAutoScheduleSideEffects(WorkOrder workOrder) { var changes = new List(); - if (!WorkOrderBoardMutationRules.ShouldAutoSchedule(workOrder.LifecycleStatus, workOrder.ScheduledDate, workOrder.AssignTo)) + if (!WorkOrderBoardMutationRules.ShouldAutoSchedule( + workOrder.LifecycleStatus, + workOrder.ScheduledDate, + workOrder.AssignTo, + workOrder.ScheduleWeekOnly)) return changes; var old = workOrder.LifecycleStatus?.ToString(); diff --git a/SeaHavenIndustries.Tests/WorkOrderBoardMutationRulesTests.cs b/SeaHavenIndustries.Tests/WorkOrderBoardMutationRulesTests.cs index 34509ce..933aa80 100644 --- a/SeaHavenIndustries.Tests/WorkOrderBoardMutationRulesTests.cs +++ b/SeaHavenIndustries.Tests/WorkOrderBoardMutationRulesTests.cs @@ -42,6 +42,16 @@ public class WorkOrderBoardMutationRulesTests "dispatcher-1")); } + [Fact] + public void ShouldAutoSchedule_FalseWhenScheduleWeekOnly() + { + Assert.False(WorkOrderBoardMutationRules.ShouldAutoSchedule( + LifecycleStatus.Incomplete, + new DateTime(2026, 6, 25), + "dispatcher-1", + scheduleWeekOnly: true)); + } + [Fact] public void IsReschedule_WhenPreviousDateExistsAndChanges() { diff --git a/SeaHavenIndustries.Tests/WorkOrderBoardUpdateServiceTests.cs b/SeaHavenIndustries.Tests/WorkOrderBoardUpdateServiceTests.cs index cb2b73a..a04425c 100644 --- a/SeaHavenIndustries.Tests/WorkOrderBoardUpdateServiceTests.cs +++ b/SeaHavenIndustries.Tests/WorkOrderBoardUpdateServiceTests.cs @@ -274,4 +274,182 @@ public class WorkOrderBoardUpdateServiceTests Assert.Equal("DuplicateWoNumber", ex.Code); } + + [Fact] + public async Task PatchField_WeekOnly_DoesNotAutoScheduleOnAssignTo() + { + var (context, service) = CreateSut(); + var wo = new WorkOrder + { + Id = 1, + LifecycleStatus = LifecycleStatus.Incomplete, + ScheduledDate = new DateTime(2026, 6, 25), + ScheduleWeekOnly = true, + TargetWeek = new DateOnly(2026, 6, 22), + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 1 } + }; + context.workOrders.Add(wo); + await context.SaveChangesAsync(); + + var result = await service.PatchFieldAsync(1, new WorkOrderBoardPatchRequestDto + { + Field = WorkOrderBoardFieldNames.AssignTo, + Value = "user-a", + WorkOrderVersion = ToVersion(wo) + }, "actor-1"); + + Assert.Equal(LifecycleStatus.Incomplete, result.LifecycleStatus); + Assert.Equal("user-a", result.DispatcherId); + } + + [Fact] + public async Task PatchField_ApptTime_UpdatesScheduledStart() + { + var (context, service) = CreateSut(); + var dispatch = new Dispatch + { + Id = 10, + WorkOrderId = 1, + VendorId = 1, + ScheduledDate = new DateTime(2026, 6, 25), + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 2 } + }; + var wo = new WorkOrder + { + Id = 1, + LifecycleStatus = LifecycleStatus.Scheduled, + PrimaryDispatchId = 10, + ScheduledDate = new DateTime(2026, 6, 25), + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 1 } + }; + context.Vendors.Add(new Vendor { Id = 1, CompanyName = "Vendor A" }); + context.Dispatches.Add(dispatch); + context.workOrders.Add(wo); + await context.SaveChangesAsync(); + + var result = await service.PatchFieldAsync(1, new WorkOrderBoardPatchRequestDto + { + Field = WorkOrderBoardFieldNames.ApptTime, + Value = "09:00-10:00", + WorkOrderVersion = ToVersion(wo), + DispatchVersion = ToVersion(dispatch), + PrimaryDispatchId = 10 + }, "actor-1"); + + Assert.Contains("09:00", result.ApptTime); + var reloaded = await context.workOrders.FindAsync(1); + Assert.Equal(new DateTime(2026, 6, 25, 9, 0, 0), reloaded!.ScheduledStart); + Assert.Equal(new DateTime(2026, 6, 25, 10, 0, 0), reloaded.ScheduledEnd); + } + + [Fact] + public async Task PatchField_ApptTime_DashPrefixedToken_TreatedAsSingleStart() + { + var (context, service) = CreateSut(); + var dispatch = new Dispatch + { + Id = 10, + WorkOrderId = 1, + VendorId = 1, + ScheduledDate = new DateTime(2026, 6, 25), + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 2 } + }; + var wo = new WorkOrder + { + Id = 1, + LifecycleStatus = LifecycleStatus.Scheduled, + PrimaryDispatchId = 10, + ScheduledDate = new DateTime(2026, 6, 25), + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 1 } + }; + context.Vendors.Add(new Vendor { Id = 1, CompanyName = "Vendor A" }); + context.Dispatches.Add(dispatch); + context.workOrders.Add(wo); + await context.SaveChangesAsync(); + + await service.PatchFieldAsync(1, new WorkOrderBoardPatchRequestDto + { + Field = WorkOrderBoardFieldNames.ApptTime, + Value = "-30", + WorkOrderVersion = ToVersion(wo), + DispatchVersion = ToVersion(dispatch), + PrimaryDispatchId = 10 + }, "actor-1"); + + var reloaded = await context.workOrders.FindAsync(1); + Assert.NotNull(reloaded!.ScheduledStart); + Assert.Equal(new DateTime(2026, 6, 25), reloaded.ScheduledEnd); + Assert.Equal(new DateTime(2026, 6, 25).Add(TimeSpan.FromDays(30)), reloaded.ScheduledStart); + } + + [Fact] + public async Task PatchField_DispatchFieldWithoutVersion_ThrowsDispatchVersionRequired() + { + var (context, service) = CreateSut(); + var dispatch = new Dispatch + { + Id = 10, + WorkOrderId = 1, + VendorId = 1, + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 2 } + }; + var wo = new WorkOrder + { + Id = 1, + LifecycleStatus = LifecycleStatus.Scheduled, + PrimaryDispatchId = 10, + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 1 } + }; + context.Vendors.Add(new Vendor { Id = 1, CompanyName = "Vendor A" }); + context.Dispatches.Add(dispatch); + context.workOrders.Add(wo); + await context.SaveChangesAsync(); + + var ex = await Assert.ThrowsAsync(() => + service.PatchFieldAsync(1, new WorkOrderBoardPatchRequestDto + { + Field = WorkOrderBoardFieldNames.ApptDate, + Value = "2026-06-26", + WorkOrderVersion = ToVersion(wo), + PrimaryDispatchId = 10 + }, "actor-1")); + + Assert.Equal("DispatchVersionRequired", ex.Code); + } + + [Fact] + public async Task PatchField_VendorIdNotFound_Throws() + { + var (context, service) = CreateSut(); + var dispatch = new Dispatch + { + Id = 10, + WorkOrderId = 1, + VendorId = 1, + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 2 } + }; + var wo = new WorkOrder + { + Id = 1, + LifecycleStatus = LifecycleStatus.Scheduled, + PrimaryDispatchId = 10, + RowVersion = new byte[] { 1, 0, 0, 0, 0, 0, 0, 1 } + }; + context.Vendors.Add(new Vendor { Id = 1, CompanyName = "Vendor A" }); + context.Dispatches.Add(dispatch); + context.workOrders.Add(wo); + await context.SaveChangesAsync(); + + var ex = await Assert.ThrowsAsync(() => + service.PatchFieldAsync(1, new WorkOrderBoardPatchRequestDto + { + Field = WorkOrderBoardFieldNames.VendorId, + Value = "999", + WorkOrderVersion = ToVersion(wo), + DispatchVersion = ToVersion(dispatch), + PrimaryDispatchId = 10 + }, "actor-1")); + + Assert.Equal("VendorNotFound", ex.Code); + } }