Finalize API envelopes and close validation drift tasks
This commit is contained in:
@@ -54,3 +54,28 @@ public record ResultItemDto(
|
||||
IReadOnlyList<int> LinkedIds,
|
||||
IReadOnlyList<string> LinkedTitles
|
||||
);
|
||||
|
||||
public record AuthSessionResponse(Guid Id, string Username, string? DisplayName, bool IsAdmin);
|
||||
|
||||
public record StateSummaryResponse(
|
||||
Phase CurrentPhase,
|
||||
bool VotesFinal,
|
||||
bool HasJoker,
|
||||
bool ResultsOpen,
|
||||
DateTimeOffset UpdatedAt,
|
||||
int Players,
|
||||
int Suggestions,
|
||||
int Votes
|
||||
);
|
||||
|
||||
public record MeResponse(
|
||||
Guid Id,
|
||||
string Username,
|
||||
string? DisplayName,
|
||||
bool IsAdmin,
|
||||
Phase CurrentPhase,
|
||||
bool VotesFinal,
|
||||
bool HasJoker
|
||||
);
|
||||
|
||||
public record PhaseTransitionResponse(Phase CurrentPhase, bool ResultsOpen);
|
||||
|
||||
@@ -36,7 +36,7 @@ public static class AdminEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.LinkSuggestionsAsync(player, request);
|
||||
});
|
||||
@@ -45,7 +45,7 @@ public static class AdminEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.UnlinkSuggestionsAsync(player, request);
|
||||
});
|
||||
|
||||
@@ -51,13 +51,12 @@ public static class AuthEndpoints
|
||||
|
||||
await PlayerIdentityExtensions.SignInPlayerAsync(ctx, player);
|
||||
|
||||
return Results.Ok(new
|
||||
{
|
||||
return Results.Ok(new AuthSessionResponse(
|
||||
player.Id,
|
||||
player.Username,
|
||||
player.DisplayName,
|
||||
player.IsAdmin
|
||||
});
|
||||
));
|
||||
});
|
||||
|
||||
group.MapPost("/login", async ([FromBody] LoginRequest request, HttpContext ctx, AppDbContext db) =>
|
||||
@@ -79,13 +78,12 @@ public static class AuthEndpoints
|
||||
|
||||
await PlayerIdentityExtensions.SignInPlayerAsync(ctx, player);
|
||||
|
||||
return Results.Ok(new
|
||||
{
|
||||
return Results.Ok(new AuthSessionResponse(
|
||||
player.Id,
|
||||
player.Username,
|
||||
player.DisplayName,
|
||||
player.IsAdmin
|
||||
});
|
||||
));
|
||||
});
|
||||
|
||||
group.MapPost("/logout", async (HttpContext ctx) =>
|
||||
|
||||
@@ -16,7 +16,7 @@ public static class ResultsEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.GetResultsAsync(player);
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
using GameList.Data;
|
||||
using GameList.Domain;
|
||||
using GameList.Contracts;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
|
||||
namespace GameList.Endpoints;
|
||||
@@ -14,21 +15,20 @@ public static class StateEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var state = await db.AppState.AsNoTracking().FirstAsync();
|
||||
var phase = EndpointHelpers.GetCurrentPhase(player.CurrentPhase, state.ResultsOpen);
|
||||
var summary = new
|
||||
{
|
||||
CurrentPhase = phase,
|
||||
var summary = new StateSummaryResponse(
|
||||
phase,
|
||||
player.VotesFinal,
|
||||
player.HasJoker,
|
||||
state.ResultsOpen,
|
||||
state.UpdatedAt,
|
||||
Players = await db.Players.CountAsync(),
|
||||
Suggestions = await db.Suggestions.CountAsync(),
|
||||
Votes = await db.Votes.CountAsync()
|
||||
};
|
||||
await db.Players.CountAsync(),
|
||||
await db.Suggestions.CountAsync(),
|
||||
await db.Votes.CountAsync()
|
||||
);
|
||||
return Results.Ok(summary);
|
||||
});
|
||||
|
||||
@@ -36,27 +36,26 @@ public static class StateEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var state = await db.AppState.AsNoTracking().FirstAsync();
|
||||
var phase = EndpointHelpers.GetCurrentPhase(player.CurrentPhase, state.ResultsOpen);
|
||||
return Results.Ok(new
|
||||
{
|
||||
return Results.Ok(new MeResponse(
|
||||
player.Id,
|
||||
player.DisplayName,
|
||||
player.Username,
|
||||
player.DisplayName,
|
||||
player.IsAdmin,
|
||||
CurrentPhase = phase,
|
||||
phase,
|
||||
player.VotesFinal,
|
||||
player.HasJoker
|
||||
});
|
||||
));
|
||||
});
|
||||
|
||||
group.MapPost("/me/phase/next", async (HttpContext ctx, AppDbContext db) =>
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var appState = await db.AppState.FirstAsync();
|
||||
var reconciled = EndpointHelpers.ReconcilePlayerPhase(player, appState.ResultsOpen);
|
||||
@@ -72,18 +71,14 @@ public static class StateEndpoints
|
||||
player.CurrentPhase = next;
|
||||
player.VotesFinal = false; // moving forward clears any prior finalize
|
||||
await db.SaveChangesAsync();
|
||||
return Results.Ok(new
|
||||
{
|
||||
player.CurrentPhase,
|
||||
appState.ResultsOpen
|
||||
});
|
||||
return Results.Ok(new PhaseTransitionResponse(player.CurrentPhase, appState.ResultsOpen));
|
||||
});
|
||||
|
||||
group.MapPost("/me/phase/prev", async (HttpContext ctx, AppDbContext db) =>
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var isAdmin = await EndpointHelpers.IsAdmin(ctx, db);
|
||||
if (!isAdmin)
|
||||
@@ -96,11 +91,7 @@ public static class StateEndpoints
|
||||
player.CurrentPhase = PrevPhase(player.CurrentPhase);
|
||||
player.VotesFinal = false;
|
||||
await db.SaveChangesAsync();
|
||||
return Results.Ok(new
|
||||
{
|
||||
player.CurrentPhase,
|
||||
appState.ResultsOpen
|
||||
});
|
||||
return Results.Ok(new PhaseTransitionResponse(player.CurrentPhase, appState.ResultsOpen));
|
||||
});
|
||||
|
||||
}
|
||||
|
||||
@@ -15,7 +15,7 @@ public static class SuggestEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.GetMineAsync(player);
|
||||
});
|
||||
@@ -24,7 +24,7 @@ public static class SuggestEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.CreateAsync(player, request);
|
||||
}).AddEndpointFilter(new PhaseOrJokerFilter());
|
||||
@@ -33,7 +33,7 @@ public static class SuggestEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var isAdmin = await EndpointHelpers.IsAdmin(ctx, db);
|
||||
return await service.DeleteAsync(player, isAdmin, id);
|
||||
@@ -43,7 +43,7 @@ public static class SuggestEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var isAdmin = player.IsAdmin;
|
||||
return await service.UpdateAsync(player, isAdmin, id, request);
|
||||
@@ -53,7 +53,7 @@ public static class SuggestEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.GetAllAsync(player);
|
||||
});
|
||||
|
||||
@@ -15,7 +15,7 @@ public static class VoteEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.GetMineAsync(player);
|
||||
});
|
||||
@@ -24,7 +24,7 @@ public static class VoteEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
return await service.UpsertAsync(player, request);
|
||||
});
|
||||
|
||||
@@ -32,7 +32,7 @@ public static class VoteEndpoints
|
||||
{
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(ctx, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
return await service.SetFinalizeAsync(player, request);
|
||||
});
|
||||
|
||||
@@ -161,6 +161,10 @@ public class AuthTests
|
||||
|
||||
var resp = await player.GetAsync("/api/admin/vote-status");
|
||||
Assert.Equal(HttpStatusCode.Unauthorized, resp.StatusCode);
|
||||
var json = await resp.Content.ReadFromJsonAsync<JsonElement>();
|
||||
Assert.Equal("Unauthorized", json.GetProperty("title").GetString());
|
||||
Assert.Equal("Unauthorized", json.GetProperty("detail").GetString());
|
||||
Assert.Equal("Unauthorized", json.GetProperty("error").GetString());
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
||||
@@ -212,7 +212,7 @@ public class StateTests
|
||||
await using var factory = new TestWebApplicationFactory();
|
||||
var anon = factory.CreateClient();
|
||||
var unauthorized = await anon.GetAsync("/api/state");
|
||||
Assert.NotEqual(HttpStatusCode.OK, unauthorized.StatusCode);
|
||||
Assert.Equal(HttpStatusCode.Unauthorized, unauthorized.StatusCode);
|
||||
|
||||
var client = factory.CreateClientWithCookies();
|
||||
await client.RegisterAsync("counting");
|
||||
@@ -224,6 +224,27 @@ public class StateTests
|
||||
Assert.True(suggestions.GetInt32() >= 1);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task State_endpoint_with_stale_cookie_returns_unauthorized_and_clears_cookie()
|
||||
{
|
||||
await using var factory = new TestWebApplicationFactory();
|
||||
var client = factory.CreateClientWithCookies();
|
||||
await client.RegisterAsync("stale");
|
||||
|
||||
await factory.WithDbContextAsync(async db =>
|
||||
{
|
||||
var player = await db.Players.FirstAsync();
|
||||
db.Players.Remove(player);
|
||||
await db.SaveChangesAsync();
|
||||
});
|
||||
|
||||
var resp = await client.GetAsync("/api/state");
|
||||
|
||||
Assert.Equal(HttpStatusCode.Unauthorized, resp.StatusCode);
|
||||
Assert.True(resp.Headers.TryGetValues("Set-Cookie", out var cookies));
|
||||
Assert.Contains(cookies, c => c.Contains("player=", StringComparison.OrdinalIgnoreCase));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Health_endpoint_ok()
|
||||
{
|
||||
|
||||
@@ -12,7 +12,7 @@ public class AdminOnlyFilter : IEndpointFilter
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(httpContext, db);
|
||||
if (player?.IsAdmin != true)
|
||||
{
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
}
|
||||
|
||||
return await next(context);
|
||||
|
||||
@@ -16,7 +16,7 @@ public class PhaseOrJokerFilter : IEndpointFilter
|
||||
var db = httpContext.RequestServices.GetRequiredService<AppDbContext>();
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(httpContext, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
|
||||
var allow = phase == Phase.Suggest || (phase == Phase.Vote && player.HasJoker);
|
||||
|
||||
@@ -12,7 +12,7 @@ public class PhaseRequirementFilter(Phase required, bool allowAdminOverride = fa
|
||||
var db = httpContext.RequestServices.GetRequiredService<AppDbContext>();
|
||||
var player = await EndpointHelpers.GetAuthenticatedPlayer(httpContext, db);
|
||||
if (player is null)
|
||||
return Results.Unauthorized();
|
||||
return EndpointHelpers.UnauthorizedError();
|
||||
|
||||
var phase = await EndpointHelpers.GetCurrentPhaseAsync(db, player.Id);
|
||||
if (phase != required && !(allowAdminOverride && player.IsAdmin))
|
||||
|
||||
35
REVIEW.md
35
REVIEW.md
@@ -10,7 +10,7 @@ Progress update (as of February 7, 2026):
|
||||
- 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:44`, `Program.cs:108`, `Endpoints/EndpointHelpers.cs:105`, `GameList.Tests/HelperTests.cs:121`, `GameList.Tests/HelperTests.cs:219`).
|
||||
- In progress: API/response contract standardization now includes typed response DTOs, shared `ProblemDetails` helpers, and client parsing compatibility (`Contracts/Responses.cs:5`, `Endpoints/EndpointHelpers.cs:90`, `wwwroot/js/api.js:25`).
|
||||
- Completed: API/response contract standardization now includes typed auth/state/workflow DTOs and shared `ProblemDetails` envelopes across endpoint/filter paths (`Contracts/Responses.cs:58`, `Endpoints/AuthEndpoints.cs:54`, `Endpoints/StateEndpoints.cs:22`, `Infrastructure/AdminOnlyFilter.cs:15`, `Endpoints/EndpointHelpers.cs:90`, `wwwroot/js/api.js:25`, `GameList.Tests/AuthTests.cs:164`).
|
||||
|
||||
Top 5 maintainability risks (priority order):
|
||||
|
||||
@@ -20,9 +20,9 @@ Top 5 maintainability risks (priority order):
|
||||
- Impact: hard-to-debug regressions and fragile refactors in UI workflows.
|
||||
|
||||
2. Rule duplication still present between backend/frontend validations (High)
|
||||
- Suggestion validation is centralized on the backend (`Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/SuggestionValidator.cs:7`) but frontend still duplicates parts (`wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`).
|
||||
- Auth validation is centralized on the backend (`Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`) while frontend length checks remain duplicated (`wwwroot/app.js:92`, `wwwroot/app.js:121`).
|
||||
- Impact: inconsistent behavior and repeated fixes across server/client.
|
||||
- Suggestion and auth validation are centralized on the backend (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`).
|
||||
- Frontend now keeps UX-only prechecks (required credentials and min/max player input affordances) instead of server-side policy duplication (`wwwroot/app.js:92`, `wwwroot/app.js:119`, `wwwroot/js/ui.js:645`).
|
||||
- Impact: lower drift risk, though UI-side range hints still require occasional sync with server limits.
|
||||
|
||||
3. High-change, high-complexity frontend hotspots (High)
|
||||
- Git churn: `wwwroot/app.js` (76 changes), `wwwroot/js/ui.js` (55), `wwwroot/js/i18n.js` (50).
|
||||
@@ -31,8 +31,9 @@ Top 5 maintainability risks (priority order):
|
||||
- Impact: every UI change risks regressions outside its feature area.
|
||||
|
||||
4. Endpoint contract consistency and error 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:8`), and typed response DTO adoption is underway (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:35`), but not all auth/filter paths emit uniform error envelopes.
|
||||
- Impact: API evolution and client compatibility changes are still high-friction.
|
||||
- Service-layer extraction is in place for suggestions, votes, admin, and results (`Endpoints/SuggestionWorkflowService.cs:8`, `Endpoints/VoteWorkflowService.cs:8`, `Endpoints/AdminWorkflowService.cs:8`, `Endpoints/ResultsWorkflowService.cs:8`), and typed response DTOs now cover auth/state/workflow payloads (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:58`, `Endpoints/AuthEndpoints.cs:54`, `Endpoints/StateEndpoints.cs:22`).
|
||||
- Endpoint/filter unauthorized paths now use uniform `ProblemDetails` envelopes (`Infrastructure/AdminOnlyFilter.cs:15`, `Infrastructure/PhaseRequirementFilter.cs:15`, `Infrastructure/PhaseOrJokerFilter.cs:19`, `Endpoints/SuggestEndpoints.cs:18`, `Endpoints/VoteEndpoints.cs:18`, `Endpoints/ResultsEndpoints.cs:19`).
|
||||
- Impact: API evolution risk is reduced; remaining inconsistency is framework-generated 401 challenge bodies for fully unauthenticated requests.
|
||||
|
||||
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.
|
||||
@@ -100,11 +101,11 @@ Worst coupling points:
|
||||
- Effort / Risk: `M / Med`.
|
||||
- Dependencies (if any): none.
|
||||
|
||||
[P0][Partial] Centralize validation rules to stop backend/frontend drift
|
||||
- Problem: Severity `High`, Category `Complexity/Duplication`. Validation rules are duplicated in multiple backend endpoints and frontend forms. Impact: inconsistent behavior and repeated fixes.
|
||||
- Evidence: backend centralized in `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`, `Endpoints/AuthValidator.cs:11`; frontend duplicates remain in `wwwroot/js/ui.js:648`, `wwwroot/js/ui.js:1019`, `wwwroot/app.js:92`.
|
||||
- Recommendation: Extract backend validators (e.g., `SuggestionValidator`, `AuthValidator`) and reuse in create/update paths; simplify frontend to UX-only prechecks and rely on server responses for source-of-truth.
|
||||
- Acceptance criteria (testable): create/update share one backend validator path; tests cover validator once and both endpoints; frontend no longer re-implements server-only security rules.
|
||||
[P0][Done] Centralize validation rules to stop backend/frontend drift
|
||||
- Problem: Severity `High`, Category `Complexity/Duplication`. Validation rules were duplicated in multiple backend endpoints and frontend forms, increasing drift risk.
|
||||
- Evidence: backend validators are centralized (`Endpoints/SuggestionValidator.cs:7`, `Endpoints/AuthValidator.cs:11`, `Endpoints/SuggestionWorkflowService.cs:39`, `Endpoints/SuggestionWorkflowService.cs:115`, `Endpoints/AuthEndpoints.cs:18`, `Endpoints/AuthEndpoints.cs:65`); frontend server-policy duplicates removed for auth length and image host/url security checks (`wwwroot/app.js:92`, `wwwroot/app.js:119`, `wwwroot/js/ui.js:645`).
|
||||
- Recommendation: Keep backend validators as the source of truth; limit frontend checks to UX hints and required-field guidance.
|
||||
- Acceptance criteria (testable): create/update share one backend validator path; tests cover validator-backed routes; frontend no longer re-implements server-only security rules.
|
||||
- Effort / Risk: `M / Med`.
|
||||
- Dependencies (if any): none.
|
||||
|
||||
@@ -142,7 +143,7 @@ Worst coupling points:
|
||||
|
||||
[P1] Remove legacy/dead paths to reduce cognitive load
|
||||
- Problem: Severity `Medium`, Category `Other`. Legacy `Reveal` phase and dead UI hooks remain in active code, increasing confusion.
|
||||
- Evidence: `Domain/Phase.cs:6`, `Endpoints/StateEndpoints.cs:107`, `wwwroot/js/data.js:30`, `wwwroot/js/ui.js:156`, `wwwroot/js/ui.js:1191`.
|
||||
- Evidence: `Domain/Phase.cs:6`, `Endpoints/StateEndpoints.cs:99`, `wwwroot/js/data.js:30`, `wwwroot/js/ui.js:156`, `wwwroot/js/ui.js:1149`.
|
||||
- Recommendation: Remove obsolete phase enum/value handling and dead UI references (`all-suggestions`, `nav-vote-next`).
|
||||
- Acceptance criteria (testable): no references to removed phase/UI ids remain; tests validate expected phase transitions only (`Suggest`, `Vote`, `Results`).
|
||||
- Effort / Risk: `S / Low`.
|
||||
@@ -164,11 +165,11 @@ Worst coupling points:
|
||||
- Effort / Risk: `M / Low`.
|
||||
- Dependencies (if any): P0 redirect-hardening task.
|
||||
|
||||
[P1][In Progress] Standardize API response contracts and error envelopes
|
||||
- Problem: Severity `Medium`, Category `API/Contracts`. Success payloads are being typed and service-level errors now use a shared `ProblemDetails` helper, but some auth/filter paths still return bare status codes.
|
||||
- Evidence: typed response DTOs added in `Contracts/Responses.cs:5` and used in service workflows (`Endpoints/SuggestionWorkflowService.cs:83`, `Endpoints/VoteWorkflowService.cs:83`, `Endpoints/AdminWorkflowService.cs:44`, `Endpoints/ResultsWorkflowService.cs:63`); shared error envelope helper added in `Endpoints/EndpointHelpers.cs:90` and used across workflows (`Endpoints/AdminWorkflowService.cs:98`, `Endpoints/SuggestionWorkflowService.cs:43`, `Endpoints/VoteWorkflowService.cs:32`, `Endpoints/ResultsWorkflowService.cs:14`); global exception handler now emits `ProblemDetails` with `error` compatibility field in `Infrastructure/PlayerIdentityExtensions.cs:48`; frontend parser supports `error`, `detail`, and `title` in `wwwroot/js/api.js:25`.
|
||||
- Recommendation: finish migrating remaining auth/filter unauthorized paths to consistent problem payloads while keeping backward-compatible `error` fields during transition; continue replacing anonymous success payloads with DTOs.
|
||||
- Acceptance criteria (testable): all API endpoints return typed success DTOs; all non-2xx responses include `ProblemDetails` fields and remain consumable by current frontend.
|
||||
[P1][Done] Standardize API response contracts and error envelopes
|
||||
- Problem: Severity `Medium`, Category `API/Contracts`. Success payloads and error envelopes were inconsistent across endpoint, service, and filter layers.
|
||||
- Evidence: typed response DTOs now cover workflow + auth + state paths (`Contracts/Responses.cs:5`, `Contracts/Responses.cs:58`, `Endpoints/AuthEndpoints.cs:54`, `Endpoints/StateEndpoints.cs:22`, `Endpoints/SuggestionWorkflowService.cs:83`, `Endpoints/VoteWorkflowService.cs:83`, `Endpoints/AdminWorkflowService.cs:44`, `Endpoints/ResultsWorkflowService.cs:63`); shared `ProblemDetails` helper is used across services and endpoint filters (`Endpoints/EndpointHelpers.cs:90`, `Endpoints/SuggestEndpoints.cs:18`, `Endpoints/VoteEndpoints.cs:18`, `Infrastructure/AdminOnlyFilter.cs:15`, `Infrastructure/PhaseRequirementFilter.cs:15`, `Infrastructure/PhaseOrJokerFilter.cs:19`); global exception handling and client parsing remain compatible via `error`/`detail`/`title` (`Infrastructure/PlayerIdentityExtensions.cs:48`, `wwwroot/js/api.js:25`, `GameList.Tests/AuthTests.cs:164`).
|
||||
- Recommendation: retain this contract as baseline and keep backward-compatible `error` extension in future error-shape changes.
|
||||
- Acceptance criteria (testable): endpoint/service/filter responses use typed success payloads and standardized `ProblemDetails` error envelopes, with current frontend parsing unchanged.
|
||||
- Effort / Risk: `M / Med`.
|
||||
- Dependencies (if any): none.
|
||||
|
||||
|
||||
@@ -89,7 +89,6 @@ function setupHandlers() {
|
||||
e.preventDefault();
|
||||
const username = $("login-username").value.trim();
|
||||
const password = $("login-password").value;
|
||||
if (username.length > 24) return toast("Username must be 24 characters or fewer.", true);
|
||||
if (!username || !password) return toast(t("auth.needCredentials"), true);
|
||||
if (!hasConsent() && !$("login-consent")?.checked) return toast(t("auth.cookieRequired"), true);
|
||||
try {
|
||||
@@ -117,8 +116,6 @@ function setupHandlers() {
|
||||
const displayName = $("register-displayName").value.trim();
|
||||
const adminKey = $("register-adminkey").value.trim();
|
||||
if (!displayName) return toast(t("toast.displayNameRequired") || "Display name is required.", true);
|
||||
if (username.length > 24) return toast("Username must be 24 characters or fewer.", true);
|
||||
if (displayName.length > 16) return toast("Display name must be 16 characters or fewer.", true);
|
||||
if (!username || !password) return toast(t("auth.needCredentials"), true);
|
||||
if (!hasConsent() && !$("register-consent")?.checked) return toast(t("auth.cookieRequired"), true);
|
||||
try {
|
||||
|
||||
@@ -630,8 +630,6 @@ function openSuggestionModal({ title, submitLabel, initial = {}, onSubmit, lockT
|
||||
const errorBox = form.querySelector('[data-error="players"]');
|
||||
const minInput = form.querySelector('input[name="minPlayers"]');
|
||||
const maxInput = form.querySelector('input[name="maxPlayers"]');
|
||||
const screenshotError = form.querySelector('[data-error="screenshot"]');
|
||||
const screenshotInput = form.querySelector('input[name="screenshotUrl"]');
|
||||
const markError = (msg) => {
|
||||
if (errorBox) {
|
||||
errorBox.textContent = msg;
|
||||
@@ -640,7 +638,6 @@ function openSuggestionModal({ title, submitLabel, initial = {}, onSubmit, lockT
|
||||
};
|
||||
const clearError = () => {
|
||||
if (errorBox) errorBox.classList.add("hidden");
|
||||
if (screenshotError) screenshotError.classList.add("hidden");
|
||||
};
|
||||
clearError();
|
||||
const min = data.minPlayers;
|
||||
@@ -657,15 +654,6 @@ function openSuggestionModal({ title, submitLabel, initial = {}, onSubmit, lockT
|
||||
markError(t("form.playersInvalid"));
|
||||
return;
|
||||
}
|
||||
if (data.screenshotUrl && !isValidImageUrl(data.screenshotUrl)) {
|
||||
if (screenshotError) {
|
||||
screenshotError.textContent = t("form.screenshotInvalid");
|
||||
screenshotError.classList.remove("hidden");
|
||||
}
|
||||
screenshotInput?.classList.add("input-error");
|
||||
return;
|
||||
}
|
||||
screenshotInput?.classList.remove("input-error");
|
||||
if (!data.name?.trim()) return toast(t("toast.nameRequired"), true);
|
||||
try {
|
||||
await onSubmit(data, close, submitBtn);
|
||||
@@ -1016,36 +1004,6 @@ function openDeleteConfirmModal(s) {
|
||||
document.body.appendChild(overlay);
|
||||
}
|
||||
|
||||
function isValidImageUrl(url) {
|
||||
if (!url) return true;
|
||||
try {
|
||||
const u = new URL(url);
|
||||
const allowed = ["http:", "https:"];
|
||||
if (!allowed.includes(u.protocol)) return false;
|
||||
const host = u.hostname.toLowerCase();
|
||||
const bannedHosts = [
|
||||
"bit.ly",
|
||||
"tinyurl.com",
|
||||
"t.co",
|
||||
"goo.gl",
|
||||
"ow.ly",
|
||||
"is.gd",
|
||||
"buff.ly",
|
||||
"rebrand.ly",
|
||||
"steamcommunity.com",
|
||||
"store.steampowered.com",
|
||||
];
|
||||
if (bannedHosts.some((h) => host === h)) return false;
|
||||
if (host === "imgur.com" && !u.pathname.startsWith("/a/") && !u.pathname.startsWith("/gallery/")) return false;
|
||||
const path = u.pathname.toLowerCase();
|
||||
return [".png", ".jpg", ".jpeg", ".gif", ".webp", ".avif"].some((ext) =>
|
||||
path.endsWith(ext),
|
||||
);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
function linkRootId(s) {
|
||||
return s?.parentSuggestionId ?? s?.id;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user