# 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