diff --git a/README.md b/README.md index c573544..8fcb39c 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,7 @@ Six parallel jobs calling org reusable workflows: | Job | Workflow | What it checks | |---|---|---| -| .NET Build & Test | `ci-dotnet.yaml` | Restore, build, test the API solution (123 xUnit tests) | +| .NET Build & Test | `ci-dotnet.yaml` | Restore, build, test the API solution (xUnit) | | Web Frontend Check | `ci-typescript-frontend.yaml` | Prettier `format:check`, build (includes `tsc -b`), vitest suite, Playwright chromium smoke (dev-login → proposal list, API mocked) | | Mobile Typecheck | `ci-typescript-cdk.yaml` | TypeScript typecheck for mobile | | Python Lint | `ci-python-sam.yaml` | ruff check + format on lambdas/ | @@ -169,6 +169,10 @@ Two-layer auth architecture with defense-in-depth: **Internal API key:** Python Lambdas call the .NET API via a Lambda Function URL with AWS_IAM auth (bypasses API Gateway JWT check). The `InternalApiKeyMiddleware` validates the `X-Internal-Api-Key` header and assigns the `admins` role to the synthetic identity. Lambdas cache the API key from Secrets Manager with a 5-minute TTL. +**Optimistic concurrency (ADR 0004):** proposal responses carry an opaque `rowVersion` token; mutations of the proposal aggregate (update, approve, return-to-review, send, revise, bulk line-item update) require `proposalVersion` in the body. Missing token → 422 `ProposalVersionRequired`, malformed → 422 `InvalidRowVersion`, stale → **409 `{ message, currentState }`** with the reloaded proposal embedded (unguarded races → 409 `{ status, message, code }`). Line-item create/delete are token-less but bump the aggregate version. Audit rows commit atomically with their mutation (stage-then-single-SaveChanges). + +**Deploy note for schema changes:** EF migrations auto-apply at API startup under a `pg_advisory_lock`. Before deploying a migration: take a manual RDS snapshot; additive-only migrations are backward-compatible with the previous Lambda version. Test the down-script against a snapshot-restored copy before any production rollback. + ## Data Flow 1. Dispatcher submits proposal request (web or mobile) diff --git a/api/src/ProposalSystem.Api/Controllers/FilesController.cs b/api/src/ProposalSystem.Api/Controllers/FilesController.cs index 2aba4db..5d4008c 100644 --- a/api/src/ProposalSystem.Api/Controllers/FilesController.cs +++ b/api/src/ProposalSystem.Api/Controllers/FilesController.cs @@ -189,8 +189,9 @@ public class FilesController : ControllerBase }; _db.GeneratedPdfs.Add(generatedPdf); + // Stage-then-single-SaveChanges: the audit row commits atomically with the PDF record. + _audit.Stage(AuditAction.GeneratePDF, proposalId); await _db.SaveChangesAsync(ct); - await _audit.LogAsync(AuditAction.GeneratePDF, proposalId, null, ct); return PhysicalFile(filePath, "application/pdf", Path.GetFileName(filePath)); } diff --git a/api/src/ProposalSystem.Api/Controllers/ProposalsController.cs b/api/src/ProposalSystem.Api/Controllers/ProposalsController.cs index dcac93f..b6cf978 100644 --- a/api/src/ProposalSystem.Api/Controllers/ProposalsController.cs +++ b/api/src/ProposalSystem.Api/Controllers/ProposalsController.cs @@ -1,5 +1,6 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ModelBinding; using ProposalSystem.Application.DTOs; using ProposalSystem.Application.Interfaces; @@ -74,33 +75,45 @@ public class ProposalsController : ControllerBase [ProducesResponseType(typeof(ProposalResponse), 200)] [ProducesResponseType(400)] [ProducesResponseType(404)] - public async Task> Approve(Guid id, CancellationToken ct) + public async Task> Approve( + Guid id, + [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] ProposalVersionRequest? request, + CancellationToken ct) { - var result = await _proposalService.ApproveAsync(id, ct); + var result = await _proposalService.ApproveAsync(id, request?.ProposalVersion, ct); return Ok(result); } [HttpPost("{id:guid}/return-to-review")] [Authorize(Roles = "admins,sysadmins")] - public async Task> ReturnToReview(Guid id, CancellationToken ct) + public async Task> ReturnToReview( + Guid id, + [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] ProposalVersionRequest? request, + CancellationToken ct) { - var result = await _proposalService.ReturnToReviewAsync(id, ct); + var result = await _proposalService.ReturnToReviewAsync(id, request?.ProposalVersion, ct); return Ok(result); } [HttpPost("{id:guid}/send")] [Authorize(Roles = "admins,sysadmins")] - public async Task> MarkSent(Guid id, CancellationToken ct) + public async Task> MarkSent( + Guid id, + [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] ProposalVersionRequest? request, + CancellationToken ct) { - var result = await _proposalService.MarkSentAsync(id, ct); + var result = await _proposalService.MarkSentAsync(id, request?.ProposalVersion, ct); return Ok(result); } [HttpPost("{id:guid}/revise")] [Authorize(Roles = "admins,sysadmins")] - public async Task> Revise(Guid id, CancellationToken ct) + public async Task> Revise( + Guid id, + [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Allow)] ProposalVersionRequest? request, + CancellationToken ct) { - var result = await _proposalService.ReviseAsync(id, ct); + var result = await _proposalService.ReviseAsync(id, request?.ProposalVersion, ct); return Ok(result); } diff --git a/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs b/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs index 619ac5f..04edb34 100644 --- a/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs +++ b/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs @@ -45,6 +45,12 @@ public class VendorProposalsController : ControllerBase proposal.VendorTotalCost = await _db.VendorProposals .Where(v => v.ProposalId == vendor.ProposalId) .SumAsync(v => v.TotalVendorCost, ct); + // Aggregate write must bump the concurrency version (scanner sweep + // finding): unguarded like line-item appends, but a lost race then + // surfaces as the fallback 409 instead of silently overwriting a + // concurrent guarded edit. + proposal.Version++; + proposal.UpdatedAt = DateTime.UtcNow; await _db.SaveChangesAsync(ct); } diff --git a/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs b/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs index af6206f..151d18c 100644 --- a/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs +++ b/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs @@ -2,6 +2,7 @@ using System.Net; using System.Text.Json; using FluentValidation; using Microsoft.AspNetCore.Mvc; +using Microsoft.EntityFrameworkCore; using ProposalSystem.Application.Common; namespace ProposalSystem.Api.Middleware; @@ -27,6 +28,8 @@ public class GlobalExceptionHandler : IMiddleware } } + private static string Sanitize(string? value) => (value ?? "").Replace('\r', '_').Replace('\n', '_'); + private static ProblemDetails MakeProblem(int status, string title, string detail, string code) { var problem = new ProblemDetails @@ -39,8 +42,50 @@ public class GlobalExceptionHandler : IMiddleware return problem; } + // Must match the MVC pipeline's serialization (camelCase + string enums): + // the 409 currentState embeds a ProposalResponse, and clients schema-validate + // it against the same shapes their 200 responses use. + private static readonly JsonSerializerOptions CamelCase = new() + { + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() }, + }; + private async Task HandleExceptionAsync(HttpContext context, Exception exception) { + // Concurrency conflicts use SHOC's envelopes verbatim (ADR 0004), NOT + // ProblemDetails: the guarded path embeds the reloaded currentState for + // immediate client refresh; the bare-DbUpdateConcurrencyException shape + // is the safety net for any unguarded write. + if (exception is ProposalConcurrencyException concurrencyEx) + { + _logger.LogWarning("Concurrency conflict on {Method} {Path}", + Sanitize(context.Request.Method), Sanitize(context.Request.Path)); + context.Response.StatusCode = StatusCodes.Status409Conflict; + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(new + { + message = concurrencyEx.Message, + currentState = concurrencyEx.CurrentState, + }, CamelCase)); + return; + } + + if (exception is DbUpdateConcurrencyException) + { + _logger.LogWarning("Unguarded concurrency conflict on {Method} {Path}", + Sanitize(context.Request.Method), Sanitize(context.Request.Path)); + context.Response.StatusCode = StatusCodes.Status409Conflict; + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(JsonSerializer.Serialize(new + { + status = "Conflict", + message = "The record was modified by another user. Refresh and retry.", + code = 409, + }, CamelCase)); + return; + } + // Every response carries a machine-readable "code" extension (SHOC // error-code vocabulary convention) so clients branch on codes, not // on human-readable text. @@ -77,7 +122,7 @@ public class GlobalExceptionHandler : IMiddleware }; _logger.LogError(exception, "Exception on {Method} {Path}: {Status}", - context.Request.Method, context.Request.Path, (int)statusCode); + Sanitize(context.Request.Method), Sanitize(context.Request.Path), (int)statusCode); context.Response.StatusCode = (int)statusCode; context.Response.ContentType = "application/problem+json"; diff --git a/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs b/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs new file mode 100644 index 0000000..d1da400 --- /dev/null +++ b/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs @@ -0,0 +1,21 @@ +using ProposalSystem.Application.DTOs; + +namespace ProposalSystem.Application.Common; + +/// +/// Optimistic-concurrency conflict on the proposal aggregate. Carries the +/// freshly reloaded current state so the 409 body can embed it for immediate +/// client refresh (SHOC contract: 409 { message, currentState }). Typed as +/// the response DTO so an EF entity (with navigation graphs) can never be +/// serialized into the error body by accident. +/// +public class ProposalConcurrencyException : Exception +{ + public ProposalResponse? CurrentState { get; } + + public ProposalConcurrencyException(ProposalResponse? currentState) + : base("The record was modified by another user. Refresh and retry.") + { + CurrentState = currentState; + } +} diff --git a/api/src/ProposalSystem.Application/Common/RowVersionCodec.cs b/api/src/ProposalSystem.Application/Common/RowVersionCodec.cs new file mode 100644 index 0000000..b9df3b3 --- /dev/null +++ b/api/src/ProposalSystem.Application/Common/RowVersionCodec.cs @@ -0,0 +1,31 @@ +using System.Buffers.Binary; + +namespace ProposalSystem.Application.Common; + +/// +/// Encodes the integer concurrency version as an opaque base64 token +/// (8 bytes, big-endian), matching SHOC's rowversion wire contract: +/// clients receive "rowVersion" strings and echo them back untouched. +/// +public static class RowVersionCodec +{ + public static string Encode(long version) + { + Span buffer = stackalloc byte[8]; + BinaryPrimitives.WriteInt64BigEndian(buffer, version); + return Convert.ToBase64String(buffer); + } + + public static bool TryDecode(string? token, out long version) + { + version = 0; + if (string.IsNullOrEmpty(token)) return false; + + Span buffer = stackalloc byte[8]; + if (!Convert.TryFromBase64String(token, buffer, out var written) || written != 8) + return false; + + version = BinaryPrimitives.ReadInt64BigEndian(buffer); + return true; + } +} diff --git a/api/src/ProposalSystem.Application/DTOs/LineItemDtos.cs b/api/src/ProposalSystem.Application/DTOs/LineItemDtos.cs index 33fd798..588ff91 100644 --- a/api/src/ProposalSystem.Application/DTOs/LineItemDtos.cs +++ b/api/src/ProposalSystem.Application/DTOs/LineItemDtos.cs @@ -14,7 +14,8 @@ public record LineItemResponse( int SortOrder, LineItemSource Source, DateTime CreatedAt, - DateTime UpdatedAt + DateTime UpdatedAt, + string RowVersion ); public record CreateLineItemRequest( @@ -29,7 +30,8 @@ public record CreateLineItemRequest( ); public record BulkUpdateLineItemsRequest( - List LineItems + List LineItems, + string? ProposalVersion = null ); public record UpdateLineItemEntry( diff --git a/api/src/ProposalSystem.Application/DTOs/ProposalDtos.cs b/api/src/ProposalSystem.Application/DTOs/ProposalDtos.cs index ff72e71..baf2511 100644 --- a/api/src/ProposalSystem.Application/DTOs/ProposalDtos.cs +++ b/api/src/ProposalSystem.Application/DTOs/ProposalDtos.cs @@ -18,9 +18,14 @@ public record UpdateProposalRequest( string? Notes, string? PoNumber, string? WorkOrderNumber, - Guid? AssignedAdminId + Guid? AssignedAdminId, + string? ProposalVersion = null ); +/// Body for state-transition endpoints (approve, return-to-review, +/// mark-sent, revise): the expected proposal version token. +public record ProposalVersionRequest(string? ProposalVersion); + public record ProposalResponse( Guid Id, string ProposalNumber, @@ -46,7 +51,8 @@ public record ProposalResponse( int CurrentRevision, Guid? ParentProposalId, DateTime CreatedAt, - DateTime UpdatedAt + DateTime UpdatedAt, + string RowVersion ); public record ProposalListResponse( @@ -60,7 +66,8 @@ public record ProposalListResponse( decimal TotalBidAmount, DateTime SubmittedAt, string? SubmittedByName, - string? AssignedAdminName + string? AssignedAdminName, + string RowVersion ); public record ProposalFilterRequest( diff --git a/api/src/ProposalSystem.Application/Interfaces/IAuditService.cs b/api/src/ProposalSystem.Application/Interfaces/IAuditService.cs index 8551944..4a1f095 100644 --- a/api/src/ProposalSystem.Application/Interfaces/IAuditService.cs +++ b/api/src/ProposalSystem.Application/Interfaces/IAuditService.cs @@ -4,5 +4,12 @@ namespace ProposalSystem.Application.Interfaces; public interface IAuditService { + /// Writes the audit entry in its own SaveChanges. For standalone + /// events (downloads, role changes) where no other write is in flight. Task LogAsync(AuditAction action, Guid? proposalId, string? details = null, CancellationToken ct = default); + + /// Stages the audit entry on the shared DbContext WITHOUT saving. + /// Mutation flows call this before their own SaveChangesAsync so the domain + /// change and its audit row commit atomically or not at all. + void Stage(AuditAction action, Guid? proposalId, string? details = null); } diff --git a/api/src/ProposalSystem.Application/Interfaces/IProposalService.cs b/api/src/ProposalSystem.Application/Interfaces/IProposalService.cs index fbcfef6..1abea91 100644 --- a/api/src/ProposalSystem.Application/Interfaces/IProposalService.cs +++ b/api/src/ProposalSystem.Application/Interfaces/IProposalService.cs @@ -8,10 +8,10 @@ public interface IProposalService Task GetByIdAsync(Guid id, CancellationToken ct = default); Task> GetAllAsync(ProposalFilterRequest filter, CancellationToken ct = default); Task UpdateAsync(Guid id, UpdateProposalRequest request, CancellationToken ct = default); - Task ApproveAsync(Guid id, CancellationToken ct = default); - Task ReturnToReviewAsync(Guid id, CancellationToken ct = default); - Task MarkSentAsync(Guid id, CancellationToken ct = default); - Task ReviseAsync(Guid id, CancellationToken ct = default); + Task ApproveAsync(Guid id, string? proposalVersion, CancellationToken ct = default); + Task ReturnToReviewAsync(Guid id, string? proposalVersion, CancellationToken ct = default); + Task MarkSentAsync(Guid id, string? proposalVersion, CancellationToken ct = default); + Task ReviseAsync(Guid id, string? proposalVersion, CancellationToken ct = default); Task> GetRevisionHistoryAsync(Guid id, CancellationToken ct = default); Task> GetAuditTrailAsync(Guid id, CancellationToken ct = default); Task GetStatsAsync(CancellationToken ct = default); diff --git a/api/src/ProposalSystem.Domain/Entities/LineItem.cs b/api/src/ProposalSystem.Domain/Entities/LineItem.cs index e876e18..db98009 100644 --- a/api/src/ProposalSystem.Domain/Entities/LineItem.cs +++ b/api/src/ProposalSystem.Domain/Entities/LineItem.cs @@ -29,6 +29,10 @@ public class LineItem public LineItemSource Source { get; set; } public DateTime CreatedAt { get; set; } public DateTime UpdatedAt { get; set; } + /// Optimistic-concurrency token. Bulk updates are guarded at the + /// proposal level (rows are replaced wholesale); this exists for per-item + /// mutations and client cache keys. + public long Version { get; set; } = 1; public Proposal? Proposal { get; set; } } diff --git a/api/src/ProposalSystem.Domain/Entities/Proposal.cs b/api/src/ProposalSystem.Domain/Entities/Proposal.cs index 4d7d9d3..ab03f92 100644 --- a/api/src/ProposalSystem.Domain/Entities/Proposal.cs +++ b/api/src/ProposalSystem.Domain/Entities/Proposal.cs @@ -52,6 +52,8 @@ public class Proposal public Guid? ParentProposalId { get; set; } public DateTime CreatedAt { get; set; } public DateTime UpdatedAt { get; set; } + /// Optimistic-concurrency token; bumps on every aggregate mutation (including line-item changes). + public long Version { get; set; } = 1; public User? SubmittedBy { get; set; } public User? AssignedAdmin { get; set; } diff --git a/api/src/ProposalSystem.Infrastructure/Data/Migrations/20260714003834_AddProposalLineItemVersion.Designer.cs b/api/src/ProposalSystem.Infrastructure/Data/Migrations/20260714003834_AddProposalLineItemVersion.Designer.cs new file mode 100644 index 0000000..17a00e5 --- /dev/null +++ b/api/src/ProposalSystem.Infrastructure/Data/Migrations/20260714003834_AddProposalLineItemVersion.Designer.cs @@ -0,0 +1,564 @@ +// +using System; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; +using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata; +using ProposalSystem.Infrastructure.Data; + +#nullable disable + +namespace ProposalSystem.Infrastructure.Data.Migrations +{ + [DbContext(typeof(ProposalDbContext))] + [Migration("20260714003834_AddProposalLineItemVersion")] + partial class AddProposalLineItemVersion + { + /// + protected override void BuildTargetModel(ModelBuilder modelBuilder) + { +#pragma warning disable 612, 618 + modelBuilder + .HasAnnotation("ProductVersion", "8.0.28") + .HasAnnotation("Relational:MaxIdentifierLength", 63); + + NpgsqlModelBuilderExtensions.UseIdentityByDefaultColumns(modelBuilder); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.AuditLog", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("Action") + .IsRequired() + .HasColumnType("text"); + + b.Property("Details") + .HasColumnType("jsonb"); + + b.Property("IpAddress") + .HasColumnType("text"); + + b.Property("ProposalId") + .HasColumnType("uuid"); + + b.Property("Timestamp") + .HasColumnType("timestamp with time zone"); + + b.Property("UserId") + .HasColumnType("uuid"); + + b.HasKey("Id"); + + b.HasIndex("ProposalId"); + + b.HasIndex("Timestamp"); + + b.HasIndex("UserId"); + + b.ToTable("AuditLogs"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.Customer", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("Addresses") + .HasColumnType("jsonb"); + + b.Property("ContactEmail") + .HasColumnType("text"); + + b.Property("CreatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("Name") + .IsRequired() + .HasColumnType("text"); + + b.Property("UpdatedAt") + .HasColumnType("timestamp with time zone"); + + b.HasKey("Id"); + + b.ToTable("Customers"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.GeneratedPdf", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("GeneratedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("GeneratedById") + .HasColumnType("uuid"); + + b.Property("ProposalId") + .HasColumnType("uuid"); + + b.Property("Revision") + .HasColumnType("integer"); + + b.Property("S3Key") + .IsRequired() + .HasColumnType("text"); + + b.HasKey("Id"); + + b.HasIndex("GeneratedById"); + + b.HasIndex("ProposalId"); + + b.ToTable("GeneratedPdfs"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.LineItem", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("CreatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("Description") + .IsRequired() + .HasColumnType("text"); + + b.Property("PricingMode") + .IsRequired() + .HasColumnType("text"); + + b.Property("ProposalId") + .HasColumnType("uuid"); + + b.Property("Quantity") + .HasPrecision(18, 4) + .HasColumnType("numeric(18,4)"); + + b.Property("SortOrder") + .HasColumnType("integer"); + + b.Property("Source") + .IsRequired() + .HasColumnType("text"); + + b.Property("TotalPrice") + .HasPrecision(18, 2) + .HasColumnType("numeric(18,2)"); + + b.Property("Unit") + .IsRequired() + .HasColumnType("text"); + + b.Property("UnitPrice") + .HasPrecision(18, 2) + .HasColumnType("numeric(18,2)"); + + b.Property("UpdatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("Version") + .IsConcurrencyToken() + .ValueGeneratedOnAdd() + .HasColumnType("bigint") + .HasDefaultValue(1L); + + b.HasKey("Id"); + + b.HasIndex("ProposalId"); + + b.ToTable("LineItems"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.PricingLibraryItem", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("CreatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("Description") + .HasColumnType("text"); + + b.Property("Keywords") + .HasColumnType("text"); + + b.Property("ServiceCategory") + .IsRequired() + .HasColumnType("text"); + + b.Property("Source") + .IsRequired() + .HasColumnType("text"); + + b.Property("Title") + .IsRequired() + .HasColumnType("text"); + + b.Property("Unit") + .HasColumnType("text"); + + b.Property("UnitPrice") + .HasPrecision(18, 2) + .HasColumnType("numeric(18,2)"); + + b.Property("UpdatedAt") + .HasColumnType("timestamp with time zone"); + + b.HasKey("Id"); + + b.ToTable("PricingLibraryItems"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.Proposal", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("ApprovedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("ApprovedById") + .HasColumnType("uuid"); + + b.Property("AssignedAdminId") + .HasColumnType("uuid"); + + b.Property("CreatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("CurrentRevision") + .HasColumnType("integer"); + + b.Property("CustomerAddress") + .IsRequired() + .HasColumnType("text"); + + b.Property("CustomerName") + .IsRequired() + .HasColumnType("text"); + + b.Property("Notes") + .IsRequired() + .HasColumnType("text"); + + b.Property("ParentProposalId") + .HasColumnType("uuid"); + + b.Property("PoNumber") + .HasColumnType("text"); + + b.Property("Priority") + .IsRequired() + .HasColumnType("text"); + + b.Property("ProposalNumber") + .IsRequired() + .HasColumnType("text"); + + b.Property("RefinedScope") + .HasColumnType("text"); + + b.Property("ScopeOfWork") + .IsRequired() + .HasColumnType("text"); + + b.Property("SentAt") + .HasColumnType("timestamp with time zone"); + + b.Property("ServiceCategory") + .IsRequired() + .HasColumnType("text"); + + b.Property("Status") + .IsRequired() + .HasColumnType("text"); + + b.Property("SubmittedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("SubmittedById") + .HasColumnType("uuid"); + + b.Property("TotalBidAmount") + .HasPrecision(18, 2) + .HasColumnType("numeric(18,2)"); + + b.Property("UpdatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("VendorTotalCost") + .HasPrecision(18, 2) + .HasColumnType("numeric(18,2)"); + + b.Property("Version") + .IsConcurrencyToken() + .ValueGeneratedOnAdd() + .HasColumnType("bigint") + .HasDefaultValue(1L); + + b.Property("WorkOrderNumber") + .IsRequired() + .HasColumnType("text"); + + b.HasKey("Id"); + + b.HasIndex("ApprovedById"); + + b.HasIndex("AssignedAdminId"); + + b.HasIndex("ParentProposalId"); + + b.HasIndex("ProposalNumber") + .IsUnique(); + + b.HasIndex("SubmittedById"); + + b.ToTable("Proposals"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.SimilarProposalReference", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("ProposalId") + .HasColumnType("uuid"); + + b.Property("ReferencedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("ReferencedById") + .HasColumnType("uuid"); + + b.Property("ReferencedLibraryItemId") + .IsRequired() + .HasColumnType("text"); + + b.Property("SimilarityScore") + .HasColumnType("real"); + + b.HasKey("Id"); + + b.HasIndex("ProposalId"); + + b.HasIndex("ReferencedById"); + + b.ToTable("SimilarProposalReferences"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.User", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("CognitoSub") + .IsRequired() + .HasColumnType("text"); + + b.Property("CreatedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("DisplayName") + .IsRequired() + .HasColumnType("text"); + + b.Property("Email") + .IsRequired() + .HasColumnType("text"); + + b.Property("IsActive") + .HasColumnType("boolean"); + + b.Property("Role") + .IsRequired() + .HasColumnType("text"); + + b.Property("UpdatedAt") + .HasColumnType("timestamp with time zone"); + + b.HasKey("Id"); + + b.HasIndex("CognitoSub") + .IsUnique(); + + b.HasIndex("Email") + .IsUnique(); + + b.ToTable("Users"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.VendorProposal", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("ExtractedData") + .HasColumnType("jsonb"); + + b.Property("FileName") + .IsRequired() + .HasColumnType("text"); + + b.Property("ProcessingStatus") + .IsRequired() + .HasColumnType("text"); + + b.Property("ProposalId") + .HasColumnType("uuid"); + + b.Property("S3Key") + .IsRequired() + .HasColumnType("text"); + + b.Property("TotalVendorCost") + .HasPrecision(18, 2) + .HasColumnType("numeric(18,2)"); + + b.Property("UploadedAt") + .HasColumnType("timestamp with time zone"); + + b.Property("VendorName") + .IsRequired() + .HasColumnType("text"); + + b.HasKey("Id"); + + b.HasIndex("ProposalId"); + + b.ToTable("VendorProposals"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.AuditLog", b => + { + b.HasOne("ProposalSystem.Domain.Entities.Proposal", "Proposal") + .WithMany() + .HasForeignKey("ProposalId"); + + b.HasOne("ProposalSystem.Domain.Entities.User", "User") + .WithMany() + .HasForeignKey("UserId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Proposal"); + + b.Navigation("User"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.GeneratedPdf", b => + { + b.HasOne("ProposalSystem.Domain.Entities.User", "GeneratedBy") + .WithMany() + .HasForeignKey("GeneratedById") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("ProposalSystem.Domain.Entities.Proposal", "Proposal") + .WithMany() + .HasForeignKey("ProposalId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("GeneratedBy"); + + b.Navigation("Proposal"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.LineItem", b => + { + b.HasOne("ProposalSystem.Domain.Entities.Proposal", "Proposal") + .WithMany("LineItems") + .HasForeignKey("ProposalId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Proposal"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.Proposal", b => + { + b.HasOne("ProposalSystem.Domain.Entities.User", "ApprovedBy") + .WithMany() + .HasForeignKey("ApprovedById") + .OnDelete(DeleteBehavior.SetNull); + + b.HasOne("ProposalSystem.Domain.Entities.User", "AssignedAdmin") + .WithMany() + .HasForeignKey("AssignedAdminId") + .OnDelete(DeleteBehavior.SetNull); + + b.HasOne("ProposalSystem.Domain.Entities.Proposal", "ParentProposal") + .WithMany() + .HasForeignKey("ParentProposalId") + .OnDelete(DeleteBehavior.SetNull); + + b.HasOne("ProposalSystem.Domain.Entities.User", "SubmittedBy") + .WithMany() + .HasForeignKey("SubmittedById") + .OnDelete(DeleteBehavior.Restrict) + .IsRequired(); + + b.Navigation("ApprovedBy"); + + b.Navigation("AssignedAdmin"); + + b.Navigation("ParentProposal"); + + b.Navigation("SubmittedBy"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.SimilarProposalReference", b => + { + b.HasOne("ProposalSystem.Domain.Entities.Proposal", "Proposal") + .WithMany() + .HasForeignKey("ProposalId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("ProposalSystem.Domain.Entities.User", "ReferencedBy") + .WithMany() + .HasForeignKey("ReferencedById") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Proposal"); + + b.Navigation("ReferencedBy"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.VendorProposal", b => + { + b.HasOne("ProposalSystem.Domain.Entities.Proposal", "Proposal") + .WithMany("VendorProposals") + .HasForeignKey("ProposalId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Proposal"); + }); + + modelBuilder.Entity("ProposalSystem.Domain.Entities.Proposal", b => + { + b.Navigation("LineItems"); + + b.Navigation("VendorProposals"); + }); +#pragma warning restore 612, 618 + } + } +} diff --git a/api/src/ProposalSystem.Infrastructure/Data/Migrations/20260714003834_AddProposalLineItemVersion.cs b/api/src/ProposalSystem.Infrastructure/Data/Migrations/20260714003834_AddProposalLineItemVersion.cs new file mode 100644 index 0000000..2e38f96 --- /dev/null +++ b/api/src/ProposalSystem.Infrastructure/Data/Migrations/20260714003834_AddProposalLineItemVersion.cs @@ -0,0 +1,40 @@ +using Microsoft.EntityFrameworkCore.Migrations; + +#nullable disable + +namespace ProposalSystem.Infrastructure.Data.Migrations +{ + /// + public partial class AddProposalLineItemVersion : Migration + { + /// + protected override void Up(MigrationBuilder migrationBuilder) + { + migrationBuilder.AddColumn( + name: "Version", + table: "Proposals", + type: "bigint", + nullable: false, + defaultValue: 1L); + + migrationBuilder.AddColumn( + name: "Version", + table: "LineItems", + type: "bigint", + nullable: false, + defaultValue: 1L); + } + + /// + protected override void Down(MigrationBuilder migrationBuilder) + { + migrationBuilder.DropColumn( + name: "Version", + table: "Proposals"); + + migrationBuilder.DropColumn( + name: "Version", + table: "LineItems"); + } + } +} diff --git a/api/src/ProposalSystem.Infrastructure/Data/Migrations/ProposalDbContextModelSnapshot.cs b/api/src/ProposalSystem.Infrastructure/Data/Migrations/ProposalDbContextModelSnapshot.cs index 0aeb2a3..d1eed98 100644 --- a/api/src/ProposalSystem.Infrastructure/Data/Migrations/ProposalDbContextModelSnapshot.cs +++ b/api/src/ProposalSystem.Infrastructure/Data/Migrations/ProposalDbContextModelSnapshot.cs @@ -17,7 +17,7 @@ namespace ProposalSystem.Infrastructure.Data.Migrations { #pragma warning disable 612, 618 modelBuilder - .HasAnnotation("ProductVersion", "8.0.27") + .HasAnnotation("ProductVersion", "8.0.28") .HasAnnotation("Relational:MaxIdentifierLength", 63); NpgsqlModelBuilderExtensions.UseIdentityByDefaultColumns(modelBuilder); @@ -162,6 +162,12 @@ namespace ProposalSystem.Infrastructure.Data.Migrations b.Property("UpdatedAt") .HasColumnType("timestamp with time zone"); + b.Property("Version") + .IsConcurrencyToken() + .ValueGeneratedOnAdd() + .HasColumnType("bigint") + .HasDefaultValue(1L); + b.HasKey("Id"); b.HasIndex("ProposalId"); @@ -293,6 +299,12 @@ namespace ProposalSystem.Infrastructure.Data.Migrations .HasPrecision(18, 2) .HasColumnType("numeric(18,2)"); + b.Property("Version") + .IsConcurrencyToken() + .ValueGeneratedOnAdd() + .HasColumnType("bigint") + .HasDefaultValue(1L); + b.Property("WorkOrderNumber") .IsRequired() .HasColumnType("text"); diff --git a/api/src/ProposalSystem.Infrastructure/Data/ProposalDbContext.cs b/api/src/ProposalSystem.Infrastructure/Data/ProposalDbContext.cs index bc7aaf9..f10b795 100644 --- a/api/src/ProposalSystem.Infrastructure/Data/ProposalDbContext.cs +++ b/api/src/ProposalSystem.Infrastructure/Data/ProposalDbContext.cs @@ -30,6 +30,7 @@ public class ProposalDbContext : DbContext entity.Property(e => e.Status).HasConversion(); entity.Property(e => e.ServiceCategory).HasConversion(); entity.Property(e => e.Priority).HasConversion(); + entity.Property(e => e.Version).IsConcurrencyToken().HasDefaultValue(1L); entity.HasOne(e => e.SubmittedBy).WithMany().HasForeignKey(e => e.SubmittedById).OnDelete(DeleteBehavior.Restrict); entity.HasOne(e => e.AssignedAdmin).WithMany().HasForeignKey(e => e.AssignedAdminId).OnDelete(DeleteBehavior.SetNull); @@ -45,6 +46,7 @@ public class ProposalDbContext : DbContext entity.Property(e => e.TotalPrice).HasPrecision(18, 2); entity.Property(e => e.PricingMode).HasConversion(); entity.Property(e => e.Source).HasConversion(); + entity.Property(e => e.Version).IsConcurrencyToken().HasDefaultValue(1L); entity.HasOne(e => e.Proposal).WithMany(p => p.LineItems).HasForeignKey(e => e.ProposalId).OnDelete(DeleteBehavior.Cascade); }); diff --git a/api/src/ProposalSystem.Infrastructure/Services/AuditService.cs b/api/src/ProposalSystem.Infrastructure/Services/AuditService.cs index b00dc02..ba7d25a 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/AuditService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/AuditService.cs @@ -17,6 +17,17 @@ public class AuditService : IAuditService } public async Task LogAsync(AuditAction action, Guid? proposalId, string? details = null, CancellationToken ct = default) + { + Stage(action, proposalId, details); + await _db.SaveChangesAsync(ct); + } + + public void Stage(AuditAction action, Guid? proposalId, string? details = null) + { + _db.AuditLogs.Add(BuildEntry(action, proposalId, details)); + } + + private AuditLog BuildEntry(AuditAction action, Guid? proposalId, string? details) { // Fix: API-M12 — accept pre-serialized JSON from callers that provide structured audit data. // If the details string is already valid JSON (starts with '{'), use it directly; @@ -31,7 +42,7 @@ public class AuditService : IAuditService jsonDetails = JsonSerializer.Serialize(new { message = details }); } - var entry = new AuditLog + return new AuditLog { Id = Guid.NewGuid(), ProposalId = proposalId, @@ -41,8 +52,5 @@ public class AuditService : IAuditService Timestamp = DateTime.UtcNow, IpAddress = _currentUser.IpAddress, }; - - _db.AuditLogs.Add(entry); - await _db.SaveChangesAsync(ct); } } diff --git a/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs b/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs index b2f4a9d..ca9dad7 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs @@ -1,6 +1,7 @@ using System.Text.Json; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; +using ProposalSystem.Application.Common; using ProposalSystem.Application.DTOs; using ProposalSystem.Application.Interfaces; using ProposalSystem.Domain.Entities; @@ -62,31 +63,36 @@ public class LineItemService : ILineItemService }; _db.LineItems.Add(lineItem); - await _db.SaveChangesAsync(ct); + + // Appends are not token-guarded (SHOC precedent: creates are unguarded), + // but they still bump the aggregate version so concurrent guarded edits + // observe the change and clients refresh their token. + proposal.Version++; + proposal.UpdatedAt = now; + + // Fix: API-M12 — capture line item details in audit trail + var addAuditDetails = JsonSerializer.Serialize(new + { + action = "add", + lineItem = new { description = request.Description, quantity = request.Quantity, unit = request.Unit, totalPrice = request.TotalPrice } + }); + _audit.Stage(AuditAction.EditLineItem, proposalId, addAuditDetails); + await SaveGuardedAsync(proposalId, ct); _logger.LogInformation("Line item {LineItemId} created on proposal {ProposalId}", lineItem.Id, proposalId); - try - { - // Fix: API-M12 — capture line item details in audit trail - var addAuditDetails = JsonSerializer.Serialize(new - { - action = "add", - lineItem = new { description = request.Description, quantity = request.Quantity, unit = request.Unit, totalPrice = request.TotalPrice } - }); - await _audit.LogAsync(AuditAction.EditLineItem, proposalId, addAuditDetails, ct); - } - catch (Exception ex) - { - _logger.LogError(ex, "Failed to write audit log for line item creation on proposal {ProposalId}", proposalId); - } - return MapToResponse(lineItem); } public async Task> BulkUpdateAsync(Guid proposalId, BulkUpdateLineItemsRequest request, CancellationToken ct = default) { - var proposal = await _db.Proposals.FindAsync(new object[] { proposalId }, ct) + // Loaded with the display navigations so a pre-check conflict's + // currentState matches the DB-race reload shape (no nulled names). + var proposal = await _db.Proposals + .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) + .Include(p => p.ApprovedBy) + .FirstOrDefaultAsync(p => p.Id == proposalId, ct) ?? throw new KeyNotFoundException($"Proposal {proposalId} not found"); if (proposal.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent) @@ -95,6 +101,11 @@ public class LineItemService : ILineItemService throw new InvalidOperationException("Cannot modify line items on approved/sent proposals"); } + // Bulk update replaces the line-item set wholesale, so the guard is the + // PROPOSAL token: any concurrent change to the aggregate (fields or items) + // bumped it, and a stale replace would silently discard those edits. + GuardProposalVersion(proposal, request.ProposalVersion); + await using var transaction = await _db.Database.BeginTransactionAsync(ct); try { @@ -126,24 +137,17 @@ public class LineItemService : ILineItemService proposal.TotalBidAmount = newItems.Sum(li => li.TotalPrice); proposal.UpdatedAt = now; - await _db.SaveChangesAsync(ct); - await transaction.CommitAsync(ct); + // Fix: API-M12 — capture before/after item counts in audit trail; + // staged so it commits atomically with the replace. + var bulkAuditDetails = JsonSerializer.Serialize(new + { + action = "bulkUpdate", + itemCount = new { old = existing.Count, @new = newItems.Count } + }); + _audit.Stage(AuditAction.EditLineItem, proposalId, bulkAuditDetails); - try - { - // Fix: API-M12 — capture before/after item counts in audit trail - var bulkAuditDetails = JsonSerializer.Serialize(new - { - action = "bulkUpdate", - itemCount = new { old = existing.Count, @new = newItems.Count } - }); - await _audit.LogAsync(AuditAction.EditLineItem, proposalId, bulkAuditDetails, ct); - } - catch (Exception ex) - { - // Fix: API-M11 — log audit failures instead of silently swallowing - _logger.LogError(ex, "Failed to write audit log for bulk update on proposal {ProposalId}", proposalId); - } + await SaveGuardedAsync(proposalId, ct); + await transaction.CommitAsync(ct); return newItems.OrderBy(li => li.SortOrder).Select(MapToResponse).ToList(); } @@ -168,9 +172,11 @@ public class LineItemService : ILineItemService } _db.LineItems.Remove(lineItem); - await _db.SaveChangesAsync(ct); - _logger.LogInformation("Line item {LineItemId} deleted from proposal {ProposalId}", lineItemId, proposalId); + // Deletes are not token-guarded (SHOC precedent) but bump the aggregate + // version so concurrent guarded edits conflict instead of losing the delete. + proposal.Version++; + proposal.UpdatedAt = DateTime.UtcNow; // Fix: API-M12 — capture deleted line item details in audit trail var deleteAuditDetails = JsonSerializer.Serialize(new @@ -178,9 +184,18 @@ public class LineItemService : ILineItemService action = "delete", lineItem = new { id = lineItemId, description = lineItem.Description, quantity = lineItem.Quantity, unit = lineItem.Unit, totalPrice = lineItem.TotalPrice } }); - await _audit.LogAsync(AuditAction.EditLineItem, proposalId, deleteAuditDetails, ct); + _audit.Stage(AuditAction.EditLineItem, proposalId, deleteAuditDetails); + await SaveGuardedAsync(proposalId, ct); + + _logger.LogInformation("Line item {LineItemId} deleted from proposal {ProposalId}", lineItemId, proposalId); } + private void GuardProposalVersion(Proposal proposal, string? proposalVersion) => + ProposalConcurrencyGuard.Guard(_db, proposal, proposalVersion); + + private Task SaveGuardedAsync(Guid proposalId, CancellationToken ct) => + ProposalConcurrencyGuard.SaveAsync(_db, proposalId, ct); + private static LineItemResponse MapToResponse(LineItem li) => new( li.Id, li.ProposalId, @@ -193,6 +208,7 @@ public class LineItemService : ILineItemService li.SortOrder, li.Source, li.CreatedAt, - li.UpdatedAt + li.UpdatedAt, + RowVersionCodec.Encode(li.Version) ); } diff --git a/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs b/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs new file mode 100644 index 0000000..79c30db --- /dev/null +++ b/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs @@ -0,0 +1,63 @@ +using Microsoft.EntityFrameworkCore; +using ProposalSystem.Application.Common; +using ProposalSystem.Application.DTOs; +using ProposalSystem.Domain.Entities; +using ProposalSystem.Infrastructure.Data; + +namespace ProposalSystem.Infrastructure.Services; + +/// +/// SHOC double-guard for the proposal aggregate (see ADR 0004). +/// Step 1 (Guard): validate the client token against the freshly loaded row and +/// stamp it as EF's original value so the UPDATE's WHERE clause re-enforces it +/// at the database. Step 2 (SaveAsync): on a lost race, reload the current +/// state and throw the conflict exception that becomes the 409 body. +/// +/// CALLER CONTRACT (AUTHZ-CG-01): the 409 currentState embeds the full +/// ProposalResponse with NO ownership filtering (unlike GetByIdAsync's +/// dispatcher check). Every endpoint that can reach Guard/SaveAsync MUST be +/// [Authorize(Roles = "admins,sysadmins")]. Enforced by +/// GuardedEndpointAuthorizationTests — extend the guard with an ownership +/// predicate before ever wiring it to a dispatcher-reachable write. +/// +internal static class ProposalConcurrencyGuard +{ + internal static void Guard(ProposalDbContext db, Proposal proposal, string? proposalVersion) + { + if (string.IsNullOrEmpty(proposalVersion)) + throw new BusinessRuleException("ProposalVersionRequired", "proposalVersion is required for this operation."); + if (!RowVersionCodec.TryDecode(proposalVersion, out var expected)) + throw new BusinessRuleException("InvalidRowVersion", "proposalVersion is not a valid version token."); + if (proposal.Version != expected) + throw new ProposalConcurrencyException(ProposalMapper.ToResponse(proposal)); + + db.Entry(proposal).Property(p => p.Version).OriginalValue = expected; + proposal.Version++; + } + + internal static async Task SaveAsync(ProposalDbContext db, Guid proposalId, CancellationToken ct) + { + try + { + await db.SaveChangesAsync(ct); + } + catch (DbUpdateConcurrencyException) + { + db.ChangeTracker.Clear(); + var currentState = await LoadCurrentStateAsync(db, proposalId, ct); + throw new ProposalConcurrencyException(currentState); + } + } + + internal static async Task LoadCurrentStateAsync( + ProposalDbContext db, Guid proposalId, CancellationToken ct) + { + var current = await db.Proposals + .AsNoTracking() + .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) + .Include(p => p.ApprovedBy) + .FirstOrDefaultAsync(p => p.Id == proposalId, ct); + return current is null ? null : ProposalMapper.ToResponse(current); + } +} diff --git a/api/src/ProposalSystem.Infrastructure/Services/ProposalMapper.cs b/api/src/ProposalSystem.Infrastructure/Services/ProposalMapper.cs new file mode 100644 index 0000000..b34cfa0 --- /dev/null +++ b/api/src/ProposalSystem.Infrastructure/Services/ProposalMapper.cs @@ -0,0 +1,39 @@ +using ProposalSystem.Application.Common; +using ProposalSystem.Application.DTOs; +using ProposalSystem.Domain.Entities; + +namespace ProposalSystem.Infrastructure.Services; + +/// Canonical Proposal → ProposalResponse mapping, shared by the +/// proposal/line-item services and the concurrency guard's currentState reload. +internal static class ProposalMapper +{ + internal static ProposalResponse ToResponse(Proposal p) => new( + p.Id, + p.ProposalNumber, + p.WorkOrderNumber, + p.PoNumber, + p.CustomerName, + p.CustomerAddress, + p.ScopeOfWork, + p.RefinedScope, + p.ServiceCategory, + p.Priority, + p.Status, + p.TotalBidAmount, + p.VendorTotalCost, + p.Notes, + p.SubmittedById, + p.SubmittedBy?.DisplayName, + p.SubmittedAt, + p.AssignedAdminId, + p.ApprovedById, + p.ApprovedAt, + p.SentAt, + p.CurrentRevision, + p.ParentProposalId, + p.CreatedAt, + p.UpdatedAt, + RowVersionCodec.Encode(p.Version) + ); +} diff --git a/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs b/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs index a0448f7..94656bb 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs @@ -2,6 +2,7 @@ using System.Text.Json; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; +using ProposalSystem.Application.Common; using ProposalSystem.Application.DTOs; using ProposalSystem.Application.Interfaces; using ProposalSystem.Domain.Entities; @@ -71,21 +72,13 @@ public class ProposalService : IProposalService }; _db.Proposals.Add(proposal); + _audit.Stage(AuditAction.Submit, proposal.Id); await _db.SaveChangesAsync(ct); await transaction.CommitAsync(ct); _logger.LogInformation("Proposal {ProposalId} created with number {ProposalNumber} by user {UserId}", proposal.Id, proposalNumber, _currentUser.UserId); - try - { - await _audit.LogAsync(AuditAction.Submit, proposal.Id, null, ct); - } - catch (Exception ex) - { - _logger.LogError(ex, "Failed to write audit log for proposal submission {ProposalId}", proposal.Id); - } - try { await _jobPublisher.PublishAsync("suggestions", new { proposalId = proposal.Id, trigger = "generate" }, ct); @@ -173,7 +166,8 @@ public class ProposalService : IProposalService p.TotalBidAmount, p.SubmittedAt, p.SubmittedBy != null ? p.SubmittedBy.DisplayName : null, - p.AssignedAdmin != null ? p.AssignedAdmin.DisplayName : null + p.AssignedAdmin != null ? p.AssignedAdmin.DisplayName : null, + RowVersionCodec.Encode(p.Version) )) .ToListAsync(ct); @@ -189,6 +183,8 @@ public class ProposalService : IProposalService .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); + GuardProposalVersion(proposal, request.ProposalVersion); + // Fix: API-M12 — capture before/after values for audit trail var changes = new Dictionary(); @@ -223,19 +219,21 @@ public class ProposalService : IProposalService } proposal.UpdatedAt = DateTime.UtcNow; - await _db.SaveChangesAsync(ct); + // Stage-then-single-SaveChanges: the audit row commits atomically with the mutation. var auditDetails = changes.Count > 0 ? JsonSerializer.Serialize(changes) : null; - await _audit.LogAsync(AuditAction.Edit, id, auditDetails, ct); + _audit.Stage(AuditAction.Edit, id, auditDetails); + await SaveGuardedAsync(id, ct); return MapToResponse(proposal); } - public async Task ApproveAsync(Guid id, CancellationToken ct = default) + public async Task ApproveAsync(Guid id, string? proposalVersion, CancellationToken ct = default) { var proposal = await _db.Proposals .Include(p => p.LineItems) .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -259,6 +257,8 @@ public class ProposalService : IProposalService throw new InvalidOperationException("Cannot approve proposal without priced line items"); } + GuardProposalVersion(proposal, proposalVersion); + // Fix: API-M12 — capture before/after status for audit trail var previousStatus = proposal.Status; proposal.Status = ProposalStatus.Approved; @@ -267,9 +267,9 @@ public class ProposalService : IProposalService proposal.TotalBidAmount = proposal.LineItems.Sum(li => li.TotalPrice); proposal.UpdatedAt = DateTime.UtcNow; - await _db.SaveChangesAsync(ct); var approveAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousStatus.ToString(), @new = ProposalStatus.Approved.ToString() } }); - await _audit.LogAsync(AuditAction.Approve, id, approveAuditDetails, ct); + _audit.Stage(AuditAction.Approve, id, approveAuditDetails); + await SaveGuardedAsync(id, ct); _logger.LogInformation("Proposal {ProposalId} approved by user {UserId}, total bid {TotalBidAmount}", id, _currentUser.UserId, proposal.TotalBidAmount); @@ -277,10 +277,11 @@ public class ProposalService : IProposalService return MapToResponse(proposal); } - public async Task ReturnToReviewAsync(Guid id, CancellationToken ct = default) + public async Task ReturnToReviewAsync(Guid id, string? proposalVersion, CancellationToken ct = default) { var proposal = await _db.Proposals .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -291,6 +292,8 @@ public class ProposalService : IProposalService if (proposal.Status != ProposalStatus.Approved) throw new InvalidOperationException("Only approved proposals can be returned to review"); + GuardProposalVersion(proposal, proposalVersion); + // Fix: API-M12 — capture before/after status for audit trail var previousStatus = proposal.Status; proposal.Status = ProposalStatus.InReview; @@ -298,17 +301,18 @@ public class ProposalService : IProposalService proposal.ApprovedAt = null; proposal.UpdatedAt = DateTime.UtcNow; - await _db.SaveChangesAsync(ct); var returnAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousStatus.ToString(), @new = ProposalStatus.InReview.ToString() } }); - await _audit.LogAsync(AuditAction.ReturnToReview, id, returnAuditDetails, ct); + _audit.Stage(AuditAction.ReturnToReview, id, returnAuditDetails); + await SaveGuardedAsync(id, ct); return MapToResponse(proposal); } - public async Task MarkSentAsync(Guid id, CancellationToken ct = default) + public async Task MarkSentAsync(Guid id, string? proposalVersion, CancellationToken ct = default) { var proposal = await _db.Proposals .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -326,15 +330,17 @@ public class ProposalService : IProposalService throw new InvalidOperationException("Only approved proposals can be marked as sent"); } + GuardProposalVersion(proposal, proposalVersion); + // Fix: API-M12 — capture before/after status for audit trail var previousStatus = proposal.Status; proposal.Status = ProposalStatus.Sent; proposal.SentAt = DateTime.UtcNow; proposal.UpdatedAt = DateTime.UtcNow; - await _db.SaveChangesAsync(ct); var sentAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousStatus.ToString(), @new = ProposalStatus.Sent.ToString() } }); - await _audit.LogAsync(AuditAction.MarkSent, id, sentAuditDetails, ct); + _audit.Stage(AuditAction.MarkSent, id, sentAuditDetails); + await SaveGuardedAsync(id, ct); _logger.LogInformation("Proposal {ProposalId} marked as sent by user {UserId}", id, _currentUser.UserId); @@ -424,10 +430,13 @@ public class ProposalService : IProposalService } } - public async Task ReviseAsync(Guid id, CancellationToken ct = default) + public async Task ReviseAsync(Guid id, string? proposalVersion, CancellationToken ct = default) { var proposal = await _db.Proposals .Include(p => p.LineItems) + .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) + .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -450,6 +459,8 @@ public class ProposalService : IProposalService throw new InvalidOperationException("Only sent proposals can be revised"); } + GuardProposalVersion(proposal, proposalVersion); + var revision = new Proposal { Id = Guid.NewGuid(), @@ -499,14 +510,13 @@ public class ProposalService : IProposalService proposal.UpdatedAt = DateTime.UtcNow; _db.Proposals.Add(revision); - await _db.SaveChangesAsync(ct); - var reviseAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousReviseStatus.ToString(), @new = ProposalStatus.Revised.ToString() }, message = $"Revised from {proposal.Id}" }); - await _audit.LogAsync(AuditAction.CreateRevision, revision.Id, reviseAuditDetails, ct); + _audit.Stage(AuditAction.CreateRevision, revision.Id, reviseAuditDetails); + await SaveGuardedAsync(id, ct); _logger.LogInformation("Proposal {ProposalId} revised to {RevisionId} (revision {RevisionNumber}) by user {UserId}", id, revision.Id, revision.CurrentRevision, _currentUser.UserId); @@ -576,31 +586,11 @@ public class ProposalService : IProposalService : new ProposalStatsResponse(counts.Total, counts.InReview, counts.Approved, counts.Sent); } - private static ProposalResponse MapToResponse(Proposal p) => new( - p.Id, - p.ProposalNumber, - p.WorkOrderNumber, - p.PoNumber, - p.CustomerName, - p.CustomerAddress, - p.ScopeOfWork, - p.RefinedScope, - p.ServiceCategory, - p.Priority, - p.Status, - p.TotalBidAmount, - p.VendorTotalCost, - p.Notes, - p.SubmittedById, - p.SubmittedBy?.DisplayName, - p.SubmittedAt, - p.AssignedAdminId, - p.ApprovedById, - p.ApprovedAt, - p.SentAt, - p.CurrentRevision, - p.ParentProposalId, - p.CreatedAt, - p.UpdatedAt - ); + private void GuardProposalVersion(Proposal proposal, string? proposalVersion) => + ProposalConcurrencyGuard.Guard(_db, proposal, proposalVersion); + + private Task SaveGuardedAsync(Guid proposalId, CancellationToken ct) => + ProposalConcurrencyGuard.SaveAsync(_db, proposalId, ct); + + private static ProposalResponse MapToResponse(Proposal p) => ProposalMapper.ToResponse(p); } diff --git a/api/tests/ProposalSystem.Tests/Common/RowVersionCodecTests.cs b/api/tests/ProposalSystem.Tests/Common/RowVersionCodecTests.cs new file mode 100644 index 0000000..808b08d --- /dev/null +++ b/api/tests/ProposalSystem.Tests/Common/RowVersionCodecTests.cs @@ -0,0 +1,37 @@ +using FluentAssertions; +using ProposalSystem.Application.Common; +using Xunit; + +namespace ProposalSystem.Tests.Common; + +public class RowVersionCodecTests +{ + [Theory(DisplayName = "Encode/TryDecode round-trips")] + [InlineData(1L)] + [InlineData(2L)] + [InlineData(long.MaxValue)] + public void RoundTrips(long version) + { + var token = RowVersionCodec.Encode(version); + + RowVersionCodec.TryDecode(token, out var decoded).Should().BeTrue(); + decoded.Should().Be(version); + } + + [Fact(DisplayName = "Version 1 encodes to the SHOC-style opaque token")] + public void EncodesOpaqueBase64() + { + RowVersionCodec.Encode(1).Should().Be("AAAAAAAAAAE="); + } + + [Theory(DisplayName = "TryDecode rejects missing/malformed/wrong-length tokens")] + [InlineData(null)] + [InlineData("")] + [InlineData("not base64!!")] + [InlineData("AAA=")] // valid base64 but not 8 bytes + [InlineData("AAAAAAAAAAAAAAAAAAAAAA==")] // 16 bytes + public void RejectsInvalid(string? token) + { + RowVersionCodec.TryDecode(token, out _).Should().BeFalse(); + } +} diff --git a/api/tests/ProposalSystem.Tests/Controllers/GuardedEndpointAuthorizationTests.cs b/api/tests/ProposalSystem.Tests/Controllers/GuardedEndpointAuthorizationTests.cs new file mode 100644 index 0000000..3361694 --- /dev/null +++ b/api/tests/ProposalSystem.Tests/Controllers/GuardedEndpointAuthorizationTests.cs @@ -0,0 +1,44 @@ +using System.Reflection; +using FluentAssertions; +using Microsoft.AspNetCore.Authorization; +using ProposalSystem.Api.Controllers; +using Xunit; + +namespace ProposalSystem.Tests.Controllers; + +/// +/// AUTHZ-CG-01 tripwire: ProposalConcurrencyGuard's 409 currentState embeds the +/// full ProposalResponse with no ownership filtering, so every endpoint that can +/// reach the guard must stay admin-gated. If a new/changed endpoint wires a +/// guarded mutation to a dispatcher-reachable route, this test fails the build +/// until the guard gains an ownership predicate. +/// +public class GuardedEndpointAuthorizationTests +{ + public static readonly TheoryData GuardedEndpoints = new() + { + { typeof(ProposalsController), "Update" }, + { typeof(ProposalsController), "Approve" }, + { typeof(ProposalsController), "ReturnToReview" }, + { typeof(ProposalsController), "MarkSent" }, + { typeof(ProposalsController), "Revise" }, + { typeof(LineItemsController), "BulkUpdate" }, + }; + + [Theory(DisplayName = "Guard-reaching endpoints require admins/sysadmins")] + [MemberData(nameof(GuardedEndpoints))] + public void GuardedEndpoint_RequiresAdminRole(Type controller, string actionName) + { + var action = controller.GetMethod(actionName, BindingFlags.Public | BindingFlags.Instance); + action.Should().NotBeNull($"{controller.Name}.{actionName} should exist — update GuardedEndpoints if renamed"); + + var authorize = action!.GetCustomAttributes(inherit: true) + .FirstOrDefault(a => a.Roles != null); + + authorize.Should().NotBeNull( + $"{controller.Name}.{actionName} reaches ProposalConcurrencyGuard and must carry a role-restricted [Authorize]"); + authorize!.Roles.Should().Contain("admins").And.Contain("sysadmins"); + authorize.Roles.Should().NotContain("dispatchers", + "the guard's 409 currentState has no ownership filter — see ProposalConcurrencyGuard caller contract"); + } +} diff --git a/api/tests/ProposalSystem.Tests/Helpers/SqliteDbContextFactory.cs b/api/tests/ProposalSystem.Tests/Helpers/SqliteDbContextFactory.cs index 6177b97..edeaa8f 100644 --- a/api/tests/ProposalSystem.Tests/Helpers/SqliteDbContextFactory.cs +++ b/api/tests/ProposalSystem.Tests/Helpers/SqliteDbContextFactory.cs @@ -37,6 +37,29 @@ public static class SqliteDbContextFactory return context; } + /// + /// Opens a shared in-memory database and returns the connection plus a factory + /// for additional contexts over the SAME database — needed by optimistic- + /// concurrency tests that simulate two competing writers. Callers dispose the + /// connection after the contexts. + /// + public static (SqliteConnection Connection, Func ContextFactory) CreateShared() + { + var connection = new SqliteConnection("DataSource=:memory:"); + connection.Open(); + RegisterPostgresStubs(connection); + + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .Options; + + var first = new ProposalDbContext(options); + first.Database.EnsureCreated(); + first.Dispose(); + + return (connection, () => new ProposalDbContext(options)); + } + private static void RegisterPostgresStubs(SqliteConnection connection) { // hashtext(text) -> integer — returns a constant; only needed so SQL parses diff --git a/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs b/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs index 78c5c04..5de457b 100644 --- a/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs +++ b/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs @@ -93,4 +93,61 @@ public class GlobalExceptionHandlerTests context.Response.ContentType.Should().Be("application/problem+json"); } + + // ── Concurrency 409 envelopes (ADR 0004) ───────────────────────────────── + // These are SHOC's shapes verbatim, NOT ProblemDetails. Both bodies are wire + // contract: web/mobile branch on message + currentState / status + code. + + private static ProposalSystem.Application.DTOs.ProposalResponse SampleState(Guid id) => new( + id, "P-001", "WO-1", null, "Customer", "Addr", "Scope", null, + ProposalSystem.Domain.Entities.ServiceCategory.HVAC, + ProposalSystem.Domain.Entities.Priority.Standard, + ProposalSystem.Domain.Entities.ProposalStatus.InReview, + 100m, null, "winner", Guid.NewGuid(), "Submitter", DateTime.UtcNow, + null, null, null, null, 1, null, DateTime.UtcNow, DateTime.UtcNow, + "AAAAAAAAAAI="); + + [Fact(DisplayName = "ProposalConcurrencyException maps to 409 { message, currentState } (SHOC envelope)")] + public async Task ConcurrencyException_Maps409WithCurrentState() + { + var id = Guid.NewGuid(); + var (status, body) = await InvokeWith(new ProposalConcurrencyException(SampleState(id))); + + status.Should().Be(409); + body.GetProperty("message").GetString() + .Should().Be("The record was modified by another user. Refresh and retry."); + body.GetProperty("currentState").GetProperty("id").GetString().Should().Be(id.ToString()); + body.GetProperty("currentState").GetProperty("notes").GetString().Should().Be("winner"); + body.GetProperty("currentState").GetProperty("rowVersion").GetString().Should().Be("AAAAAAAAAAI="); + // Enums must serialize as strings (matching the MVC pipeline), or client + // schema validation of currentState fails and the state is discarded. + body.GetProperty("currentState").GetProperty("serviceCategory").GetString().Should().Be("HVAC"); + body.GetProperty("currentState").GetProperty("status").GetString().Should().Be("InReview"); + // The guarded envelope is NOT the fallback shape. + body.TryGetProperty("status", out _).Should().BeFalse(); + body.TryGetProperty("code", out _).Should().BeFalse(); + } + + [Fact(DisplayName = "ProposalConcurrencyException with no reloadable state carries currentState: null")] + public async Task ConcurrencyException_NullState_SerializesNull() + { + var (status, body) = await InvokeWith(new ProposalConcurrencyException(null)); + + status.Should().Be(409); + body.GetProperty("currentState").ValueKind.Should().Be(JsonValueKind.Null); + } + + [Fact(DisplayName = "Bare DbUpdateConcurrencyException maps to the 409 fallback { status, message, code }")] + public async Task DbUpdateConcurrencyException_Maps409Fallback() + { + var (status, body) = await InvokeWith( + new Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException("boom")); + + status.Should().Be(409); + body.GetProperty("status").GetString().Should().Be("Conflict"); + body.GetProperty("message").GetString() + .Should().Be("The record was modified by another user. Refresh and retry."); + body.GetProperty("code").GetInt32().Should().Be(409); + body.TryGetProperty("currentState", out _).Should().BeFalse(); + } } diff --git a/api/tests/ProposalSystem.Tests/Services/LineItemServiceStateGuardTests.cs b/api/tests/ProposalSystem.Tests/Services/LineItemServiceStateGuardTests.cs index c3e068d..715eb30 100644 --- a/api/tests/ProposalSystem.Tests/Services/LineItemServiceStateGuardTests.cs +++ b/api/tests/ProposalSystem.Tests/Services/LineItemServiceStateGuardTests.cs @@ -121,7 +121,7 @@ public class LineItemServiceStateGuardTests : IDisposable }); // Act - var act = () => _sut.BulkUpdateAsync(proposal.Id, request); + var act = () => _sut.BulkUpdateAsync(proposal.Id, request with { ProposalVersion = Ver(proposal.Id) }); // Assert await act.Should().ThrowAsync() @@ -143,7 +143,7 @@ public class LineItemServiceStateGuardTests : IDisposable }); // Act - var result = await _sut.BulkUpdateAsync(proposal.Id, request); + var result = await _sut.BulkUpdateAsync(proposal.Id, request with { ProposalVersion = Ver(proposal.Id) }); // Assert result.Should().HaveCount(2); @@ -165,7 +165,7 @@ public class LineItemServiceStateGuardTests : IDisposable }); // Act - await _sut.BulkUpdateAsync(proposal.Id, request); + await _sut.BulkUpdateAsync(proposal.Id, request with { ProposalVersion = Ver(proposal.Id) }); // Assert var updated = await _db.Proposals.FindAsync(proposal.Id); @@ -199,7 +199,7 @@ public class LineItemServiceStateGuardTests : IDisposable }); // Act - var result = await _sut.BulkUpdateAsync(proposal.Id, request); + var result = await _sut.BulkUpdateAsync(proposal.Id, request with { ProposalVersion = Ver(proposal.Id) }); // Assert result.Should().HaveCount(1); @@ -214,7 +214,7 @@ public class LineItemServiceStateGuardTests : IDisposable public async Task BulkUpdateAsync_NonexistentProposal_ThrowsKeyNotFound() { var request = new BulkUpdateLineItemsRequest(new List()); - var act = () => _sut.BulkUpdateAsync(Guid.NewGuid(), request); + var act = () => _sut.BulkUpdateAsync(Guid.NewGuid(), request with { ProposalVersion = Ver(Guid.NewGuid()) }); await act.Should().ThrowAsync(); } @@ -317,11 +317,9 @@ public class LineItemServiceStateGuardTests : IDisposable await _sut.CreateAsync(proposal.Id, request); // Assert - await _audit.Received(1).LogAsync( - AuditAction.EditLineItem, + _audit.Received(1).Stage(AuditAction.EditLineItem, proposal.Id, - Arg.Is(s => s != null && s.Contains(request.Description)), - Arg.Any()); + Arg.Is(s => s != null && s.Contains(request.Description))); } [Fact(DisplayName = "QA-C2: BulkUpdateAsync logs audit event on success")] @@ -338,14 +336,12 @@ public class LineItemServiceStateGuardTests : IDisposable }); // Act - await _sut.BulkUpdateAsync(proposal.Id, request); + await _sut.BulkUpdateAsync(proposal.Id, request with { ProposalVersion = Ver(proposal.Id) }); // Assert — audit detail may be plain text ("Bulk update: N items") or JSON - await _audit.Received(1).LogAsync( - AuditAction.EditLineItem, + _audit.Received(1).Stage(AuditAction.EditLineItem, proposal.Id, - Arg.Is(s => s != null), - Arg.Any()); + Arg.Is(s => s != null)); } [Fact(DisplayName = "QA-C2: DeleteAsync logs audit event on success")] @@ -374,11 +370,9 @@ public class LineItemServiceStateGuardTests : IDisposable await _sut.DeleteAsync(proposal.Id, lineItem.Id); // Assert - await _audit.Received(1).LogAsync( - AuditAction.EditLineItem, + _audit.Received(1).Stage(AuditAction.EditLineItem, proposal.Id, - Arg.Is(s => s != null && s.Contains("Audit test item")), - Arg.Any()); + Arg.Is(s => s != null && s.Contains("Audit test item"))); } #endregion @@ -418,4 +412,7 @@ public class LineItemServiceStateGuardTests : IDisposable Source: LineItemSource.Manual ); } + private string Ver(Guid id) => + ProposalSystem.Application.Common.RowVersionCodec.Encode(_db.Proposals.Find(id)?.Version ?? 1); + } diff --git a/api/tests/ProposalSystem.Tests/Services/ProposalConcurrencyTests.cs b/api/tests/ProposalSystem.Tests/Services/ProposalConcurrencyTests.cs new file mode 100644 index 0000000..39a92fd --- /dev/null +++ b/api/tests/ProposalSystem.Tests/Services/ProposalConcurrencyTests.cs @@ -0,0 +1,249 @@ +using FluentAssertions; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using NSubstitute; +using ProposalSystem.Application.Common; +using ProposalSystem.Application.DTOs; +using ProposalSystem.Application.Interfaces; +using ProposalSystem.Domain.Entities; +using ProposalSystem.Infrastructure.Data; +using ProposalSystem.Infrastructure.Services; +using ProposalSystem.Tests.Helpers; +using Xunit; + +namespace ProposalSystem.Tests.Services; + +/// +/// Optimistic-concurrency tests for the proposal aggregate (ADR 0004, SHOC +/// double-guard). Covers the pre-check path, both 422 token codes, the +/// DB-level race window (two writers over one SQLite database), version +/// bumping, and audit atomicity (staged rows must not survive a failed save). +/// +public class ProposalConcurrencyTests : IDisposable +{ + private readonly SqliteConnection _connection; + private readonly Func _contextFactory; + private readonly ProposalDbContext _db; + private readonly ICurrentUserService _currentUser; + private readonly ProposalService _sut; + private readonly LineItemService _lineItems; + private readonly Guid _userId = Guid.NewGuid(); + + public ProposalConcurrencyTests() + { + (_connection, _contextFactory) = SqliteDbContextFactory.CreateShared(); + _db = _contextFactory(); + + _currentUser = Substitute.For(); + _currentUser.UserId.Returns(_userId); + _currentUser.Role.Returns(UserRole.Admin); + + _db.Users.Add(new User + { + Id = _userId, + CognitoSub = $"sub-{_userId}", + Email = "admin@test.com", + DisplayName = "Test Admin", + Role = UserRole.Admin, + CreatedAt = DateTime.UtcNow, + UpdatedAt = DateTime.UtcNow, + }); + _db.SaveChanges(); + + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { { "GENERATED_BUCKET", "test-bucket" } }) + .Build(); + + // Real AuditService on the SAME context so staged-audit atomicity is exercised. + var audit = new AuditService(_db, _currentUser); + _sut = new ProposalService(_db, _currentUser, Substitute.For(), audit, + Substitute.For(), Substitute.For(), Substitute.For(), + config, Substitute.For>()); + _lineItems = new LineItemService(_db, audit, Substitute.For>()); + } + + public void Dispose() + { + _db.Dispose(); + _connection.Dispose(); + } + + private Proposal SeedProposal(ProposalStatus status = ProposalStatus.InReview) + { + var proposal = new Proposal + { + Id = Guid.NewGuid(), + ProposalNumber = $"P-{Guid.NewGuid():N}"[..12], + WorkOrderNumber = "WO-001", + CustomerName = "Test Customer", + CustomerAddress = "123 Test St", + ScopeOfWork = "Test scope of work", + ServiceCategory = ServiceCategory.HVAC, + Priority = Priority.Standard, + Status = status, + Notes = "", + SubmittedById = _userId, + SubmittedAt = DateTime.UtcNow.AddDays(-1), + CurrentRevision = 1, + CreatedAt = DateTime.UtcNow.AddDays(-1), + UpdatedAt = DateTime.UtcNow.AddDays(-1), + }; + _db.Proposals.Add(proposal); + _db.SaveChanges(); + return proposal; + } + + private static UpdateProposalRequest UpdateNotes(string notes, string? version) => + new(null, notes, null, null, null, version); + + [Fact(DisplayName = "Missing proposalVersion -> 422 ProposalVersionRequired")] + public async Task MissingVersion_ThrowsVersionRequired() + { + var proposal = SeedProposal(); + + var act = () => _sut.UpdateAsync(proposal.Id, UpdateNotes("x", null)); + + var ex = await act.Should().ThrowAsync(); + ex.Which.Code.Should().Be("ProposalVersionRequired"); + } + + [Fact(DisplayName = "Malformed proposalVersion -> 422 InvalidRowVersion")] + public async Task MalformedVersion_ThrowsInvalidRowVersion() + { + var proposal = SeedProposal(); + + var act = () => _sut.UpdateAsync(proposal.Id, UpdateNotes("x", "not-a-token!")); + + var ex = await act.Should().ThrowAsync(); + ex.Which.Code.Should().Be("InvalidRowVersion"); + } + + [Fact(DisplayName = "Stale token fails the pre-check and carries currentState")] + public async Task StaleToken_PreCheck_ThrowsWithCurrentState() + { + var proposal = SeedProposal(); + + var act = () => _sut.UpdateAsync(proposal.Id, UpdateNotes("x", RowVersionCodec.Encode(99))); + + var ex = await act.Should().ThrowAsync(); + var state = ex.Which.CurrentState.Should().BeOfType().Subject; + state.Id.Should().Be(proposal.Id); + state.RowVersion.Should().Be(RowVersionCodec.Encode(1)); + } + + [Fact(DisplayName = "DB-level race: concurrent writer wins, loser gets 409 state with the winner's values")] + public async Task LostRace_ReloadsCurrentState() + { + var proposal = SeedProposal(); + var token = RowVersionCodec.Encode(1); + + // Load into the service context first (pre-check will pass against version 1)... + var tracked = await _db.Proposals.FirstAsync(p => p.Id == proposal.Id); + tracked.Should().NotBeNull(); + + // ...then a second writer commits between the pre-check read and our save. + await using (var other = _contextFactory()) + { + var competing = await other.Proposals.FirstAsync(p => p.Id == proposal.Id); + other.Entry(competing).Property(p => p.Version).OriginalValue = 1L; + competing.Notes = "the winner's notes"; + competing.Version = 2; + await other.SaveChangesAsync(); + } + + var act = () => _sut.UpdateAsync(proposal.Id, UpdateNotes("the loser's notes", token)); + + var ex = await act.Should().ThrowAsync(); + var state = ex.Which.CurrentState.Should().BeOfType().Subject; + state.Notes.Should().Be("the winner's notes"); + state.RowVersion.Should().Be(RowVersionCodec.Encode(2)); + } + + [Fact(DisplayName = "Audit atomicity: a save lost to a concurrent writer persists no audit row")] + public async Task LostRace_PersistsNoAuditRow() + { + var proposal = SeedProposal(); + await _db.Proposals.FirstAsync(p => p.Id == proposal.Id); + + await using (var other = _contextFactory()) + { + var competing = await other.Proposals.FirstAsync(p => p.Id == proposal.Id); + competing.Notes = "winner"; + competing.Version = 2; + await other.SaveChangesAsync(); + } + + var act = () => _sut.UpdateAsync(proposal.Id, UpdateNotes("loser", RowVersionCodec.Encode(1))); + await act.Should().ThrowAsync(); + + await using var check = _contextFactory(); + (await check.AuditLogs.CountAsync()).Should().Be(0); + } + + [Fact(DisplayName = "Successful update bumps the version and commits exactly one audit row atomically")] + public async Task SuccessfulUpdate_BumpsVersionAndCommitsAudit() + { + var proposal = SeedProposal(); + + var result = await _sut.UpdateAsync(proposal.Id, UpdateNotes("new notes", RowVersionCodec.Encode(1))); + + result.RowVersion.Should().Be(RowVersionCodec.Encode(2)); + await using var check = _contextFactory(); + (await check.AuditLogs.CountAsync(a => a.ProposalId == proposal.Id)).Should().Be(1); + (await check.Proposals.SingleAsync(p => p.Id == proposal.Id)).Version.Should().Be(2); + } + + [Fact(DisplayName = "Bulk line-item update is guarded by the proposal token")] + public async Task BulkUpdate_StaleProposalToken_Conflicts() + { + var proposal = SeedProposal(); + var request = new BulkUpdateLineItemsRequest( + new List + { + new(null, "Item", 1, "each", null, 100m, PricingMode.TotalPrice, 0, LineItemSource.Manual), + }, + RowVersionCodec.Encode(42)); + + var act = () => _lineItems.BulkUpdateAsync(proposal.Id, request); + + await act.Should().ThrowAsync(); + } + + [Fact(DisplayName = "Line-item create bumps the proposal aggregate version")] + public async Task LineItemCreate_BumpsProposalVersion() + { + var proposal = SeedProposal(); + + var created = await _lineItems.CreateAsync(proposal.Id, + new CreateLineItemRequest("Item", 1, "each", null, 50m, PricingMode.TotalPrice, 0, LineItemSource.Manual)); + + created.RowVersion.Should().Be(RowVersionCodec.Encode(1)); + await using var check = _contextFactory(); + (await check.Proposals.SingleAsync(p => p.Id == proposal.Id)).Version.Should().Be(2); + } + + [Fact(DisplayName = "Approve with the current token succeeds and returns the bumped token")] + public async Task Approve_WithCurrentToken_Succeeds() + { + var proposal = SeedProposal(); + _db.LineItems.Add(new LineItem + { + Id = Guid.NewGuid(), + ProposalId = proposal.Id, + Description = "Priced item", + Quantity = 1, + Unit = "each", + TotalPrice = 500m, + PricingMode = PricingMode.TotalPrice, + Source = LineItemSource.Manual, + }); + _db.SaveChanges(); + + var result = await _sut.ApproveAsync(proposal.Id, RowVersionCodec.Encode(1)); + + result.Status.Should().Be(ProposalStatus.Approved); + result.RowVersion.Should().Be(RowVersionCodec.Encode(2)); + } +} diff --git a/api/tests/ProposalSystem.Tests/Services/ProposalDeliveryTests.cs b/api/tests/ProposalSystem.Tests/Services/ProposalDeliveryTests.cs index 157dac2..bd23b61 100644 --- a/api/tests/ProposalSystem.Tests/Services/ProposalDeliveryTests.cs +++ b/api/tests/ProposalSystem.Tests/Services/ProposalDeliveryTests.cs @@ -108,7 +108,7 @@ public class ProposalDeliveryTests : IDisposable .Throws(new InvalidOperationException("SES is down")); // Act — should NOT throw - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert — transition completed despite email failure result.Status.Should().Be(ProposalStatus.Sent); @@ -146,7 +146,7 @@ public class ProposalDeliveryTests : IDisposable .Returns("https://s3.example.com/acme.pdf"); // Act - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Sent); @@ -176,7 +176,7 @@ public class ProposalDeliveryTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Sent); @@ -193,7 +193,7 @@ public class ProposalDeliveryTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Sent); @@ -219,7 +219,7 @@ public class ProposalDeliveryTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Sent); @@ -248,4 +248,7 @@ public class ProposalDeliveryTests : IDisposable UpdatedAt = DateTime.UtcNow.AddDays(-1), }; } + private string Ver(Guid id) => + ProposalSystem.Application.Common.RowVersionCodec.Encode(_db.Proposals.Find(id)?.Version ?? 1); + } diff --git a/api/tests/ProposalSystem.Tests/Services/ProposalStateMachineTests.cs b/api/tests/ProposalSystem.Tests/Services/ProposalStateMachineTests.cs index feea3f8..1a3b4c3 100644 --- a/api/tests/ProposalSystem.Tests/Services/ProposalStateMachineTests.cs +++ b/api/tests/ProposalSystem.Tests/Services/ProposalStateMachineTests.cs @@ -92,7 +92,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.ApproveAsync(proposal.Id); + var result = await _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Approved); @@ -109,7 +109,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Sent); @@ -137,7 +137,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.ReviseAsync(proposal.Id); + var result = await _sut.ReviseAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.InReview); @@ -159,7 +159,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.ApproveAsync(proposal.Id); + var result = await _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Approved); @@ -174,7 +174,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.MarkSentAsync(proposal.Id); + var result = await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert result.Status.Should().Be(ProposalStatus.Sent); @@ -193,7 +193,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var act = () => _sut.MarkSentAsync(proposal.Id); + var act = () => _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert await act.Should().ThrowAsync() @@ -209,7 +209,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var act = () => _sut.ReviseAsync(proposal.Id); + var act = () => _sut.ReviseAsync(proposal.Id, Ver(proposal.Id)); // Assert await act.Should().ThrowAsync() @@ -236,7 +236,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var act = () => _sut.ApproveAsync(proposal.Id); + var act = () => _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); // Assert await act.Should().ThrowAsync() @@ -252,7 +252,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var act = () => _sut.ReviseAsync(proposal.Id); + var act = () => _sut.ReviseAsync(proposal.Id, Ver(proposal.Id)); // Assert await act.Should().ThrowAsync() @@ -267,7 +267,7 @@ public class ProposalStateMachineTests : IDisposable _db.Proposals.Add(proposal); await _db.SaveChangesAsync(); - var act = () => _sut.ApproveAsync(proposal.Id); + var act = () => _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); await act.Should().ThrowAsync() .WithMessage("*in review*"); @@ -282,7 +282,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var act = () => _sut.ApproveAsync(proposal.Id); + var act = () => _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); // Assert await act.Should().ThrowAsync() @@ -309,7 +309,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var act = () => _sut.ApproveAsync(proposal.Id); + var act = () => _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); // Assert await act.Should().ThrowAsync() @@ -321,9 +321,9 @@ public class ProposalStateMachineTests : IDisposable { var missingId = Guid.NewGuid(); - var approveAct = () => _sut.ApproveAsync(missingId); - var sendAct = () => _sut.MarkSentAsync(missingId); - var reviseAct = () => _sut.ReviseAsync(missingId); + var approveAct = () => _sut.ApproveAsync(missingId, Ver(missingId)); + var sendAct = () => _sut.MarkSentAsync(missingId, Ver(missingId)); + var reviseAct = () => _sut.ReviseAsync(missingId, Ver(missingId)); await approveAct.Should().ThrowAsync(); await sendAct.Should().ThrowAsync(); @@ -368,7 +368,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - var result = await _sut.ReviseAsync(proposal.Id); + var result = await _sut.ReviseAsync(proposal.Id, Ver(proposal.Id)); // Assert var revision = await _db.Proposals.FindAsync(result.Id); @@ -398,10 +398,10 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - await _sut.ApproveAsync(proposal.Id); + await _sut.ApproveAsync(proposal.Id, Ver(proposal.Id)); // Assert - await _audit.Received(1).LogAsync(AuditAction.Approve, proposal.Id, Arg.Any(), Arg.Any()); + _audit.Received(1).Stage(AuditAction.Approve, proposal.Id, Arg.Any()); } [Fact(DisplayName = "QA-C2: MarkSent publishes library-ingest job")] @@ -413,7 +413,7 @@ public class ProposalStateMachineTests : IDisposable await _db.SaveChangesAsync(); // Act - await _sut.MarkSentAsync(proposal.Id); + await _sut.MarkSentAsync(proposal.Id, Ver(proposal.Id)); // Assert await _jobPublisher.Received(1).PublishAsync("library-ingest", Arg.Any(), Arg.Any()); @@ -442,4 +442,7 @@ public class ProposalStateMachineTests : IDisposable UpdatedAt = DateTime.UtcNow.AddDays(-1), }; } + private string Ver(Guid id) => + ProposalSystem.Application.Common.RowVersionCodec.Encode(_db.Proposals.Find(id)?.Version ?? 1); + } diff --git a/docs/adr/0004-optimistic-concurrency-convention.md b/docs/adr/0004-optimistic-concurrency-convention.md new file mode 100644 index 0000000..2182220 --- /dev/null +++ b/docs/adr/0004-optimistic-concurrency-convention.md @@ -0,0 +1,67 @@ +# ADR 0004 — Optimistic concurrency: SHOC wire contract on a Postgres version column + +- **Status:** Accepted (2026-07-13) +- **Decision owner:** Adam Moussa +- **Scope:** `api/` proposal aggregate, `shared/api-contracts`, all clients (web, mobile, suggestions Lambda) + +## Context + +SHOC-alignment Phase 6 ports shoc-backend's optimistic-concurrency convention +(PRs #10/#13–#18) to the proposal aggregate. SHOC's mechanics are built on SQL +Server `rowversion` (`byte[8]`, auto-rotated, base64 on the wire) with a +double-guard: pre-check the client token against the loaded row, stamp it as +EF's original value so the UPDATE's WHERE clause re-enforces it, and on a lost +race reload and embed the winner's state in a 409. PostgreSQL has no +`rowversion`; the candidates were the `xmin` system column or an explicit +version column. + +## Decision + +1. **Explicit `long Version` column** on `Proposals` and `LineItems`, + `IsConcurrencyToken`, additive migration with `DEFAULT 1`, incremented by + the mutating services. Not `xmin`: the xUnit suite runs on InMemory/SQLite + where xmin doesn't exist (SHOC needed an InMemory shim for the same + reason), the handbook expects a real, reversible migration, and xmin leaks + storage internals onto the wire. +2. **SHOC's wire contract verbatim.** Tokens are opaque base64 strings + (`RowVersionCodec`: 8-byte big-endian long — same shape as SHOC's + `"AQAAAAAAAAA="` tokens). Responses carry `rowVersion`; guarded requests + carry `proposalVersion`. Missing token → 422 `ProposalVersionRequired`; + malformed → 422 `InvalidRowVersion`; conflict → **409 + `{ message, currentState }`** with the reloaded `ProposalResponse` + embedded; unguarded races → 409 `{ status, message, code }` fallback. + Both envelopes are deliberately **not** ProblemDetails (SHOC parity) and + serialize with the MVC pipeline's conventions (camelCase, string enums). +3. **One aggregate, one token.** Deviation from the drafted plan, forced by + the code: bulk line-item update is delete-all-and-recreate, so per-item + tokens are meaningless. The proposal token guards proposal fields, state + transitions, and the bulk replace; every line-item mutation + (create/bulk/delete) bumps the proposal version so nothing goes stale + silently. `LineItem.Version` exists (additive, on the wire) for future + per-item mutations only. +4. **Guard scope per SHOC precedent.** Updates and state transitions demand + the token; creates and deletes don't, but still bump the aggregate version + — a lost race there surfaces as the fallback 409 instead of a silent + overwrite (this includes `VendorProposalsController`'s vendor-cost + recalc). Internal writers (suggestions Lambda) fetch-and-echo the token + with one conflict retry. +5. **Caller contract:** the 409 `currentState` reload has no ownership + filter, so guard-reaching endpoints must stay admin-gated — enforced by + `GuardedEndpointAuthorizationTests`. Extend the guard with an ownership + predicate before wiring it to any dispatcher-reachable write. +6. **Audit atomicity (same phase):** `IAuditService.Stage` adds to the shared + context; every mutation stages before its single `SaveChangesAsync`, so + the domain change and its audit row commit or fail together. Self-saving + `LogAsync` remains for standalone events only. + +## Consequences + +- Breaking API change for mutating clients; web, mobile, and the suggestions + Lambda ship the token pass-through in the same change set. +- Deploys: migration auto-applies at API startup under `pg_advisory_lock`; + the column is additive with a default, so the previous Lambda version keeps + working against the migrated schema. Manual RDS snapshot before deploy; + down-script is two `DropColumn`s, to be tested against a snapshot-restored + copy before any production rollback. +- ADR 0002's boundary holds: the convention and wire contract converge with + SHOC; the platform (Postgres, integer column, explicit increments) does not. diff --git a/lambdas/suggestions/app.py b/lambdas/suggestions/app.py index 424e430..d22a705 100644 --- a/lambdas/suggestions/app.py +++ b/lambdas/suggestions/app.py @@ -447,17 +447,36 @@ def post_line_items(proposal_id: str, items: list[dict], existing_items: list[di ) try: - resp = _retry_request( - "PUT", - f"{API_BASE_URL}/api/proposals/{proposal_id}/line-items", - json={"lineItems": line_items_payload}, - headers=_api_headers(), - timeout=15, - ) - if resp.status_code not in (200, 201): + # Fix: CONC-L1 — the bulk PUT is version-guarded (ADR 0004): echo the + # proposal's current rowVersion as proposalVersion, and retry once with + # a fresh token if a concurrent edit wins the race (409/stale 422). + for attempt in range(2): + proposal = fetch_proposal(proposal_id) + token = (proposal or {}).get("rowVersion") + if not token: + logger.error( + "Cannot post line items: no rowVersion for proposal %s", proposal_id + ) + return + resp = _retry_request( + "PUT", + f"{API_BASE_URL}/api/proposals/{proposal_id}/line-items", + json={"lineItems": line_items_payload, "proposalVersion": token}, + headers=_api_headers(), + timeout=15, + ) + if resp.status_code in (200, 201): + return + if resp.status_code == 409 and attempt == 0: + logger.warning( + "Concurrency conflict posting line items for %s; retrying with fresh token", + proposal_id, + ) + continue logger.error( "Failed to post line items: %s %s", resp.status_code, resp.text ) + return except Exception as e: # Fix: LAM-M5 — include stack trace in error logging logger.exception("Error posting line items: %s", e) diff --git a/lambdas/tests/test_suggestions.py b/lambdas/tests/test_suggestions.py index 641cdfc..2edca76 100644 --- a/lambdas/tests/test_suggestions.py +++ b/lambdas/tests/test_suggestions.py @@ -138,9 +138,14 @@ class TestProcessSuggestion: @patch.object(suggestions_app, "_retry_request") def test_post_line_items_preserves_non_ai_items(self, mock_request): """QA-C6: Existing non-AI items are preserved when new suggestions are generated.""" - mock_response = MagicMock() - mock_response.status_code = 200 - mock_request.return_value = mock_response + # Fix: CONC-L1 — post_line_items now fetches the proposal's rowVersion + # first and echoes it as proposalVersion on the guarded bulk PUT. + get_response = MagicMock() + get_response.status_code = 200 + get_response.json.return_value = {"rowVersion": "AAAAAAAAAAE="} + put_response = MagicMock() + put_response.status_code = 200 + mock_request.side_effect = [get_response, put_response] existing_items = [ { @@ -175,11 +180,13 @@ class TestProcessSuggestion: suggestions_app.post_line_items("abc", new_items, existing_items) - # Verify the API was called - mock_request.assert_called_once() + # GET (token fetch) + PUT (guarded bulk update) + assert mock_request.call_count == 2 call_kwargs = mock_request.call_args payload = call_kwargs.kwargs.get("json") or call_kwargs[1].get("json") + # The guarded PUT must echo the proposal's version token + assert payload["proposalVersion"] == "AAAAAAAAAAE=" # Should have 2 items: 1 preserved Manual + 1 new AI (old AI items are replaced) assert len(payload["lineItems"]) == 2 sources = [li["source"] for li in payload["lineItems"]] @@ -191,6 +198,37 @@ class TestProcessSuggestion: assert payload["lineItems"][1]["source"] == "AI" assert payload["lineItems"][1]["description"] == "New AI item" + @patch.object(suggestions_app, "_retry_request") + def test_post_line_items_retries_once_on_conflict(self, mock_request): + """CONC-L1: a 409 conflict refetches the token and retries the PUT once.""" + get_1 = MagicMock(status_code=200) + get_1.json.return_value = {"rowVersion": "AAAAAAAAAAE="} + put_conflict = MagicMock(status_code=409, text="conflict") + get_2 = MagicMock(status_code=200) + get_2.json.return_value = {"rowVersion": "AAAAAAAAAAI="} + put_ok = MagicMock(status_code=200) + mock_request.side_effect = [get_1, put_conflict, get_2, put_ok] + + suggestions_app.post_line_items( + "abc", + [ + { + "description": "AI item", + "quantity": 1, + "unit": "each", + "totalPrice": 100, + "pricingMode": "TotalPrice", + } + ], + [], + ) + + assert mock_request.call_count == 4 + final_payload = mock_request.call_args.kwargs.get( + "json" + ) or mock_request.call_args[1].get("json") + assert final_payload["proposalVersion"] == "AAAAAAAAAAI=" + @patch.object(suggestions_app, "post_line_items") @patch.object(suggestions_app, "generate_line_items") @patch.object(suggestions_app, "retrieve_similar") diff --git a/mobile/src/lib/api/admin.ts b/mobile/src/lib/api/admin.ts index 634c795..3fb41f2 100644 --- a/mobile/src/lib/api/admin.ts +++ b/mobile/src/lib/api/admin.ts @@ -1,4 +1,5 @@ import apiClient from './client'; +import type { ProposalDetail } from './proposals'; export interface DashboardStats { pendingCount: number; @@ -11,6 +12,8 @@ export interface UpdateProposalRequest { refinedScope?: string; notes?: string; assignedAdminId?: string; + /** Expected proposal rowVersion — REQUIRED by the server (422 ProposalVersionRequired when missing). */ + proposalVersion?: string; } export interface AuditEntry { @@ -30,23 +33,47 @@ export const adminApi = { return res.data; }, + // Fix: CONC-L2 — guarded mutations (optimistic concurrency): every write + // below requires the proposal's rowVersion token and returns the fresh + // proposal state — the server rotates the token on each guarded write, so + // callers chain from the RESPONSE's rowVersion, never the one they started + // with. updateProposal: async ( id: string, data: UpdateProposalRequest, - ): Promise => { - await apiClient.put(`/proposals/${id}`, data); + ): Promise => { + const res = await apiClient.put(`/proposals/${id}`, data); + return res.data; }, - approveProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/approve`); + approveProposal: async ( + id: string, + proposalVersion?: string, + ): Promise => { + const res = await apiClient.post(`/proposals/${id}/approve`, { + proposalVersion, + }); + return res.data; }, - sendProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/send`); + sendProposal: async ( + id: string, + proposalVersion?: string, + ): Promise => { + const res = await apiClient.post(`/proposals/${id}/send`, { + proposalVersion, + }); + return res.data; }, - reviseProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/revise`); + reviseProposal: async ( + id: string, + proposalVersion?: string, + ): Promise => { + const res = await apiClient.post(`/proposals/${id}/revise`, { + proposalVersion, + }); + return res.data; }, getAudit: async (id: string): Promise => { diff --git a/mobile/src/lib/api/client.ts b/mobile/src/lib/api/client.ts index 82c5868..d3fdeab 100644 --- a/mobile/src/lib/api/client.ts +++ b/mobile/src/lib/api/client.ts @@ -2,6 +2,19 @@ import axios from 'axios'; import Config from '../../config'; import { tokenStorage } from '../storage'; +// Fix: CONC-L2 — carries HTTP status + response body so screens can branch on +// optimistic-concurrency conflicts (409 { message, currentState }). +export class ApiError extends Error { + constructor( + message: string, + public status: number, + public data?: unknown, + ) { + super(message); + this.name = 'ApiError'; + } +} + type RefreshFn = () => Promise; let _refreshTokens: RefreshFn | null = null; @@ -108,7 +121,7 @@ apiClient.interceptors.response.use( const message = data?.detail || data?.title || data?.message || 'An error occurred'; - return Promise.reject(new Error(message)); + return Promise.reject(new ApiError(message, status, data)); } if (error.request) { diff --git a/mobile/src/lib/api/lineItems.ts b/mobile/src/lib/api/lineItems.ts index a8f759c..4e3ee24 100644 --- a/mobile/src/lib/api/lineItems.ts +++ b/mobile/src/lib/api/lineItems.ts @@ -14,6 +14,8 @@ export interface LineItem { source: LineItemSource; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface UpdateLineItemEntry { @@ -34,12 +36,16 @@ export const lineItemsApi = { return res.data; }, + // Fix: CONC-L2 — the bulk replace is guarded by the PROPOSAL rowVersion + // token; the server rotates it on success, so refetch the proposal after. bulkUpdate: async ( proposalId: string, lineItems: UpdateLineItemEntry[], + proposalVersion?: string, ): Promise => { const res = await apiClient.put(`/proposals/${proposalId}/line-items`, { lineItems, + proposalVersion, }); return res.data; }, diff --git a/mobile/src/lib/api/proposals.ts b/mobile/src/lib/api/proposals.ts index 39cc916..2ed1a5b 100644 --- a/mobile/src/lib/api/proposals.ts +++ b/mobile/src/lib/api/proposals.ts @@ -13,6 +13,8 @@ export interface ProposalListItem { submittedAt: string; submittedByName: string | null; assignedAdminName: string | null; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface ProposalDetail { @@ -40,6 +42,8 @@ export interface ProposalDetail { parentProposalId: string | null; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface PagedResponse { diff --git a/mobile/src/screens/admin/LineItemEditScreen.tsx b/mobile/src/screens/admin/LineItemEditScreen.tsx index 44d201b..feb7c15 100644 --- a/mobile/src/screens/admin/LineItemEditScreen.tsx +++ b/mobile/src/screens/admin/LineItemEditScreen.tsx @@ -16,7 +16,9 @@ import { } from 'react-native-paper'; import { useRoute, useNavigation } from '@react-navigation/native'; import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { ApiError } from '../../lib/api/client'; import { lineItemsApi, type UpdateLineItemEntry } from '../../lib/api/lineItems'; +import { proposalsApi, type ProposalDetail } from '../../lib/api/proposals'; import { QUERY_KEYS } from '../../constants/queryKeys'; import type { NativeStackScreenProps } from '@react-navigation/native-stack'; import type { ProposalsStackParamList } from '../../navigation/types'; @@ -98,16 +100,39 @@ export function LineItemEditScreen() { { ...entry, sortOrder: existing.length }, ]; - return lineItemsApi.bulkUpdate(proposalId, updated); + // Fix: CONC-L2 — the bulk replace is guarded by the proposal's + // rowVersion token; use the one the workspace already loaded, or fetch + // a fresh one if the cache is empty. + const cachedProposal = qc.getQueryData([ + QUERY_KEYS.proposal, + proposalId, + ]); + const proposalVersion = + cachedProposal?.rowVersion ?? + (await proposalsApi.getById(proposalId)).rowVersion; + + return lineItemsApi.bulkUpdate(proposalId, updated, proposalVersion); }, onSuccess: () => { qc.invalidateQueries({ queryKey: [QUERY_KEYS.proposalLineItems, proposalId], }); + // Fix: CONC-L2 — the server rotated the proposal token; refetch it. + qc.invalidateQueries({ queryKey: [QUERY_KEYS.proposal, proposalId] }); setSnackbar('Line item saved'); setTimeout(() => navigation.goBack(), 800); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: (err: Error) => { + Alert.alert('Error', err.message); + // Fix: CONC-L2 — on a 409 concurrency conflict, refetch so the + // workspace picks up the fresh state + rotated token. + if (err instanceof ApiError && err.status === 409) { + qc.invalidateQueries({ queryKey: [QUERY_KEYS.proposal, proposalId] }); + qc.invalidateQueries({ + queryKey: [QUERY_KEYS.proposalLineItems, proposalId], + }); + } + }, }); const handleAutoCalc = () => { diff --git a/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx b/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx index 81b4829..cff0b18 100644 --- a/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx +++ b/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx @@ -24,6 +24,7 @@ import ReactNativeHapticFeedback from 'react-native-haptic-feedback'; import { StatusChip } from '../../components/StatusChip'; import { PriorityChip } from '../../components/PriorityChip'; import { LineItemRow } from '../../components/LineItemRow'; +import { ApiError } from '../../lib/api/client'; import { proposalsApi } from '../../lib/api/proposals'; import { lineItemsApi } from '../../lib/api/lineItems'; import { adminApi } from '../../lib/api/admin'; @@ -67,31 +68,45 @@ export function ProposalWorkspaceScreen() { }); }; + // Fix: CONC-L2 — on a 409 concurrency conflict, surface the server message + // and refetch so the workspace picks up the fresh state + rotated token. + const handleGuardedError = (err: Error) => { + Alert.alert('Error', err.message); + if (err instanceof ApiError && err.status === 409) { + invalidate(); + } + }; + const approveMutation = useMutation({ - mutationFn: () => adminApi.approveProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.approveProposal(proposalId, proposal?.rowVersion), + onSuccess: (updated) => { ReactNativeHapticFeedback.trigger('notificationSuccess'); setSnackbar('Proposal approved'); + qc.setQueryData([QUERY_KEYS.proposal, proposalId], updated); invalidate(); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: handleGuardedError, }); const sendMutation = useMutation({ - mutationFn: () => adminApi.sendProposal(proposalId), - onSuccess: () => { + mutationFn: () => adminApi.sendProposal(proposalId, proposal?.rowVersion), + onSuccess: (updated) => { setSnackbar('Marked as sent'); + qc.setQueryData([QUERY_KEYS.proposal, proposalId], updated); invalidate(); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: handleGuardedError, }); const regenerateMutation = useMutation({ mutationFn: async () => { if (scopeInput.trim()) { - await adminApi.updateProposal(proposalId, { + const updated = await adminApi.updateProposal(proposalId, { refinedScope: scopeInput.trim(), + proposalVersion: proposal?.rowVersion, }); + qc.setQueryData([QUERY_KEYS.proposal, proposalId], updated); } await adminApi.generateSuggestions(proposalId); }, @@ -100,7 +115,7 @@ export function ProposalWorkspaceScreen() { setSnackbar('AI suggestions regenerating...'); setTimeout(invalidate, 5000); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: handleGuardedError, }); const handleDownloadPdf = async () => { diff --git a/shared/api-contracts/src/index.ts b/shared/api-contracts/src/index.ts index f189eb2..2c07502 100644 --- a/shared/api-contracts/src/index.ts +++ b/shared/api-contracts/src/index.ts @@ -34,6 +34,8 @@ export interface ProposalListItem { submittedAt: string; submittedByName: string | null; assignedAdminName: string | null; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface ProposalDetail { @@ -62,6 +64,8 @@ export interface ProposalDetail { parentProposalId: string | null; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface CreateProposalRequest { @@ -81,6 +85,16 @@ export interface UpdateProposalRequest { poNumber?: string; workOrderNumber?: string; assignedAdminId?: string; + /** Expected proposal rowVersion — REQUIRED by the server (422 ProposalVersionRequired when missing). */ + proposalVersion?: string; +} + +/** Body for the state-transition endpoints (approve, return-to-review, send, + * revise): the expected proposal rowVersion. The server rejects a missing + * token with 422 {code:"ProposalVersionRequired"} and a malformed one with + * 422 {code:"InvalidRowVersion"}. */ +export interface ProposalVersionRequest { + proposalVersion?: string; } export interface ProposalFilters { @@ -116,6 +130,8 @@ export interface LineItem { source: LineItemSource; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface CreateLineItemRequest { @@ -143,6 +159,8 @@ export interface UpdateLineItemEntry { export interface BulkUpdateLineItemsRequest { lineItems: UpdateLineItemEntry[]; + /** Expected proposal rowVersion — the bulk replace is guarded by the PROPOSAL token. */ + proposalVersion?: string; } // ── Customers (CustomerDtos.cs) ─────────────────────────────────────────── @@ -285,3 +303,13 @@ export interface ApiProblem { detail: string; code: ApiProblemCode; } + +// Optimistic-concurrency conflict envelope (SHOC ADR 0004 — NOT ProblemDetails): +// guarded writes that lose a race return 409 with the reloaded currentState so +// clients can refresh immediately. The unguarded-race safety net returns +// 409 { status: "Conflict", message, code: 409 } instead — same `message` +// field, no currentState. +export interface ConcurrencyConflict { + message: string; + currentState: T | null; +} diff --git a/shared/api-contracts/src/schemas.ts b/shared/api-contracts/src/schemas.ts index 6929c86..0de0d7f 100644 --- a/shared/api-contracts/src/schemas.ts +++ b/shared/api-contracts/src/schemas.ts @@ -14,6 +14,8 @@ import type { ProposalDetail, CreateProposalRequest, UpdateProposalRequest, + ProposalVersionRequest, + ConcurrencyConflict, LineItem, CreateLineItemRequest, UpdateLineItemEntry, @@ -56,6 +58,7 @@ export const proposalListItemSchema = z.object({ submittedAt: z.string(), submittedByName: z.string().nullable(), assignedAdminName: z.string().nullable(), + rowVersion: z.string(), }) satisfies z.ZodType; export const proposalDetailSchema = z.object({ @@ -84,6 +87,7 @@ export const proposalDetailSchema = z.object({ parentProposalId: z.string().nullable(), createdAt: z.string(), updatedAt: z.string(), + rowVersion: z.string(), }) satisfies z.ZodType; export const createProposalRequestSchema = z.object({ @@ -103,8 +107,13 @@ export const updateProposalRequestSchema = z.object({ poNumber: z.string().optional(), workOrderNumber: z.string().optional(), assignedAdminId: z.string().optional(), + proposalVersion: z.string().optional(), }) satisfies z.ZodType; +export const proposalVersionRequestSchema = z.object({ + proposalVersion: z.string().optional(), +}) satisfies z.ZodType; + export const proposalStatsSchema = z.object({ totalCount: z.number(), inReviewCount: z.number(), @@ -126,6 +135,7 @@ export const lineItemSchema = z.object({ source: lineItemSourceSchema, createdAt: z.string(), updatedAt: z.string(), + rowVersion: z.string(), }) satisfies z.ZodType; export const createLineItemRequestSchema = z.object({ @@ -153,6 +163,7 @@ export const updateLineItemEntrySchema = z.object({ export const bulkUpdateLineItemsRequestSchema = z.object({ lineItems: z.array(updateLineItemEntrySchema), + proposalVersion: z.string().optional(), }) satisfies z.ZodType; // ── Customers ───────────────────────────────────────────────────────────── @@ -274,3 +285,11 @@ export const apiProblemSchema = z.object({ detail: z.string(), code: z.string(), }) satisfies z.ZodType; + +// 409 concurrency-conflict envelope. The proposal aggregate is the only +// guarded resource today, so the concrete schema is coupled to ProposalDetail; +// build others via the same pattern when new aggregates gain guards. +export const proposalConcurrencyConflictSchema = z.object({ + message: z.string(), + currentState: proposalDetailSchema.nullable(), +}) satisfies z.ZodType>; diff --git a/web/e2e/smoke.spec.ts b/web/e2e/smoke.spec.ts index 7fa5f1d..4a81900 100644 --- a/web/e2e/smoke.spec.ts +++ b/web/e2e/smoke.spec.ts @@ -38,6 +38,7 @@ const proposals = [ submittedAt: '2026-07-01T12:00:00Z', submittedByName: 'Adam Moussa', assignedAdminName: null, + rowVersion: 'AAAAAAAAAAE=', }, { id: 'p-2', @@ -51,6 +52,7 @@ const proposals = [ submittedAt: '2026-07-05T09:30:00Z', submittedByName: 'Adam Moussa', assignedAdminName: 'Sarah Chen', + rowVersion: 'AAAAAAAAAAI=', }, ]; diff --git a/web/src/domain/__tests__/admin.use-cases.test.tsx b/web/src/domain/__tests__/admin.use-cases.test.tsx index 50a7ab3..c888143 100644 --- a/web/src/domain/__tests__/admin.use-cases.test.tsx +++ b/web/src/domain/__tests__/admin.use-cases.test.tsx @@ -1,14 +1,20 @@ // Admin domain use-case hooks: state transitions must cross-domain -// invalidate the proposals/lineItems keys (domain README rule 3). The domain -// api modules are mocked — no axios traffic. +// invalidate the proposals/lineItems keys (domain README rule 3), thread the +// optimistic-concurrency token from the cached detail into guarded mutations, +// and recover from 409 conflicts by refreshing from the server's currentState. +// The domain api modules are mocked — no axios traffic. import { describe, it, expect, vi, beforeEach } from 'vitest'; import { renderHook, waitFor } from '@testing-library/react'; import { toast } from 'react-toastify'; import { createQueryHarness } from './hookTestUtils'; -import { useApproveProposal } from '../admin/use-cases'; +import { useApproveProposal, useSaveProposalWorkspace } from '../admin/use-cases'; import { proposalsKeys } from '../proposals/use-cases'; import { lineItemsKeys } from '../lineItems/use-cases'; import { adminApi } from '../admin/api'; +import { lineItemsApi } from '../lineItems/api'; +import { proposalsApi } from '../proposals/api'; +import { ConflictError } from '../../lib/api/errors'; +import type { ProposalDetail } from '../proposals/types'; vi.mock('../admin/api', () => ({ adminApi: { @@ -26,8 +32,8 @@ vi.mock('../admin/api', () => ({ }, })); -// useSaveProposalWorkspace pulls lineItemsApi directly; mock it so no test -// path can reach axios. +// useSaveProposalWorkspace pulls lineItemsApi and proposalsApi directly; +// mock both so no test path can reach axios. vi.mock('../lineItems/api', () => ({ lineItemsApi: { getAll: vi.fn(), @@ -37,27 +43,51 @@ vi.mock('../lineItems/api', () => ({ }, })); +vi.mock('../proposals/api', () => ({ + proposalsApi: { + create: vi.fn(), + getAll: vi.fn(), + getById: vi.fn(), + uploadAttachment: vi.fn(), + confirmUpload: vi.fn(), + getStats: vi.fn(), + getVendors: vi.fn(), + }, +})); + vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), warning: vi.fn(), info: vi.fn() }, })); const PROPOSAL_ID = 'p-7'; +const detail = (rowVersion: string): ProposalDetail => + ({ id: PROPOSAL_ID, rowVersion }) as unknown as ProposalDetail; + beforeEach(() => { vi.clearAllMocks(); }); describe('useApproveProposal', () => { - it('invalidates the proposal detail and its line items keys and toasts on success', async () => { - vi.mocked(adminApi.approveProposal).mockResolvedValue(undefined); + it('sends the cached rowVersion token, invalidates the proposal detail and its line items keys, and toasts on success', async () => { + vi.mocked(adminApi.approveProposal).mockResolvedValue(detail('AAAAAAAAAAM=')); const { queryClient, wrapper } = createQueryHarness(); + // The workspace always fetches the detail before actions are possible — + // the guarded mutation reads its token from this cache at mutate time. + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); const { result } = renderHook(() => useApproveProposal(PROPOSAL_ID), { wrapper }); result.current.mutate(); await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(adminApi.approveProposal).toHaveBeenCalledWith(PROPOSAL_ID); + expect(adminApi.approveProposal).toHaveBeenCalledWith(PROPOSAL_ID, 'AAAAAAAAAAI='); + // The response carries the rotated token — it must land in the detail + // cache immediately so a follow-on guarded action doesn't race the + // invalidation refetch. + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual( + detail('AAAAAAAAAAM='), + ); // Approval changes proposal status AND locks/reprices line items — both // domains' keys must be refetched (cross-domain invalidation, rule 3). expect(invalidateSpy).toHaveBeenCalledWith({ @@ -94,4 +124,78 @@ describe('useApproveProposal', () => { expect(invalidateSpy).not.toHaveBeenCalled(); expect(toast.success).not.toHaveBeenCalled(); }); + + it('on 409 writes the server currentState into the detail cache, refreshes views, and toasts the conflict', async () => { + const currentState = detail('AAAAAAAAAAo='); + vi.mocked(adminApi.approveProposal).mockRejectedValue( + new ConflictError('Proposal was modified by another user.', currentState), + ); + const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); + + const { result } = renderHook(() => useApproveProposal(PROPOSAL_ID), { wrapper }); + result.current.mutate(); + + await waitFor(() => expect(result.current.isError).toBe(true)); + // Refresh-and-retry semantics: the stale detail is replaced by the + // server's reloaded state (fresh token included)... + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual(currentState); + // ...every proposal view is refetched (stale-queue invariant holds even + // on the failure path)... + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: proposalsKeys.detail(PROPOSAL_ID) }); + expect(invalidateSpy).toHaveBeenCalledWith({ + queryKey: lineItemsKeys.byProposal(PROPOSAL_ID), + }); + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: proposalsKeys.lists() }); + // ...and the user gets the conflict message, not the generic failure toast. + expect(toast.warning).toHaveBeenCalledWith('Proposal was modified by another user.'); + expect(toast.error).not.toHaveBeenCalled(); + }); +}); + +describe('useSaveProposalWorkspace', () => { + it('rotates the token across the guarded pair and refetches the detail for the fresh token', async () => { + vi.mocked(adminApi.updateProposal).mockResolvedValue(detail('AAAAAAAAAAM=')); + vi.mocked(lineItemsApi.bulkUpdate).mockResolvedValue([]); + vi.mocked(proposalsApi.getById).mockResolvedValue(detail('AAAAAAAAAAQ=')); + const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); + + const { result } = renderHook(() => useSaveProposalWorkspace(PROPOSAL_ID), { wrapper }); + result.current.mutate({ refinedScope: 'refined', lineItems: [] }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + // PUT sends the token the workspace loaded with... + expect(adminApi.updateProposal).toHaveBeenCalledWith(PROPOSAL_ID, { + refinedScope: 'refined', + proposalVersion: 'AAAAAAAAAAI=', + }); + // ...the bulk replace sends the token the PUT rotated to... + expect(lineItemsApi.bulkUpdate).toHaveBeenCalledWith(PROPOSAL_ID, [], 'AAAAAAAAAAM='); + // ...and the detail cache ends on the post-bulk token so approve-after-save + // never sends a stale version. + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual( + detail('AAAAAAAAAAQ='), + ); + expect(toast.success).toHaveBeenCalledWith('Changes saved'); + }); + + it('on 409 refreshes from currentState and toasts the conflict instead of the save-failed toast', async () => { + const currentState = detail('AAAAAAAAAAo='); + vi.mocked(adminApi.updateProposal).mockRejectedValue( + new ConflictError('Proposal was modified by another user.', currentState), + ); + const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('AAAAAAAAAAI=')); + + const { result } = renderHook(() => useSaveProposalWorkspace(PROPOSAL_ID), { wrapper }); + result.current.mutate({ refinedScope: 'refined', lineItems: [] }); + + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(lineItemsApi.bulkUpdate).not.toHaveBeenCalled(); + expect(queryClient.getQueryData(proposalsKeys.detail(PROPOSAL_ID))).toEqual(currentState); + expect(toast.warning).toHaveBeenCalledWith('Proposal was modified by another user.'); + expect(toast.error).not.toHaveBeenCalled(); + }); }); diff --git a/web/src/domain/__tests__/lineItems.use-cases.test.tsx b/web/src/domain/__tests__/lineItems.use-cases.test.tsx index 9cde471..5ffe757 100644 --- a/web/src/domain/__tests__/lineItems.use-cases.test.tsx +++ b/web/src/domain/__tests__/lineItems.use-cases.test.tsx @@ -11,7 +11,9 @@ import { useSaveProposalWorkspace } from '../admin/use-cases'; import { proposalsKeys } from '../proposals/use-cases'; import { adminApi } from '../admin/api'; import { lineItemsApi } from '../lineItems/api'; +import { proposalsApi } from '../proposals/api'; import type { UpdateLineItemEntry } from '../lineItems/types'; +import type { ProposalDetail } from '../proposals/types'; vi.mock('../admin/api', () => ({ adminApi: { @@ -38,6 +40,19 @@ vi.mock('../lineItems/api', () => ({ }, })); +// The save flow ends with a detail refetch (post-bulk token rotation). +vi.mock('../proposals/api', () => ({ + proposalsApi: { + create: vi.fn(), + getAll: vi.fn(), + getById: vi.fn(), + uploadAttachment: vi.fn(), + confirmUpload: vi.fn(), + getStats: vi.fn(), + getVendors: vi.fn(), + }, +})); + vi.mock('react-toastify', () => ({ toast: { success: vi.fn(), error: vi.fn(), warning: vi.fn(), info: vi.fn() }, })); @@ -47,6 +62,9 @@ const entries = [ { description: 'Labor', quantity: 2, unitPrice: 150 }, ] as unknown as UpdateLineItemEntry[]; +const detail = (rowVersion: string): ProposalDetail => + ({ id: PROPOSAL_ID, rowVersion }) as unknown as ProposalDetail; + beforeEach(() => { vi.clearAllMocks(); }); @@ -60,17 +78,24 @@ describe('lineItemsKeys', () => { describe('useSaveProposalWorkspace', () => { it('persists scope + entries and invalidates line items, detail, and list/stats views', async () => { - vi.mocked(adminApi.updateProposal).mockResolvedValue(undefined); + vi.mocked(adminApi.updateProposal).mockResolvedValue(detail('v2')); vi.mocked(lineItemsApi.bulkUpdate).mockResolvedValue([]); + vi.mocked(proposalsApi.getById).mockResolvedValue(detail('v3')); const { queryClient, wrapper } = createQueryHarness(); + queryClient.setQueryData(proposalsKeys.detail(PROPOSAL_ID), detail('v1')); const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); const { result } = renderHook(() => useSaveProposalWorkspace(PROPOSAL_ID), { wrapper }); result.current.mutate({ refinedScope: 'refined', lineItems: entries }); await waitFor(() => expect(result.current.isSuccess).toBe(true)); - expect(adminApi.updateProposal).toHaveBeenCalledWith(PROPOSAL_ID, { refinedScope: 'refined' }); - expect(lineItemsApi.bulkUpdate).toHaveBeenCalledWith(PROPOSAL_ID, entries); + // Token rotation: the PUT sends the cached token, the bulk replace sends + // the PUT response's rotated token. + expect(adminApi.updateProposal).toHaveBeenCalledWith(PROPOSAL_ID, { + refinedScope: 'refined', + proposalVersion: 'v1', + }); + expect(lineItemsApi.bulkUpdate).toHaveBeenCalledWith(PROPOSAL_ID, entries, 'v2'); expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: lineItemsKeys.byProposal(PROPOSAL_ID), }); @@ -85,7 +110,7 @@ describe('useSaveProposalWorkspace', () => { }); it('toasts a save failure and skips invalidation', async () => { - vi.mocked(adminApi.updateProposal).mockRejectedValue(new Error('409 conflict')); + vi.mocked(adminApi.updateProposal).mockRejectedValue(new Error('validation failed')); const { queryClient, wrapper } = createQueryHarness(); const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); @@ -93,7 +118,7 @@ describe('useSaveProposalWorkspace', () => { result.current.mutate({ refinedScope: 'refined', lineItems: entries }); await waitFor(() => expect(result.current.isError).toBe(true)); - expect(toast.error).toHaveBeenCalledWith('Save failed: 409 conflict'); + expect(toast.error).toHaveBeenCalledWith('Save failed: validation failed'); expect(lineItemsApi.bulkUpdate).not.toHaveBeenCalled(); expect(invalidateSpy).not.toHaveBeenCalled(); }); diff --git a/web/src/domain/admin/api.ts b/web/src/domain/admin/api.ts index 472f339..90728ca 100644 --- a/web/src/domain/admin/api.ts +++ b/web/src/domain/admin/api.ts @@ -9,24 +9,34 @@ export const adminApi = { return res.data; }, - updateProposal: async (id: string, data: UpdateProposalRequest): Promise => { - await apiClient.put(`/proposals/${id}`, data); + // Guarded mutations (optimistic concurrency): every write below requires the + // proposal's rowVersion token and returns the fresh proposal state — the + // server rotates the token on each guarded write, so callers chain from the + // RESPONSE's rowVersion, never the one they started with. + updateProposal: async (id: string, data: UpdateProposalRequest): Promise => { + const res = await apiClient.put(`/proposals/${id}`, data); + return res.data; }, - approveProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/approve`); + approveProposal: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/approve`, { proposalVersion }); + return res.data; }, - returnToReview: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/return-to-review`); + returnToReview: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/return-to-review`, { proposalVersion }); + return res.data; }, - sendProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/send`); + sendProposal: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/send`, { proposalVersion }); + return res.data; }, - reviseProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/revise`); + /** Returns the NEW revision (different id), not the revised original. */ + reviseProposal: async (id: string, proposalVersion?: string): Promise => { + const res = await apiClient.post(`/proposals/${id}/revise`, { proposalVersion }); + return res.data; }, getHistory: async (id: string): Promise => { diff --git a/web/src/domain/admin/use-cases.ts b/web/src/domain/admin/use-cases.ts index 9c0f8d7..69abe3c 100644 --- a/web/src/domain/admin/use-cases.ts +++ b/web/src/domain/admin/use-cases.ts @@ -8,9 +8,12 @@ import { useMutation, useQuery, useQueryClient, type QueryClient } from '@tansta import { toast } from 'react-toastify'; import { adminApi } from './api'; import { lineItemsApi } from '../lineItems/api'; +import { proposalsApi } from '../proposals/api'; import { proposalsKeys } from '../proposals/use-cases'; import { lineItemsKeys } from '../lineItems/use-cases'; +import { ConflictError } from '../../lib/api/errors'; import type { UpdateLineItemEntry } from '../lineItems/types'; +import type { ProposalDetail } from '../proposals/types'; export const adminKeys = { all: ['admin'] as const, @@ -33,6 +36,41 @@ function invalidateProposalViews(queryClient: QueryClient, proposalId: string) { queryClient.invalidateQueries({ queryKey: adminKeys.dashboard() }); } +/** + * The optimistic-concurrency token for a guarded mutation, read from the + * cached proposal detail at mutate time (the workspace always fetches the + * detail before any action is possible). Read lazily inside mutationFn — + * never captured at render — so a save-then-approve chain sees the token the + * save wrote back, not the one the page rendered with. + */ +function cachedProposalVersion(queryClient: QueryClient, proposalId: string): string | undefined { + return queryClient.getQueryData(proposalsKeys.detail(proposalId))?.rowVersion; +} + +/** + * 409 recovery (refresh-and-retry semantics): write the server's reloaded + * currentState into the detail cache (fresh token immediately available), + * refetch every proposal view, and tell the user their view was stale. + * Returns true when the error was a concurrency conflict — callers skip + * their generic failure toast in that case. + */ +function handleConcurrencyConflict( + queryClient: QueryClient, + proposalId: string, + error: Error, +): boolean { + if (!(error instanceof ConflictError)) return false; + // Only seed the cache when the embedded state is actually this proposal — + // a mismatched or partial envelope falls through to invalidation, and the + // refetch restores truth. + if (error.currentState && error.currentState.id === proposalId) { + queryClient.setQueryData(proposalsKeys.detail(proposalId), error.currentState); + } + invalidateProposalViews(queryClient, proposalId); + toast.warning(error.message); + return true; +} + /** Admin dashboard KPIs (AdminDashboard). */ export function useAdminDashboard() { return useQuery({ @@ -63,14 +101,36 @@ export function useSaveProposalWorkspace(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ mutationFn: async ({ refinedScope, lineItems }: SaveWorkspaceVariables) => { - await adminApi.updateProposal(proposalId, { refinedScope }); - await lineItemsApi.bulkUpdate(proposalId, lineItems); + // Guarded pair: the PUT rotates the proposal token, so the bulk replace + // must send the PUT response's token, not the one the save started with. + const updated = await adminApi.updateProposal(proposalId, { + refinedScope, + proposalVersion: cachedProposalVersion(queryClient, proposalId), + }); + await lineItemsApi.bulkUpdate(proposalId, lineItems, updated.rowVersion); + // The bulk replace rotates the token AGAIN but returns only line items — + // refetch the detail so a chained guarded action (approve-after-save) + // holds the current token instead of racing the invalidation refetch. + // Both writes are already committed here: if only this trailing GET + // fails, the save must still report success, with invalidation + // recovering the token instead (CONC-L4). + try { + return await proposalsApi.getById(proposalId); + } catch { + return null; + } }, - onSuccess: () => { + onSuccess: (fresh) => { + if (fresh) { + queryClient.setQueryData(proposalsKeys.detail(proposalId), fresh); + } else { + queryClient.removeQueries({ queryKey: proposalsKeys.detail(proposalId) }); + } invalidateProposalViews(queryClient, proposalId); toast.success('Changes saved'); }, onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Save failed: ${error.message}`); }, }); @@ -79,12 +139,15 @@ export function useSaveProposalWorkspace(proposalId: string) { export function useApproveProposal(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.approveProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.approveProposal(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (proposal) => { + queryClient.setQueryData(proposalsKeys.detail(proposal.id), proposal); invalidateProposalViews(queryClient, proposalId); toast.success('Proposal approved'); }, onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Approval failed: ${error.message}`); }, }); @@ -93,14 +156,17 @@ export function useApproveProposal(proposalId: string) { export function useSendProposal(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.sendProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.sendProposal(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (proposal) => { + queryClient.setQueryData(proposalsKeys.detail(proposal.id), proposal); invalidateProposalViews(queryClient, proposalId); toast.success('Proposal marked as sent'); }, // Fix: WEB-H5 — mutation must surface failures to the user (relocated // from AdminWorkspace during the domain-layer refactor) onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Send failed: ${error.message}`); }, }); @@ -109,14 +175,19 @@ export function useSendProposal(proposalId: string) { export function useReviseProposal(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.reviseProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.reviseProposal(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (revision) => { + // The response is the NEW revision (different id) — seed its detail + // cache; the invalidation below refreshes the original's views. + queryClient.setQueryData(proposalsKeys.detail(revision.id), revision); invalidateProposalViews(queryClient, proposalId); toast.success('Revision created'); }, // Fix: WEB-H6 — mutation must surface failures to the user (relocated // from AdminWorkspace during the domain-layer refactor) onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Revision failed: ${error.message}`); }, }); @@ -125,12 +196,15 @@ export function useReviseProposal(proposalId: string) { export function useReturnToReview(proposalId: string) { const queryClient = useQueryClient(); return useMutation({ - mutationFn: () => adminApi.returnToReview(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.returnToReview(proposalId, cachedProposalVersion(queryClient, proposalId)), + onSuccess: (proposal) => { + queryClient.setQueryData(proposalsKeys.detail(proposal.id), proposal); invalidateProposalViews(queryClient, proposalId); toast.success('Proposal returned to review'); }, onError: (error: Error) => { + if (handleConcurrencyConflict(queryClient, proposalId, error)) return; toast.error(`Return to review failed: ${error.message}`); }, }); diff --git a/web/src/domain/lineItems/api.ts b/web/src/domain/lineItems/api.ts index 6d18f1d..a19721a 100644 --- a/web/src/domain/lineItems/api.ts +++ b/web/src/domain/lineItems/api.ts @@ -13,8 +13,18 @@ export const lineItemsApi = { return res.data; }, - bulkUpdate: async (proposalId: string, lineItems: UpdateLineItemEntry[]): Promise => { - const res = await apiClient.put(`/proposals/${proposalId}/line-items`, { lineItems }); + /** Guarded by the PROPOSAL token: the wholesale replace conflicts with any + * concurrent change to the aggregate, so the server requires the proposal's + * current rowVersion. Create/delete stay unguarded (single-item ops). */ + bulkUpdate: async ( + proposalId: string, + lineItems: UpdateLineItemEntry[], + proposalVersion?: string, + ): Promise => { + const res = await apiClient.put(`/proposals/${proposalId}/line-items`, { + lineItems, + proposalVersion, + }); return res.data; }, diff --git a/web/src/lib/api/__tests__/client.test.ts b/web/src/lib/api/__tests__/client.test.ts index c2aa9b7..7ed530d 100644 --- a/web/src/lib/api/__tests__/client.test.ts +++ b/web/src/lib/api/__tests__/client.test.ts @@ -7,11 +7,13 @@ * - Response interceptor clears the stored session and redirects on 401 * - Response interceptor returns friendly error message on 403 * - Response interceptor returns friendly error message on 404 + * - Response interceptor throws ConflictError (with currentState) on 409 * - Response interceptor extracts server error detail from response body * - Response interceptor handles network errors (no response) */ import { describe, it, expect, vi, beforeAll, beforeEach } from 'vitest'; import type { InternalAxiosRequestConfig } from 'axios'; +import { ConflictError } from '../errors'; // vi.hoisted returns values accessible in both the hoisted mock scope and test scope. const { interceptors } = vi.hoisted(() => { @@ -209,6 +211,113 @@ describe('API client interceptors', () => { await expect(promise).rejects.toThrow('The requested resource was not found.'); }); + it('409 with the guarded conflict envelope rejects with ConflictError carrying currentState', async () => { + // Must be schema-valid: the interceptor validates the envelope with + // proposalConcurrencyConflictSchema before trusting currentState. + const currentState = { + id: 'p-1', + proposalNumber: 'P-2026-001', + workOrderNumber: 'WO-1', + poNumber: null, + customerName: 'Cust', + customerAddress: 'Addr', + scopeOfWork: 'Scope', + refinedScope: null, + serviceCategory: 'HVAC', + priority: 'Standard', + status: 'Approved', + totalBidAmount: 100, + vendorTotalCost: null, + notes: '', + submittedById: 'u-1', + submittedByName: 'Submitter', + submittedAt: '2026-07-01T00:00:00Z', + assignedAdminId: null, + approvedById: null, + approvedAt: null, + sentAt: null, + currentRevision: 1, + parentProposalId: null, + createdAt: '2026-07-01T00:00:00Z', + updatedAt: '2026-07-01T00:00:00Z', + rowVersion: 'AAAAAAAAAAM=', + }; + const error = { + response: { + status: 409, + data: { message: 'Proposal was modified by another user.', currentState }, + }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toMatchObject({ + name: 'ConflictError', + message: 'Proposal was modified by another user.', + currentState, + }); + await expect(promise).rejects.toBeInstanceOf(ConflictError); + }); + + it('409 with a malformed currentState degrades to a state-less ConflictError', async () => { + const error = { + response: { + status: 409, + data: { + message: 'Proposal was modified by another user.', + currentState: { id: 'p-1', rowVersion: 12345, unexpected: 'shape' }, + }, + }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toMatchObject({ + name: 'ConflictError', + message: 'Proposal was modified by another user.', + currentState: null, + }); + }); + + it('409 with the unguarded fallback envelope rejects with ConflictError and null currentState', async () => { + const error = { + response: { + status: 409, + data: { + status: 'Conflict', + message: 'The record was modified by another user. Refresh and retry.', + code: 409, + }, + }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toBeInstanceOf(ConflictError); + await expect(promise).rejects.toMatchObject({ + message: 'The record was modified by another user. Refresh and retry.', + currentState: null, + }); + }); + + it('409 with an empty body falls back to a generic conflict message', async () => { + const error = { + response: { status: 409, data: {} }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toBeInstanceOf(ConflictError); + await expect(promise).rejects.toMatchObject({ + message: 'This record was modified by another user. Refresh and retry.', + currentState: null, + }); + }); + it('extracts detail message from server error response', async () => { const error = { response: { diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 664cc31..5e29813 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -1,6 +1,8 @@ import axios from 'axios'; import { API_URL, STORAGE_KEY_TOKEN } from '../../constants'; import { AUTH_SESSION_CLEARED_EVENT, clearAuth } from '../auth/authStorage'; +import { proposalConcurrencyConflictSchema } from '@proposal-system/api-contracts/schemas'; +import { ConflictError } from './errors'; const apiClient = axios.create({ baseURL: API_URL, @@ -51,6 +53,23 @@ apiClient.interceptors.response.use( return Promise.reject(new Error('The requested resource was not found.')); } + if (status === 409) { + // Optimistic-concurrency conflict (SHOC ADR 0004): the guarded path + // carries { message, currentState }; the unguarded fallback carries + // { status, message, code } with no state. Validate the envelope before + // trusting it — a malformed currentState must never seed typed caches, + // so parse failures degrade to a state-less conflict (refetch recovers). + const parsed = proposalConcurrencyConflictSchema.safeParse(data); + return Promise.reject( + new ConflictError( + parsed.success + ? parsed.data.message + : data?.message || 'This record was modified by another user. Refresh and retry.', + parsed.success ? parsed.data.currentState : null, + ), + ); + } + const message = data?.detail || data?.title || data?.message || 'An error occurred'; return Promise.reject(new Error(message)); } diff --git a/web/src/lib/api/errors.ts b/web/src/lib/api/errors.ts new file mode 100644 index 0000000..ab8789e --- /dev/null +++ b/web/src/lib/api/errors.ts @@ -0,0 +1,19 @@ +// Typed transport errors thrown by the response interceptor in ./client. +import type { ConcurrencyConflict, ProposalDetail } from '@proposal-system/api-contracts'; + +/** + * HTTP 409 — optimistic-concurrency conflict (SHOC ADR 0004 envelope). + * Carries the server's reloaded `currentState` so callers can refresh their + * caches without a round trip. The proposal aggregate is the only guarded + * resource, so the state is typed to ProposalDetail; the unguarded-race + * fallback envelope has no state (`currentState` is null). + */ +export class ConflictError extends Error implements ConcurrencyConflict { + readonly currentState: ProposalDetail | null; + + constructor(message: string, currentState: ProposalDetail | null = null) { + super(message); + this.name = 'ConflictError'; + this.currentState = currentState; + } +}