From f61aff06ff5d1c8f8a16bcb47214ab3294d1dc35 Mon Sep 17 00:00:00 2001 From: Arthur Bassi Date: Fri, 17 Jul 2026 10:37:25 -0300 Subject: [PATCH] docs(work-orders): add Fase 2 staff review findings Record adversarial review results and fixes for service notes, search, paging, and migration hardening. --- docs/work-orders/staff-review-findings.md | 276 ++++++++++++++++++++++ 1 file changed, 276 insertions(+) create mode 100644 docs/work-orders/staff-review-findings.md diff --git a/docs/work-orders/staff-review-findings.md b/docs/work-orders/staff-review-findings.md new file mode 100644 index 0000000..2ae4210 --- /dev/null +++ b/docs/work-orders/staff-review-findings.md @@ -0,0 +1,276 @@ +# Staff Review Findings — Work Orders (Fase 2) + +Adversarial reviews per delivery. Do not treat preliminary inventário notes as final. + +--- + +## Entrega A — Structured Service Notes / POC / Tech-Vendor + +### How it works + +1. **Create** `POST /api/workorders/board`: FluentValidation → resolve `primaryService`/`trade` → serialize `extraServices` JSON → persist POC/service/tech/vendor on `WorkOrder`; if vendor/appt present, create `Dispatch` and store tech/vendor notes there instead of WO. +2. **Patch** `PATCH .../board` with `pm` | `serviceNotes` | `extraServices`. +3. **Read**: board projection prefers WO POC fields over `WorkOrderContacts`; tech phone = Dispatch.TechPhone → WO.TechPhone → Vendor.Phone. Detail exposes `serviceNotes`, `extraServices`, `vendorNotes`. + +### Domain invariants + +- Extra services require a primary service on create. +- Tech/vendor notes live on dispatch when a vendor dispatch exists; otherwise on WO. +- Board POC free-text overrides linked contact projection. + +### Half-done + +- No board PATCH for `pocName` / `pocPhone` / `pocNotes` / `techPhone` / `vendorNotes` (create-only for those). +- `ParseExtraServices` duplicated in board + detail services. +- Create validation file has excessive blank lines (noise). +- Test `PatchField_ExtraServicesWithoutPrimary_Throws` disagrees with promotion logic in `ApplyExtraServices`. + +### Findings + +#### A1 — ExtraServices without primary: test vs promotion + +- **Severity:** High +- **File:** `SeaHaven.Services/Implementation/WorkOrderBoardUpdateService.cs` (~294–298); test `WorkOrderBoardUpdateServiceTests.cs` (~331) +- **Explanation:** Create requires primary when extras exist; PATCH promotes first extra to `Trade` without auditing Trade. Test expects `InvalidValue`. +- **Scenario:** Legacy WO with empty Trade; FE patches `extraServices` only. +- **Repro:** PATCH `extraServices=["Filter change"]` on WO with null Trade. +- **Fix:** Reject when Trade empty (align create + test); do not promote silently. +- **When:** **Now** (blocks green CI / domain consistency). + +#### A2 — ExtraServices JSON can exceed nvarchar(2000) + +- **Severity:** High +- **File:** Create `SerializeExtraServices`; Update `ApplyExtraServices` +- **Explanation:** Per-item max 128; no check on serialized JSON length vs column 2000. +- **Scenario:** Many extras → `DbUpdateException` 500. +- **Repro:** PATCH/create with ~20×128-char labels. +- **Fix:** After serialize, throw `InvalidValue` if length > 2000. +- **When:** **Now**. + +#### A3 — Create does not cap ExtraServices JSON length / per-item in serializer + +- **Severity:** Medium +- **File:** `WorkOrderBoardCreateService.SerializeExtraServices` +- **Explanation:** FluentValidation caps each item at 128 but create serializer does not re-validate length or total JSON; update path has item length check, create does not. +- **Fix:** Share normalize helper; enforce item + JSON caps on create. +- **When:** **Now** (with A2). + +#### A4 — POC / tech / vendor notes not patchable on board + +- **Severity:** Medium +- **File:** `WorkOrderBoardFieldNames.cs` / UpdateService +- **Explanation:** Create persists structured POC/tech/vendor; no PATCH fields → FE cannot edit after create without another API. +- **When:** **Later** if FE has another edit path; otherwise before FE ship. + +#### A5 — Dual POC sources can diverge + +- **Severity:** Low +- **File:** CreateService + Projection +- **Explanation:** `PocContactId` + free-text both allowed; board prefers free-text; contact row may hold different notes. +- **When:** Later (document or clear contact notes when free-text set). + +#### A6 — Duplicate ParseExtraServices + +- **Severity:** Low +- **File:** `WorkOrderBoardService.cs`, `WorkOrderDetailService.cs` +- **When:** Later (extract helper). + +### Verdict A + +**Merge blocked** until A1–A3 fixed. A4 is a product gap for FE edit flows. + +--- + +## Entrega B — Flag Color + +### How it works + +PATCH `flagColor` → normalize `#RRGGBB` → whitelist palette → persist on WO → project on board/detail. Distinct from dispatcher avatar `color`. + +### Half-done + +- None material. Migration + model + PATCH + tests present. + +### Findings + +#### B1 — No finding Critical/High + +Palette enforcement and clear semantics are consistent with tests. + +#### B2 — Snapshot/model coupled with Entrega A + +- **Severity:** Low (process) +- **Explanation:** Same `WorkerOrder` / snapshot file as service notes; branch split needs careful ordering. +- **When:** During organize. + +### Verdict B + +**OK to merge** after A schema lands (or same branch ordered commits). No code blockers unique to B. + +--- + +## Entrega C — Search / overdue / paging + +### How it works + +Advanced search: normalize page (≥0), pageSize≤100, resolve Mon–Fri presets, whitelist sortBy, apply type/overdue/unscheduled filters, project board rows. Board GET maps `ArgumentException` → 400. + +### Half-done / risks + +- Shared `PagedResult` flipped to 0-based helpers while Location/Contact/etc. still use 1-based `page` defaults. + +### Findings + +#### C1 — PagedResult 0-based breaks 1-based APIs + +- **Severity:** Critical +- **File:** `SeaHaven.Services/DTOs/ContactDTOs.cs` +- **Explanation:** `HasPrevious`/`HasNext` assume 0-based; Location etc. pass `page=1` on first page → `HasPrevious=true` incorrectly; last-page `HasNext` wrong for 0-based search too if mixed. +- **Scenario:** Any paged Location/Contact/Vendor list UI using helpers. +- **Fix:** Restore 1-based helpers by default; opt-in 0-based for WO search only (`PageBase` or dedicated DTO). +- **When:** **Now**. + +#### C2 — Unscheduled always included in date range + +- **Severity:** High +- **File:** `WorkOrderBoardQueryFilters.ApplyDateRangeFilter` +- **Explanation:** Every advanced search with a date range also returns all unscheduled non-terminal WOs → unbounded result growth. +- **Scenario:** Narrow custom range + large backlog of undated WOs. +- **Fix:** Explicit `includeUnscheduled` or cap; confirm FE intent. +- **When:** Confirm with FE; fix before prod if unintentional. + +#### C3 — WorkOrderType.Other = overdue sentinel + +- **Severity:** High (domain) +- **File:** `WorkOrderBoardQueryFilters.ApplyTypeAndOverdueFilter` +- **Explanation:** Cannot filter real `Other` types via `types=99`. +- **When:** Document now; long-term separate `isPastDue` query param. + +#### C4 — Label search `label.Contains(s)` matches short tokens broadly + +- **Severity:** Medium +- **File:** `WorkOrderBoardSearchFilter` +- **When:** Later / monitor perf. + +#### C5 — Overdue uses UTC date vs possibly local scheduled dates + +- **Severity:** Medium +- **File:** QueryFilters / search filter +- **When:** Later if US sites report off-by-one day. + +#### C6 — Search helper methods not EF-translatable (FIXED) + +- **Severity:** Critical +- **File:** `WorkOrderBoardSearchFilter.Apply` +- **Explanation:** Refactor extracted `Matches*` private methods; EF Core InMemory/SQL cannot translate them → all text searches throw at runtime. +- **Repro:** Any `search` with length ≥ 2 on board/search. +- **Fix:** Inline predicates in the `Where` expression (applied). +- **When:** **Now** — fixed during Fase 2. + +### Verdict C + +**Merge blocked** on C1 (fixed via `PageBase`) and C6 (fixed via inline predicates). C2 needs product confirmation. Prefer separate PR. + +--- + +## Entrega D — Comments edit + display names + +### How it works + +`GetDisplayNamesByIdsAsync` → comment/audit projections. PATCH comment: author or Admin; General+WorkOrder only; read-only WO blocked. + +### Half-done + +- Comment edit not written to audit log. + +### Findings + +#### D1 — Comment updates leave no audit trail + +- **Severity:** Medium +- **File:** `WorkOrderCommentService.UpdateCommentAsync` +- **When:** Later if compliance requires it; note in PR. + +#### D2 — No max length on comment text on update + +- **Severity:** Low (pre-existing on create) +- **When:** Later. + +### Verdict D + +**OK to merge** with D1 noted. No Critical/High code defects found. + +--- + +## Entrega E — Sites + Vendor ContactName + +### How it works + +`GET /api/locations/sites?search=` → Id/Code/City/State (`Code` ← `Locations.Name`). Vendor Dropdown adds `contactName`. + +### Findings + +#### E1 — GetSites unbounded + +- **Severity:** Medium +- **File:** `LocationDataService.GetSiteOptionsAsync` +- **Explanation:** No Take/limit; large Locations table → heavy payload. +- **When:** Later or add Take(500) if volume unknown. + +#### E2 — No soft-delete filter + +- **Severity:** Low +- **Explanation:** Locations inherit audit entity; if soft-delete used elsewhere, sites may include deleted. +- **When:** Later if soft-delete applies to Locations. + +### Verdict E + +**OK to merge** independently. + +--- + +## Entrega F1 — Phase4 migration hardening + +### How it works + +Before creating indexes, alter `SiteCode`/`InternalWONumber` to nvarchar(450). + +### Findings + +#### F1-1 — Editing already-shipped migration + +- **Severity:** High (ops) +- **File:** `20260624200000_Phase4_SearchIndexes.cs` +- **Explanation:** Environments that already applied old `Up` will not re-run. Failed partial applies need manual repair or a **new** migration. +- **When:** Verify `__EFMigrationsHistory` per environment **before** merge. Prefer new corrective migration if any env already applied. + +### Verdict F1 + +**Conditional merge** after env check. Apply on `feat/work-orders-phase-4` only. + +--- + +## Entrega F2 — Phase6 migration hardening + +### How it works + +Idempotent SQL for Category column, CompletionDocTemplates, index; EF attributes for discovery. + +### Findings + +#### F2-1 — Same applied-migration caveat as F1 + +- **Severity:** High (ops) +- **When:** Verify env history; idempotent SQL helps partial DBs if migration re-run manually, not if EF skips applied migrations. + +### Verdict F2 + +**Conditional merge** on slideover branch after env check. + +--- + +## Cross-cutting mandatory fixes before organize + +1. A1 — ExtraServices without primary throws +2. A2/A3 — JSON length ≤ 2000 +3. C1 — PagedResult 1-based default + WO search 0-based opt-in