sports-division/bug-fixes.md

# Bug Findings & Fixes — SportsDivision

Original scan: 2026-08-11 against `master` @ `78c526e` (45 findings, two sweeps).
**Fix pass completed 2026-08-11.** Every finding was re-verified against the code
before changes were made; **all 45 were confirmed real and accurately described**
(one correction of detail: the AutoMapper advisory is patched only in 15.1.1/16.1.1,
not in any 13.x/14.x — see #21).

State after fixes: `dotnet build` succeeds, `dotnet test` passes **63/63**
(52 pre-existing + 11 new tests covering the fixed logic). A new EF migration
`SchemaIntegrityFixes` carries the schema changes (#13, #38) and is confirmed in
sync with the model.

---

## ⚠️ Required manual actions (cannot be done from the repo)

1. **Rotate the leaked credentials** (#15). The following were committed and are in
   git history; removing them from HEAD (done) is not sufficient:
   - Postgres superuser password for `74.50.64.180` (production)
   - the local/dev Postgres password
   - the Gmail app password for `pcgurudm@gmail.com` (revoke it — the mail feature
     doesn't even exist)
2. **Provide secrets via environment at deploy time.** `docker-compose.yml` now
   requires `DB_CONNECTION_STRING` (and optionally `SEED_ADMIN_PASSWORD`), e.g. from
   an untracked `.env` next to the compose file on the VPS or injected by Jenkins.
   The Jenkins deploy stage runs `docker compose up` from a fresh checkout, so until
   a `.env` exists on the host path (or the pipeline exports the variable), deploys
   will fail fast with a clear compose error. Local dev: set the connection string in
   `appsettings.Development.json` (placeholder committed) or user-secrets.
3. **Change the admin password** if the seeded `admin@sportsdivision.dm` /
   `Admin@123!` account exists in any database — the seeder no longer creates it
   with a hardcoded password (#16), but existing rows keep the old one.
4. **Decide on git history purge** for the leaked secrets (e.g. `git filter-repo`).
   Not done here because rewriting published history must be coordinated.
5. **Decide on AutoMapper 15.1.1** (#21): the DoS advisory is only patched in
   15.1.1+/16.1.1+, which are under the new RPL/commercial dual licence. Upgraded to
   14.0.0 (last version under the original licence); the NU1903 warning remains
   deliberately visible. Exploitability here is negligible — the app only maps its
   own bounded EF entity graphs, never attacker-controlled object graphs.

---

## P1 — Correctness & crashes

### 1. "Complete Heat" navigated to the wrong round (or 404) — **FIXED** ✅
Verified: `CompleteHeat` redirected with `roundId = heatId`.
Fix: the hidden `complete-heat` form in `ManageRound.cshtml` now posts `roundId`
alongside `heatId`, and `TrackEventController.CompleteHeat(int heatId, int roundId)`
redirects with the real round id.

### 2. FluentValidation never registered — **FIXED** ✅
Verified: four validators existed with zero registration; every `ModelState.IsValid`
guard was unconditionally true.
Fix: `AddValidatorsFromAssembly(...)` in `AddApplication()` (new
`FluentValidation.DependencyInjectionExtensions` package reference) and
`AddFluentValidationAutoValidation()` in `Program.cs`. All four validators
(student, school, tournament incl. `EndDate >= StartDate`, registration) now run;
`Register.cshtml` gained a validation summary so registration rule failures are shown.

### 3. NullReferenceException registering a relay team — **FIXED** ✅
Verified: `dto.StudentId!.Value` dereferenced null for relay-only posts.
Fix: `RegisterStudentAsync` branches on `StudentId.HasValue`; relay-only
registrations are rejected with a clear `InvalidOperationException` ("not supported
yet") since the relay feature has no management UI or eligibility rules anywhere in
the app. Covered by two new tests.

### 4. Advancement flags never cleared before recalculation — **FIXED** ✅
Verified. Fix: `CalculateAdvancementAsync` resets `IsAdvanced`/`AdvanceReason` on
every lane in the round before applying the top-N and fastest-loser passes.
Covered by a new test (`HeatAdvancementResetTests`).

### 5. "Send Advancers" duplicated heats when re-run — **FIXED** ✅
Verified. Fix: `PopulateNextRoundAsync` now clears the next round's existing heats
and lanes first (mirroring `SeedHeatsAsync`), and errors clearly when no athletes
are marked as advancing.

### 6. `SeedingMethod.ByPerformance` / `.Manual` silently ignored — **FIXED** ✅
Verified: only `Random` was implemented; the UI only ever posted `Random`.
Fix: the unimplemented members were removed from the enum (nothing persists or
references them), with a comment explaining what a future implementation needs.

### 7. `lanesPerHeat = 0` wiped a round's heats — **FIXED** ✅
Verified (crafted-POST-only reachability also confirmed).
Fix: `SeedHeatsAsync` validates `lanesPerHeat` (1–10) **before** any deletion and
throws; the controller surfaces the message via the notification banner.

### 8. High jump attempts 2 and 3 unreachable — **FIXED** ✅
Verified: the read-only branch rendered as soon as any attempt row existed.
Fix: `HighJump/Index.cshtml` now always renders one dropdown per attempt slot,
pre-selected from `Attempt1/2/3` — attempts 2/3 are recordable and mistakes are
correctable. The controller also rejects a blank result (which would otherwise
bind to the enum default and record a clearance), and the OUT badge still shows.

### 9. "Remove height" bounced to the event-level picker — **FIXED** ✅
Verified. Fix: the form posts the missing `tournamentEventLevelId` hidden input.

### 10. High jump results left stale scores behind — **FIXED** ✅
Verified. Fix: `CalculateResultsAsync` deletes `Score` rows for registrations in
the event level that are absent from the ranked results, mirroring the track path.

### 11. Field-event placements meaningless without scoring constants — **FIXED** ✅
Verified: 20 of 32 seeded events have constants; ranking was on `CalculatedPoints`.
Fixes:
- `CalculatePlacementsAsync` ranks on `RawPerformance` (ascending for Track,
  descending for Field), so placements are correct even without a constant.
- Ties share a placement (competition ranking 1, 2, 2, 4) and split the pooled
  placement points — same convention as the high-jump path. New tests cover
  direction, ties and pooled points.
- `CalculateFinalScoresAsync` now **throws a clear error** instead of silently
  returning when the event has no `ScoringConstant`; the Field Event page shows it.
- Scores with no valid mark get their stale placement cleared (see also #27).
- The missing constants themselves can now be added in-app (#36).

### 12. Deleting an in-use entity returned a 500 — **FIXED** ✅
Verified for all four paths (`DeleteBehavior.Restrict` confirmed in configuration).
Fix: `StudentService.DeleteAsync`, `SchoolService.DeleteAsync`,
`EventService.DeleteAsync` and `RegistrationService.UnregisterAsync` check for
dependants first and throw `InvalidOperationException` with an actionable message
(which the controllers already surface). A new repository helper
(`IHighJumpHeightRepository.HasAttemptsForRegistrationAsync`) covers the
high-jump-attempt dependency.

### 13. Second student without an "existing student ID" crashed — **FIXED** ✅
Verified: `""` collided on the unfiltered unique index.
Fix: `Student.ExistingStudentId` is now nullable; the unique index is filtered to
non-null (`SchemaIntegrityFixes` migration, which also normalises existing `""`
rows to NULL); the service trims/normalises blank input to null and pre-checks
duplicates so a repeat ID gets a friendly message instead of a 500.

### 14. Eligibility ignored active flags and tournament status — **FIXED** ✅
Verified. Fix: `CheckEligibilityAsync` now rejects deactivated students,
deactivated schools, and completed or archived tournaments, each with a clear
reason. Covered by four new tests.

---

## P2 — Security & silent failures

### 15. Live production credentials committed — **FIXED in HEAD** ⚠️ rotation required
Verified: both appsettings files tracked, with production Postgres, dev Postgres
and a Gmail app password.
Fix: all secrets removed from `appsettings.json` / `appsettings.Production.json`;
the dead `EmailSettings` block deleted (grep confirmed no code reads it);
configuration now flows from the environment (`ConnectionStrings__DefaultConnection`)
supplied by docker-compose; `.env` added to `.gitignore`.
**Rotation and history purge are manual actions — see the top of this document.**

### 16. Hardcoded seeded admin account — **FIXED** ✅
Verified. Fix: the seeder reads `SeedAdmin:Email` / `SeedAdmin:Password` from
configuration; if no password is configured and no admin exists it logs a warning
and skips (nothing is created with a known password). Forced password change on
first login is not natively supported by ASP.NET Identity — the seeder logs a
reminder instead; change the password after first sign-in.

### 17. Brute-force protection disabled + returnUrl 500 — **FIXED** ✅
Verified. Fix: `lockoutOnFailure: true` (Identity default: 5 attempts/5 min), with
a distinct "temporarily locked" message; `LocalRedirect` replaced with
`Url.IsLocalUrl(...) ? Redirect(...) : RedirectToAction("Index", "Home")`.

### 18. Roles seeded but never enforced — **FIXED** ✅
Verified: only 2 of 13 authorized controllers restricted by role.
Fix (role matrix now enforced):
- `TrackEvent`, `FieldEvent`, `HighJump`: `Admin,Official` (controller-level).
- `Student`, `School`, `Event`, `Tournament`: viewing for any signed-in user;
  every Create/Edit/Delete/status/archive/event-level action requires `Admin,Official`.
- `Registration`: viewing for any signed-in user; Register/Unregister for
  `Admin,Official,Coach,Principal` — and coaches/principals are **scoped to their
  own school** via `ApplicationUser.SchoolId` (previously never read outside user
  management).
- `UserManagement`, `ScoringConfig`: `Admin` (as before).

### 19. Heat times writable across events (IDOR) — **FIXED** ✅
Verified. Fix: `SaveHeatTimesAsync` loads the heat's own lanes and ignores any
posted `HeatLaneId` outside that set. The same pattern in
`HighJumpService.RecordAttemptAsync` is fixed too (the registration must belong to
the bar's event level). `RemoveEventLevelAsync`/`ToggleAgeWaiverAsync` redirect
mismatches remain cosmetic (the actions themselves operate on the posted id and
are now role-restricted).

### 20. HTTPS redirect behind Caddy without forwarded headers — **FIXED** ✅
Verified. Fix: `UseForwardedHeaders` (`X-Forwarded-Proto|For`, known-proxy lists
cleared because the proxy's Docker-network address isn't fixed) registered before
`UseHttpsRedirection`. `Request.IsHttps`, HSTS and secure-cookie behaviour are now
correct behind the proxy.

### 21. AutoMapper NU1903 advisory — **PARTIALLY FIXED** ⚠️ licensing decision
Verified (GHSA-rvv3-g6hj-g44x, uncontrolled-recursion DoS). Correction to the
original finding: there is **no patched 13.x**; the fix ships in 15.1.1/16.1.1,
which fall under AutoMapper's new commercial/RPL dual licence.
Done: upgraded 13.0.1 → **14.0.0** (last original-licence version). The warning is
left visible on purpose. Residual risk is negligible here (only trusted, bounded
EF entity graphs are mapped). Full remediation = the licensing decision above.
Related hygiene, all fixed: `EXPOSE 80` moved to the Dockerfile runtime stage;
`${IMAGE_TAG}` defaults to `latest` so plain `docker compose up` works; the unused
`./data` volume removed. `AllowedHosts: "*"` was left as-is deliberately — the
public hostname isn't recorded in the repo and Caddy fronts the app; set it to the
real domain if desired.

---

## P3 — Performance & robustness

### 22. N+1 query storm in high-jump results — **FIXED** ✅
Verified. Fix: `CalculateResultsAsync` and `IsEliminatedAsync` use the attempts
already eager-loaded by `GetByTournamentEventLevelAsync`; the per-registration ×
per-height re-fetch is gone (240 queries → 1 for the doc's 20×12 example).

### 23. Per-event-level query loops in reports/standings/dashboard — **FIXED** ✅
Verified. Fix: new `IEventRegistrationRepository.GetByTournamentAsync` loads a
tournament's registrations (student, school, zone, event, level, score) in one
query; `GetSchoolStandingsAsync`, all six `ReportService` methods and
`DashboardService` now aggregate in memory. New
`ITournamentEventLevelRepository.GetByCategoryAsync` collapses the scoring-page
picker's per-tournament loop to one query.

### 24. Pagination loaded the entire table — **FIXED (students)** ✅
Verified. Fix: `IStudentRepository.GetPagedAsync` pushes filtering, search,
ordering, `Skip/Take` and the true total count into SQL; `StudentController.Index`
uses it (stale page numbers are clamped and re-queried). The silent `SearchAsync`
`.Take(50)` is gone — search results are properly paged with a correct total.
Schools (~70 rows) and tournaments (a handful) keep in-memory paging deliberately:
bounded data, no measurable benefit.

### 25. Read-then-write race on high-jump SortOrder — **FIXED** ✅
Verified. Fix: ordering everywhere (repository, service, view) now derives from
`Height` itself, and #38's unique `(TournamentEventLevelId, Height)` index makes
the countback's "highest bar" deterministic regardless of insertion races.

### 26. Lane assignment ignored standard seeding — **FIXED** ✅
Verified. Fix: lanes are assigned centre-out (4, 5, 3, 6, 2, 7, 1, 8 for 8 lanes);
`PopulateNextRoundAsync` feeds athletes fastest-first, so the fastest qualifiers
get the middle lanes.

### 27. Smaller items — **ALL FIXED** ✅
| Item | Resolution |
|---|---|
| Recorded score didn't recalculate points | `RecordScoreAsync` recomputes `CalculatedPoints` from the event's constant, and refreshes placements if they had already been calculated for the event |
| `RawPerformance == 0` silently skipped | Points path no longer skips zero; placements explicitly exclude no-mark scores **and clear their stale placement** (tested) |
| Empty events counted as "in progress" | Dashboard counts only event levels with registrations |
| `IsAgeBased` with no `MaxAge` | "Open Boys/Girls" seeded with `IsAgeBased = false` (existing rows unaffected — seeder skips populated tables) |
| Dead `IsEliminated` branch | Removed (`FailCount` covers it) |
| `Repository.Update` on tracked entities | `Update` now attaches only detached entities; tracked ones rely on change detection |
| Deactivation check after sign-in | `IsActive` checked **before** `PasswordSignInAsync`, same generic error (no account enumeration) |
| CDN dependency for icons | Bootstrap Icons 1.11.3 vendored to `wwwroot/lib/bootstrap-icons/`; both layouts use the local copy — icons now work offline |
| Unused volume mount | Removed from docker-compose |

---

## Second sweep findings

### 28. `/Student/Details/{id}` missing view — **FIXED** ✅
Verified (only missing view in the app). Fix: `Views/Student/Details.cshtml` added
in the style of the sibling Details pages, and a Details link added to the student
list row actions.

### 29. Eight ViewBag key mismatches — **FIXED** ✅
All eight verified. Fixes: `Student/Index` reads `SelectedSchoolId`/`SearchTerm`;
`School/Index` reads `SelectedZoneId`; `FieldEventController`,
`HighJumpController` and both `RegistrationController` actions now set
`EventName`/`LevelName` the way `TrackEventController` does (officials can see
which event and age group they are scoring); `ByStudent.cshtml` reads the
`ViewBag.Student` DTO.

### 30. School level filter wrong cast — **FIXED** ✅
Verified. Fix: `ViewBag.SelectedLevel as SchoolLevel?` with enum comparisons.

### 31. Every notification rendered twice — **FIXED** ✅
Verified (exactly the 13 listed views). Fix: all per-view
`<partial name="_Notification" />` removed; the layout's single render remains.

### 32. "Keep this alert visible" defeated — **FIXED** ✅
Verified. Fix: the duplicate timer in `site.js` is gone. The remaining timer in
`_Notification.cshtml` was also tightened: it now dismisses only alerts the
partial itself marks `data-autodismiss` (success/error) — warnings (the jump-off
message) and informational page content ("No heats yet", "no event levels") are
never auto-dismissed.

### 33. Eligibility pre-check computed then thrown away — **FIXED** ✅
Verified. Fix: `Register.cshtml` got `@model EventRegistrationCreateDto`,
re-requests the page with `studentId` on selection change, renders the
eligibility verdict (green/red panel, register button disabled when ineligible),
and keeps the selection on a rejected submission.

### 34. Events couldn't be edited or deleted from the UI — **FIXED** ✅
Verified. Fix: Edit and Delete (with confirm) buttons per event row; inactive
events are badged.

### 35. Event category filter unreachable — **FIXED** ✅
Verified. Fix: a category dropdown drives `?category=`; grouping comes from
`Enum.GetValues<EventCategory>()` so future categories appear automatically.

### 36. Missing scoring constants couldn't be added in-app — **FIXED** ✅
Verified. Fix: `ScoringConfig` now supports **add** (dropdown offers only events
without a constant — i.e. the 12 broken ones) and **delete** for scoring
constants, and add/delete for placement points (9th place etc.). All validation
failures surface via the notification banner instead of a silent redirect.

### 37. Archiving hid a tournament's reports — **FIXED** ✅
Verified. Fix: the report picker includes archived tournaments, labelled
"(archived)".

### 38. No uniqueness on round order / heat number / bar height — **FIXED** ✅
Verified. Fix: unique indexes on `(TournamentEventLevelId, RoundOrder)`,
`(RoundId, HeatNumber)` and `(TournamentEventLevelId, Height)` in the
`SchemaIntegrityFixes` migration. Friendly pre-checks added where users can hit
them (duplicate round order, duplicate bar height) so the constraint is a backstop,
not the error message.

### 39. A round couldn't be re-seeded — **FIXED** ✅
Verified. Fix: rounds with heats show a "Re-seed" button behind an explicit
confirm describing what will be replaced (handles late scratches/entries).

### 40. Admin lock-out — **FIXED** ✅
Verified. Fix: `ToggleActive` and `Edit` refuse to deactivate/demote the current
user and refuse any change that would leave zero active Admins.

### 41. `test-data.sql` targeted tournaments the seeder never creates — **FIXED** ✅
Verified (the seeder seeds no tournaments at all). Fix: the script now creates the
two named tournaments idempotently and resolves their IDs by name everywhere
(temp table `_tt`); the header comment describes reality; the summary query no
longer assumes IDs 1/2.

### 42. Extra DB work per page / user list — **FIXED** ✅
Verified. Fixes: a claims principal factory stamps first/last name into the
sign-in cookie and `_LoginPartial` reads claims — no more per-request
`GetUserAsync` (existing sessions fall back to the email initial until next
sign-in). `UserManagement.Index` does one query per role instead of one per user.

### 43. Excel exports wrote `"-"` into numeric columns — **FIXED** ✅
Verified. Fix: missing values are left blank; columns keep a uniform numeric type.

### 44. Orphaned and mislabelled files — **FIXED** ✅
Verified. Fixes: `userguide.html` deleted (duplicate of the live Help page);
`ViewModels/Placeholder.cs` deleted with its `_ViewImports` using;
`ScoreSheetDocument.cs` renamed to `QuestPdfLicenseInitializer.cs` to match its
content.

### 45. Test suite covered only pure functions — **IMPROVED** ✅ / structural note
Verified. Done: the wrong leap-year comment fixed, and 11 new service-level tests
added with mocked `IUnitOfWork` covering the highest-risk fixed logic —
eligibility (inactive student/school, completed/archived tournament, relay
rejection), placement ranking direction, tie pooling, stale-placement clearing,
and advancement-flag reset. The structural recommendation stands: an integration
test suite against a real Postgres (Testcontainers) would have caught #12, #13,
#28 and #41; it needs Docker in CI and is left as follow-up work.

---

## Summary of the fix pass

- **45/45 findings verified accurate** (one detail corrected in #21).
- **43 fully fixed in code**; #15 fixed in HEAD with mandatory manual rotation;
  #21 mitigated pending a licensing decision.
- New EF migration: `20260811121912_SchemaIntegrityFixes` (nullable + filtered
  unique `ExistingStudentId` with data normalisation; three new unique indexes).
  It applies automatically at startup (`Database.MigrateAsync`).
- Build clean (only the deliberate NU1903 remains); tests 63/63.
- Behavioural notes: scoring pages now require the `Official` (or `Admin`) role;
  coaches/principals can register/unregister only their own school's students;
  deploys need `DB_CONNECTION_STRING` in the environment.