Conversation
|
[bot:copilot-review-request] @copilot Please review this PR for correctness, regressions, edge cases, and any focused fixes needed before human handoff. Keep feedback scoped to the current PR. |
There was a problem hiding this comment.
Pull request overview
Adds an admin-only, read-only report at /incomplete_submissions to help admins quickly identify active players who have not completed their bracket submissions, reusing existing pick-counting logic.
Changes:
- Add
Admin::incomplete_submissionsaction to compute and stash an incomplete-submissions report. - Add a new admin template to render the report as a table (total, regional, and Final Four pick counts).
- Add an “Incomplete Submissions” entry to the admin links and extend the admin controller test coverage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/Bracket/Controller/Admin.pm |
Implements the /incomplete_submissions report action and stashes data for the view. |
root/admin/incomplete_submissions.tt |
New admin-facing template rendering incomplete submission rows. |
root/admin/links.tt |
Adds a navigation link to the new report. |
t/controller_Admin.t |
Adds a test exercising the new admin report behavior. |
| my $expected_total_picks = 63; | ||
| my $total_picks_by_player = $c->model('DBIC')->count_player_picks || {}; | ||
| my @active_players = $c->model('DBIC::Player')->search( | ||
| { | ||
| active => 1, | ||
| id => { '!=' => 1 }, | ||
| }, | ||
| { | ||
| order_by => [qw/last_name first_name id/], | ||
| } | ||
| )->all; | ||
|
|
||
| my @incomplete_submissions; | ||
| foreach my $player (@active_players) { | ||
| my $player_id = $player->id; | ||
| my $total_picks = $total_picks_by_player->{$player_id} || 0; | ||
| next if $total_picks >= $expected_total_picks; |
There was a problem hiding this comment.
$total_picks is derived from count_player_picks, which ultimately counts pick rows rather than distinct games. Because the pick table schema only has a surrogate PK (id) and no uniqueness on (player, game), duplicate pick rows are possible, which can cause this report to incorrectly treat an incomplete bracket as complete. Consider counting distinct game IDs per player (or otherwise de-duplicating by (player, game)) when computing completeness for this report.
| my @incomplete_submissions; | ||
| foreach my $player (@active_players) { | ||
| my $player_id = $player->id; | ||
| my $total_picks = $total_picks_by_player->{$player_id} || 0; | ||
| next if $total_picks >= $expected_total_picks; | ||
|
|
||
| my $region_counts = $c->model('DBIC')->count_region_picks($player_id) || {}; | ||
| my $final4_count = $c->model('DBIC')->count_final4_picks($player_id) || 0; | ||
|
|
||
| push @incomplete_submissions, { | ||
| player_id => $player_id, | ||
| player_name => join(' ', grep { defined && length } $player->first_name, $player->last_name), | ||
| username => $player->username, | ||
| total_picks => $total_picks, | ||
| missing_total => $expected_total_picks - $total_picks, | ||
| region_counts => $region_counts, | ||
| final4_picks => $final4_count, | ||
| final4_missing => 3 - $final4_count, | ||
| }; |
There was a problem hiding this comment.
This action can generate an N+1 query pattern for incomplete players: after loading all active players, it runs count_region_picks and count_final4_picks separately per incomplete player, and those model methods each execute a separate SQL statement. If there are many incomplete players (especially early in the tournament), this can become slow. Consider batching these counts (grouped by player) in a single query per metric, or adding a model method that returns region/final4 counts for a list of player IDs.
lib/Bracket/Controller/Admin.pm
Outdated
| missing_total => $expected_total_picks - $total_picks, | ||
| region_counts => $region_counts, | ||
| final4_picks => $final4_count, | ||
| final4_missing => 3 - $final4_count, |
There was a problem hiding this comment.
final4_missing is computed and stashed into each row but is not used by the template (which only renders final4_picks). If it’s not needed, consider removing it to keep the report payload minimal; otherwise render it (or use it for sorting/filtering) so it doesn’t become dead data.
| final4_missing => 3 - $final4_count, |
Agent-Logs-Url: https://github.com/mateu/Bracket/sessions/2a2fe8d0-1351-4cc7-b983-1fe0b8f88023 Co-authored-by: mateu <31550+mateu@users.noreply.github.com>
Review complete. Found and fixed two issues (commit
No other correctness issues found: admin guard ( |
What
Add an admin-only, read-only report at
/incomplete_submissionsthat lists active players with incomplete brackets.Why
Issue #32 asks for a fast way to identify incomplete submissions without digging through raw pick data.
How
Reuse existing submission-count logic (
count_player_picks,count_region_picks,count_final4_picks) to avoid schema or scoring changes.Add a focused admin template and an admin-links entry.
Filter to active non-perfect players and present total/region/final4 completion counts in one table.
Testing
script/test-env.sh prove -lv t/controller_Admin.tscript/test-env.sh prove -lr tQuality Report
Changes: 4 files changed, 163 insertions(+)
Code scan: clean
Tests: skipped
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline