Refactor Final Four paths to graph-derived structure#29
Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates Final Four/top-of-bracket topology logic into a graph-derived service so controllers, validation, and tests no longer depend on hardcoded game IDs.
Changes:
- Added
Bracket::Service::BracketStructureto derive championship/semifinal/Final Four game IDs and region-winner games fromgame_graph/game_team_graph. - Updated
BracketValidatorFinal Four validation to use derived Final Four game IDs. - Updated
Final4controller and tests to seed/use derived region-winner and Final Four game IDs; added a dedicated structure test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
lib/Bracket/Service/BracketStructure.pm |
New service that derives bracket topology from graph ancestry and exposes helper methods. |
lib/Bracket/Service/BracketValidator.pm |
Uses derived Final Four game IDs instead of hardcoded 61/62/63. |
lib/Bracket/Controller/Final4.pm |
Loads region-winner picks via derived region-winner game IDs. |
t/bracket_validator.t |
Updates Final Four validation test setup to use derived topology. |
t/bracket_structure.t |
New tests to assert derived championship/semifinal/Final Four and region-winner mappings. |
lib/Bracket/Controller/Final4.pm
Outdated
| my $region_winner_picks = Bracket::Service::BracketStructure->region_winner_games_by_region( | ||
| $c->model('DBIC')->schema | ||
| ); | ||
| foreach my $region_id (sort { $a <=> $b } keys %{$region_winner_picks}) { | ||
| my $region_name = 'region' . "_${region_id}"; | ||
| my $game = $region_winner_picks{$region_id}; | ||
| my $game = $region_winner_picks->{$region_id}; | ||
| $c->stash->{$region_name} = | ||
| $c->model('DBIC::Pick')->search({ player => $player_id, game => $game })->first; |
There was a problem hiding this comment.
Final4 now derives the region-winner game ids dynamically, but the Final Four templates/JS still hardcode game ids (e.g., root/final4/bracket.tt and root/static/javascript/final4_picks.js reference 15/30/45/60 and 61/62/63). If the derived ids differ, the page will render incorrect CSS classes/DOM ids and the JS will submit picks for the wrong games. To make the refactor effective, consider stashing the derived game-id mapping (region winners + semifinal/championship ids) and updating the templates/JS to use it instead of constants.
t/bracket_validator.t
Outdated
| my $region_winner_games = Bracket::Service::BracketStructure->region_winner_games_by_region($schema); | ||
| ok(keys %{$region_winner_games} == 4, 'derived region-winner games cover all regions'); | ||
| my $final4_games = Bracket::Service::BracketStructure->final4_game_ids($schema); | ||
| ok(@{$final4_games} >= 3, 'derived final4 game structure is available'); | ||
|
|
||
| my @final4_game_rows = map { | ||
| $schema->resultset('Game')->find({ id => $_ }) | ||
| } @{$final4_games}; | ||
| @final4_game_rows = sort { | ||
| $a->round <=> $b->round || $a->id <=> $b->id | ||
| } @final4_game_rows; | ||
|
|
||
| my $championship_game = $final4_game_rows[-1]->id; | ||
| my @semifinal_games = sort { $a <=> $b } grep { $_ != $championship_game } @{$final4_games}; | ||
|
|
There was a problem hiding this comment.
The test is treating derived Final Four games as ">= 3" and then inferring semifinal/championship ids by sorting Game rows. Since Final Four should be a precise shape, this can let incorrect derivations slip by (or behave unpredictably if more than 3 ids are returned). Consider asserting the exact expected count and using BracketStructure->describe_bracket (championship_game_id + semifinal_game_ids) directly to make the test more robust and targeted.
| push @{$parents_by_game{$edge->game}}, $edge->parent_game; | ||
| push @{$children_by_parent{$edge->parent_game}}, $edge->game; | ||
| $game_ids{$edge->game} = 1; | ||
| $game_ids{$edge->parent_game} = 1; |
There was a problem hiding this comment.
In _derive_structure, GameGraph columns are nullable; if any edge row has a NULL game or parent_game, this will push undef into %parents_by_game/%children_by_parent and can corrupt the derived topology (and emit warnings). Consider skipping edges with missing endpoints (or failing with a clear error) before building the adjacency maps.
| push @{$parents_by_game{$edge->game}}, $edge->parent_game; | |
| push @{$children_by_parent{$edge->parent_game}}, $edge->game; | |
| $game_ids{$edge->game} = 1; | |
| $game_ids{$edge->parent_game} = 1; | |
| my $game_id = $edge->game; | |
| my $parent_game_id = $edge->parent_game; | |
| next if !defined $game_id || !defined $parent_game_id; | |
| push @{$parents_by_game{$game_id}}, $parent_game_id; | |
| push @{$children_by_parent{$parent_game_id}}, $game_id; | |
| $game_ids{$game_id} = 1; | |
| $game_ids{$parent_game_id} = 1; |
| foreach my $seed_row (@seed_rows) { | ||
| my $team = $schema->resultset('Team')->find({ id => $seed_row->team }); | ||
| next if !$team; | ||
| my $region = $team->region; | ||
| next if !$region; | ||
| $regions{$region->id} = 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
_region_for_game does one Team lookup per GameTeamGraph row (N+1 queries) and will be called multiple times during structure derivation. It would be more efficient to fetch the needed team->region ids in a single query (e.g., join GameTeamGraph->Team->Region or bulk-load teams with an -in query) and then compute the unique region set in memory.
| foreach my $seed_row (@seed_rows) { | |
| my $team = $schema->resultset('Team')->find({ id => $seed_row->team }); | |
| next if !$team; | |
| my $region = $team->region; | |
| next if !$region; | |
| $regions{$region->id} = 1; | |
| } | |
| # Collect team IDs from the seed rows to avoid N+1 Team lookups. | |
| my %team_ids; | |
| foreach my $seed_row (@seed_rows) { | |
| my $team_id = $seed_row->team; | |
| next if !defined $team_id; | |
| $team_ids{$team_id} = 1; | |
| } | |
| if (%team_ids) { | |
| my @teams = $schema->resultset('Team')->search( | |
| { id => { -in => [ keys %team_ids ] } }, | |
| { prefetch => 'region' }, | |
| )->all; | |
| foreach my $team (@teams) { | |
| next if !$team; | |
| my $region = $team->region; | |
| next if !$region; | |
| $regions{$region->id} = 1; | |
| } | |
| } |
| my $final4_game_ids = Bracket::Service::BracketStructure->final4_game_ids($schema); | ||
| if (!@{$final4_game_ids}) { | ||
| return { ok => 0, errors => ['Could not derive Final Four game structure'] }; | ||
| } |
There was a problem hiding this comment.
validate_final4_payload currently only errors if no Final Four games can be derived, but it will proceed (and potentially validate incorrectly) if the derived structure is incomplete (e.g., only 1–2 games due to bad/missing graph edges). Consider requiring the expected shape (exactly 3 Final Four games and 2 semifinals) and returning an error if it cannot be derived reliably.
| } | |
| } | |
| if (@{$final4_game_ids} != 3) { | |
| return { ok => 0, errors => ['Could not derive complete Final Four game structure'] }; | |
| } |
|
[bot:copilot-fixup-request] @copilot Please address the actionable issues from your review on this PR as a new commit on the same branch. Keep scope limited to the current PR, avoid unrelated cleanup, and preserve existing intent. |
- Skip NULL game/parent_game edges in _derive_structure to prevent corrupted adjacency maps and undef key warnings - Replace N+1 Team->find calls in _region_for_game with a single bulk Team->search with prefetch => 'region' - Add game_routes to describe_bracket output (source_game_id => target_game_id), derived from graph; expose via game_routes() method - Require exactly 3 Final Four games in validate_final4_payload (was only checking for !@, now requires == 3) - Update bracket_validator.t to use describe_bracket directly and check == 3 instead of >= 3; read championship/semifinal IDs from structure instead of sorting game rows - Stash region_winner_game_id_for, semifinal_game_ids, championship_game_id, and game_routes in Final4 controller (make and view actions) - Update bracket.tt to use stashed TT variables instead of hardcoded game IDs 15/30/45/60/61/62/63; add data-routes-to attributes - Update final4_picks.js to read data-routes-to attribute instead of hardcoded game routing conditionals Agent-Logs-Url: https://github.com/mateu/Bracket/sessions/e6a21252-8574-47ab-bc40-96fc12478711 Co-authored-by: mateu <31550+mateu@users.noreply.github.com>
Addressed all actionable review issues in commit 54a7399:
|
What
Create a consolidated successor to #19 and #24 that derives Final Four and region-winner paths from graph data instead of fixed game IDs.
Why
Both prior PRs were solving the same bracket-topology problem. This combines the narrower runtime shape from #24 with the richer structure surface/assertions from #19 so we can review and merge one reliable implementation.
How
Bracket::Service::BracketStructureas the graph-derived topology servicegame_graph/game_team_graphancestrygame/parent_gameedges before building adjacency maps to prevent topology corruption_region_for_game(single bulk query withprefetch => 'region') to eliminate N+1 queriesgame_routes(source game id → next game id) indescribe_bracketoutput and viagame_routes()class methodFinal4controller stash loading to derived region-winner games; stash full structure (region_winner_game_id_for,semifinal_game_ids,championship_game_id,game_routes) so templates/JS are not reliant on hardcoded idsbracket.ttto use stashed TT variables throughout; adddata-routes-toattributes to clickable<p>elementsfinal4_picks.jsto readdata-routes-toattribute instead of hardcoded game-number routing conditionsBracketValidatorto require exactly 3 derived Final Four games (not just non-empty) before accepting a payloaddescribe_bracketdirectly and assert exact countsSupersedes
Testing
Attempted locally:
script/test-env.sh prove -lv t/bracket_structure.t t/bracket_validator.tALLOW_GLOBAL_PERL5=1 script/test-env.sh prove -lv t/bracket_structure.t t/bracket_validator.tBoth are currently blocked in this environment because
DBIx::Class::Schemais not available in the local/global Perl library path on the host running the branch prep. CI or a repo-prepared dev environment should validate the branch normally.📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.