diff --git a/Api.SeaHavenIndustries.Tests/VendorCompanyRosterDataServiceTests.cs b/Api.SeaHavenIndustries.Tests/VendorCompanyRosterDataServiceTests.cs index 3b503af..2997c41 100644 --- a/Api.SeaHavenIndustries.Tests/VendorCompanyRosterDataServiceTests.cs +++ b/Api.SeaHavenIndustries.Tests/VendorCompanyRosterDataServiceTests.cs @@ -500,6 +500,69 @@ public class VendorCompanyRosterDataServiceTests private static byte[] InitialRowVersion() => new byte[] { 0, 0, 0, 0, 0, 0, 0, 1 }; + [Fact] + public async Task AddTechnicians_RenameToTakenName_ThrowsDuplicateNameConflict() + { + var dbName = Guid.NewGuid().ToString(); + int companyId; + using (var seed = NewContext(dbName)) + { + var (cid, _) = await SeedCompanyWithTechnicianAsync(seed, "Gateway Plumbing", InitialRowVersion(), "First"); + companyId = cid; + await SeedCompanyWithTechnicianAsync(seed, "Harbor Electric", InitialRowVersion(), "Other"); + } + + using (var act = NewContext(dbName)) + { + var service = new VendorCompanyRosterDataService(act); + var call = () => service.AddTechniciansAsync(new VendorCompanyRosterAddWriteModel + { + CompanyId = companyId, + RowVersion = InitialRowVersion(), + ActorUserId = "42", + CompanyFields = new VendorRosterCompanyFieldsWriteModel { Name = "Harbor Electric" }, + AddTechnicians = new List() + }, CancellationToken.None); + + // SH-250: a colliding rename must surface as a stable client conflict, not as an + // unhandled DbUpdateException that the controller reports as a 500. + await call.Should().ThrowAsync(); + } + + using var verify = NewContext(dbName); + verify.VendorCompanies.Single(c => c.Id == companyId).Name.Should().Be("Gateway Plumbing"); + } + + [Fact] + public async Task AddTechnicians_RenameToFreeName_Succeeds() + { + var dbName = Guid.NewGuid().ToString(); + int companyId; + using (var seed = NewContext(dbName)) + { + var (cid, _) = await SeedCompanyWithTechnicianAsync(seed, "Gateway Plumbing", InitialRowVersion(), "First"); + companyId = cid; + } + + using (var act = NewContext(dbName)) + { + var service = new VendorCompanyRosterDataService(act); + await service.AddTechniciansAsync(new VendorCompanyRosterAddWriteModel + { + CompanyId = companyId, + RowVersion = InitialRowVersion(), + ActorUserId = "42", + CompanyFields = new VendorRosterCompanyFieldsWriteModel { Name = "Gateway Plumbing & Drain" }, + AddTechnicians = new List() + }, CancellationToken.None); + } + + using var verify = NewContext(dbName); + var company = verify.VendorCompanies.Single(c => c.Id == companyId); + company.Name.Should().Be("Gateway Plumbing & Drain"); + company.NormalizedName.Should().Be("gateway plumbing & drain"); + } + [Fact] public async Task AddTechnicians_LeavesAllPreExistingTechniciansIntact() { diff --git a/Api.SeaHavenIndustries.Tests/VendorCompanyRosterServiceTests.cs b/Api.SeaHavenIndustries.Tests/VendorCompanyRosterServiceTests.cs index aa4669b..17c1280 100644 --- a/Api.SeaHavenIndustries.Tests/VendorCompanyRosterServiceTests.cs +++ b/Api.SeaHavenIndustries.Tests/VendorCompanyRosterServiceTests.cs @@ -508,9 +508,52 @@ public class VendorCompanyRosterServiceTests await NewService(data).AddTechniciansAsync(7, dto, "42", CancellationToken.None); + // SH-250: every field is blank, so there is no company change to apply. Forwarding a + // non-null write model made the data layer bump RowVersion and rewrite every + // technician's LastModificationTime for a request that changes no company data. + captured!.CompanyFields.Should().BeNull(); + } + + [Fact] + public async Task AddTechnicians_AllBlankCompanyFieldsAndNoTechnicians_FailsValidation() + { + var data = new Mock(); + + var dto = new AddTechniciansVendorRosterDTO + { + RowVersion = "AAAAAAAAD8I=", + AddTechnicians = new List(), + CompanyFields = new VendorRosterCompanyFieldsDTO { CompanyPhone = " ", Email = "" } + }; + + var act = () => NewService(data).AddTechniciansAsync(7, dto, "42", CancellationToken.None); + + await act.Should().ThrowAsync(); + data.Verify( + x => x.AddTechniciansAsync(It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task AddTechnicians_RealCompanyValueStillForwarded() + { + var data = new Mock(); + VendorCompanyRosterAddWriteModel? captured = null; + data.Setup(x => x.AddTechniciansAsync(It.IsAny(), It.IsAny())) + .Callback((model, _) => captured = model) + .ReturnsAsync(SampleReadModel(7, 1)); + + var dto = new AddTechniciansVendorRosterDTO + { + RowVersion = "AAAAAAAAD8I=", + AddTechnicians = new List(), + CompanyFields = new VendorRosterCompanyFieldsDTO { City = "Norfolk" } + }; + + await NewService(data).AddTechniciansAsync(7, dto, "42", CancellationToken.None); + captured!.CompanyFields.Should().NotBeNull(); - captured.CompanyFields!.CompanyPhone.Should().BeNull(); - captured.CompanyFields.Email.Should().BeNull(); + captured.CompanyFields!.City.Should().Be("Norfolk"); } [Fact] diff --git a/Api.SeaHavenIndustries/Controllers/VendorCompanyRosterController.cs b/Api.SeaHavenIndustries/Controllers/VendorCompanyRosterController.cs index e8df631..8180b3b 100644 --- a/Api.SeaHavenIndustries/Controllers/VendorCompanyRosterController.cs +++ b/Api.SeaHavenIndustries/Controllers/VendorCompanyRosterController.cs @@ -171,6 +171,16 @@ namespace Api.SeaHavenIndustries.Controllers { return NotFound(new Response { Status = "Error", Message = "Vendor company not found" }); } + catch (VendorRosterDuplicateNameException dup) + { + // SH-250: a colliding rename is a client conflict, not a server fault. + return Conflict(new + { + Status = "Conflict", + Message = _logger.Sanitize(dup, "Another vendor company already uses that name."), + Code = 409 + }); + } catch (DbUpdateConcurrencyException) { return Conflict(new diff --git a/SeaHaven.DataServices/Implementation/VendorCompanyRosterDataService.cs b/SeaHaven.DataServices/Implementation/VendorCompanyRosterDataService.cs index 9ba5412..2a23676 100644 --- a/SeaHaven.DataServices/Implementation/VendorCompanyRosterDataService.cs +++ b/SeaHaven.DataServices/Implementation/VendorCompanyRosterDataService.cs @@ -286,6 +286,7 @@ namespace SeaHaven.DataServices.Implementation var now = DateTime.UtcNow; int? actorId = int.TryParse(roster.ActorUserId, out var parsedActorId) ? parsedActorId : null; + string? renamedTo = null; // Partial company update: only fields present in CompanyFields are applied; // a null field leaves the stored value untouched. The company row is always @@ -297,8 +298,28 @@ namespace SeaHaven.DataServices.Implementation if (fields.Name != null) { var trimmedName = fields.Name.Trim(); + var normalized = trimmedName.ToLowerInvariant(); + + if (normalized != company.NormalizedName) + { + var nameTaken = await _context.VendorCompanies + .AnyAsync( + other => other.Id != company.Id + && (other.IsDeleted == null || other.IsDeleted == false) + && other.NormalizedName == normalized, + cancellationToken); + + if (nameTaken) + throw new VendorRosterDuplicateNameException( + "Another vendor company already uses that name.", + new InvalidOperationException( + $"NormalizedName '{normalized}' is already in use.")); + + renamedTo = normalized; + } + company.Name = trimmedName; - company.NormalizedName = trimmedName.ToLowerInvariant(); + company.NormalizedName = normalized; } if (fields.CompanyPhone != null) @@ -371,7 +392,21 @@ namespace SeaHaven.DataServices.Implementation await _context.Vendors.AddAsync(vendor, cancellationToken); } - await _context.SaveChangesAsync(cancellationToken); + // SH-250: a rename can collide with the unique NormalizedName index. Without + // this guard the DbUpdateException reached the controller's generic handler + // and surfaced as a 500, even though SQL Server rolls the batch back cleanly. + // The pre-check above handles the ordinary case; this covers the race where a + // competing rename commits between that check and this save. + try + { + await _context.SaveChangesAsync(cancellationToken); + } + catch (DbUpdateException ex) when (renamedTo != null && ex is not DbUpdateConcurrencyException) + { + throw new VendorRosterDuplicateNameException( + "Another vendor company already uses that name.", + ex); + } return await GetRosterAsync(null, company.Id, cancellationToken) ?? throw new InvalidOperationException("Roster could not be reloaded after save."); diff --git a/SeaHaven.DataServices/Models/VendorCompanyRosterModels.cs b/SeaHaven.DataServices/Models/VendorCompanyRosterModels.cs index bbe6a74..33e1225 100644 --- a/SeaHaven.DataServices/Models/VendorCompanyRosterModels.cs +++ b/SeaHaven.DataServices/Models/VendorCompanyRosterModels.cs @@ -86,6 +86,21 @@ namespace SeaHaven.DataServices.Models // Raised by the roster data service when a technician removed from the snapshot // still has open linked work orders, so the whole reconcile must fail. Carries the // blocking work orders so the API can surface a stable 409 without leaking internals. + /// + /// Raised when a company rename collides with the unique NormalizedName index. + /// Distinct from , which reports open + /// linked work orders, so callers can return a stable client conflict instead of + /// letting the raw + /// surface as a 500. + /// + public sealed class VendorRosterDuplicateNameException : Exception + { + public VendorRosterDuplicateNameException(string message, Exception inner) + : base(message, inner) + { + } + } + public sealed class VendorRosterConflictException : Exception { public IReadOnlyList BlockedWorkOrders { get; } diff --git a/SeaHaven.Services/Implementation/VendorCompanyRosterService.cs b/SeaHaven.Services/Implementation/VendorCompanyRosterService.cs index 4ce91b1..16f2c36 100644 --- a/SeaHaven.Services/Implementation/VendorCompanyRosterService.cs +++ b/SeaHaven.Services/Implementation/VendorCompanyRosterService.cs @@ -126,7 +126,8 @@ namespace SeaHaven.Services.Implementation nameof(AddTechniciansVendorRosterDTO.AddTechnicians), "Technician ids are not allowed when adding technicians.")); - if (dto.AddTechnicians.Count == 0 && dto.CompanyFields == null) + var hasCompanyChange = dto.CompanyFields != null && HasAnyCompanyValue(dto.CompanyFields); + if (dto.AddTechnicians.Count == 0 && !hasCompanyChange) failures.Add(new ValidationFailure( nameof(AddTechniciansVendorRosterDTO.AddTechnicians), "At least one technician to add or a company update is required.")); @@ -135,8 +136,17 @@ namespace SeaHaven.Services.Implementation VendorRosterCompanyFieldsWriteModel? companyFields = null; if (dto.CompanyFields != null) + { companyFields = ValidateAndMapCompanyFields(dto.CompanyFields, failures); + // SH-250: blank company fields all map to null ("unchanged"), so an empty + // or all-blank CompanyFields object carries no company update. Forwarding + // it as a non-null write model made the data layer bump RowVersion and + // rewrite every technician's LastModificationTime for a no-op request. + if (HasNoCompanyChange(companyFields)) + companyFields = null; + } + if (failures.Count > 0) throw new ValidationException(failures); @@ -250,6 +260,28 @@ namespace SeaHaven.Services.Implementation // Validates optional company-level updates for the additive path and maps them // to the write model. A null (or blank) field means "leave unchanged"; blank // values normalize to null so a partial update can never clear data by accident. + private static bool HasAnyCompanyValue(VendorRosterCompanyFieldsDTO fields) => + !string.IsNullOrWhiteSpace(fields.Name) + || !string.IsNullOrWhiteSpace(fields.CompanyPhone) + || !string.IsNullOrWhiteSpace(fields.Email) + || !string.IsNullOrWhiteSpace(fields.Address) + || !string.IsNullOrWhiteSpace(fields.City) + || !string.IsNullOrWhiteSpace(fields.State) + || !string.IsNullOrWhiteSpace(fields.Zip) + || !string.IsNullOrWhiteSpace(fields.GoogleMapsUrl) + || !string.IsNullOrWhiteSpace(fields.Notes); + + private static bool HasNoCompanyChange(VendorRosterCompanyFieldsWriteModel model) => + model.Name == null + && model.CompanyPhone == null + && model.Email == null + && model.Address == null + && model.City == null + && model.State == null + && model.Zip == null + && model.GoogleMapsUrl == null + && model.Notes == null; + private static VendorRosterCompanyFieldsWriteModel ValidateAndMapCompanyFields( VendorRosterCompanyFieldsDTO fields, List failures)