xTr1m/GameList
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Extract admin and results workflows into services

Browse Source
This commit is contained in:
Frank Tovar
2026-02-07 01:06:22 +01:00
parent 5d40d555d1
commit 0d60108036
6 changed files with 377 additions and 314 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
REVIEW.md
View File

@@ -4,12 +4,12 @@
This codebase is functional and reasonably tested on backend behavior, but long-term change safety is currently limited by concentration risk (god modules), hidden side effects in read paths, and drifting operational contracts.
Progress update (as of February 6, 2026):
Progress update (as of February 7, 2026):
- Completed: phase reads are side-effect free with explicit reconciliation on write routes (`Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:62`).
- Completed: admin auth docs aligned to account-based admin sessions (`API.md:3`).
- Completed: build/test guardrails added (`.github/workflows/ci.yml`) and root ownership/setup docs added (`README.md:1`).
- Completed: backend validators centralized for suggestions and auth (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`).
- Completed: request safety hardened for redirects and forwarded headers (`Program.cs:42`, `Program.cs:106`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`).
- Completed: request safety hardened for redirects and forwarded headers (`Program.cs:44`, `Program.cs:108`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`).
Top 5 maintainability risks (priority order):
@@ -29,9 +29,9 @@ Top 5 maintainability risks (priority order):
- Hidden module coupling through globals: `wwwroot/js/data.js:131`-`wwwroot/js/data.js:134`, plus `window` callbacks consumed in `wwwroot/js/ui.js:473`, `wwwroot/js/ui.js:696`, `wwwroot/js/ui.js:1009`.
- Impact: every UI change risks regressions outside its feature area.
4. Service-layer extraction is partially complete; admin/results workflows still inline (High)
- Suggestion and vote workflows have moved to services (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`), but admin/results orchestration remains endpoint-heavy (`Endpoints/AdminEndpoints.cs:105`, `Endpoints/ResultsEndpoints.cs:30`).
- Impact: high cognitive load and slower, riskier feature changes.
4. Endpoint contract consistency and response shaping are still uneven (High)
- Service-layer extraction is now in place for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:7`), but response shapes are still mostly anonymous objects and ad-hoc error payloads.
- Impact: API evolution and client compatibility changes are still high-friction.
5. Static-analysis and frontend lint guardrails remain incomplete (Medium)
- Build/test CI exists (`.github/workflows/ci.yml`) and project content rules are fixed (`GameList.csproj:17`-`GameList.csproj:21`), but analyzers/lint/format gates are still absent.
@@ -71,13 +71,13 @@ Worst coupling points:
- `Endpoints/EndpointHelpers.cs` (84 call sites): de facto god module.
- `wwwroot/js/ui.js` + global `state` object (131 direct state references across JS modules).
- Suggestion and phase workflows span endpoint functions, helper functions, filters, and duplicated client-side checks.
- Operational behavior is split across `API.md` and runtime filters with contradictory contracts.
- Operational behavior is documented but still spread across multiple files (`API.md`, `IIS.md`, `README.md`) without a single versioned runbook artifact.
## C) Critical task list
[P0][Done] Make phase reads side-effect free and move reconciliation to explicit writes
- Problem: Severity `Critical`, Category `Architecture`. Read endpoints/filters previously relied on mutating phase reads. Impact: unsafe refactors and non-deterministic behavior.
- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:20`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsEndpoints.cs:26`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`.
- Evidence: `Endpoints/EndpointHelpers.cs:37`, `Endpoints/EndpointHelpers.cs:61`, `Endpoints/StateEndpoints.cs:20`, `Infrastructure/PhaseRequirementFilter.cs:17`, `Endpoints/ResultsWorkflowService.cs:9`, `GameList.Tests/StateTests.cs:236`, `GameList.Tests/FiltersTests.cs:55`.
- Recommendation: Split into `GetCurrentPhaseAsync` (pure read) and explicit `ReconcilePhaseAsync` (write command). Run reconciliation only on intentional transition points (admin toggle, phase change commands, migration job), not on GET paths.
- Acceptance criteria (testable): GET `/api/state` and GET `/api/me` never call `SaveChangesAsync`; integration tests verify no phase mutations occur during read-only requests; filters perform one phase check path without side effects.
- Effort / Risk: `M / Med`.
@@ -109,15 +109,15 @@ Worst coupling points:
[P0][Done] Harden request safety defaults (forwarded headers and redirect handling)
- Problem: Severity `High`, Category `Security`. Forwarded headers are trusted without explicit proxy/network allowlist, and image validation likely follows redirects despite "no redirects" policy.
- Evidence: `Program.cs:42`, `Program.cs:72`, `Program.cs:106`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`, `IIS.md:17`.
- Evidence: `Program.cs:44`, `Program.cs:74`, `Program.cs:108`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`, `IIS.md:17`.
- Recommendation: Configure known proxies/networks for forwarded headers; enforce `AllowAutoRedirect = false` in image validation client and add tests for redirect-chain and private-host edge cases.
- Acceptance criteria (testable): integration tests prove redirected URLs are rejected; forwarded header spoofing test fails when source is untrusted; documentation updated with trusted proxy requirements.
- Effort / Risk: `M / Med`.
- Dependencies (if any): none.
[P1][Partial] Extract service-layer workflows from endpoint lambdas
[P1][Done] Extract service-layer workflows from endpoint lambdas
- Problem: Severity `High`, Category `Architecture`. Endpoint files contain business orchestration, persistence, and policy logic inline; large lambdas are hard to reason about and reuse.
- Evidence: extraction completed for suggestions and votes (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Program.cs:37`, `Program.cs:38`); remaining inline orchestration is concentrated in `Endpoints/AdminEndpoints.cs:105`, `Endpoints/ResultsEndpoints.cs:30`.
- Evidence: extraction completed for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:7`, `Program.cs:37`, `Program.cs:38`, `Program.cs:39`, `Program.cs:40`).
- Recommendation: Introduce focused application services (`SuggestionService`, `VoteService`, `AdminWorkflowService`) and keep endpoints as transport adapters.
- Acceptance criteria (testable): endpoint handlers reduced to routing + DTO mapping + service calls; domain rule tests target service methods directly; endpoint tests remain green.
- Effort / Risk: `L / Med`.
@@ -149,7 +149,7 @@ Worst coupling points:
[P1] Make write workflows transaction-consistent and explicit
- Problem: Severity `Medium`, Category `Correctness/Architecture`. Several multi-step state changes rely on multiple DB commands without explicit transaction grouping.
- Evidence: `Endpoints/SuggestionWorkflowService.cs:71`, `Endpoints/SuggestionWorkflowService.cs:75`, `Endpoints/AdminEndpoints.cs:16`-`Endpoints/AdminEndpoints.cs:31`, `Endpoints/AdminEndpoints.cs:220`-`Endpoints/AdminEndpoints.cs:229`.
- Evidence: `Endpoints/SuggestionWorkflowService.cs:71`, `Endpoints/SuggestionWorkflowService.cs:75`, `Endpoints/AdminWorkflowService.cs:74`, `Endpoints/AdminWorkflowService.cs:208`, `Endpoints/AdminWorkflowService.cs:227`.
- Recommendation: Wrap multi-entity updates in explicit transactions where consistency matters, or refactor into idempotent command handlers with compensating behavior.
- Acceptance criteria (testable): fault-injection tests prove no partial state after exceptions; transaction boundaries documented per workflow.
- Effort / Risk: `M / Med`.
@@ -171,9 +171,9 @@ Worst coupling points:
- Effort / Risk: `M / Low`.
- Dependencies (if any): frontend module split helps.
[P2] Improve repository-level engineering docs and ownership map
- Problem: Severity `Low`, Category `Documentation`. There is no root README/architecture map/runbook tying module ownership, local setup, and deployment flow together.
- Evidence: root README absent; operational docs fragmented across `API.md`, `SPEC.md`, `IIS.md`, `scripts/deploy-ftp.ps1`.
[P2][Done] Improve repository-level engineering docs and ownership map
- Problem: Severity `Low`, Category `Documentation`. Contributor setup and ownership context needed consolidation.
- Evidence: root `README.md` now provides setup/ownership map and links to `API.md`, `SPEC.md`, and `IIS.md`.
- Recommendation: add concise `README.md` with architecture diagram, local run/test commands, operational boundaries, and links to deeper docs.
- Acceptance criteria (testable): new contributors can run app + tests using README only; docs align with live auth/deploy behavior.
- Effort / Risk: `S / Low`.
@@ -182,9 +182,9 @@ Worst coupling points:
## D) Quick wins vs strategic refactors
Quick wins (hours to 1 day each):
- Remove stale admin key wording in `API.md` and align with actual auth behavior.
- Add `Content Remove="GameList.Tests\**\*"` (or equivalent) and verify clean build output.
- Add a build check script that fails on warnings and run it locally/CI.
- Completed: removed stale admin key wording in `API.md` and aligned with actual auth behavior.
- Completed: added `Content Remove="GameList.Tests\**\*"` and verified clean build output.
- Completed: added CI build/test gate for warning regressions.
- Completed: removed dead `DeletePlayerRequest` from `Contracts/Dtos.cs`.
- Completed: removed unused endpoint handler parameters in `Endpoints/StateEndpoints.cs`.
- Remove dead UI references (`all-suggestions`, `nav-vote-next`) or add explicit TODO with owner/date.