mirror of
https://github.com/Sea-Haven-Industries/proposal-system.git
synced 2026-10-07 11:38:56 +00:00
fix: resolve Phase 6c gate findings (2 high, 5 low) across api, web, lambda, mobile
Gemini scanner sweep (token-bypass), GPT-4.1 cross-review, and the 6-detector /sh-security-review fan-out ran against 6acfdab..HEAD; every confirmed finding fixed: HIGH (deployment blockers, logic detector): - CONC-L1: suggestions lambda's bulk line-item PUT sent no proposalVersion — every AI suggestion job would 422 and be silently swallowed. Now fetches the proposal's rowVersion, echoes it, and retries once with a fresh token on 409. Pytest updated (38 green). - CONC-L2: mobile admin surface (update/approve/send/revise, bulk line items) sent no tokens — the entire mobile admin workflow would 422. Tokens threaded through mobile api layer + workspace/line-item screens with 409 refetch handling. tsc clean. MEDIUM-adjacent (scanner): - VendorProposalsController: the VendorTotalCost write on Proposal now bumps Version (was a silent lost-update path bypassing the guard). - FilesController: GeneratePDF audit staged into the same SaveChanges. LOW (detectors): - 409 envelope is schema-validated client-side (proposalConcurrencyConflictSchema.safeParse) and id-checked before seeding the react-query cache; malformed state degrades to invalidation (INJ-409-01/WEB-CONC-L1). - ProposalConcurrencyException.CurrentState typed ProposalResponse? so an EF entity can never serialize into the 409 body (SC-1). - Guard caller contract documented + GuardedEndpointAuthorizationTests reflection tripwire: guard-reaching endpoints must stay admin-gated (AUTHZ-CG-01). - Pre-check currentState now loads display navigations so both 409 paths return the same shape (CONC-L3). - Save chain's trailing getById failure no longer misreports a committed save; falls back to invalidation (CONC-L4). Also caught during fix verification: the handler's manual currentState serialization lacked JsonStringEnumConverter — enums would serialize as numbers, client schema validation would reject every guarded 409, and the state would always be discarded. Now matches the MVC pipeline and is pinned by a wire test. 193 xUnit / 70 vitest / 38 pytest green; mobile + shared tsc clean; Playwright smoke 2/2.
This commit is contained in:
parent
d6404edefc
commit
9632c1048c
20 changed files with 356 additions and 48 deletions
|
|
@ -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));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -40,9 +40,13 @@ 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)
|
||||
|
|
|
|||
|
|
@ -1,15 +1,19 @@
|
|||
using ProposalSystem.Application.DTOs;
|
||||
|
||||
namespace ProposalSystem.Application.Common;
|
||||
|
||||
/// <summary>
|
||||
/// 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 }).
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public class ProposalConcurrencyException : Exception
|
||||
{
|
||||
public object? CurrentState { get; }
|
||||
public ProposalResponse? CurrentState { get; }
|
||||
|
||||
public ProposalConcurrencyException(object? currentState)
|
||||
public ProposalConcurrencyException(ProposalResponse? currentState)
|
||||
: base("The record was modified by another user. Refresh and retry.")
|
||||
{
|
||||
CurrentState = currentState;
|
||||
|
|
|
|||
|
|
@ -86,7 +86,13 @@ public class LineItemService : ILineItemService
|
|||
|
||||
public async Task<IReadOnlyList<LineItemResponse>> 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)
|
||||
|
|
|
|||
|
|
@ -12,6 +12,13 @@ namespace ProposalSystem.Infrastructure.Services;
|
|||
/// 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.
|
||||
/// </summary>
|
||||
internal static class ProposalConcurrencyGuard
|
||||
{
|
||||
|
|
|
|||
|
|
@ -233,6 +233,7 @@ public class ProposalService : IProposalService
|
|||
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");
|
||||
|
|
@ -280,6 +281,7 @@ public class ProposalService : IProposalService
|
|||
{
|
||||
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");
|
||||
|
|
@ -310,6 +312,7 @@ public class ProposalService : IProposalService
|
|||
{
|
||||
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");
|
||||
|
|
@ -431,6 +434,9 @@ public class ProposalService : IProposalService
|
|||
{
|
||||
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");
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,44 @@
|
|||
using System.Reflection;
|
||||
using FluentAssertions;
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
using ProposalSystem.Api.Controllers;
|
||||
using Xunit;
|
||||
|
||||
namespace ProposalSystem.Tests.Controllers;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public class GuardedEndpointAuthorizationTests
|
||||
{
|
||||
public static readonly TheoryData<Type, string> 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<AuthorizeAttribute>(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");
|
||||
}
|
||||
}
|
||||
|
|
@ -98,17 +98,31 @@ public class GlobalExceptionHandlerTests
|
|||
// 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 (status, body) = await InvokeWith(
|
||||
new ProposalConcurrencyException(new { Id = "p1", Notes = "winner" }));
|
||||
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("p1");
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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<void> => {
|
||||
await apiClient.put(`/proposals/${id}`, data);
|
||||
): Promise<ProposalDetail> => {
|
||||
const res = await apiClient.put(`/proposals/${id}`, data);
|
||||
return res.data;
|
||||
},
|
||||
|
||||
approveProposal: async (id: string): Promise<void> => {
|
||||
await apiClient.post(`/proposals/${id}/approve`);
|
||||
approveProposal: async (
|
||||
id: string,
|
||||
proposalVersion?: string,
|
||||
): Promise<ProposalDetail> => {
|
||||
const res = await apiClient.post(`/proposals/${id}/approve`, {
|
||||
proposalVersion,
|
||||
});
|
||||
return res.data;
|
||||
},
|
||||
|
||||
sendProposal: async (id: string): Promise<void> => {
|
||||
await apiClient.post(`/proposals/${id}/send`);
|
||||
sendProposal: async (
|
||||
id: string,
|
||||
proposalVersion?: string,
|
||||
): Promise<ProposalDetail> => {
|
||||
const res = await apiClient.post(`/proposals/${id}/send`, {
|
||||
proposalVersion,
|
||||
});
|
||||
return res.data;
|
||||
},
|
||||
|
||||
reviseProposal: async (id: string): Promise<void> => {
|
||||
await apiClient.post(`/proposals/${id}/revise`);
|
||||
reviseProposal: async (
|
||||
id: string,
|
||||
proposalVersion?: string,
|
||||
): Promise<ProposalDetail> => {
|
||||
const res = await apiClient.post(`/proposals/${id}/revise`, {
|
||||
proposalVersion,
|
||||
});
|
||||
return res.data;
|
||||
},
|
||||
|
||||
getAudit: async (id: string): Promise<AuditEntry[]> => {
|
||||
|
|
|
|||
|
|
@ -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<void>;
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -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<LineItem[]> => {
|
||||
const res = await apiClient.put(`/proposals/${proposalId}/line-items`, {
|
||||
lineItems,
|
||||
proposalVersion,
|
||||
});
|
||||
return res.data;
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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<T> {
|
||||
|
|
|
|||
|
|
@ -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<ProposalDetail>([
|
||||
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 = () => {
|
||||
|
|
|
|||
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -60,7 +60,10 @@ function handleConcurrencyConflict(
|
|||
error: Error,
|
||||
): boolean {
|
||||
if (!(error instanceof ConflictError)) return false;
|
||||
if (error.currentState) {
|
||||
// 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);
|
||||
|
|
@ -108,10 +111,21 @@ export function useSaveProposalWorkspace(proposalId: string) {
|
|||
// 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.
|
||||
return proposalsApi.getById(proposalId);
|
||||
// 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: (fresh) => {
|
||||
queryClient.setQueryData(proposalsKeys.detail(proposalId), fresh);
|
||||
if (fresh) {
|
||||
queryClient.setQueryData(proposalsKeys.detail(proposalId), fresh);
|
||||
} else {
|
||||
queryClient.removeQueries({ queryKey: proposalsKeys.detail(proposalId) });
|
||||
}
|
||||
invalidateProposalViews(queryClient, proposalId);
|
||||
toast.success('Changes saved');
|
||||
},
|
||||
|
|
|
|||
|
|
@ -212,7 +212,36 @@ describe('API client interceptors', () => {
|
|||
});
|
||||
|
||||
it('409 with the guarded conflict envelope rejects with ConflictError carrying currentState', async () => {
|
||||
const currentState = { id: 'p-1', status: 'Approved', rowVersion: 'AAAAAAAAAAM=' };
|
||||
// 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,
|
||||
|
|
@ -231,6 +260,27 @@ describe('API client interceptors', () => {
|
|||
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: {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
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({
|
||||
|
|
@ -55,12 +56,16 @@ apiClient.interceptors.response.use(
|
|||
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. Throw a typed error so the
|
||||
// domain layer can refresh from currentState instead of just toasting.
|
||||
// { 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(
|
||||
data?.message || 'This record was modified by another user. Refresh and retry.',
|
||||
data?.currentState ?? null,
|
||||
parsed.success
|
||||
? parsed.data.message
|
||||
: data?.message || 'This record was modified by another user. Refresh and retry.',
|
||||
parsed.success ? parsed.data.currentState : null,
|
||||
),
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue