shoc-backend/docs/refactor-backend-findings.md

# Backend architecture refactor — findings

Tracking doc for `refactor/backend-architecture-cleanup`. Each item is verified
against the actual code (not just a grep hit) before being listed here.
Checked off as fixed, with tests, on this branch.

## 1. Layering / dependency-injection violations

- [ ] `AssetController` injects `UserManager<ApplicationUser>` via constructor
      but never uses it anywhere in the file — dead dependency, remove it.
- [ ] `ContactController` depends on both `IContactService` **and**
      `ILocationService` and implements full Location CRUD (`GetAllLocationsAsync`,
      `CreateLocationAsync`, `UpdateLocationAsync`, `DeleteLocationAsync`,
      `GetLocationsPagedAsync`) even though a dedicated `LocationController`
      with the same `ILocationService` already exists. Move the Location
      endpoints out of `ContactController` into `LocationController`; a
      Contact controller should depend only on `IContactService`.
- [ ] `ArchitectureTests` (G2) currently passes with 0 failures — dependency
      *direction* is otherwise clean (no controller/service injects
      `DbContext`/`IConfiguration`/a concrete class). Re-run after each fix
      above to confirm it stays green.

## 2. Validation coverage gap

Established pattern: `I{Action}{Feature}Validation : IValidator<{Dto}>` +
`{Action}{Feature}Validation : AbstractValidator<{Dto}>` in
`SeaHaven.Services/Validation/`, injected into the owning business service
(see `LocationValidation.cs` / `LocationService`). Confirmed: **validation
stays in the service layer** (not moved to data services) — only extending
coverage to features that lack it.

Have a validator today: Account, Asset, Contact, Dispatch, Employee, FollowUp,
Location, PMSchedule, Vendor, WorkOrder, WorkOrderBoardCreate.

Confirmed missing (real external Create/Update DTO, no validator):
- [ ] `CalendarService.CreateEventAsync` / `UpdateEventAsync`
- [ ] `CompletionDocTemplateService.CreateAsync` / `UpdateAsync`
- [ ] `QuotesService.CreateQuoteAsync` / `UpdateQuoteAsync`
- [ ] `ServicesRegistryService.CreateAsync` / `UpdateAsync`
- [ ] `TaskListTemplateService.CreateAsync` / `UpdateAsync`
- [ ] `TeamMemberInviteService.CreateAsync`
- [ ] `TeamMemberService.CreateAsync` / `UpdateAsync`
- [ ] `VendorCompanyRosterService.CreateRosterAsync`
- [ ] `VendorOperationsService.UpdateAvailabilityAsync` / `UpdateCommercialStatusAsync`
- [ ] `VendorPortalService.UpdateChecklistItemAsync`
- [ ] `WorkOrderCommentService.UpdateCommentAsync`
- [ ] `WorkOrderDispatchService.UpdateDispatchAsync` / `UpdateChecklistItemAsync`
- [ ] `WorkOrderMediaService.UpdateMediaCategoryAsync`
- [ ] `WorkOrderPocService.UpdatePocAsync`
- [ ] `WorkOrderUpliftService.CreateAsync`
- [ ] `DropdownOptionsService.CreateAsync` / `UpdateAsync`
- [ ] `AuthenticationService.UpdateProfileAsync`

Ruled out (internal/no external DTO, not a validator candidate):
`AuthenticationService.CreateSessionAsync`, `VendorPortalTokenService.GetOrCreateActiveTokenAsync`,
`WorkOrderAccountResolver` (no real Create/Update method; grep false positive).

## 3. N+1 / query-shape issues (verified, not grep-only)

- [ ] **`WorkOrderService.DeleteWorkOrderCascadeAsync`** loads all comments
      then calls `_commentDataService.DeleteAsync(comment.Id)` once per
      comment. `CommentDataService.DeleteAsync` does its own `GetByIdAsync`
      (1 SELECT) + `SaveChangesAsync` (1 commit) **per call** — N comments =
      2N round trips and N separate commits, breaking the one-commit
      convention (§4) as well as being slow. Fix: add a batch delete
      (`DeleteAllForWorkOrderAsync` via `ExecuteDeleteAsync`, or load once +
      `RemoveRange` + one `SaveChangesAsync`) and call it once.
- [ ] **`WorkOrderService.SaveAttachmentsAndPhotos`** loops over uploaded
      attachments calling `_workOrderDataService.AddWorkOrderAttachmentAsync`
      per file; that method does `AddAsync` + `SaveChangesAsync` **per call** —
      N attachments = N separate commits. Fix: collect the `WorkOrderAttachments`
      entities in the loop (file storage save can stay per-file — that's I/O,
      not DB), then add a batch method that calls `AddRangeAsync` + one
      `SaveChangesAsync`.
- [ ] **`VendorCompanyRosterDataService`** (3 call sites: roster update,
      create, and a third) loads `existingTechnicians` once via a single
      query, then does `existingTechnicians.FirstOrDefault(e => e.Id == ...)`
      **inside** a loop over the incoming roster — O(n·m) in-memory scan.
      Fix: build `existingTechnicians.ToDictionary(e => e.Id)` once before the
      loop and `TryGetValue` inside it — O(n+m).

## Ruled out during the sweep (false positives / legitimate design)

- `WorkOrderAuditDataService.PersistAsync`: the `foreach` detaches conflicting
  entries from a caught `DbUpdateException`; the retry `SaveChangesAsync` is
  after the loop, not inside it. Not an issue.
- `VendorPortalTokenService.RotateAsync` / `RevokeAllAsync`: the `foreach`
  only mutates already-tracked entities in memory (`t.RevokedAt = now`); the
  single `SaveChangesAsync` is after the loop. Not an issue.
- `WorkOrderWeekRolledService`: calls `TryProcessWeekRolledAsync` once per
  work order inside a batch-job loop, each with its own try/catch. This is
  intentional per-item fault isolation for a background job — collapsing it
  into one transaction would change the failure semantics (one bad work order
  would abort the whole batch). Not an issue; do not "optimize" this away.
- Legacy `SeaHavenIndustries` project (old Blazor admin app) has its own
  ad-hoc `Service` classes touching `DbContext` directly, violating the
  layering model wholesale — but it is being actively sunset (see commit
  `64ca9e0`, "add legacy deprecation middleware and Blazor WO sunset guard").
  Out of scope: refactoring code on its way out is wasted effort.

## Still to audit

- Cancellation-token forwarding (§6) on data-service methods — not yet swept.
- Error disclosure (§5) — spot-check only so far (`VendorCompanyRosterController`
  catches `DbUpdateConcurrencyException` directly; check whether that
  duplicates the existing `ConcurrencyExceptionFilter`).
- Tenant-scope derivation (§2) — not yet swept.
- `AsNoTracking()` coverage on read-only data-service queries — not yet swept.