From aea32e95d61e1e7356d263e90b5e091ce3a28dfd Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 10:44:18 +0000 Subject: [PATCH 1/3] Store a goal where it happened, and draw half time across the timeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A goal now carries the two facts a substitution already did — the half being played and the reading on the match clock — and the minute anyone sees is derived from them rather than frozen on the row when it was logged. Editing a match's duration no longer silently reinterprets goals already on file, and correcting a half's timings now moves its goals with its substitutions instead of leaving the two kinds of event disagreeing about the same timeline. Ordering follows: both kinds sort on elapsed seconds, which runs on across the break and needs no pair to keep a stoppage-time goal above the restart. That retires GameGoal.AdditionalMinute and MatchMinute.CompareTo, and leaves MatchMinute as the display it always looked like. The migration adds GamePeriodId and AtSeconds and drops AdditionalMinute, with no backfill: nothing left in an old row says whether a stored 37 was stoppage time or a minute typed in by hand, so those goals go on reading and sorting off Minute exactly as they do today. With the half on every event, the timeline can say where the break was — a dashed rule between the second half's entries and the first's, which is what issue #83 asked for on top of the model change. Closes #83 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XD7SBD1CvRzK4Xrhx3dtf7 --- docs/architecture.md | 6 +- docs/known_issues.md | 17 +- docs/models.md | 34 +- docs/ui_components.md | 25 +- .../Configurations/GameEventConfigurations.cs | 7 + ...102938_StoreGoalPeriodAndClock.Designer.cs | 606 ++++++++++++++++++ .../20260814102938_StoreGoalPeriodAndClock.cs | 85 +++ .../Migrations/AppDbContextModelSnapshot.cs | 16 +- src/FootballFormation.Core/Models/GameGoal.cs | 36 +- .../Models/MatchMinute.cs | 16 +- .../Reporting/MatchClockReport.cs | 50 +- .../Reporting/ScoreProgressionReport.cs | 9 +- .../Services/MatchGoalService.cs | 20 +- .../Pages/LiveMatch.razor | 12 +- .../Pages/LiveMatch.razor.cs | 41 +- .../Pages/LiveMatch.razor.css | 22 + .../Pages/MatchResult.razor | 9 +- .../MatchClockReportTests.cs | 93 ++- .../MatchGoalServiceTests.cs | 55 +- .../ScoreProgressionReportTests.cs | 32 +- tests/ui/specs/match-day.spec.js | 38 ++ tests/ui/specs/selectors.spec.js | 3 +- 22 files changed, 1100 insertions(+), 132 deletions(-) create mode 100644 src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.Designer.cs create mode 100644 src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs diff --git a/docs/architecture.md b/docs/architecture.md index 9f67573..1d92de0 100644 --- a/docs/architecture.md +++ b/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 diff --git a/docs/known_issues.md b/docs/known_issues.md index 86d56fc..2f77b79 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -373,13 +373,16 @@ 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 migration deliberately does not backfill, because nothing + left in an old row says which half a stored `37` belonged to or whether it was stoppage time. 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. Sort with `GameGoal.TimelineSeconds`, which covers both, and 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. ## Authentication - **`ExpireTimeSpan` does not keep anyone signed in — `IsPersistent` does.** `SignInAsync` without diff --git a/docs/models.md b/docs/models.md index 271b811..4277a4c 100644 --- a/docs/models.md +++ b/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 | | 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 (`GameGoal.TimelineSeconds`, `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) ``` diff --git a/docs/ui_components.md b/docs/ui_components.md index 4c85103..c6cae15 100644 --- a/docs/ui_components.md +++ b/docs/ui_components.md @@ -145,14 +145,23 @@ watches the same URL read-only. Every control sits in an ` 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. + entity.HasOne(g => g.GamePeriod) + .WithMany() + .HasForeignKey(g => g.GamePeriodId) + .OnDelete(DeleteBehavior.Cascade); } } diff --git a/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.Designer.cs b/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.Designer.cs new file mode 100644 index 0000000..5276ce5 --- /dev/null +++ b/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.Designer.cs @@ -0,0 +1,606 @@ +// +using System; +using FootballFormation.Core.Data; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; + +#nullable disable + +namespace FootballFormation.Core.Migrations +{ + [DbContext(typeof(AppDbContext))] + [Migration("20260814102938_StoreGoalPeriodAndClock")] + partial class StoreGoalPeriodAndClock + { + /// + protected override void BuildTargetModel(ModelBuilder modelBuilder) + { +#pragma warning disable 612, 618 + modelBuilder.HasAnnotation("ProductVersion", "10.0.10"); + + modelBuilder.Entity("FootballFormation.Core.Models.AppUser", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("DisplayName") + .IsRequired() + .HasMaxLength(100) + .HasColumnType("TEXT"); + + b.Property("MustChangePassword") + .HasColumnType("INTEGER"); + + b.Property("PasswordHash") + .IsRequired() + .HasColumnType("TEXT"); + + b.Property("Role") + .HasColumnType("INTEGER"); + + b.Property("SecurityStamp") + .IsRequired() + .HasMaxLength(64) + .HasColumnType("TEXT"); + + b.Property("Username") + .IsRequired() + .HasMaxLength(50) + .HasColumnType("TEXT"); + + b.HasKey("Id"); + + b.HasIndex("Username") + .IsUnique(); + + b.ToTable("Users", (string)null); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.Game", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("ClockAccumulatedSeconds") + .HasColumnType("INTEGER"); + + b.Property("ClockRunningSince") + .HasColumnType("TEXT"); + + b.Property("Date") + .HasColumnType("TEXT"); + + b.Property("FormationType") + .HasColumnType("INTEGER"); + + b.Property("GameDurationMinutes") + .HasColumnType("INTEGER"); + + b.Property("GuestPlayerIds") + .IsRequired() + .HasColumnType("TEXT"); + + b.Property("IsHomeGame") + .HasColumnType("INTEGER"); + + b.Property("LivePeriodId") + .HasColumnType("INTEGER"); + + b.Property("MatchState") + .HasColumnType("INTEGER"); + + b.Property("MatchType") + .HasColumnType("INTEGER"); + + b.Property("Opponent") + .IsRequired() + .HasMaxLength(100) + .HasColumnType("TEXT"); + + b.Property("ScoreAway") + .HasColumnType("INTEGER"); + + b.Property("ScoreHome") + .HasColumnType("INTEGER"); + + b.Property("SeasonId") + .HasColumnType("INTEGER"); + + b.Property("SplitType") + .HasColumnType("INTEGER"); + + b.Property("UnavailablePlayerIds") + .IsRequired() + .HasColumnType("TEXT"); + + b.HasKey("Id"); + + b.HasIndex("SeasonId"); + + b.ToTable("Games"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GameComment", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("AuthorId") + .HasColumnType("INTEGER"); + + b.Property("Body") + .IsRequired() + .HasMaxLength(2000) + .HasColumnType("TEXT"); + + b.Property("CreatedAt") + .HasColumnType("TEXT"); + + b.Property("EditedAt") + .HasColumnType("TEXT"); + + b.Property("GameId") + .HasColumnType("INTEGER"); + + b.Property("IsPublic") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("AuthorId"); + + b.HasIndex("GameId", "CreatedAt"); + + b.ToTable("GameComments"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GameGoal", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("AssisterId") + .HasColumnType("INTEGER"); + + b.Property("AtSeconds") + .HasColumnType("INTEGER"); + + b.Property("GameId") + .HasColumnType("INTEGER"); + + b.Property("GamePeriodId") + .HasColumnType("INTEGER"); + + b.Property("IsOpponentGoal") + .HasColumnType("INTEGER"); + + b.Property("IsOwnGoal") + .HasColumnType("INTEGER"); + + b.Property("Minute") + .HasColumnType("INTEGER"); + + b.Property("RecordedAt") + .HasColumnType("TEXT"); + + b.Property("ScorerId") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("AssisterId"); + + b.HasIndex("GameId"); + + b.HasIndex("GamePeriodId"); + + b.HasIndex("ScorerId"); + + b.ToTable("GameGoals"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GamePeriod", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("EndedAtSeconds") + .HasColumnType("INTEGER"); + + b.Property("FormationTypeOverride") + .HasColumnType("INTEGER"); + + b.Property("GameId") + .HasColumnType("INTEGER"); + + b.Property("PeriodType") + .HasColumnType("INTEGER"); + + b.Property("StartedAtSeconds") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("GameId"); + + b.ToTable("GamePeriods"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GamePlayerPosition", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("GamePeriodId") + .HasColumnType("INTEGER"); + + b.Property("IsSubstitute") + .HasColumnType("INTEGER"); + + b.Property("PlayerId") + .HasColumnType("INTEGER"); + + b.Property("Position") + .HasColumnType("INTEGER"); + + b.Property("SlotIndex") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("PlayerId"); + + b.HasIndex("GamePeriodId", "PlayerId") + .IsUnique(); + + b.ToTable("GamePlayerPositions"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GameSubstitution", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("AtSeconds") + .HasColumnType("INTEGER"); + + b.Property("GameId") + .HasColumnType("INTEGER"); + + b.Property("GamePeriodId") + .HasColumnType("INTEGER"); + + b.Property("PlayerOffId") + .HasColumnType("INTEGER"); + + b.Property("PlayerOnId") + .HasColumnType("INTEGER"); + + b.Property("Position") + .HasColumnType("INTEGER"); + + b.Property("RecordedAt") + .HasColumnType("TEXT"); + + b.Property("SlotIndex") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("GameId"); + + b.HasIndex("GamePeriodId"); + + b.HasIndex("PlayerOffId"); + + b.HasIndex("PlayerOnId"); + + b.ToTable("GameSubstitutions"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.MatchPreferences", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("DefaultFormation") + .HasColumnType("INTEGER"); + + b.Property("DefaultSplitType") + .HasColumnType("INTEGER"); + + b.Property("GameDurationMinutes") + .HasColumnType("INTEGER"); + + b.Property("MatchDay") + .HasColumnType("INTEGER"); + + b.Property("SeasonId") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("SeasonId") + .IsUnique(); + + b.ToTable("MatchPreferences"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.Player", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("AlternativePositions") + .IsRequired() + .HasColumnType("TEXT"); + + b.Property("FirstName") + .IsRequired() + .HasMaxLength(50) + .HasColumnType("TEXT"); + + b.Property("IsArchived") + .HasColumnType("INTEGER"); + + b.Property("PreferredPosition") + .HasColumnType("INTEGER"); + + b.Property("ShirtNumber") + .HasColumnType("INTEGER"); + + b.Property("Surname") + .HasMaxLength(50) + .HasColumnType("TEXT"); + + b.HasKey("Id"); + + b.ToTable("Players"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.Season", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("EndDate") + .HasColumnType("TEXT"); + + b.Property("IsCurrent") + .HasColumnType("INTEGER"); + + b.Property("Name") + .IsRequired() + .HasMaxLength(20) + .HasColumnType("TEXT"); + + b.Property("StartDate") + .HasColumnType("TEXT"); + + b.HasKey("Id"); + + b.HasIndex("StartDate") + .IsUnique(); + + b.ToTable("Seasons"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.SeasonSquadMember", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("INTEGER"); + + b.Property("IsGuest") + .HasColumnType("INTEGER"); + + b.Property("PlayerId") + .HasColumnType("INTEGER"); + + b.Property("SeasonId") + .HasColumnType("INTEGER"); + + b.HasKey("Id"); + + b.HasIndex("PlayerId"); + + b.HasIndex("SeasonId", "PlayerId") + .IsUnique(); + + b.ToTable("SeasonSquadMembers"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.Game", b => + { + b.HasOne("FootballFormation.Core.Models.Season", "Season") + .WithMany("Games") + .HasForeignKey("SeasonId") + .OnDelete(DeleteBehavior.Restrict) + .IsRequired(); + + b.Navigation("Season"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GameComment", b => + { + b.HasOne("FootballFormation.Core.Models.AppUser", "Author") + .WithMany() + .HasForeignKey("AuthorId") + .OnDelete(DeleteBehavior.SetNull); + + b.HasOne("FootballFormation.Core.Models.Game", "Game") + .WithMany("Comments") + .HasForeignKey("GameId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Author"); + + b.Navigation("Game"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GameGoal", b => + { + b.HasOne("FootballFormation.Core.Models.Player", "Assister") + .WithMany() + .HasForeignKey("AssisterId") + .OnDelete(DeleteBehavior.SetNull); + + b.HasOne("FootballFormation.Core.Models.Game", "Game") + .WithMany("Goals") + .HasForeignKey("GameId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("FootballFormation.Core.Models.GamePeriod", "GamePeriod") + .WithMany() + .HasForeignKey("GamePeriodId") + .OnDelete(DeleteBehavior.Cascade); + + b.HasOne("FootballFormation.Core.Models.Player", "Scorer") + .WithMany() + .HasForeignKey("ScorerId") + .OnDelete(DeleteBehavior.SetNull); + + b.Navigation("Assister"); + + b.Navigation("Game"); + + b.Navigation("GamePeriod"); + + b.Navigation("Scorer"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GamePeriod", b => + { + b.HasOne("FootballFormation.Core.Models.Game", "Game") + .WithMany("Periods") + .HasForeignKey("GameId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Game"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GamePlayerPosition", b => + { + b.HasOne("FootballFormation.Core.Models.GamePeriod", "GamePeriod") + .WithMany("PlayerPositions") + .HasForeignKey("GamePeriodId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("FootballFormation.Core.Models.Player", "Player") + .WithMany() + .HasForeignKey("PlayerId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("GamePeriod"); + + b.Navigation("Player"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GameSubstitution", b => + { + b.HasOne("FootballFormation.Core.Models.Game", "Game") + .WithMany("Substitutions") + .HasForeignKey("GameId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("FootballFormation.Core.Models.GamePeriod", "GamePeriod") + .WithMany() + .HasForeignKey("GamePeriodId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("FootballFormation.Core.Models.Player", "PlayerOff") + .WithMany() + .HasForeignKey("PlayerOffId") + .OnDelete(DeleteBehavior.Restrict) + .IsRequired(); + + b.HasOne("FootballFormation.Core.Models.Player", "PlayerOn") + .WithMany() + .HasForeignKey("PlayerOnId") + .OnDelete(DeleteBehavior.Restrict) + .IsRequired(); + + b.Navigation("Game"); + + b.Navigation("GamePeriod"); + + b.Navigation("PlayerOff"); + + b.Navigation("PlayerOn"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.MatchPreferences", b => + { + b.HasOne("FootballFormation.Core.Models.Season", "Season") + .WithMany() + .HasForeignKey("SeasonId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Season"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.SeasonSquadMember", b => + { + b.HasOne("FootballFormation.Core.Models.Player", "Player") + .WithMany() + .HasForeignKey("PlayerId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.HasOne("FootballFormation.Core.Models.Season", "Season") + .WithMany("SquadMembers") + .HasForeignKey("SeasonId") + .OnDelete(DeleteBehavior.Cascade) + .IsRequired(); + + b.Navigation("Player"); + + b.Navigation("Season"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.Game", b => + { + b.Navigation("Comments"); + + b.Navigation("Goals"); + + b.Navigation("Periods"); + + b.Navigation("Substitutions"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.GamePeriod", b => + { + b.Navigation("PlayerPositions"); + }); + + modelBuilder.Entity("FootballFormation.Core.Models.Season", b => + { + b.Navigation("Games"); + + b.Navigation("SquadMembers"); + }); +#pragma warning restore 612, 618 + } + } +} diff --git a/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs b/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs new file mode 100644 index 0000000..3aaa715 --- /dev/null +++ b/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs @@ -0,0 +1,85 @@ +using Microsoft.EntityFrameworkCore.Migrations; + +#nullable disable + +namespace FootballFormation.Core.Migrations +{ + /// + /// Gives a goal the two facts a substitution already carries — the half it happened in and the + /// reading on the match clock — so the minute shown is derived rather than frozen, and + /// correcting a half's timings corrects its goals with it. + /// + /// Deliberately not backfilled, and AdditionalMinute is dropped rather than folded into + /// anything. Nothing left in an old row says which half a stored 37 belonged to or + /// whether it was stoppage time, so those goals keep Minute and go on reading and + /// sorting exactly as they do today; only goals logged from here on carry a clock. + /// + /// + /// The drop is last on purpose. Both operations rebuild the table on SQLite, and a rebuild is + /// not transactional — leaving it until the new columns and the foreign key are in place means + /// a half-applied run has lost nothing that the next attempt needs. + /// + /// + public partial class StoreGoalPeriodAndClock : Migration + { + /// + protected override void Up(MigrationBuilder migrationBuilder) + { + migrationBuilder.AddColumn( + name: "AtSeconds", + table: "GameGoals", + type: "INTEGER", + nullable: true); + + migrationBuilder.AddColumn( + name: "GamePeriodId", + table: "GameGoals", + type: "INTEGER", + nullable: true); + + migrationBuilder.CreateIndex( + name: "IX_GameGoals_GamePeriodId", + table: "GameGoals", + column: "GamePeriodId"); + + migrationBuilder.AddForeignKey( + name: "FK_GameGoals_GamePeriods_GamePeriodId", + table: "GameGoals", + column: "GamePeriodId", + principalTable: "GamePeriods", + principalColumn: "Id", + onDelete: ReferentialAction.Cascade); + + migrationBuilder.DropColumn( + name: "AdditionalMinute", + table: "GameGoals"); + } + + /// + protected override void Down(MigrationBuilder migrationBuilder) + { + migrationBuilder.AddColumn( + name: "AdditionalMinute", + table: "GameGoals", + type: "INTEGER", + nullable: false, + defaultValue: 0); + + migrationBuilder.DropForeignKey( + name: "FK_GameGoals_GamePeriods_GamePeriodId", + table: "GameGoals"); + + migrationBuilder.DropIndex( + name: "IX_GameGoals_GamePeriodId", + table: "GameGoals"); + + migrationBuilder.DropColumn( + name: "AtSeconds", + table: "GameGoals"); + + migrationBuilder.DropColumn( + name: "GamePeriodId", + table: "GameGoals"); + } + } +} diff --git a/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs b/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs index 87b3827..26d5bfa 100644 --- a/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs +++ b/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs @@ -162,15 +162,18 @@ protected override void BuildModel(ModelBuilder modelBuilder) .ValueGeneratedOnAdd() .HasColumnType("INTEGER"); - b.Property("AdditionalMinute") + b.Property("AssisterId") .HasColumnType("INTEGER"); - b.Property("AssisterId") + b.Property("AtSeconds") .HasColumnType("INTEGER"); b.Property("GameId") .HasColumnType("INTEGER"); + b.Property("GamePeriodId") + .HasColumnType("INTEGER"); + b.Property("IsOpponentGoal") .HasColumnType("INTEGER"); @@ -192,6 +195,8 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.HasIndex("GameId"); + b.HasIndex("GamePeriodId"); + b.HasIndex("ScorerId"); b.ToTable("GameGoals"); @@ -456,6 +461,11 @@ protected override void BuildModel(ModelBuilder modelBuilder) .OnDelete(DeleteBehavior.Cascade) .IsRequired(); + b.HasOne("FootballFormation.Core.Models.GamePeriod", "GamePeriod") + .WithMany() + .HasForeignKey("GamePeriodId") + .OnDelete(DeleteBehavior.Cascade); + b.HasOne("FootballFormation.Core.Models.Player", "Scorer") .WithMany() .HasForeignKey("ScorerId") @@ -465,6 +475,8 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.Navigation("Game"); + b.Navigation("GamePeriod"); + b.Navigation("Scorer"); }); diff --git a/src/FootballFormation.Core/Models/GameGoal.cs b/src/FootballFormation.Core/Models/GameGoal.cs index e35dab0..70ca5d8 100644 --- a/src/FootballFormation.Core/Models/GameGoal.cs +++ b/src/FootballFormation.Core/Models/GameGoal.cs @@ -14,19 +14,27 @@ public class GameGoal public Player? Assister { get; set; } /// - /// The minute on the scoreboard clock, which never runs past the end of the half — the overrun - /// is . Null on a goal recorded with no minute at all, which the - /// result page allows. + /// The half that was being played when the ball went in, as the line-up playing it — the same + /// fact carries. Null for a goal typed in on the + /// result page, which has no half behind it. /// - public int? Minute { get; set; } + public int? GamePeriodId { get; set; } + public GamePeriod? GamePeriod { get; set; } + + /// + /// Match-clock second the ball went in, from the same clock a substitution is stamped from. + /// Null for a goal typed in by hand. With it is everything the + /// displayed minute is derived from, so correcting a half's timings corrects its goals too. + /// + public int? AtSeconds { get; set; } /// - /// Minutes into stoppage time, counted from 1, or zero for a goal in normal play. Stored beside - /// the minute rather than added into it because the two together are what orders the timeline: - /// a goal at 35+2 belongs before one at 36, and a single number that had counted on to 37 would - /// put it after. Always zero on a goal typed in by hand, which has no clock behind it. + /// The minute somebody typed in on the result page, where there is no clock to read. Also what + /// a goal logged before this row carried a clock still shows: those rows have no + /// and were never backfilled, because nothing left in one says whether + /// a 37 in a 35-minute half was stoppage time or a number typed in by hand. /// - public int AdditionalMinute { get; set; } + public int? Minute { get; set; } /// One of ours put it in our own net. Counts for the opponent. public bool IsOwnGoal { get; set; } @@ -48,4 +56,14 @@ public class GameGoal /// ScoreProgressionReport walks them one at a time for the live timeline. /// public bool CountsForUs => !IsOwnGoal && !IsOpponentGoal; + + /// + /// Where this goal sits on the match timeline, as elapsed seconds — the one scale goals and + /// substitutions can be ordered on together, and the reason the timeline no longer compares + /// scoreboard minutes in pairs. when there is one; otherwise the start + /// of the minute typed in, which is the best a row with no clock behind it offers. Zero for a + /// goal with neither, which puts it at the top of the match rather than nowhere. + /// + public int TimelineSeconds => + AtSeconds ?? (Minute is { } minute ? Math.Max(0, (minute - 1) * 60) : 0); } diff --git a/src/FootballFormation.Core/Models/MatchMinute.cs b/src/FootballFormation.Core/Models/MatchMinute.cs index 3163b60..1cc41ef 100644 --- a/src/FootballFormation.Core/Models/MatchMinute.cs +++ b/src/FootballFormation.Core/Models/MatchMinute.cs @@ -4,23 +4,17 @@ namespace FootballFormation.Core.Models; /// The minute an event is written down against, the way football writes one: 35, or 35+2 once the /// half has been played out and the clock is into stoppage time. /// -/// The pair is also what puts events in order, which a single number cannot do. A goal in first-half -/// stoppage time and one just after the restart are barely a minute apart, but a clock that counted -/// straight on would write the first as 37 and the second as 36 and list them the wrong way round. -/// Comparing on first and second is -/// chronological across the whole match, because a half's clock stops at the half and the half that -/// follows starts above it. +/// Display only. Ordering a timeline is 's job and +/// 's — the real elapsed clock, which runs on across the +/// break and needs no pair to say that a goal at 35+2 came before one in the 36th minute of the +/// second half. /// /// /// The minute on the clock, never past the end of the half being played. /// Minutes into stoppage time, counted from 1; zero during normal play. -public readonly record struct MatchMinute(int Minute, int Additional) : IComparable +public readonly record struct MatchMinute(int Minute, int Additional) { public bool IsAdditional => Additional > 0; - public int CompareTo(MatchMinute other) => Minute != other.Minute - ? Minute.CompareTo(other.Minute) - : Additional.CompareTo(other.Additional); - public override string ToString() => IsAdditional ? $"{Minute}+{Additional}" : $"{Minute}"; } diff --git a/src/FootballFormation.Core/Reporting/MatchClockReport.cs b/src/FootballFormation.Core/Reporting/MatchClockReport.cs index 93ff336..f090ce6 100644 --- a/src/FootballFormation.Core/Reporting/MatchClockReport.cs +++ b/src/FootballFormation.Core/Reporting/MatchClockReport.cs @@ -24,7 +24,7 @@ public record MatchClock(int Seconds, int AdditionalSeconds, MatchMinute Minute) /// The second half starts at half the match duration whatever the first half actually cost, /// so an over-running first half does not push the whole second half out of step. /// The minute an event is written down against follows that same reading, as a -/// — 35+2 rather than a 37 that would sort after the restart. +/// — 35+2 rather than a 37 nobody at the pitch would recognise. /// /// This is presentation only. What is stored stays the real elapsed time, so playing time, /// substitution timings and the season statistics are unaffected. @@ -56,7 +56,7 @@ public static MatchClock Build(Game game, GamePeriod? displayHalf, int elapsedSe // Once the half is played out the clock stands still at the cap, so the minute stands still // with it and the overrun is counted alongside as 35+1, 35+2 — the reading a scoreboard - // shows, and the only way several stoppage-time events keep their order. + // shows, and what an event in stoppage time is written down against. if (intoHalf >= halfSeconds) { return new MatchClock( @@ -70,20 +70,48 @@ public static MatchClock Build(Game game, GamePeriod? displayHalf, int elapsedSe /// /// The minute a substitution is written down against: the reading the clock showed when it was - /// made, which is the half's reading and not the raw elapsed time. Falls back to the raw minute - /// for a substitution whose half was not loaded — a wrong-looking minute beats claiming 1'. + /// made, which is the half's reading and not the raw elapsed time. /// public static MatchMinute MinuteOf(Game game, GameSubstitution substitution) => - game.Periods.FirstOrDefault(p => p.Id == substitution.GamePeriodId) is { } half - ? Build(game, half, substitution.AtSeconds).Minute - : PlainMinute(substitution.AtSeconds); + MinuteAt(game, substitution.GamePeriodId, substitution.AtSeconds); /// - /// The minute a goal was written down against, or null for one recorded without a minute — - /// which the result page allows and the timeline then has nothing to place. + /// The minute a goal is written down against — derived the same way a substitution's is, from + /// the clock reading and the half it was scored in, so correcting a half's timings corrects + /// its goals with it. A goal with no clock behind it falls back to the minute stored on the + /// row, and one with neither has no minute at all, which the result page allows. /// - public static MatchMinute? MinuteOf(GameGoal goal) => - goal.Minute is { } minute ? new MatchMinute(minute, goal.AdditionalMinute) : null; + public static MatchMinute? MinuteOf(Game game, GameGoal goal) => goal switch + { + { AtSeconds: { } at } => MinuteAt(game, goal.GamePeriodId, at), + { Minute: { } minute } => new MatchMinute(minute, 0), + _ => null + }; + + /// + /// The half an event belongs to, which is what puts the half-time break on the timeline. Its + /// own line-up's half when it has one; otherwise whichever side of the second half's kick-off + /// its clock reading falls, which is all a goal typed in by hand leaves to go on. First half + /// when nothing says otherwise — a match never run from the touchline has no kick-off to be + /// past, and one unbroken list is the honest way to show it. + /// + public static PeriodType HalfOf(Game game, int? periodId, int atSeconds) => + FindPeriod(game, periodId) is { } period ? period.PeriodType.Half() + : HalfKickedOffAt(game, PeriodType.SecondHalf) is { } restart && atSeconds >= restart + ? PeriodType.SecondHalf + : PeriodType.FirstHalf; + + /// + /// The reading the clock showed at a moment in a given half. Falls back to the raw minute when + /// that half was not loaded — a wrong-looking minute beats claiming 1'. + /// + private static MatchMinute MinuteAt(Game game, int? periodId, int atSeconds) => + FindPeriod(game, periodId) is { } half + ? Build(game, half, atSeconds).Minute + : PlainMinute(atSeconds); + + private static GamePeriod? FindPeriod(Game game, int? periodId) => + periodId is { } id ? game.Periods.FirstOrDefault(p => p.Id == id) : null; /// The minute a plain clock reading falls in. The first minute of play is 1'. private static MatchMinute PlainMinute(int seconds) => new((seconds / 60) + 1, 0); diff --git a/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs b/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs index e2e5dac..4a1ebaf 100644 --- a/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs +++ b/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs @@ -25,11 +25,12 @@ public static IReadOnlyDictionary Build(IEnumerable g var us = 0; var them = 0; - // The order the live screen shows events in, read forwards: the match minute first — both - // halves of it, so a stoppage-time goal stays inside the half it was scored in — then the - // moment it was entered, then the id. See LiveMatch.Timeline for why all three. + // The order the live screen shows events in, read forwards: the elapsed match clock first, + // which runs on across the break and so keeps a stoppage-time goal inside the half it was + // scored in, then the moment it was entered, then the id. See LiveMatch.Timeline for why + // all three. var chronological = goals - .OrderBy(g => MatchClockReport.MinuteOf(g) ?? default) + .OrderBy(g => g.TimelineSeconds) .ThenBy(g => g.RecordedAt) .ThenBy(g => g.Id); diff --git a/src/FootballFormation.Core/Services/MatchGoalService.cs b/src/FootballFormation.Core/Services/MatchGoalService.cs index 092bdba..77fd5a1 100644 --- a/src/FootballFormation.Core/Services/MatchGoalService.cs +++ b/src/FootballFormation.Core/Services/MatchGoalService.cs @@ -1,6 +1,5 @@ using FootballFormation.Core.Data; using FootballFormation.Core.Models; -using FootballFormation.Core.Reporting; using FootballFormation.Core.Security; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; @@ -9,7 +8,8 @@ namespace FootballFormation.Core.Services; /// /// Goals as they are logged at the touchline. What this adds is the one thing only a match in -/// progress knows: the minute the clock showed when the ball went in. Storing the goal — and, at +/// progress knows: where in the match the ball went in — the half being played and the reading on +/// the clock, the same pair a substitution carries. Storing the goal — and, at /// the touchline, recounting the scoreline in the same save — is delegated to /// , so there is one implementation of it and the two rows are written /// together rather than across two contexts. @@ -34,27 +34,23 @@ public Task> LogGoalAsync( { await using var db = await dbFactory.CreateDbContextAsync(cancellationToken); - // Line-ups included: the minute follows the scoreboard clock, which is measured from - // the half being played rather than from kick-off. + // Line-ups included: the half being played is half of what places the goal. var game = await db.LoadWithPeriodsAsync(gameId, cancellationToken); if (game is null) return LiveMatchQueries.GameNotFound(gameId); if (scorerId is null && !isOpponentGoal) return Result.Failure("A goal for us needs a scorer"); - var clock = MatchClockReport.Build( - game, game.CurrentOrLastHalf(), game.ElapsedSecondsAt(UtcNow)); - var goal = new GameGoal { GameId = gameId, ScorerId = scorerId, AssisterId = assisterId, - // The minute the clock showed, so an over-running first half does not push every - // second-half goal out by the overrun. Stoppage time is kept in the second half of - // the pair — see GameGoal.AdditionalMinute for why it is not folded into the first. - Minute = clock.Minute.Minute, - AdditionalMinute = clock.Minute.Additional, + // Where it happened, not what a scoreboard made of it. The displayed minute is + // derived from these two, so a half whose timings are corrected later takes its + // goals with it — exactly as it already does its substitutions. + GamePeriodId = game.CurrentOrLastHalf()?.Id, + AtSeconds = game.ElapsedSecondsAt(UtcNow), IsOwnGoal = isOwnGoal, IsOpponentGoal = isOpponentGoal }; diff --git a/src/FootballFormation.UI/Pages/LiveMatch.razor b/src/FootballFormation.UI/Pages/LiveMatch.razor index 579aae0..b0d7191 100644 --- a/src/FootballFormation.UI/Pages/LiveMatch.razor +++ b/src/FootballFormation.UI/Pages/LiveMatch.razor @@ -197,11 +197,19 @@
@foreach (var entry in Timeline) { + @* The list runs newest first, so the break lands between the second half's + events and the first's — the only thing on the timeline that is not + something somebody did. *@ + @if (entry.HalfTimeAbove) + { +
@L["Half time"]
+ } + var against = entry.Goal is { } g && (g.IsOwnGoal || g.IsOpponentGoal);
- @(entry.Minute)' + @(entry.Minute is { } at ? $"{at}'" : "—") + Icon="@(entry.Goal is not null ? Icons.Material.Filled.SportsSoccer : Icons.Material.Filled.SwapHoriz)" />
@if (entry.Goal is { } goal) { diff --git a/src/FootballFormation.UI/Pages/LiveMatch.razor.cs b/src/FootballFormation.UI/Pages/LiveMatch.razor.cs index bc4771f..269dbea 100644 --- a/src/FootballFormation.UI/Pages/LiveMatch.razor.cs +++ b/src/FootballFormation.UI/Pages/LiveMatch.razor.cs @@ -13,18 +13,23 @@ namespace FootballFormation.UI.Pages; /// /// One entry on the match timeline — a goal or a substitution — so both can be listed together. -/// is the scoreboard reading rather than the raw elapsed time, which is -/// what keeps a first-half stoppage entry above the restart instead of below it. -/// orders events that share a minute, which the minute alone cannot. +/// is the elapsed match clock, which is what orders the list: it runs +/// on across the break, so a first-half stoppage entry stays above the restart without anyone +/// comparing scoreboard readings. is that reading, for display only, and +/// null for a goal recorded without one. +/// orders events that share a second, which the clock alone cannot. /// settles the rest: two entries of the same kind entered in one instant /// share a , and rows older than that column all read /// 0001-01-01. Across the two kinds the ids come from different tables, so a tie there is /// arbitrary — but it is stable, which is what the list needs. +/// is where the break falls; marks the one +/// entry the break is drawn above, which only a neighbour can decide. /// is the scoreline as it stood after a goal, and null for a substitution. /// public record MatchEvent( - MatchMinute Minute, DateTime RecordedAt, int Id, bool IsGoal, GameGoal? Goal, - GameSubstitution? Substitution, MatchScore? Score = null); + int AtSeconds, MatchMinute? Minute, PeriodType Half, DateTime RecordedAt, int Id, + GameGoal? Goal, GameSubstitution? Substitution, MatchScore? Score = null, + bool HalfTimeAbove = false); /// /// The sideline screen. An admin runs the clock and records what happens; everyone else sees the @@ -258,20 +263,34 @@ private List Timeline var progression = ScoreProgressionReport.Build(GameData.Goals); var goals = GameData.Goals.Select(g => new MatchEvent( - MatchClockReport.MinuteOf(g) ?? default, g.RecordedAt, g.Id, true, g, null, progression[g.Id])); + g.TimelineSeconds, + MatchClockReport.MinuteOf(GameData, g), + MatchClockReport.HalfOf(GameData, g.GamePeriodId, g.TimelineSeconds), + g.RecordedAt, g.Id, g, null, progression[g.Id])); IEnumerable subs = ShowSubstitutions ? GameData.Substitutions.Select(s => new MatchEvent( - MatchClockReport.MinuteOf(GameData, s), s.RecordedAt, s.Id, false, null, s)) + s.AtSeconds, + MatchClockReport.MinuteOf(GameData, s), + MatchClockReport.HalfOf(GameData, s.GamePeriodId, s.AtSeconds), + s.RecordedAt, s.Id, null, s)) : []; - // A goal and the sub that followed it commonly share a minute; the entry time keeps + // A goal and the sub that followed it commonly share a second; the entry time keeps // them in the order they actually happened rather than the order they were queried. // The id then settles a double substitution, so the entry this list shows on top is // the one MatchSubstitutionService.RemoveSubstitutionAsync will let an admin undo. - return [.. goals.Concat(subs) - .OrderByDescending(e => e.Minute) + var ordered = goals.Concat(subs) + .OrderByDescending(e => e.AtSeconds) .ThenByDescending(e => e.RecordedAt) - .ThenByDescending(e => e.Id)]; + .ThenByDescending(e => e.Id) + .ToList(); + + // Where the second half's events give way to the first's, reading down a list that + // runs newest first. Marked here because the markup renders one entry at a time and + // cannot see the one above it — and because the filter above decides who the + // neighbours are. + return [.. ordered.Select((e, i) => + i > 0 && ordered[i - 1].Half != e.Half ? e with { HalfTimeAbove = true } : e)]; } } diff --git a/src/FootballFormation.UI/Pages/LiveMatch.razor.css b/src/FootballFormation.UI/Pages/LiveMatch.razor.css index 86406cf..2df7883 100644 --- a/src/FootballFormation.UI/Pages/LiveMatch.razor.css +++ b/src/FootballFormation.UI/Pages/LiveMatch.razor.css @@ -182,6 +182,28 @@ border-bottom: none; } +/* Half time: a dashed rule with the words sitting in it, so the two halves read apart at a glance + without the break looking like another event. The row above it keeps its own solid rule, which + is why nothing here draws a top border. */ +.live-event-break { + display: flex; + align-items: center; + gap: 10px; + padding: 6px 0; + font-size: 0.7rem; + font-weight: 700; + letter-spacing: 0.06em; + text-transform: uppercase; + color: var(--ink-faint); +} + +.live-event-break::before, +.live-event-break::after { + content: ""; + flex: 1; + border-top: 1px dashed color-mix(in srgb, var(--ink) 25%, transparent); +} + .live-event-min { min-width: 34px; font-weight: 700; diff --git a/src/FootballFormation.UI/Pages/MatchResult.razor b/src/FootballFormation.UI/Pages/MatchResult.razor index 66b5d0b..43ba245 100644 --- a/src/FootballFormation.UI/Pages/MatchResult.razor +++ b/src/FootballFormation.UI/Pages/MatchResult.razor @@ -74,14 +74,15 @@ @if (GameData.Goals.Count > 0) {
- @* Both halves of the minute, so a goal in first-half stoppage time keeps its place - above the restart rather than sorting past it — see MatchMinute. *@ - @foreach (var goal in GameData.Goals.OrderBy(g => MatchClockReport.MinuteOf(g) ?? default)) + @* Ordered on the elapsed match clock, which runs on across the break, so a goal in + first-half stoppage time keeps its place above the restart rather than sorting + past it — see GameGoal.TimelineSeconds. *@ + @foreach (var goal in GameData.Goals.OrderBy(g => g.TimelineSeconds)) { @* Own goals and opponent goals both count against us, so they share the styling. *@
- @(MatchClockReport.MinuteOf(goal) is { } at ? $"{at}'" : "—") + @(MatchClockReport.MinuteOf(GameData, goal) is { } at ? $"{at}'" : "—")
diff --git a/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs b/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs index 98405b3..b0b8cdc 100644 --- a/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs +++ b/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs @@ -149,10 +149,12 @@ public void The_minute_stops_with_the_clock_and_additional_time_is_counted_besid /// /// The whole point of the pair: a goal in first-half stoppage time and one just after the - /// restart are a minute apart, and a single number would have written them 32 and 31. + /// restart are a minute apart, and a single counted-on number would have written them 32 and + /// 31. Which of them came first is the elapsed clock's answer, not the pair's — the pair only + /// has to say something a scoreboard would. /// [Fact] - public void A_stoppage_time_minute_comes_before_the_first_minute_of_the_next_half() + public void A_stoppage_time_minute_stays_inside_the_half_it_was_played_in() { var game = QuartersGame(); Period(game, PeriodType.FirstQuarter).StartedAtSeconds = 0; @@ -162,8 +164,8 @@ public void A_stoppage_time_minute_comes_before_the_first_minute_of_the_next_hal var afterTheBreak = MatchClockReport.Build(game, Period(game, PeriodType.ThirdQuarter), 32 * 60); Assert.Equal(new MatchMinute(30, 2), stoppage.Minute); + Assert.Equal("30+2", stoppage.Minute.ToString()); Assert.Equal(new MatchMinute(31, 0), afterTheBreak.Minute); - Assert.True(stoppage.Minute.CompareTo(afterTheBreak.Minute) < 0); } [Fact] @@ -199,12 +201,89 @@ public void A_substitution_is_written_against_the_clock_its_own_half_was_showing Assert.Equal(new MatchMinute(35, 0), MatchClockReport.MinuteOf(game, orphan)); } + /// + /// A goal is placed the same way a substitution is, off its own half's clock — which is the + /// point of storing the pair on the row rather than the minute they produce. + /// + [Fact] + public void A_goal_is_written_against_the_clock_its_own_half_was_showing() + { + var game = QuartersGame(); + Period(game, PeriodType.FirstQuarter).StartedAtSeconds = 0; + var secondHalf = Period(game, PeriodType.ThirdQuarter); + secondHalf.StartedAtSeconds = 32 * 60; + secondHalf.Id = 7; + + var goal = new GameGoal { GamePeriodId = 7, AtSeconds = 34 * 60 }; + Assert.Equal(new MatchMinute(33, 0), MatchClockReport.MinuteOf(game, goal)); + + // Played out and still going: the reading a scoreboard would show, not a counted-on 32. + var stoppage = new GameGoal { GamePeriodId = 7, AtSeconds = 63 * 60 }; + Assert.Equal(new MatchMinute(60, 2), MatchClockReport.MinuteOf(game, stoppage)); + } + + /// + /// Correcting a half's timings corrects the goals scored in it. That is what deriving the + /// minute buys over storing it, and it is the whole reason the column moved. + /// [Fact] - public void A_goal_recorded_without_a_minute_has_none_to_show() + public void Moving_a_halfs_kick_off_moves_the_goals_scored_in_it() { - Assert.Null(MatchClockReport.MinuteOf(new GameGoal())); - Assert.Equal(new MatchMinute(35, 2), - MatchClockReport.MinuteOf(new GameGoal { Minute = 35, AdditionalMinute = 2 })); + var game = QuartersGame(); + Period(game, PeriodType.FirstQuarter).StartedAtSeconds = 0; + var secondHalf = Period(game, PeriodType.ThirdQuarter); + secondHalf.Id = 7; + secondHalf.StartedAtSeconds = 32 * 60; + + // Eight minutes into a second half whose clock starts at 30: the 39th minute. + var goal = new GameGoal { GamePeriodId = 7, AtSeconds = 40 * 60 }; + Assert.Equal(new MatchMinute(39, 0), MatchClockReport.MinuteOf(game, goal)); + + // The half really kicked off three minutes later than recorded, so the goal moves back + // with it. A stored minute would have stayed where it was and disagreed with the + // substitutions around it. + secondHalf.StartedAtSeconds = 35 * 60; + Assert.Equal(new MatchMinute(36, 0), MatchClockReport.MinuteOf(game, goal)); + } + + [Fact] + public void A_goal_with_no_clock_behind_it_falls_back_to_the_minute_on_the_row() + { + var game = QuartersGame(); + + // Typed in on the result page, and every goal logged before the clock reading was stored. + Assert.Equal(new MatchMinute(35, 0), + MatchClockReport.MinuteOf(game, new GameGoal { Minute = 35 })); + + // Neither one nor the other: the result page allows a goal with no minute at all. + Assert.Null(MatchClockReport.MinuteOf(game, new GameGoal())); + } + + /// + /// Where the timeline draws half time. An event knows its own half; a goal typed in by hand + /// knows only a clock reading, and the second half's kick-off is the line it falls one side of. + /// + [Fact] + public void An_event_belongs_to_the_half_its_line_up_played_or_to_the_side_of_the_restart_it_falls() + { + var game = QuartersGame(); + Period(game, PeriodType.FirstQuarter).StartedAtSeconds = 0; + var secondHalf = Period(game, PeriodType.ThirdQuarter); + secondHalf.Id = 7; + secondHalf.StartedAtSeconds = 32 * 60; + + // Q4 is planned for the middle of the second half, and is still the second half. + var fourth = Period(game, PeriodType.FourthQuarter); + fourth.Id = 9; + + Assert.Equal(PeriodType.SecondHalf, MatchClockReport.HalfOf(game, 7, 34 * 60)); + Assert.Equal(PeriodType.SecondHalf, MatchClockReport.HalfOf(game, 9, 34 * 60)); + Assert.Equal(PeriodType.FirstHalf, MatchClockReport.HalfOf(game, null, 31 * 60)); + Assert.Equal(PeriodType.SecondHalf, MatchClockReport.HalfOf(game, null, 32 * 60)); + + // Nothing kicked off after the break, so there is no line to be the far side of. + secondHalf.StartedAtSeconds = null; + Assert.Equal(PeriodType.FirstHalf, MatchClockReport.HalfOf(game, null, 55 * 60)); } [Fact] diff --git a/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs b/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs index 68eda05..47df388 100644 --- a/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs +++ b/tests/FootballFormation.Core.Tests/MatchGoalServiceTests.cs @@ -1,6 +1,7 @@ using System.Data.Common; using FootballFormation.Core.Data; using FootballFormation.Core.Models; +using FootballFormation.Core.Reporting; using FootballFormation.Core.Services; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Diagnostics; @@ -9,34 +10,40 @@ namespace FootballFormation.Core.Tests; /// -/// What logging a goal at the touchline adds over storing one: the minute it is stamped with, and -/// the scoreline that has to follow from the goals on file. +/// What logging a goal at the touchline adds over storing one: where in the match it happened — +/// the half being played and the reading on the clock — and the scoreline that has to follow from +/// the goals on file. The minute anyone sees is derived from that pair; these tests are about the +/// pair being written, and about what it is read back as. /// public class MatchGoalServiceTests : LiveMatchTestBase { [Fact] - public async Task A_goal_is_stamped_with_the_minute_the_clock_showed_counting_from_one() + public async Task A_goal_is_stamped_with_the_half_being_played_and_the_clock_it_was_scored_on() { var game = await SeedGameAsync(); await MatchClock.StartMatchAsync(game.Id); var players = await PlayersAsync(); - - // Minute 0 reads oddly on a timeline, so the first minute of play is 1'. - var opening = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); - Assert.Equal(1, opening.Value!.Minute); + var firstHalf = (await ReloadAsync(game.Id)).Periods.Single(p => p.PeriodType == PeriodType.FirstHalf); Time.Advance(TimeSpan.FromSeconds(1500)); // 25:00 - var later = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); - Assert.Equal(26, later.Value!.Minute); + + var goal = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); + + Assert.Equal(firstHalf.Id, goal.Value!.GamePeriodId); + Assert.Equal(1500, goal.Value.AtSeconds); + + // Nothing presentational on the row: the minute is the clock's to derive, and stays null + // so that a half whose timings are corrected later takes its goals with it. + Assert.Null(goal.Value.Minute); } /// - /// The second half's clock starts at half the match however long the first half really took, - /// and the goal minute follows the clock — otherwise every second-half goal is pushed out by - /// the first half's overrun. + /// The half is recorded, not inferred from the clock reading. The two disagree the moment a + /// first half over-runs — this one is three minutes long past its 30 — and inferring is what + /// let a change to the match duration silently reinterpret goals already on file. /// [Fact] - public async Task A_second_half_goal_is_stamped_off_the_scoreboard_clock_not_the_elapsed_time() + public async Task A_second_half_goal_is_stamped_with_the_second_half_however_long_the_first_ran() { var game = await SeedGameAsync(); await MatchClock.StartMatchAsync(game.Id); @@ -50,16 +57,21 @@ public async Task A_second_half_goal_is_stamped_off_the_scoreboard_clock_not_the var goal = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); - // 35:xx on the clock, so the 36th minute — not the 39th the raw elapsed time would give. - Assert.Equal(36, goal.Value!.Minute); + var reloaded = await ReloadAsync(game.Id); + var secondHalf = reloaded.Periods.Single(p => p.PeriodType == PeriodType.SecondHalf); + Assert.Equal(secondHalf.Id, goal.Value!.GamePeriodId); + Assert.Equal(38 * 60, goal.Value.AtSeconds); + + // 35:xx on the scoreboard, so the 36th minute — not the 39th the raw elapsed time reads. + Assert.Equal(new MatchMinute(36, 0), MatchClockReport.MinuteOf(reloaded, goal.Value)); } /// - /// A goal after the half has been played out is written 30+2, not 32. The two halves of the - /// minute are stored apart because that is what keeps it above the restart on the timeline. + /// A goal after the half has been played out belongs to that half, and is shown 30+2 rather + /// than 32. The clock reading runs on past the cap; only the display stops at it. /// [Fact] - public async Task A_goal_in_stoppage_time_is_stamped_with_the_minute_it_is_added_to() + public async Task A_goal_in_stoppage_time_belongs_to_the_half_that_is_over_running() { var game = await SeedGameAsync(); await MatchClock.StartMatchAsync(game.Id); @@ -69,8 +81,11 @@ public async Task A_goal_in_stoppage_time_is_stamped_with_the_minute_it_is_added var goal = await Goals.LogGoalAsync(game.Id, players[1].Id, null, false, false); - Assert.Equal(30, goal.Value!.Minute); - Assert.Equal(2, goal.Value.AdditionalMinute); + var reloaded = await ReloadAsync(game.Id); + Assert.Equal(PeriodType.FirstHalf, + reloaded.Periods.Single(p => p.Id == goal.Value!.GamePeriodId).PeriodType); + Assert.Equal(31 * 60, goal.Value!.AtSeconds); + Assert.Equal(new MatchMinute(30, 2), MatchClockReport.MinuteOf(reloaded, goal.Value)); } [Fact] diff --git a/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs b/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs index 2e858ac..a4c0aae 100644 --- a/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs +++ b/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs @@ -10,13 +10,13 @@ namespace FootballFormation.Core.Tests; ///
public class ScoreProgressionReportTests { + /// A goal off the touchline, placed by the elapsed match clock it was logged at. private static GameGoal Goal( - int id, int minute, int additional = 0, bool ownGoal = false, bool opponentGoal = false) => + int id, int minute, bool ownGoal = false, bool opponentGoal = false) => new() { Id = id, - Minute = minute, - AdditionalMinute = additional, + AtSeconds = (minute - 1) * 60, IsOwnGoal = ownGoal, IsOpponentGoal = opponentGoal, RecordedAt = new DateTime(2026, 8, 11, 14, 0, minute, DateTimeKind.Utc) @@ -89,14 +89,15 @@ public void The_last_goals_score_is_the_final_score() /// /// A goal in first-half stoppage time was scored before one just after the restart, and the - /// running total has to follow that — a minute counted straight on would have made 30+2 read - /// as 32 and put it after the 31st minute of the second half. + /// running total follows that without anyone comparing scoreboard readings — the elapsed clock + /// runs on across the break, where the scoreboard reads 30+2 and then 31 all over again. /// [Fact] public void A_stoppage_time_goal_is_counted_inside_the_half_it_was_scored_in() { - var stoppage = Goal(1, 30, additional: 2); - var afterTheBreak = Goal(2, 31, opponentGoal: true); + // Two minutes past a 30-minute half, then a minute into a second half that kicked off at 32. + var stoppage = Goal(1, 32); + var afterTheBreak = Goal(2, 33, opponentGoal: true); afterTheBreak.RecordedAt = stoppage.RecordedAt.AddMinutes(16); var progression = ScoreProgressionReport.Build([afterTheBreak, stoppage]); @@ -105,6 +106,23 @@ public void A_stoppage_time_goal_is_counted_inside_the_half_it_was_scored_in() Assert.Equal(new MatchScore(1, 1), progression[2]); } + /// + /// A goal typed in on the result page has no clock behind it, so the minute on the row is what + /// places it — which is the only thing keeping goals recorded before the clock was stored in + /// the order they have always had. + /// + [Fact] + public void A_goal_with_only_a_minute_is_counted_in_that_minute() + { + var typedIn = new GameGoal { Id = 1, Minute = 40 }; + var logged = Goal(2, 10, opponentGoal: true); + + var progression = ScoreProgressionReport.Build([typedIn, logged]); + + Assert.Equal(new MatchScore(0, 1), progression[2]); + Assert.Equal(new MatchScore(1, 1), progression[1]); + } + [Fact] public void A_match_with_no_goals_has_nothing_to_report() { diff --git a/tests/ui/specs/match-day.spec.js b/tests/ui/specs/match-day.spec.js index 7ac0479..70dfce4 100644 --- a/tests/ui/specs/match-day.spec.js +++ b/tests/ui/specs/match-day.spec.js @@ -239,6 +239,44 @@ test('the timeline can be narrowed to the goals', async ({ page }) => { await expect(page.locator('.live-bench')).toBeVisible(); }); +test('the timeline draws half time between the two halves', async ({ page }) => { + const id = await matchWithId(page, 'FC Rust'); + await fillLineup(page, 2); + + await goto(page, `/games/${id}/live`); + await clickFor( + page.getByRole('button', { name: 'Start match' }), + () => expect(page.getByRole('button', { name: 'Finish match' })).toBeVisible(), + ); + + const events = page.locator('.live-event'); + const halfTime = page.locator('.live-event-break'); + + // One goal in each half. Until the second one there is only one half on the list, and a break + // above the only thing on it would be a line drawn through nothing. + await clickFor(page.getByRole('button', { name: 'Goal against' }), () => expect(events).toHaveCount(1)); + await expect(halfTime).toHaveCount(0); + + const controls = page.locator('.live-controls'); + await clickFor( + controls.getByRole('button', { name: 'Half time' }), + () => expect(controls.getByRole('button', { name: 'Start 2nd Half' })).toBeVisible(), + ); + await clickFor( + controls.getByRole('button', { name: 'Start 2nd Half' }), + () => expect(controls.getByRole('button', { name: 'Half time' })).toHaveCount(0), + ); + + await clickFor(page.getByRole('button', { name: 'Goal against' }), () => expect(events).toHaveCount(2)); + + // Exactly one break, and it sits between the two — the list runs newest first, so the second + // half's goal is above it and the first half's below. + await expect(halfTime).toHaveCount(1); + await expect(halfTime).toHaveText('Half time'); + await expect(page.locator('.live-timeline > *')).toHaveCount(3); + await expect(page.locator('.live-timeline > *').nth(1)).toHaveClass(/live-event-break/); +}); + test('the playing-time table drops its estimate once the match has been run', async ({ page }) => { const id = await matchWithId(page, 'FC Speeltijd'); await fillLineup(page, 2); diff --git a/tests/ui/specs/selectors.spec.js b/tests/ui/specs/selectors.spec.js index 7afecc2..7da8229 100644 --- a/tests/ui/specs/selectors.spec.js +++ b/tests/ui/specs/selectors.spec.js @@ -31,7 +31,8 @@ const SELECTORS = { 'the formation builder': ['pitch', 'pitch-empty', 'pitch-player', 'draggable-player'], 'the playing-time table': ['playtime-table', 'pt-total', 'playtime-note'], 'the live screen': ['live-lineup', 'live-controls', 'live-score-value', 'live-score-away', - 'live-event', 'live-event-score', 'live-bench', 'live-timeline-toggle', + 'live-event', 'live-event-score', 'live-event-break', 'live-bench', + 'live-timeline-toggle', 'live-timeline', 'live-minutes-card', 'card-label', 'planned-row'], 'the phone layout': ['dialog-sheet', 'stacked-table', 'topbar-nav'], 'the squad': ['badge-archived'], From 2f2526cfb30ba595a49b46191300b4cb39c2aab5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 11:28:51 +0000 Subject: [PATCH 2/3] Place a typed-in minute on the clock it names, not on the elapsed one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A minute somebody types on the result page is a scoreboard reading, and the scoreboard only agrees with the elapsed clock while the halves run to length. Ordering the timeline on (Minute - 1) * 60 quietly assumed they were the same scale, so on a match whose first half was whistled off three minutes long a second-half goal sorted ahead of one scored in first-half stoppage time — a wrong running score out of ScoreProgressionReport, and the goal drawn on the wrong side of the new half-time rule. MatchClockReport.ElapsedOf converts the typed minute back through the half's own timings, which is Build's arithmetic run the other way, and is now the only thing that produces an ordering key for a goal. GameGoal.TimelineSeconds is gone: it looked like it could answer this without the match, and it could not. Also drops the unused GamePeriod navigation on GameGoal — every reader resolves the half against the game already loaded — and corrects two claims in the migration that its own SQL contradicts: EF folds the column drop into the foreign key's table rebuild rather than running it last, and goals logged in stoppage time during the day AdditionalMinute existed do lose the overrun from their display. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XD7SBD1CvRzK4Xrhx3dtf7 --- docs/known_issues.md | 19 +++++-- docs/models.md | 2 +- docs/ui_components.md | 6 +- .../Configurations/GameEventConfigurations.cs | 3 +- ...12353_StoreGoalPeriodAndClock.Designer.cs} | 6 +- ...20260814112353_StoreGoalPeriodAndClock.cs} | 26 +++++++-- .../Migrations/AppDbContextModelSnapshot.cs | 4 +- src/FootballFormation.Core/Models/GameGoal.cs | 16 ++---- .../Models/MatchMinute.cs | 7 +-- .../Reporting/MatchClockReport.cs | 34 +++++++++++- .../Reporting/ScoreProgressionReport.cs | 8 +-- .../Pages/LiveMatch.razor.cs | 16 ++++-- .../Pages/MatchResult.razor | 5 +- .../MatchClockReportTests.cs | 48 ++++++++++++++++ .../ScoreProgressionReportTests.cs | 55 ++++++++++++++++--- 15 files changed, 197 insertions(+), 58 deletions(-) rename src/FootballFormation.Core/Migrations/{20260814102938_StoreGoalPeriodAndClock.Designer.cs => 20260814112353_StoreGoalPeriodAndClock.Designer.cs} (99%) rename src/FootballFormation.Core/Migrations/{20260814102938_StoreGoalPeriodAndClock.cs => 20260814112353_StoreGoalPeriodAndClock.cs} (62%) diff --git a/docs/known_issues.md b/docs/known_issues.md index 2f77b79..1888eea 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -378,11 +378,20 @@ Avoid repeating these mistakes: 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 migration deliberately does not backfill, because nothing - left in an old row says which half a stored `37` belonged to or whether it was stoppage time. 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. Sort with `GameGoal.TimelineSeconds`, which covers both, and 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. + left in an old row says which half a stored `37` 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`, so a goal logged in stoppage time during the day + that column existed now shows the capped minute (`30'`, not `30+2`); it sorts where it always did. +- **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 diff --git a/docs/models.md b/docs/models.md index 4277a4c..bf7e28f 100644 --- a/docs/models.md +++ b/docs/models.md @@ -242,7 +242,7 @@ 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 (`GameGoal.TimelineSeconds`, `GameSubstitution.AtSeconds`), then by `RecordedAt`, +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 diff --git a/docs/ui_components.md b/docs/ui_components.md index c6cae15..d51d3fe 100644 --- a/docs/ui_components.md +++ b/docs/ui_components.md @@ -153,8 +153,10 @@ watches the same URL read-only. Every control sits in an ` @@ -464,7 +464,7 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) .OnDelete(DeleteBehavior.Cascade) .IsRequired(); - b.HasOne("FootballFormation.Core.Models.GamePeriod", "GamePeriod") + b.HasOne("FootballFormation.Core.Models.GamePeriod", null) .WithMany() .HasForeignKey("GamePeriodId") .OnDelete(DeleteBehavior.Cascade); @@ -478,8 +478,6 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) b.Navigation("Game"); - b.Navigation("GamePeriod"); - b.Navigation("Scorer"); }); diff --git a/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs b/src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs similarity index 62% rename from src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs rename to src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs index 3aaa715..fea0f46 100644 --- a/src/FootballFormation.Core/Migrations/20260814102938_StoreGoalPeriodAndClock.cs +++ b/src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs @@ -10,14 +10,28 @@ namespace FootballFormation.Core.Migrations /// correcting a half's timings corrects its goals with it. /// /// Deliberately not backfilled, and AdditionalMinute is dropped rather than folded into - /// anything. Nothing left in an old row says which half a stored 37 belonged to or - /// whether it was stoppage time, so those goals keep Minute and go on reading and - /// sorting exactly as they do today; only goals logged from here on carry a clock. + /// anything. Nothing left in an old row says which half a stored 37 belonged to, so + /// those goals keep Minute and go on sorting where they always have; only goals logged + /// from here on carry a clock. Folding the overrun back in would have been worse than losing + /// it — a 30+2 counted on to 32 sorts past a goal in the 31st minute of the + /// second half, which is the bug the pair was introduced to fix. /// /// - /// The drop is last on purpose. Both operations rebuild the table on SQLite, and a rebuild is - /// not transactional — leaving it until the new columns and the foreign key are in place means - /// a half-applied run has lost nothing that the next attempt needs. + /// It does lose something. A goal logged in stoppage time between + /// AddGoalAdditionalMinute shipping and this migration has a real overrun on the row, + /// and afterwards reads as the capped minute — 30+2 becomes 30'. Only the + /// display: the minute it sorts on is unchanged. That window is about a day, and the + /// alternative was a reconstruction dressed up as a reading. + /// + /// + /// The order below is not what SQLite runs. EF folds the DropColumn into the table + /// rebuild that AddForeignKey already forces — the temp table is simply created without + /// the column — so this is one rebuild whatever order the operations are written in. What that + /// leaves is a migration that cannot be retried: the two ADD COLUMNs commit in + /// their own transaction, and a run interrupted after that point has not recorded itself in + /// __EFMigrationsHistory, so the next boot starts again from the top and dies on + /// duplicate column name: AtSeconds. Recovery is the pre-migration snapshot + /// Program.cs takes, which is the only thing standing behind any of this. /// ///
public partial class StoreGoalPeriodAndClock : Migration diff --git a/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs b/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs index 26d5bfa..44928c3 100644 --- a/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs +++ b/src/FootballFormation.Core/Migrations/AppDbContextModelSnapshot.cs @@ -461,7 +461,7 @@ protected override void BuildModel(ModelBuilder modelBuilder) .OnDelete(DeleteBehavior.Cascade) .IsRequired(); - b.HasOne("FootballFormation.Core.Models.GamePeriod", "GamePeriod") + b.HasOne("FootballFormation.Core.Models.GamePeriod", null) .WithMany() .HasForeignKey("GamePeriodId") .OnDelete(DeleteBehavior.Cascade); @@ -475,8 +475,6 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.Navigation("Game"); - b.Navigation("GamePeriod"); - b.Navigation("Scorer"); }); diff --git a/src/FootballFormation.Core/Models/GameGoal.cs b/src/FootballFormation.Core/Models/GameGoal.cs index 70ca5d8..4942c33 100644 --- a/src/FootballFormation.Core/Models/GameGoal.cs +++ b/src/FootballFormation.Core/Models/GameGoal.cs @@ -17,9 +17,13 @@ public class GameGoal /// The half that was being played when the ball went in, as the line-up playing it — the same /// fact carries. Null for a goal typed in on the /// result page, which has no half behind it. + /// + /// The id alone, with no navigation beside it: every reader resolves it against the half + /// already loaded on the game (MatchClockReport), so a navigation would only be an + /// invitation to Include the same row a second time. + /// /// public int? GamePeriodId { get; set; } - public GamePeriod? GamePeriod { get; set; } /// /// Match-clock second the ball went in, from the same clock a substitution is stamped from. @@ -56,14 +60,4 @@ public class GameGoal /// ScoreProgressionReport walks them one at a time for the live timeline. /// public bool CountsForUs => !IsOwnGoal && !IsOpponentGoal; - - /// - /// Where this goal sits on the match timeline, as elapsed seconds — the one scale goals and - /// substitutions can be ordered on together, and the reason the timeline no longer compares - /// scoreboard minutes in pairs. when there is one; otherwise the start - /// of the minute typed in, which is the best a row with no clock behind it offers. Zero for a - /// goal with neither, which puts it at the top of the match rather than nowhere. - /// - public int TimelineSeconds => - AtSeconds ?? (Minute is { } minute ? Math.Max(0, (minute - 1) * 60) : 0); } diff --git a/src/FootballFormation.Core/Models/MatchMinute.cs b/src/FootballFormation.Core/Models/MatchMinute.cs index 1cc41ef..c1d63ad 100644 --- a/src/FootballFormation.Core/Models/MatchMinute.cs +++ b/src/FootballFormation.Core/Models/MatchMinute.cs @@ -4,10 +4,9 @@ namespace FootballFormation.Core.Models; /// The minute an event is written down against, the way football writes one: 35, or 35+2 once the /// half has been played out and the clock is into stoppage time. /// -/// Display only. Ordering a timeline is 's job and -/// 's — the real elapsed clock, which runs on across the -/// break and needs no pair to say that a goal at 35+2 came before one in the 36th minute of the -/// second half. +/// Display only. Ordering a timeline is the elapsed clock's job — MatchClockReport.ElapsedOf +/// and — which runs on across the break and needs no pair +/// to say that a goal at 35+2 came before one in the 36th minute of the second half. /// /// /// The minute on the clock, never past the end of the half being played. diff --git a/src/FootballFormation.Core/Reporting/MatchClockReport.cs b/src/FootballFormation.Core/Reporting/MatchClockReport.cs index f090ce6..e9a5091 100644 --- a/src/FootballFormation.Core/Reporting/MatchClockReport.cs +++ b/src/FootballFormation.Core/Reporting/MatchClockReport.cs @@ -88,10 +88,42 @@ public static MatchMinute MinuteOf(Game game, GameSubstitution substitution) => _ => null }; + /// + /// Where a goal sits on the elapsed match clock — the scale the timeline is ordered on, and + /// the one a substitution is already stored in. + /// + /// A goal logged from the touchline has that reading on the row. A goal typed in on the result + /// page has only the minute somebody wrote, which is a scoreboard reading, and the two + /// scales part company the moment a half over-runs: on a 60-minute match whose first half ran + /// to 33, the scoreboard's 32' is 34 minutes of elapsed play. Reading the typed minute as + /// though it were elapsed time filed it before the restart, under the half-time rule and ahead + /// of goals that were really scored first, so it is converted back through the half timings + /// here — the same arithmetic does, run the other way. + /// + /// + public static int ElapsedOf(Game game, GameGoal goal) + { + if (goal.AtSeconds is { } at) return at; + if (goal.Minute is not { } minute) return 0; + + // The start of the minute written down, on the scoreboard's scale. + var onScoreboard = Math.Max(0, (minute - 1) * 60); + + var halfSeconds = GameSplitType.Halves.PeriodDurationSeconds(game.GameDurationMinutes); + if (halfSeconds <= 0) return onScoreboard; + + // A match never run from the touchline has no timings to convert through, and the fallbacks + // are what Build assumes in the same position — so its goals keep the order the typed + // minutes put them in, which is the only order they have. + return onScoreboard < halfSeconds + ? (HalfKickedOffAt(game, PeriodType.FirstHalf) ?? 0) + onScoreboard + : (HalfKickedOffAt(game, PeriodType.SecondHalf) ?? halfSeconds) + (onScoreboard - halfSeconds); + } + /// /// The half an event belongs to, which is what puts the half-time break on the timeline. Its /// own line-up's half when it has one; otherwise whichever side of the second half's kick-off - /// its clock reading falls, which is all a goal typed in by hand leaves to go on. First half + /// its elapsed reading falls, which is all a goal typed in by hand leaves to go on. First half /// when nothing says otherwise — a match never run from the touchline has no kick-off to be /// past, and one unbroken list is the honest way to show it. /// diff --git a/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs b/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs index 4a1ebaf..ce663e0 100644 --- a/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs +++ b/src/FootballFormation.Core/Reporting/ScoreProgressionReport.cs @@ -18,8 +18,8 @@ namespace FootballFormation.Core.Reporting; /// public static class ScoreProgressionReport { - /// Every goal in the match, in any order. - public static IReadOnlyDictionary Build(IEnumerable goals) + /// The match, for the half timings a typed-in minute is placed through. + public static IReadOnlyDictionary Build(Game game) { var progression = new Dictionary(); var us = 0; @@ -29,8 +29,8 @@ public static IReadOnlyDictionary Build(IEnumerable g // which runs on across the break and so keeps a stoppage-time goal inside the half it was // scored in, then the moment it was entered, then the id. See LiveMatch.Timeline for why // all three. - var chronological = goals - .OrderBy(g => g.TimelineSeconds) + var chronological = game.Goals + .OrderBy(g => MatchClockReport.ElapsedOf(game, g)) .ThenBy(g => g.RecordedAt) .ThenBy(g => g.Id); diff --git a/src/FootballFormation.UI/Pages/LiveMatch.razor.cs b/src/FootballFormation.UI/Pages/LiveMatch.razor.cs index 269dbea..bddc8d9 100644 --- a/src/FootballFormation.UI/Pages/LiveMatch.razor.cs +++ b/src/FootballFormation.UI/Pages/LiveMatch.razor.cs @@ -260,13 +260,17 @@ private List Timeline // Counted forwards over the whole match, then looked up per goal: this list runs // newest first, so a total accumulated while rendering it would count down. - var progression = ScoreProgressionReport.Build(GameData.Goals); + var progression = ScoreProgressionReport.Build(GameData); - var goals = GameData.Goals.Select(g => new MatchEvent( - g.TimelineSeconds, - MatchClockReport.MinuteOf(GameData, g), - MatchClockReport.HalfOf(GameData, g.GamePeriodId, g.TimelineSeconds), - g.RecordedAt, g.Id, g, null, progression[g.Id])); + var goals = GameData.Goals.Select(g => + { + var at = MatchClockReport.ElapsedOf(GameData, g); + return new MatchEvent( + at, + MatchClockReport.MinuteOf(GameData, g), + MatchClockReport.HalfOf(GameData, g.GamePeriodId, at), + g.RecordedAt, g.Id, g, null, progression[g.Id]); + }); IEnumerable subs = ShowSubstitutions ? GameData.Substitutions.Select(s => new MatchEvent( s.AtSeconds, diff --git a/src/FootballFormation.UI/Pages/MatchResult.razor b/src/FootballFormation.UI/Pages/MatchResult.razor index 43ba245..1a9882c 100644 --- a/src/FootballFormation.UI/Pages/MatchResult.razor +++ b/src/FootballFormation.UI/Pages/MatchResult.razor @@ -76,8 +76,9 @@
@* Ordered on the elapsed match clock, which runs on across the break, so a goal in first-half stoppage time keeps its place above the restart rather than sorting - past it — see GameGoal.TimelineSeconds. *@ - @foreach (var goal in GameData.Goals.OrderBy(g => g.TimelineSeconds)) + past it. A minute typed in on this page is a scoreboard reading and is converted + onto that clock — see MatchClockReport.ElapsedOf. *@ + @foreach (var goal in GameData.Goals.OrderBy(g => MatchClockReport.ElapsedOf(GameData, g))) { @* Own goals and opponent goals both count against us, so they share the styling. *@
diff --git a/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs b/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs index b0b8cdc..c13f4eb 100644 --- a/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs +++ b/tests/FootballFormation.Core.Tests/MatchClockReportTests.cs @@ -259,6 +259,54 @@ public void A_goal_with_no_clock_behind_it_falls_back_to_the_minute_on_the_row() Assert.Null(MatchClockReport.MinuteOf(game, new GameGoal())); } + /// + /// The timeline orders on elapsed seconds, so a minute typed in by hand has to be converted + /// onto that scale rather than read as though it already were one. The two only agree while + /// the halves run to length: this first half is three minutes long, and from the restart the + /// scoreboard trails the elapsed clock by exactly that. + /// + [Fact] + public void A_typed_in_minute_is_converted_onto_the_elapsed_clock_through_its_half() + { + var game = QuartersGame(); + Period(game, PeriodType.FirstQuarter).StartedAtSeconds = 0; + Period(game, PeriodType.ThirdQuarter).StartedAtSeconds = 33 * 60; + + int Elapsed(int minute) => MatchClockReport.ElapsedOf(game, new GameGoal { Minute = minute }); + + // First half: the scoreboard is the elapsed clock, because it kicked off at zero. + Assert.Equal(0, Elapsed(1)); + Assert.Equal(20 * 60, Elapsed(21)); + + // Second half: 31' is the first minute after a scoreboard restart at 30, which really + // happened at 33:00. Read as elapsed seconds it would have landed at 30:00 — before a goal + // scored in first-half stoppage time, and on the wrong side of the half-time rule. + Assert.Equal(33 * 60, Elapsed(31)); + Assert.Equal(35 * 60, Elapsed(33)); + Assert.Equal(PeriodType.SecondHalf, MatchClockReport.HalfOf(game, null, Elapsed(31))); + + // The clock on the row always wins — it is the reading, not a reconstruction of one. + Assert.Equal(1234, MatchClockReport.ElapsedOf(game, new GameGoal { AtSeconds = 1234, Minute = 5 })); + + // Nothing at all: the top of the match, which is where a goal with no minute has always sat. + Assert.Equal(0, MatchClockReport.ElapsedOf(game, new GameGoal())); + } + + /// + /// A match nobody ran from the touchline has no timings to convert through, so the typed + /// minutes keep the only order they have — and go on reading exactly as they did before goals + /// carried a clock at all. + /// + [Fact] + public void Typed_in_minutes_stand_on_their_own_when_no_half_was_ever_kicked_off() + { + var game = QuartersGame(); + + Assert.Equal(0, MatchClockReport.ElapsedOf(game, new GameGoal { Minute = 1 })); + Assert.Equal(30 * 60, MatchClockReport.ElapsedOf(game, new GameGoal { Minute = 31 })); + Assert.Equal(59 * 60, MatchClockReport.ElapsedOf(game, new GameGoal { Minute = 60 })); + } + /// /// Where the timeline draws half time. An event knows its own half; a goal typed in by hand /// knows only a clock reading, and the second half's kick-off is the line it falls one side of. diff --git a/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs b/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs index a4c0aae..b721cbc 100644 --- a/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs +++ b/tests/FootballFormation.Core.Tests/ScoreProgressionReportTests.cs @@ -10,6 +10,14 @@ namespace FootballFormation.Core.Tests; /// public class ScoreProgressionReportTests { + /// A 60-minute match — two 30-minute halves — carrying the goals it was scored in. + private static Game Match(params GameGoal[] goals) + { + var game = TestData.Game(durationMinutes: 60); + game.Goals.AddRange(goals); + return game; + } + /// A goal off the touchline, placed by the elapsed match clock it was logged at. private static GameGoal Goal( int id, int minute, bool ownGoal = false, bool opponentGoal = false) => @@ -32,7 +40,7 @@ public void Each_goal_carries_the_score_it_made_it() Goal(3, 30) }; - var progression = ScoreProgressionReport.Build(goals); + var progression = ScoreProgressionReport.Build(Match([.. goals])); Assert.Equal(new MatchScore(1, 0), progression[1]); Assert.Equal(new MatchScore(1, 1), progression[2]); @@ -44,7 +52,7 @@ public void An_own_goal_climbs_the_opponents_half_of_the_scoreline() { var goals = new List { Goal(1, 20, ownGoal: true) }; - var progression = ScoreProgressionReport.Build(goals); + var progression = ScoreProgressionReport.Build(Match([.. goals])); Assert.Equal(new MatchScore(0, 1), progression[1]); } @@ -54,7 +62,7 @@ public void The_goals_are_counted_in_the_order_they_were_scored_not_the_order_th { var goals = new List { Goal(2, 40), Goal(1, 10, opponentGoal: true) }; - var progression = ScoreProgressionReport.Build(goals); + var progression = ScoreProgressionReport.Build(Match([.. goals])); Assert.Equal(new MatchScore(0, 1), progression[1]); Assert.Equal(new MatchScore(1, 1), progression[2]); @@ -69,7 +77,7 @@ public void Two_goals_in_the_same_minute_are_separated_by_when_they_were_entered var second = Goal(4, 20); second.RecordedAt = first.RecordedAt.AddSeconds(20); - var progression = ScoreProgressionReport.Build([second, first]); + var progression = ScoreProgressionReport.Build(Match(second, first)); Assert.Equal(new MatchScore(0, 1), progression[7]); Assert.Equal(new MatchScore(1, 1), progression[4]); @@ -80,7 +88,7 @@ public void The_last_goals_score_is_the_final_score() { var goals = new List { Goal(1, 5), Goal(2, 25), Goal(3, 50, ownGoal: true) }; - var progression = ScoreProgressionReport.Build(goals); + var progression = ScoreProgressionReport.Build(Match([.. goals])); var final = progression[3]; Assert.Equal(Game.CountOurGoals(goals), final.Us); @@ -100,7 +108,7 @@ public void A_stoppage_time_goal_is_counted_inside_the_half_it_was_scored_in() var afterTheBreak = Goal(2, 33, opponentGoal: true); afterTheBreak.RecordedAt = stoppage.RecordedAt.AddMinutes(16); - var progression = ScoreProgressionReport.Build([afterTheBreak, stoppage]); + var progression = ScoreProgressionReport.Build(Match(afterTheBreak, stoppage)); Assert.Equal(new MatchScore(1, 0), progression[1]); Assert.Equal(new MatchScore(1, 1), progression[2]); @@ -117,15 +125,46 @@ public void A_goal_with_only_a_minute_is_counted_in_that_minute() var typedIn = new GameGoal { Id = 1, Minute = 40 }; var logged = Goal(2, 10, opponentGoal: true); - var progression = ScoreProgressionReport.Build([typedIn, logged]); + var progression = ScoreProgressionReport.Build(Match(typedIn, logged)); Assert.Equal(new MatchScore(0, 1), progression[2]); Assert.Equal(new MatchScore(1, 1), progression[1]); } + /// + /// A typed-in minute is a scoreboard reading, and on a match whose first half over-ran the + /// scoreboard and the elapsed clock disagree by the overrun. Counting the typed minute as + /// elapsed time filed a second-half goal ahead of one scored in first-half stoppage time, and + /// handed both the wrong running score. + /// + [Fact] + public void A_minute_typed_in_afterwards_is_placed_through_the_half_it_names() + { + // A first half whistled off three minutes long, so the second half kicks off at 33:00 + // while its scoreboard still starts at 30'. + var game = Match(); + game.AddPeriod(PeriodType.FirstHalf); + game.AddPeriod(PeriodType.SecondHalf); + game.Periods.Single(p => p.PeriodType == PeriodType.FirstHalf).StartedAtSeconds = 0; + game.Periods.Single(p => p.PeriodType == PeriodType.SecondHalf).StartedAtSeconds = 33 * 60; + + // Logged live two minutes into first-half stoppage — the scoreboard read 30+2. + var stoppage = new GameGoal { Id = 1, AtSeconds = 32 * 60 }; + + // Typed in afterwards as the 32nd minute, which is two minutes into the second half. + var typedIn = new GameGoal { Id = 2, Minute = 32, IsOpponentGoal = true }; + + game.Goals.AddRange([typedIn, stoppage]); + + var progression = ScoreProgressionReport.Build(game); + + Assert.Equal(new MatchScore(1, 0), progression[1]); + Assert.Equal(new MatchScore(1, 1), progression[2]); + } + [Fact] public void A_match_with_no_goals_has_nothing_to_report() { - Assert.Empty(ScoreProgressionReport.Build([])); + Assert.Empty(ScoreProgressionReport.Build(Match())); } } From 851cd93c3084f6bfb353d19a045dd124ed8a6faa Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 11:39:58 +0000 Subject: [PATCH 3/3] Move the goals that said they were stoppage time onto the clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration dropped AdditionalMinute without rescuing the rows that had one, so a goal logged 30+2 during the day that column existed came out reading 30'. Those rows are not ambiguous: an overrun on a goal says outright that it was scored in stoppage time, so its half follows from the minute and its clock reading from that half's own kick-off — including when the half over-ran, which is exactly where a reconstruction that ignored the kick-off would land three minutes out. They read 30+2 again, which is the point, because 32 is the 32nd minute and that is two minutes into a second half. Goals with no overrun are still left alone. A stored 37 could be a minute typed in by hand on the result page, and guessing a half for it is how the frozen minute went wrong to begin with. GoalClockBackfillTests migrates a seeded database across the boundary and asserts what the app then shows rather than what landed in a column — the only migration here with a body worth testing, and it runs unattended against the live volume on the next deploy. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XD7SBD1CvRzK4Xrhx3dtf7 --- docs/known_issues.md | 20 +- docs/models.md | 2 +- docs/testing.md | 1 + .../20260814112353_StoreGoalPeriodAndClock.cs | 62 ++++-- src/FootballFormation.Core/Models/GameGoal.cs | 7 +- .../GoalClockBackfillTests.cs | 181 ++++++++++++++++++ 6 files changed, 251 insertions(+), 22 deletions(-) create mode 100644 tests/FootballFormation.Core.Tests/GoalClockBackfillTests.cs diff --git a/docs/known_issues.md b/docs/known_issues.md index 1888eea..02a51c9 100644 --- a/docs/known_issues.md +++ b/docs/known_issues.md @@ -377,13 +377,19 @@ Avoid repeating these mistakes: 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 migration deliberately does not backfill, because nothing - left in an old row says which half a stored `37` 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`, so a goal logged in stoppage time during the day - that column existed now shows the capped minute (`30'`, not `30+2`); it sorts where it always did. + 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 diff --git a/docs/models.md b/docs/models.md index bf7e28f..fc32d76 100644 --- a/docs/models.md +++ b/docs/models.md @@ -215,7 +215,7 @@ bench, never both and never twice. | AssisterId | int? | FK → Player, SetNull | | 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 | +| 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 | diff --git a/docs/testing.md b/docs/testing.md index 36ac64d..7550be6 100644 --- a/docs/testing.md +++ b/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 | diff --git a/src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs b/src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs index fea0f46..f45fb83 100644 --- a/src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs +++ b/src/FootballFormation.Core/Migrations/20260814112353_StoreGoalPeriodAndClock.cs @@ -9,19 +9,18 @@ namespace FootballFormation.Core.Migrations /// reading on the match clock — so the minute shown is derived rather than frozen, and /// correcting a half's timings corrects its goals with it. /// - /// Deliberately not backfilled, and AdditionalMinute is dropped rather than folded into - /// anything. Nothing left in an old row says which half a stored 37 belonged to, so - /// those goals keep Minute and go on sorting where they always have; only goals logged - /// from here on carry a clock. Folding the overrun back in would have been worse than losing - /// it — a 30+2 counted on to 32 sorts past a goal in the 31st minute of the - /// second half, which is the bug the pair was introduced to fix. + /// Backfilled for exactly the rows that can be, which is the ones carrying an overrun. + /// AdditionalMinute > 0 is a row saying outright that it was scored in stoppage time, + /// so nothing has to be guessed: the minute names the half, the half names its kick-off, and + /// the clock reading falls out of the two. Those goals go on reading 30+2 afterwards, + /// which is the point — 32 is the 32nd minute, two minutes into a second half, and not + /// what happened. /// /// - /// It does lose something. A goal logged in stoppage time between - /// AddGoalAdditionalMinute shipping and this migration has a real overrun on the row, - /// and afterwards reads as the capped minute — 30+2 becomes 30'. Only the - /// display: the minute it sorts on is unchanged. That window is about a day, and the - /// alternative was a reconstruction dressed up as a reading. + /// Every other old row is deliberately left alone. A goal with AdditionalMinute = 0 says + /// nothing about which half it belongs to — a stored 37 could be a minute typed in by + /// hand on the result page — so it keeps Minute and goes on sorting where it always has. + /// Guessing a half for those is how the frozen minute went wrong in the first place. /// /// /// The order below is not what SQLite runs. EF folds the DropColumn into the table @@ -51,6 +50,47 @@ protected override void Up(MigrationBuilder migrationBuilder) type: "INTEGER", nullable: true); + // The one set of rows that can be moved across without guessing: a goal with an + // overrun on it says outright that it was scored in stoppage time, so which half it + // belongs to follows from its minute and the clock reading follows from the half's + // own kick-off. Everything else keeps Minute and is left alone — see the remarks. + // + // Reconstructed to the minute, which is all the row ever held. Read back through + // MatchClockReport it lands on the same 30+2 it was written as: the half's clock is + // capped at halfSeconds and the remainder is counted alongside from 1. + // + // A goal whose half was never kicked off, or whose game has no duration, selects NULL + // for both columns — which is exactly the state it would have had with no backfill. + migrationBuilder.Sql(""" + UPDATE GameGoals + SET AtSeconds = ( + SELECT p.StartedAtSeconds + (g.GameDurationMinutes * 60 / 2) + + ((GameGoals.AdditionalMinute - 1) * 60) + FROM GamePeriods p + JOIN Games g ON g.Id = p.GameId + WHERE p.GameId = GameGoals.GameId + AND g.GameDurationMinutes > 0 + AND p.StartedAtSeconds IS NOT NULL + AND (CASE WHEN p.PeriodType IN (0, 2, 3) THEN 0 ELSE 1 END) + = (CASE WHEN GameGoals.Minute * 60 + <= (g.GameDurationMinutes * 60 / 2) THEN 0 ELSE 1 END) + ORDER BY p.StartedAtSeconds + LIMIT 1), + GamePeriodId = ( + SELECT p.Id + FROM GamePeriods p + JOIN Games g ON g.Id = p.GameId + WHERE p.GameId = GameGoals.GameId + AND g.GameDurationMinutes > 0 + AND p.StartedAtSeconds IS NOT NULL + AND (CASE WHEN p.PeriodType IN (0, 2, 3) THEN 0 ELSE 1 END) + = (CASE WHEN GameGoals.Minute * 60 + <= (g.GameDurationMinutes * 60 / 2) THEN 0 ELSE 1 END) + ORDER BY p.StartedAtSeconds + LIMIT 1) + WHERE AdditionalMinute > 0 AND Minute IS NOT NULL; + """); + migrationBuilder.CreateIndex( name: "IX_GameGoals_GamePeriodId", table: "GameGoals", diff --git a/src/FootballFormation.Core/Models/GameGoal.cs b/src/FootballFormation.Core/Models/GameGoal.cs index 4942c33..caee347 100644 --- a/src/FootballFormation.Core/Models/GameGoal.cs +++ b/src/FootballFormation.Core/Models/GameGoal.cs @@ -34,9 +34,10 @@ public class GameGoal /// /// The minute somebody typed in on the result page, where there is no clock to read. Also what - /// a goal logged before this row carried a clock still shows: those rows have no - /// and were never backfilled, because nothing left in one says whether - /// a 37 in a 35-minute half was stoppage time or a number typed in by hand. + /// a goal logged before this row carried a clock still shows, unless it was scored in stoppage + /// time — those said so on the row and were moved onto the clock by StoreGoalPeriodAndClock. + /// The rest keep only this, because nothing left in one says whether a 37 in a 35-minute half + /// was stoppage time or a number typed in by hand. /// public int? Minute { get; set; } diff --git a/tests/FootballFormation.Core.Tests/GoalClockBackfillTests.cs b/tests/FootballFormation.Core.Tests/GoalClockBackfillTests.cs new file mode 100644 index 0000000..cf81b47 --- /dev/null +++ b/tests/FootballFormation.Core.Tests/GoalClockBackfillTests.cs @@ -0,0 +1,181 @@ +using FootballFormation.Core.Data; +using FootballFormation.Core.Reporting; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; + +namespace FootballFormation.Core.Tests; + +/// +/// The one migration in here that rewrites rows rather than adding columns to them. +/// +/// StoreGoalPeriodAndClock moves goals logged in stoppage time onto the clock, and it runs +/// unattended against the live volume on the next deploy — so what it does is checked here rather +/// than only rehearsed by hand. Every other migration is exercised implicitly by +/// ServiceTestBase, which builds the schema from the model; this one has a body worth +/// asserting on, and the property that matters is not a column value but what the app then shows: +/// a goal written 30+2 still reads 30+2. +/// +/// +/// Migrated rather than created, because a backfill only exists on the path from the old schema. +/// Two contexts over one held-open in-memory connection, the same arrangement +/// ServiceTestBase uses. +/// +/// +public class GoalClockBackfillTests : IDisposable +{ + private const string BeforeTheBackfill = "20260813062317_AddGoalAdditionalMinute"; + + /// Ids given to the two halves below, so the assertions can name them. + private const int FirstHalfId = 10; + private const int SecondHalfId = 11; + + private readonly SqliteConnection _connection; + + public GoalClockBackfillTests() + { + _connection = new SqliteConnection("Filename=:memory:"); + _connection.Open(); + } + + private AppDbContext Context() => + new(new DbContextOptionsBuilder() + .UseSqlite(_connection) + .AddInterceptors(new DateInSqlInterceptor()) + .Options); + + /// + /// A 60-minute match in halves whose first half over-ran to 33:00, so the second half kicked + /// off there and not at the 30:00 its scoreboard restarts at. That gap is the whole difficulty: + /// a reconstruction that ignored the real kick-off would put every second-half goal three + /// minutes out. + /// + private async Task SeedBeforeTheBackfillAsync() + { + await using (var db = Context()) + await db.GetService().MigrateAsync(BeforeTheBackfill); + + var cmd = _connection.CreateCommand(); + cmd.CommandText = $""" + INSERT INTO Seasons (Name, StartDate, EndDate, IsCurrent) + VALUES ('25/26','2026-08-01','2027-06-30',1); + INSERT INTO Games (SeasonId, Opponent, Date, ScoreHome, ScoreAway, FormationType, + SplitType, GameDurationMinutes, IsHomeGame, MatchState, + ClockAccumulatedSeconds, MatchType, GuestPlayerIds, UnavailablePlayerIds) + VALUES (1,'Opp','2026-08-10',0,4,0,0,60,1,2,4200,0,'',''); + INSERT INTO GamePeriods (Id, GameId, PeriodType, StartedAtSeconds, EndedAtSeconds) + VALUES ({FirstHalfId},1,0,0,1980), ({SecondHalfId},1,1,1980,4200); + + -- Written by the live screen as a minute and an overrun, the shape being replaced. + INSERT INTO GameGoals (Id, GameId, Minute, AdditionalMinute, IsOwnGoal, IsOpponentGoal, RecordedAt) + VALUES (1,1,30,2,0,1,'2026-08-10 10:32:00'), -- first-half stoppage + (2,1,60,1,0,1,'2026-08-10 11:05:00'), -- second-half stoppage + (3,1,37, 0,0,1,'2026-08-10 10:40:00'), -- a plain minute: ambiguous + (4,1,NULL,0,0,1,'2026-08-10 10:41:00'); -- no minute at all + """; + await cmd.ExecuteNonQueryAsync(); + + await using (var db = Context()) await db.Database.MigrateAsync(); + } + + [Fact] + public async Task A_goal_scored_in_stoppage_time_still_reads_as_stoppage_time_afterwards() + { + await SeedBeforeTheBackfillAsync(); + + await using var db = Context(); + var game = await db.Games.Include(g => g.Periods).Include(g => g.Goals).FirstAsync(); + + string Shown(int goalId) => + MatchClockReport.MinuteOf(game, game.Goals.Single(g => g.Id == goalId))?.ToString() ?? "—"; + + // 32 would be the 32nd minute — two minutes into the second half — which is not when + // either of these was scored. + Assert.Equal("30+2", Shown(1)); + Assert.Equal("60+1", Shown(2)); + + Assert.Equal("37", Shown(3)); + Assert.Equal("—", Shown(4)); + } + + [Fact] + public async Task The_overrun_is_rewritten_as_the_half_and_the_clock_it_was_scored_on() + { + await SeedBeforeTheBackfillAsync(); + + await using var db = Context(); + var goals = await db.GameGoals.ToDictionaryAsync(g => g.Id); + + // 30:00 into a first half that started at 0, plus the minute of stoppage already counted. + Assert.Equal(FirstHalfId, goals[1].GamePeriodId); + Assert.Equal(31 * 60, goals[1].AtSeconds); + + // The second half really kicked off at 33:00, so its 60+1 is 63 minutes of play — not the + // 61 the scoreboard reading alone would have suggested. + Assert.Equal(SecondHalfId, goals[2].GamePeriodId); + Assert.Equal(63 * 60, goals[2].AtSeconds); + } + + /// + /// A goal without an overrun says nothing about which half it belongs to — a stored 37 on a + /// 30-minute half could be a minute somebody typed on the result page. Guessing one is how the + /// frozen minute went wrong in the first place, so these rows are left exactly as they are. + /// + [Fact] + public async Task A_goal_with_only_a_minute_is_left_alone_rather_than_guessed_at() + { + await SeedBeforeTheBackfillAsync(); + + await using var db = Context(); + var goals = await db.GameGoals.ToDictionaryAsync(g => g.Id); + + Assert.Null(goals[3].GamePeriodId); + Assert.Null(goals[3].AtSeconds); + Assert.Equal(37, goals[3].Minute); + + Assert.Null(goals[4].GamePeriodId); + Assert.Null(goals[4].AtSeconds); + Assert.Null(goals[4].Minute); + } + + /// + /// The reason the backfill is worth doing at all: on the elapsed clock the rewritten goals sit + /// inside the halves they were scored in, so the timeline and the running score agree with the + /// minutes on screen. + /// + [Fact] + public async Task The_rewritten_goals_sort_inside_the_halves_they_were_scored_in() + { + await SeedBeforeTheBackfillAsync(); + + await using var db = Context(); + var game = await db.Games.Include(g => g.Periods).Include(g => g.Goals).FirstAsync(); + + var order = game.Goals + .OrderBy(g => MatchClockReport.ElapsedOf(game, g)) + .ThenBy(g => g.RecordedAt) + .Select(g => g.Id); + + // No minute, then 30+2, then the 37th minute, then 60+1. + Assert.Equal([4, 1, 3, 2], order); + } + + [Fact] + public async Task The_migration_leaves_the_foreign_keys_intact() + { + await SeedBeforeTheBackfillAsync(); + + var check = _connection.CreateCommand(); + check.CommandText = "PRAGMA foreign_key_check"; + await using var violations = await check.ExecuteReaderAsync(); + + Assert.False(await violations.ReadAsync(), "the rebuilt GameGoals table has a broken row"); + } + + public void Dispose() + { + _connection.Dispose(); + GC.SuppressFinalize(this); + } +}