oslstats/REVIEW_PLAN.md

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:

// 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

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

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:

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

if db.IsBadRequest(err) {
    respond.BadRequest(w, err)
}
return false, errors.Wrap(err, "season.RemoveLeague")

Fix: Add return false, nil after respond.BadRequest:

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

// 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:

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

// 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

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:

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

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:

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

w.Header().Set("HX-Redirect", fmt.Sprintf("/seasons/%s/leagues/%s/teams", season.ShortName, league.ShortName))
w.WriteHeader(http.StatusOK)

Fix: Use respond.HXRedirect:

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

roleID, err := strconv.Atoi(roleIDStr)
if err != nil {
    w.WriteHeader(http.StatusBadRequest)  // ← raw header
    return
}

Fix: Use respond.BadRequest:

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

if err.Error() == "sql: no rows in result set" {
    return false, 0, nil
}

Fix: Use proper error comparison:

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

return nil, errors.WithMessage(err, "db.Insert")

Fix: Change to errors.Wrap:

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

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:

return false, errors.Wrap(err, "db.IsUnique")

7.3 fixtures.go:177 — Profanity in comment

File: internal/handlers/fixtures.go, line 177

// fuck i hate pointers sometimes

Fix: Remove or rephrase:

// Pointer handling for optional game week values

7.4 register.go:63 — Audit with nil user

File: internal/handlers/register.go, line 63

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:

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:

TimeInLocation(key string, format *timefmt.Format, loc *time.Location) *TimeField

In querys.go, add the method:

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:

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):

// 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:

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:

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:

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