review plan added

2026-03-08 12:40:25 +11:00
parent 7c32dec1a3
commit e482925294
1 changed files with 644 additions and 0 deletions
--- a/REVIEW_PLAN.md
+++ 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