From e4829252942e10d3e9f35671bc495eaf985c3f31 Mon Sep 17 00:00:00 2001 From: Haelnorr Date: Sun, 8 Mar 2026 12:40:25 +1100 Subject: [PATCH] review plan added --- REVIEW_PLAN.md | 644 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 644 insertions(+) create mode 100644 REVIEW_PLAN.md diff --git a/REVIEW_PLAN.md b/REVIEW_PLAN.md new file mode 100644 index 0000000..76465ee --- /dev/null +++ b/REVIEW_PLAN.md @@ -0,0 +1,644 @@ +# oslstats Codebase Review — Implementation Plan + +## Dependency Order Rationale + +Changes are ordered so that **foundational/infrastructure changes come first**, which later changes depend on. This prevents doing work twice or having to revisit files. + +--- + +## Phase 1: Foundation — Establish Response System Rules + +Before fixing individual handlers, establish clear rules about when to use each response system. This is the root cause of most inconsistencies. + +### 1.1 Document the three response systems + +Create a comment block or doc at the top of each package (or a shared document) defining when to use each: + +| System | When to use | Example | +|--------|-------------|---------| +| **`throw`** | Full-page navigation (GET requests), or errors so severe the page can't render | `throw.NotFound`, `throw.BadRequest` | +| **`notify`** | HTMX partials/modal actions where you want a WebSocket toast notification | `notify.Warn`, `notify.Success` | +| **`respond`** | Pure API-style responses (JSON body, no page render, no toast) — uniqueness checks, programmatic callers | `respond.BadRequest`, `respond.OK` | + +**Rule of thumb**: If the handler renders templ → `throw` for errors. If it's an HTMX action → `notify`. If it's a pure data endpoint → `respond`. + +### 1.2 Remove duplicate `parseErrorDetails` + +**File**: `internal/handlers/errors.go` (lines 14-29) + +This function is an exact duplicate of `notify.ParseErrorDetails` in `internal/notify/util.go` (lines 29-44). + +**Fix**: Delete `parseErrorDetails` from `handlers/errors.go` and update any callers to use `notify.ParseErrorDetails` instead. + +**Check for callers first:** + +``` +grep -rn "parseErrorDetails" internal/handlers/ +``` + +If no callers exist (the function is unexported and may be dead code), simply delete it. + +--- + +## Phase 2: Transaction Misuse — Read-only operations using write transactions + +`WithNotifyTx` always commits (it's a write transaction with notify-based error handling). Several handlers use it for **read-only** operations. This should be `WithReadTx` (which always rolls back). + +However, there's a missing helper: a `WithNotifyReadTx` that uses `notify` for errors instead of `throw`. You need to decide: + +**Option A**: Add a `WithNotifyReadTx` helper to `internal/db/txhelpers.go` +**Option B**: Convert these to `WithReadTx` and accept that read-only errors render error pages + +Recommendation: **Option A** — add `WithNotifyReadTx`. + +### 2.1 Add `WithNotifyReadTx` to `internal/db/txhelpers.go` + +After `WithReadTx` (line 38), add: + +```go +// WithNotifyReadTx executes a read-only transaction with notification-based error handling +// Uses notify.InternalServiceError instead of throw.InternalServiceError +func (db *DB) WithNotifyReadTx( + s *hws.Server, + w http.ResponseWriter, + r *http.Request, + fn TxFunc, +) bool { + ctx, cancel := context.WithTimeout(r.Context(), timeout) + defer cancel() + ok, err := db.withTx(ctx, fn, false) + if err != nil { + notify.InternalServiceError(s, w, r, "Database error", err) + } + return ok +} +``` + +### 2.2 Fix read-only handlers using `WithNotifyTx` + +These handlers only **read** data but use `WithNotifyTx` (write transaction): + +| File | Line | Handler | Fix | +|------|------|---------|-----| +| `admin_roles.go` | 117 | `AdminRoleManage` | → `WithNotifyReadTx` | +| `admin_roles.go` | 147 | `AdminRoleDeleteConfirm` | → `WithNotifyReadTx` | +| `admin_roles.go` | 238 | `AdminRolePermissionsModal` | → `WithNotifyReadTx` | +| `isunique.go` | 36 | `IsUnique` | → `WithNotifyReadTx` | + +For each, simply change `conn.WithNotifyTx` to `conn.WithNotifyReadTx`. + +--- + +## Phase 3: Double Response / Redundant Error Handling Bugs + +These are actual bugs where two HTTP responses are sent to the same request. + +### 3.1 `fixtures.go:71` — Double response after `ValidateAndNotify` + +**File**: `internal/handlers/fixtures.go`, lines 70-72 + +```go +if !getter.ValidateAndNotify(s, w, r) { + w.WriteHeader(http.StatusBadRequest) // ← BUG: ValidateAndNotify already sent notifications + return +} +``` + +**Fix**: Remove line 71 (`w.WriteHeader(http.StatusBadRequest)`). The `ValidateAndNotify` already handles the response via WebSocket. + +### 3.2 `fixtures.go:100-103` — `respond.BadRequest` then `return false, errors.Wrap(...)` + +**File**: `internal/handlers/fixtures.go`, lines 100-103 + +```go +if db.IsBadRequest(err) { + respond.BadRequest(w, errors.Wrap(err, "db.UpdateFixtureGameWeeks")) +} +return false, errors.Wrap(err, "db.UpdateFixtureGameWeeks") // ← runs for BOTH bad request AND other errors +``` + +The `return false, errors.Wrap(...)` runs unconditionally. If it's a bad request, `respond.BadRequest` fires, then `return false, err` causes `WithNotifyTx` to also call `notify.InternalServiceError`. + +**Fix**: Add `return false, nil` after `respond.BadRequest`: + +```go +if db.IsBadRequest(err) { + respond.BadRequest(w, errors.Wrap(err, "db.UpdateFixtureGameWeeks")) + return false, nil +} +return false, errors.Wrap(err, "db.UpdateFixtureGameWeeks") +``` + +### 3.3 `season_leagues.go:80-83` — Same pattern + +**File**: `internal/handlers/season_leagues.go`, lines 80-83 + +```go +if db.IsBadRequest(err) { + respond.BadRequest(w, err) +} +return false, errors.Wrap(err, "season.RemoveLeague") +``` + +**Fix**: Add `return false, nil` after `respond.BadRequest`: + +```go +if db.IsBadRequest(err) { + respond.BadRequest(w, err) + return false, nil +} +return false, errors.Wrap(err, "season.RemoveLeague") +``` + +### 3.4 `team_roster_manage.go:25-26` and `35-37` — Redundant responses + +**File**: `internal/handlers/team_roster_manage.go` + +```go +// Line 23-26: +getter, ok := validation.ParseFormOrNotify(s, w, r) +if !ok { + respond.BadRequest(w, errors.New("failed to parse form")) // ← redundant + return +} + +// Line 35-37: +if !getter.ValidateAndNotify(s, w, r) { + respond.BadRequest(w, errors.New("invalid form data")) // ← redundant + return +} +``` + +`ParseFormOrNotify` and `ValidateAndNotify` already handle the response via notifications. The extra `respond.BadRequest` sends a second response. + +**Fix**: Remove both `respond.BadRequest` calls: + +```go +getter, ok := validation.ParseFormOrNotify(s, w, r) +if !ok { + return +} +// ... +if !getter.ValidateAndNotify(s, w, r) { + return +} +``` + +### 3.5 `free_agents.go:94-96` and `100-102` — Same redundant pattern + +**File**: `internal/handlers/free_agents.go`, `RegisterFreeAgent` handler + +```go +// Line 93-96: +getter, ok := validation.ParseFormOrNotify(s, w, r) +if !ok { + respond.BadRequest(w, errors.New("failed to parse form")) // ← redundant + return +} + +// Line 99-102: +if !getter.ValidateAndNotify(s, w, r) { + respond.BadRequest(w, errors.New("invalid form data")) // ← redundant + return +} +``` + +**Fix**: Same as 3.4 — remove both `respond.BadRequest` calls. + +Also apply the same fix in the `UnregisterFreeAgent` handler (lines 167-174), which has the identical pattern. + +### 3.6 `login.go:37-39` — `notify` then `respond.OK` + +**File**: `internal/handlers/login.go`, lines 36-39 + +```go +if err != nil { + notify.ServiceUnavailable(s, w, r, "Login currently unavailable", err) + respond.OK(w) // ← sends 200 OK after the notification + return +} +``` + +The `notify.ServiceUnavailable` sends a WebSocket message. Then `respond.OK` writes a 200 status. The 200 status tells HTMX the request succeeded, which is misleading. + +**Fix**: Remove `respond.OK(w)`. The notification alone is sufficient for the HTMX POST handler: + +```go +if err != nil { + notify.ServiceUnavailable(s, w, r, "Login currently unavailable", err) + return +} +``` + +--- + +## Phase 4: Raw Response Patterns → Use Proper Helpers + +### 4.1 `season_league_add_team.go:42` — Raw `w.WriteHeader` + +**File**: `internal/handlers/season_league_add_team.go`, line 42 + +```go +if db.IsBadRequest(err) { + w.WriteHeader(http.StatusBadRequest) // ← raw header, no error body + return false, nil +} +``` + +**Fix**: Use `notify.Warn` since this is inside a `WithNotifyTx` context: + +```go +if db.IsBadRequest(err) { + notify.Warn(s, w, r, "Invalid Request", err.Error(), nil) + return false, nil +} +``` + +### 4.2 `season_league_add_team.go:53-54` — Manual HX-Redirect + +**File**: `internal/handlers/season_league_add_team.go`, lines 53-54 + +```go +w.Header().Set("HX-Redirect", fmt.Sprintf("/seasons/%s/leagues/%s/teams", season.ShortName, league.ShortName)) +w.WriteHeader(http.StatusOK) +``` + +**Fix**: Use `respond.HXRedirect`: + +```go +respond.HXRedirect(w, "/seasons/%s/leagues/%s/teams", season.ShortName, league.ShortName) +``` + +This will also require adding `"git.haelnorr.com/h/oslstats/internal/respond"` to the imports and potentially removing `"fmt"` if no longer used. + +### 4.3 `admin_roles.go:142` — Raw `w.WriteHeader` + +**File**: `internal/handlers/admin_roles.go`, line 142 + +```go +roleID, err := strconv.Atoi(roleIDStr) +if err != nil { + w.WriteHeader(http.StatusBadRequest) // ← raw header + return +} +``` + +**Fix**: Use `respond.BadRequest`: + +```go +roleID, err := strconv.Atoi(roleIDStr) +if err != nil { + respond.BadRequest(w, err) + return +} +``` + +--- + +## Phase 5: `strconv.Atoi` Error Handling Standardization + +Currently, `strconv.Atoi` failures for path parameters are handled inconsistently: + +| Pattern | Used in | +|---------|---------| +| `throw.BadRequest(s, w, r, "Invalid X ID", err)` | `fixture_detail.go` (x15), `forfeit.go`, `fixture_result.go` (x4), `free_agents.go` (x2), `admin_preview_role.go`, `admin_audit.go` | +| `throw.NotFound(s, w, r, r.URL.Path)` | `team_detail.go`, `season_league_team_detail.go`, `player_view.go`, `player_link_slapid.go` | +| `respond.BadRequest(w, err)` | `admin_roles.go` (x4), `fixtures.go:121` | +| `w.WriteHeader(http.StatusBadRequest)` | `admin_roles.go:142` | + +**Recommended standard**: For path parameters in URL routing: +- If it's a page navigation (user-visible URL) → `throw.NotFound` (an invalid ID in a URL means the resource doesn't exist) +- If it's an HTMX action endpoint → `respond.BadRequest` (the ID came from the UI, so it's a programming error or tampered request) + +Apply this consistently: + +### Files to update: + +**To `throw.NotFound`** (page navigation handlers): +- No changes needed — `team_detail.go`, `season_league_team_detail.go`, `player_view.go` already do this correctly + +**To `respond.BadRequest`** (HTMX action endpoints): +- `admin_roles.go:110` — Already uses `respond.BadRequest` ✓ +- `admin_roles.go:170`, `227`, `291` — Already use `respond.BadRequest` ✓ +- `admin_roles.go:142` — Fixed in Phase 4.3 + +**Mixed handlers** (both page navigation and HTMX actions): +- `fixture_detail.go` — The page-rendering handlers use `throw.BadRequest`. Since these are often navigated to via URL, consider changing to `throw.NotFound` for the "page" handlers, and keeping `throw.BadRequest` for the action handlers. This is a judgment call — at minimum, be consistent within the file. + +--- + +## Phase 6: `sql.ErrNoRows` Comparison Fix + +### 6.1 `fixture.go:51` — String comparison for `sql.ErrNoRows` + +**File**: `internal/db/fixture.go`, line 51 + +```go +if err.Error() == "sql: no rows in result set" { + return false, 0, nil +} +``` + +**Fix**: Use proper error comparison: + +```go +import "database/sql" +// ... +if errors.Is(err, sql.ErrNoRows) { + return false, 0, nil +} +``` + +This requires adding `"database/sql"` to imports (and `"github.com/pkg/errors"` may already handle `Is` — check if you need to also import the standard `errors` package or if `pkg/errors` supports `Is`). + +--- + +## Phase 7: Code Style Fixes + +### 7.1 `season.go:70` — `errors.WithMessage` instead of `errors.Wrap` + +**File**: `internal/db/season.go`, line 70 + +```go +return nil, errors.WithMessage(err, "db.Insert") +``` + +**Fix**: Change to `errors.Wrap`: + +```go +return nil, errors.Wrap(err, "db.Insert") +``` + +`errors.WithMessage` doesn't capture a new stack frame; `errors.Wrap` does. The codebase convention is `errors.Wrap`. + +### 7.2 `register.go:67` — Wrong function name in error wrap + +**File**: `internal/handlers/register.go`, line 67 + +```go +return false, errors.Wrap(err, "db.IsUsernameUnique") +``` + +The actual function called is `db.IsUnique` (line 65). The wrap message references a non-existent function. + +**Fix**: + +```go +return false, errors.Wrap(err, "db.IsUnique") +``` + +### 7.3 `fixtures.go:177` — Profanity in comment + +**File**: `internal/handlers/fixtures.go`, line 177 + +```go +// fuck i hate pointers sometimes +``` + +**Fix**: Remove or rephrase: + +```go +// Pointer handling for optional game week values +``` + +### 7.4 `register.go:63` — Audit with nil user + +**File**: `internal/handlers/register.go`, line 63 + +```go +var user *db.User +audit := db.NewAudit(r.RemoteAddr, r.UserAgent(), user) // user is nil here +``` + +This passes a nil user to `NewAudit` before the user exists. The audit will have no user associated. This is somewhat intentional (registration happens before the user exists), but it's worth documenting: + +**Fix**: Add a comment explaining the intentional nil: + +```go +var user *db.User +// User is nil before registration — audit tracks the request source (IP/UA) only +audit := db.NewAudit(r.RemoteAddr, r.UserAgent(), user) +``` + +--- + +## Phase 8: Validation Interface Gap + +### 8.1 `TimeInLocation` missing from `Getter` interface + +**File**: `internal/validation/validation.go`, lines 22-38 + +The `Getter` interface defines `Time(key, format)` but not `TimeInLocation(key, format, loc)`. The `FormGetter` has `TimeInLocation` (line 95 of `forms.go`), but `QueryGetter` doesn't, and neither does the interface. + +**Fix**: Add `TimeInLocation` to the `Getter` interface and to `QueryGetter`: + +In `validation.go`, add to the `Getter` interface: + +```go +TimeInLocation(key string, format *timefmt.Format, loc *time.Location) *TimeField +``` + +In `querys.go`, add the method: + +```go +func (q *QueryGetter) TimeInLocation(key string, format *timefmt.Format, loc *time.Location) *TimeField { + return newTimeFieldInLocation(key, format, loc, q) +} +``` + +Also add `"time"` to the imports of `querys.go`. + +--- + +## Phase 9: Duplicate Code Consolidation + +### 9.1 Duplicate leaderboard computation pattern + +The same pattern appears in **5 places**: + +1. `handlers/index.go:54-69` +2. `handlers/season_league_table.go:40-55` +3. `handlers/season_league_team_detail.go:87-100` +4. `db/team.go:126-142` +5. `db/match_preview.go:222` + +The pattern is: +```go +fixtures, err := db.GetAllocatedFixtures(ctx, tx, seasonID, leagueID) +fixtureIDs := make([]int, len(fixtures)) +for i, f := range fixtures { fixtureIDs[i] = f.ID } +resultMap, err := db.GetFinalizedResultsForFixtures(ctx, tx, fixtureIDs) +leaderboard := db.ComputeLeaderboard(teams, fixtures, resultMap) +``` + +**Fix**: Create a helper function in `internal/db/` (e.g. in `fixture_result.go` near `ComputeLeaderboard`): + +```go +// GetLeaderboard fetches fixtures and results, then computes the leaderboard for a season+league. +func GetLeaderboard(ctx context.Context, tx bun.Tx, seasonID, leagueID int, teams []*Team) ([]*LeaderboardEntry, error) { + fixtures, err := GetAllocatedFixtures(ctx, tx, seasonID, leagueID) + if err != nil { + return nil, errors.Wrap(err, "GetAllocatedFixtures") + } + fixtureIDs := make([]int, len(fixtures)) + for i, f := range fixtures { + fixtureIDs[i] = f.ID + } + resultMap, err := GetFinalizedResultsForFixtures(ctx, tx, fixtureIDs) + if err != nil { + return nil, errors.Wrap(err, "GetFinalizedResultsForFixtures") + } + return ComputeLeaderboard(teams, fixtures, resultMap), nil +} +``` + +Then replace all 5 occurrences with a single call: + +```go +leaderboard, err := db.GetLeaderboard(ctx, tx, seasonID, leagueID, teams) +if err != nil { + return false, errors.Wrap(err, "db.GetLeaderboard") +} +``` + +### 9.2 Duplicate player stats SQL + +**File**: `internal/db/player.go`, lines 116-206 + +Three functions — `GetPlayerAllTimeStats`, `GetPlayerStatsBySeason`, `GetPlayerStatsByTeam` — share 90% identical SQL. Only the `WHERE` clause differs. + +**Fix**: Extract a common base function: + +```go +func getPlayerStats(ctx context.Context, tx bun.Tx, playerID int, extraJoin string, extraWhere string, extraArgs ...any) (*PlayerAllTimeStats, error) { + if playerID == 0 { + return nil, errors.New("playerID not provided") + } + stats := new(PlayerAllTimeStats) + query := ` + SELECT + COUNT(DISTINCT frps.fixture_result_id) AS games_played, + COALESCE(SUM(frps.periods_played), 0) AS total_periods_played, + COALESCE(SUM(frps.goals), 0) AS total_goals, + COALESCE(SUM(frps.assists), 0) AS total_assists, + COALESCE(SUM(frps.saves), 0) AS total_saves, + COALESCE(SUM(frps.shots), 0) AS total_shots, + COALESCE(SUM(frps.blocks), 0) AS total_blocks, + COALESCE(SUM(frps.passes), 0) AS total_passes + FROM fixture_result_player_stats frps + JOIN fixture_results fr ON fr.id = frps.fixture_result_id + ` + extraJoin + ` + WHERE fr.finalized = true + AND frps.player_id = ? + AND frps.period_num = 3 + ` + extraWhere + + args := append([]any{playerID}, extraArgs...) + err := tx.NewRaw(query, args...).Scan(ctx, stats) + if err != nil { + return nil, errors.Wrap(err, "tx.NewRaw") + } + return stats, nil +} + +func GetPlayerAllTimeStats(ctx context.Context, tx bun.Tx, playerID int) (*PlayerAllTimeStats, error) { + return getPlayerStats(ctx, tx, playerID, "", "") +} + +func GetPlayerStatsBySeason(ctx context.Context, tx bun.Tx, playerID, seasonID int) (*PlayerAllTimeStats, error) { + if seasonID == 0 { + return nil, errors.New("seasonID not provided") + } + return getPlayerStats(ctx, tx, playerID, + "JOIN fixtures f ON f.id = fr.fixture_id", + "AND f.season_id = ?", seasonID) +} + +func GetPlayerStatsByTeam(ctx context.Context, tx bun.Tx, playerID, teamID int) (*PlayerAllTimeStats, error) { + if teamID == 0 { + return nil, errors.New("teamID not provided") + } + return getPlayerStats(ctx, tx, playerID, + "", + "AND frps.team_id = ?", teamID) +} +``` + +--- + +## Phase 10: Pattern Consistency (Lower Priority) + +### 10.1 Leagues list page — missing GET/POST pattern and pagination + +**File**: `internal/handlers/leagues_list.go` + +Unlike `seasons_list.go` and `teams_list.go`, this handler: +1. Always renders the full page (no GET vs POST check) +2. Fetches ALL leagues without pagination + +**Fix**: Follow the established pattern from `seasons_list.go`: + +```go +func LeaguesList(s *hws.Server, conn *db.DB) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + pageOpts, ok := db.GetPageOpts(s, w, r) + if !ok { + return + } + var leagues *db.List[db.League] + if ok := conn.WithReadTx(s, w, r, func(ctx context.Context, tx bun.Tx) (bool, error) { + var err error + leagues, err = db.ListLeagues(ctx, tx, pageOpts) // New paginated function + if err != nil { + return false, errors.Wrap(err, "db.ListLeagues") + } + return true, nil + }); !ok { + return + } + if r.Method == "GET" { + renderSafely(leaguesview.ListPage(leagues), s, r, w) + } else { + renderSafely(leaguesview.LeaguesList(leagues), s, r, w) + } + }) +} +``` + +This requires: +- Adding a `ListLeagues` function in `internal/db/league.go` using `GetList[League](tx).GetPaged(ctx, pageOpts, defaults)` +- Updating the template to support the new paginated list pattern +- Adding a `LeaguesList` partial template + +**Note**: This is a more involved change and may warrant its own feature task. + +--- + +## Summary Checklist + +| # | Phase | Issue | Severity | Files | +|---|-------|-------|----------|-------| +| 1.2 | Foundation | Duplicate `parseErrorDetails` | Low | `handlers/errors.go` | +| 2.1 | Transactions | Add `WithNotifyReadTx` | Medium | `db/txhelpers.go` | +| 2.2 | Transactions | Fix 4 read-only handlers | Medium | `admin_roles.go`, `isunique.go` | +| 3.1 | Double Response | `ValidateAndNotify` + `WriteHeader` | **High** | `fixtures.go:71` | +| 3.2 | Double Response | `respond.BadRequest` + `return err` | **High** | `fixtures.go:100-103` | +| 3.3 | Double Response | `respond.BadRequest` + `return err` | **High** | `season_leagues.go:80-83` | +| 3.4 | Double Response | Redundant respond after notify | **High** | `team_roster_manage.go:25,36` | +| 3.5 | Double Response | Redundant respond after notify | **High** | `free_agents.go:95,101,168,174` | +| 3.6 | Double Response | `notify` then `respond.OK` | **High** | `login.go:38` | +| 4.1 | Raw Responses | Raw `w.WriteHeader` | Medium | `season_league_add_team.go:42` | +| 4.2 | Raw Responses | Manual HX-Redirect | Low | `season_league_add_team.go:53-54` | +| 4.3 | Raw Responses | Raw `w.WriteHeader` | Medium | `admin_roles.go:142` | +| 5 | Standardization | `strconv.Atoi` error handling | Low | Multiple files | +| 6.1 | Bug Fix | String comparison for `sql.ErrNoRows` | **High** | `db/fixture.go:51` | +| 7.1 | Style | `errors.WithMessage` → `errors.Wrap` | Low | `db/season.go:70` | +| 7.2 | Style | Wrong wrap message | Low | `register.go:67` | +| 7.3 | Style | Profanity in comment | Low | `fixtures.go:177` | +| 7.4 | Style | Nil user audit — add comment | Low | `register.go:63` | +| 8.1 | Interface | `TimeInLocation` missing from `Getter` | Medium | `validation/validation.go`, `querys.go` | +| 9.1 | Duplication | Leaderboard computation | Medium | 5 files | +| 9.2 | Duplication | Player stats SQL | Medium | `db/player.go` | +| 10.1 | Consistency | Leagues list pattern | Low | `leagues_list.go`, `db/league.go`, templates | + +**Recommended execution order**: Phases 1 → 2 → 3 → 6 → 4 → 7 → 8 → 9 → 5 → 10