Store a goal where it happened, and draw half time across the timeline

docs/architecture.md

@@ -12,7 +12,8 @@ Models/
   Game.cs                — Game entity (incl. SeasonId + live match clock/state), GameSplitType and MatchState enums
   GamePeriod.cs          — GamePeriod entity, PeriodType enum, PeriodTypeExtensions
   GamePlayerPosition.cs  — Links player to position in a period (IsSubstitute flag)
-  GameGoal.cs            — A goal: scorer (null for the opponent), assister, minute (+ stoppage), own/opponent flags
+  GameGoal.cs            — A goal: scorer (null for the opponent), assister, the half + clock reading it was
+                           scored at, own/opponent flags
   GameSubstitution.cs    — A timestamped change made during a live match
   MatchPreferences.cs    — Per-season game defaults (duration, split, formation, match day)
   GameComment.cs         — An admin's note on a game: body, public/private, author, edited marker
@@ -52,7 +53,8 @@ Reporting/
   PlayerStatsReport.cs    — Per-player aggregates (PlayerStats, PositionStat, PlayerGameStat)
   PositionFitHelper.cs    — 5-tier position fit: Preferred, NaturalFit, Alternative, Compatible, OutOfPosition
   MatchClockReport.cs     — Derives the live clock and the half's reading from the stored anchor +
-                           banked total, and the MatchMinute an event is written down against
+                           banked total, the MatchMinute an event is written down against, and the
+                           half it belongs to
   PlannedChangesReport.cs — What the plan for the middle of a half changes versus the line-up on the
                            pitch, minus the swaps play has already overtaken, for
                            UI/Components/PlannedChangesList

docs/known_issues.md

@@ -373,13 +373,31 @@ Avoid repeating these mistakes:
   it. Q2 and Q4 reach the touchline only as `Game.MidHalfPlan()`, behind the live screen's
   `Changes (n)` pop-up. Do not "fix" a Q2 with no timings, and do not read `PeriodCount` as a count
   of stages the clock stops for.
-- **A goal's minute is not one number.** `GameGoal.Minute` stops at the end of the half and
-  `AdditionalMinute` counts the overrun beside it, because the two together are what orders a
-  timeline: counted on into a single number, a goal at 35+2 reads 37 and sorts after a goal in the
-  36th minute of the second half, which happened a minute later. Rows written before the split have
-  `AdditionalMinute = 0` and keep whatever number they were given — the migration deliberately does
-  not backfill, because nothing left in the row says whether a 37 was stoppage time or typed in by
-  hand.
+- **A goal's minute is derived, not stored — and two goals in the same table are placed by
+  different columns.** A goal logged from `/live` carries `GamePeriodId` and `AtSeconds`, the same
+  pair a substitution carries, and the minute anyone sees comes out of `MatchClockReport.MinuteOf`.
+  A goal typed in on `/result` has neither and falls back to `Minute`. So do all the goals logged
+  before `StoreGoalPeriodAndClock` that were not scored in stoppage time: that migration backfills
+  only what an old row states outright, and a plain minute does not say which half it belonged to.
+  The trap is reading `Minute` directly and finding it null on a live match, or assuming a row that
+  has one was typed in by hand. Never reinstate the previous shape — a minute frozen on the row
+  moved under stored data whenever `GameDurationMinutes` changed, and could not be corrected when a
+  half's timings were.
+  The same migration dropped `AdditionalMinute`, but **backfilled the rows that carried one first**:
+  an overrun on a row says outright that it was stoppage time, so the half follows from the minute
+  and the clock reading from that half's kick-off, and those goals still read `30+2` afterwards.
+  `32` would be the 32nd minute — two minutes into a second half — which is a different moment.
+  Rows with `AdditionalMinute = 0` were left alone, because a stored `37` could equally be a minute
+  typed in by hand. `GoalClockBackfillTests` migrates a database across that boundary and asserts
+  what the app then shows.
+- **A stored `Minute` is a scoreboard reading, and the timeline is ordered on elapsed seconds — do
+  not mix the two.** They agree only while the halves run to length. On a match whose first half
+  was whistled off three minutes long, the scoreboard's 31' is 33 minutes of elapsed play, so
+  taking `(Minute - 1) * 60` as an ordering key files a second-half goal *before* one scored in
+  first-half stoppage time — wrong running score out of `ScoreProgressionReport`, and the goal
+  drawn on the wrong side of the half-time rule. `MatchClockReport.ElapsedOf` is the conversion,
+  and it is the only thing that should produce an ordering key for a goal. It cost a review round
+  on the change that introduced it.
 
 ## Authentication
 - **`ExpireTimeSpan` does not keep anyone signed in — `IsPersistent` does.** `SignInAsync` without

docs/models.md

@@ -213,8 +213,9 @@ bench, never both and never twice.
 | GameId | int | FK → Game (cascade delete) |
 | ScorerId | int? | FK → Player, **SetNull**. Null for an opponent goal — we don't track their players |
 | AssisterId | int? | FK → Player, SetNull |
-| Minute | int? | Free-typed on `/result`; stamped from the scoreboard clock on `/live`, and never past the end of the half |
-| AdditionalMinute | int | Minutes into stoppage time, from 1; 0 in normal play. Stored apart from `Minute` so 35+2 sorts before 36 — see `MatchMinute` |
+| GamePeriodId | int? | FK → GamePeriod (cascade delete). The half that was being played. Null for a goal typed in on `/result` |
+| AtSeconds | int? | Match-clock second the ball went in. Null for the same reason |
+| Minute | int? | Free-typed on `/result`, and the fallback for goals logged before `AtSeconds` existed. Not written by `/live` any more. A scoreboard reading, not elapsed time — convert with `MatchClockReport.ElapsedOf` before ordering on it |
 | IsOwnGoal | bool | One of ours into our own net. Counts for the opponent |
 | IsOpponentGoal | bool | The opponent scored. Counts for them, and has no scorer |
 | RecordedAt | DateTime | UTC entry time — orders events that share a minute |
@@ -231,18 +232,21 @@ bench, never both and never twice.
 | Position | PlayerPosition | The position that changed hands |
 | RecordedAt | DateTime | UTC entry time — orders events that share a minute |
 
-A substitution has no stored minute: `MatchClockReport.MinuteOf` derives it from `AtSeconds` and
-the half the change belongs to, so it reads off the same scoreboard clock a goal was stamped from
-rather than the raw elapsed time. A goal cannot be derived that way — one typed in on `/result` has
-no clock behind it at all — which is why its minute is stored, both halves of it.
-
-`RecordedAt` exists on both `GameGoal` and `GameSubstitution` because the minute alone cannot order
-a timeline: a goal and the substitution that followed it routinely share one, and several events in
-the opening minute is the normal case, not the edge case. The live timeline sorts by minute, then by
-`RecordedAt`, then by `Id`, all descending. Rows written before the column existed default to
-`0001-01-01`, and two changes entered in one instant share it, so `RecordedAt` cannot settle a
-double substitution on its own — the id is the last word, and it is the same one
-`RemoveSubstitutionAsync` uses, so the entry the timeline puts on top is the entry whose Undo works.
+**Neither kind of event stores the minute it is shown against.** Both store where they happened —
+the half, and the reading on the match clock — and `MatchClockReport.MinuteOf` derives the minute
+from that pair, so the two kinds read off one code path and correcting a half's `StartedAtSeconds`
+corrects the goals in it as well as the substitutions. A goal typed in on `/result` has no clock
+behind it and falls back to `Minute`; a goal with neither shows no minute at all, which the result
+page allows.
+
+`RecordedAt` exists on both `GameGoal` and `GameSubstitution` because the clock alone cannot order
+a timeline: a goal and the substitution that followed it routinely share a second, and several
+events in the opening minute is the normal case, not the edge case. The live timeline sorts by
+elapsed seconds (`MatchClockReport.ElapsedOf`, `GameSubstitution.AtSeconds`), then by `RecordedAt`,
+then by `Id`, all descending. Rows written before the column existed default to `0001-01-01`, and
+two changes entered in one instant share it, so `RecordedAt` cannot settle a double substitution on
+its own — the id is the last word, and it is the same one `RemoveSubstitutionAsync` uses, so the
+entry the timeline puts on top is the entry whose Undo works.
 Ids from the two tables are not comparable with each other, so a goal and a substitution that tie on
 both minute and `RecordedAt` keep an arbitrary (but stable) order.
 
@@ -374,6 +378,8 @@ runs on every startup and does nothing once any account exists, so a changed pas
 Season 1──* Game 1──* GamePeriod 1──* GamePlayerPosition *──1 Player
 Season 1──* SeasonSquadMember *──1 Player
 Game 1──* GameGoal *──1 Player (scorer, assister — both SetNull)
+GamePeriod 1──* GameGoal (the half it was scored in — nullable, cascade)
+GamePeriod 1──* GameSubstitution (the half it was made in — cascade)
 Game 1──* GameSubstitution *──1 Player (off, on — both Restrict)
 Game 1──* GameComment *──1 AppUser (author — SetNull)
 ```

docs/testing.md

@@ -33,6 +33,7 @@ Every test class, so a gap here is visible rather than assumed:
 | **Authorization** | `AuthorizationTests` | That every write refuses a non-admin *at the service*, not only in the markup — the guard the whole write path rests on |
 | Accounts | `UserServiceTests`, `SeededAdminTests` | Credentials, security stamps, the last-admin guard, and the seeded account being no working login |
 | Boot safety | `DatabaseSafetyTests`, `HealthReportTests` | The pre-migration snapshot and what `/health` is allowed to call healthy |
+| Migrations that rewrite rows | `GoalClockBackfillTests` | The only migration with a backfill in it. Migrates a seeded database across the boundary and asserts what the app then *shows* — a goal written `30+2` still reads `30+2` — rather than what landed in a column. Every other migration is covered implicitly, because `ServiceTestBase` builds the schema from the model |
 | Service lifetime | `ServiceLifetimeTests` | Concurrent reads, and detached entities round-tripping through update |
 | `Result` | `ResultTests` | Error keys, arguments, the guard on reading a failed value, and that a cancellation stays one when carried between types |
 | Cancellation | `CancellationTests` | That a caller going away is an ordinary outcome and not a logged error — including that an `OperationCanceledException` nobody asked for still is one |

docs/ui_components.md

@@ -145,14 +145,25 @@ watches the same URL read-only. Every control sits in an `<AuthorizeView Roles="
   scoreboard's order — home side first. It is counted forwards over the whole match and looked up
   by goal id, because the timeline itself runs newest first and a total accumulated while rendering
   would count down.
-- **Events are written and ordered as `MatchMinute`, a pair — 35, or 35+2 in stoppage time.** The
-  minute alone cannot order them: once a half is played out the scoreboard clock stops, so a goal
-  two minutes into first-half stoppage and one just after the restart both read in the thirties and
-  a single counted-on number puts them the wrong way round. A goal stores both halves of the pair
-  (`GameGoal.Minute` + `AdditionalMinute`); a substitution derives its own from `AtSeconds` and the
-  half it belongs to (`MatchClockReport.MinuteOf`), so a second-half swap is written off the
-  scoreboard clock rather than the raw elapsed time. The timeline, the result page's goal list and
-  `ScoreProgressionReport` all sort on the pair, then `RecordedAt`, then the id.
+- **Events are shown as a `MatchMinute` — 35, or 35+2 in stoppage time — and ordered on the elapsed
+  match clock.** The two are different scales and that is the point: the scoreboard reading stops at
+  the end of the half, so a goal two minutes into first-half stoppage and one just after the restart
+  both read in the thirties, while the elapsed clock runs on across the break and puts them in the
+  order they happened without anyone comparing pairs. Neither kind of event stores the minute it
+  displays: a goal carries `GamePeriodId` + `AtSeconds` exactly as a substitution does, and
+  `MatchClockReport.MinuteOf` derives the reading from the half's own timings. The timeline, the
+  result page's goal list and `ScoreProgressionReport` all sort on elapsed seconds
+  (`MatchClockReport.ElapsedOf`), then `RecordedAt`, then the id. A goal typed in on `/result` has
+  only a scoreboard minute, and `ElapsedOf` converts it back through the half timings rather than
+  reading it as elapsed time — the two scales part company by however long a half over-ran, and
+  taking one for the other puts a second-half goal under the half-time rule.
+- **Half time is a dashed rule across the timeline** (`.live-event-break`), not an event. The list
+  runs newest first, so it lands where the second half's entries give way to the first's;
+  `MatchClockReport.HalfOf` decides which side an entry is on, from its own line-up's half or —
+  for a goal typed in by hand — from which side of the second half's kick-off its clock reading
+  falls. `LiveMatch.Timeline` marks the one entry it is drawn above, because the markup renders an
+  entry at a time and cannot see its neighbour, and because the substitutions filter decides who
+  the neighbours are.
 - A **"Show substitutions" checkbox** (`.live-timeline-toggle`) drops the substitutions from the
   timeline and leaves the goals: a rotated squad buries the goals among swaps nobody is scrolling
   back for. The state is per circuit and deliberately not stored.

src/FootballFormation.Core/Data/Configurations/GameEventConfigurations.cs

@@ -21,6 +21,14 @@ public void Configure(EntityTypeBuilder<GameGoal> entity)
             .WithMany()
             .HasForeignKey(g => g.AssisterId)
             .OnDelete(DeleteBehavior.SetNull);
+
+        // Cascade like GameSubstitution's: the half and the events recorded during it are one
+        // record, and a goal pointing at a line-up that no longer exists has no minute to show.
+        // Declared without a navigation — see GameGoal.GamePeriodId.
+        entity.HasOne<GamePeriod>()
+            .WithMany()
+            .HasForeignKey(g => g.GamePeriodId)
+            .OnDelete(DeleteBehavior.Cascade);
     }
 }