mirror of
https://github.com/Sea-Haven-Industries/shoc-backend.git
synced 2026-10-07 16:19:07 +00:00
f61aff06ff
Record adversarial review results and fixes for service notes, search, paging, and migration hardening.
276 lines
9.8 KiB
Markdown
276 lines
9.8 KiB
Markdown
# 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
|