Skip to content

refactor: use dynamic region IDs in scoring #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mateu merged 2 commits into from
Apr 7, 2026
Merged

refactor: use dynamic region IDs in scoring #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ae11e1
refactor: derive region ids from schema in scoring paths
mateu Apr 6, 2026
fa4f33b
fix: add (1..4) fallback when Region table is empty in all three sites
Copilot Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/Bracket/Controller/Admin.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ sub update_player_points : Global {

$c->stash->{template} = 'player/all_home.tt';
my @players = $c->model('DBIC::Player')->all;
my @region_ids = map { $_->id }
$c->model('DBIC')->schema->resultset('Region')->search({}, { order_by => 'id' })->all;
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@region_ids is derived purely from the Region table; if that table is empty, this action will skip the per-region loop entirely and won't create/update any RegionScore rows. Consider adding the same fallback used in incomplete_submissions (e.g., default to 1..4 when no Region rows exist) so admin score refresh remains consistent on a fresh/partially populated DB.

Suggested change
$c->model('DBIC')->schema->resultset('Region')->search({}, { order_by => 'id' })->all;
$c->model('DBIC')->schema->resultset('Region')->search({}, { order_by => 'id' })->all;
@region_ids = (1 .. 4) unless @region_ids;

Copilot uses AI. Check for mistakes.
@region_ids = (1 .. 4) unless @region_ids;

# Get player scores
foreach my $player (@players) {
Expand All @@ -52,7 +55,7 @@ sub update_player_points : Global {
my $points_ref = $c->forward('player_points', [ $player->id ]);
$player->points($points_ref->[0] || 0);
$player->update;
foreach my $region_id (1 .. 4) {
foreach my $region_id (@region_ids) {
my $region_score = $c->model('DBIC::RegionScore')->update_or_create(
{
player => $player->id,
Expand Down Expand Up @@ -138,7 +141,9 @@ sub incomplete_submissions : Global {
my $expected_region_picks_by_region = $pick_targets->{region_picks_by_region} || {};
my @region_ids = sort { $a <=> $b } keys %{$expected_region_picks_by_region};
if (!@region_ids) {
@region_ids = (1 .. 4);
@region_ids = map { $_->id }
$c->model('DBIC')->schema->resultset('Region')->search({}, { order_by => 'id' })->all;
@region_ids = (1 .. 4) if !@region_ids;
$expected_region_picks_by_region = {
map { $_ => 15 } @region_ids
};
Expand Down
20 changes: 14 additions & 6 deletions lib/Bracket/Model/DBIC.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ sub _update_points_portable {
my %times;
my $current_time = time();
my $previous_time = $current_time;
my @region_ids = _region_ids_for_schema($schema);
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_region_ids_for_schema can return an empty list when the Region table is empty; in that case the portable update path will not update/create any RegionScore rows (even though points_for may still accumulate by team.region). To match the intended behavior described in the PR (and the controller fallback), consider defaulting @region_ids to (1..4) when no Region rows are present.

Suggested change
my @region_ids = _region_ids_for_schema($schema);
my @region_ids = _region_ids_for_schema($schema);
@region_ids = (1 .. 4) unless @region_ids;

Copilot uses AI. Check for mistakes.
@region_ids = (1 .. 4) unless @region_ids;

my @perfect_picks = $schema->resultset('Pick')->search(
{ player => 1 },
Expand Down Expand Up @@ -282,7 +284,7 @@ sub _update_points_portable {
$schema->txn_do(sub {
foreach my $player ($schema->resultset('Player')->search({})->all) {
my $player_id = $player->get_column('id');
foreach my $region_id (1 .. 4) {
foreach my $region_id (@region_ids) {
my $points = $points_for{$player_id}{$region_id} || 0;
$schema->resultset('RegionScore')->update_or_create({
player => $player_id,
Expand All @@ -299,10 +301,7 @@ sub _update_points_portable {
$schema->txn_do(sub {
foreach my $player ($schema->resultset('Player')->search({})->all) {
my $player_id = $player->get_column('id');
my $total_points = 0;
foreach my $region_id (1 .. 4) {
$total_points += $points_for{$player_id}{$region_id} || 0;
}
my $total_points = sum(values %{$points_for{$player_id} || {}}) || 0;
$player->update({ points => $total_points });
}
});
Expand Down Expand Up @@ -345,6 +344,8 @@ Displayed on Player home page.

sub count_region_picks {
my ($self, $player_id) = @_;
my @region_ids = _region_ids_for_schema($self->schema);
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

count_region_picks now initializes its result map from Region rows; if there are no Region rows, it will return an empty hashref, which makes callers that iterate expected region IDs see undef instead of 0. Consider applying the same empty-region fallback here (e.g., (1..4)) so the return shape stays stable even when the Region table is empty.

Suggested change
my @region_ids = _region_ids_for_schema($self->schema);
my @region_ids = _region_ids_for_schema($self->schema);
@region_ids = (1 .. 4) unless @region_ids;

Copilot uses AI. Check for mistakes.
@region_ids = (1 .. 4) unless @region_ids;
my $storage = $self->schema->storage;
return $storage->dbh_do(
sub {
Expand All @@ -360,7 +361,7 @@ sub count_region_picks {
;'
);
$sth->execute($player_id) or die $sth->errstr;;
my $picks_per_region = { 1 => 0, 2 => 0, 3 => 0, 4 => 0 };
my $picks_per_region = { map { $_ => 0 } @region_ids };
my $result = $sth->fetchall_arrayref;
foreach my $row (@{$result}) {
$picks_per_region->{$row->[0]} = $row->[1];
Expand Down Expand Up @@ -634,6 +635,13 @@ sub count_final4_picks {
)->count;
}

sub _region_ids_for_schema {
my ($schema) = @_;
return () if !$schema;
return map { $_->id }
$schema->resultset('Region')->search({}, { order_by => 'id' })->all;
}

=head1 NAME

Bracket::Model::DBIC - Catalyst DBIC Schema Model
Expand Down
2 changes: 1 addition & 1 deletion t/controller_Admin.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use Bracket::Service::BracketStructure;

sub count_region_picks {
my ($self, $player_id) = @_;
my %counts = (1 => 0, 2 => 0, 3 => 0, 4 => 0);
my %counts = map { $_->id => 0 } $self->{schema}->resultset('Region')->search({})->all;
my $rs = $self->{schema}->resultset('Pick')->search(
{
'me.player' => $player_id,
Expand Down
59 changes: 59 additions & 0 deletions t/model_dynamic_region_counts.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
use strict;
use warnings;

use Test::More;

use lib qw(lib t/lib);
use BracketTestSchema;
use Bracket::Model::DBIC;

{
package TestDBICModel;
sub new { my ($class, $schema) = @_; return bless { schema => $schema }, $class; }
sub schema { return $_[0]->{schema}; }
}

my $schema = BracketTestSchema->init_schema(populate => 1);

$schema->resultset('Region')->create({
id => 5,
name => 'Wildcard',
});
$schema->resultset('Team')->create({
id => 500,
seed => 1,
name => 'Wildcard Team',
region => 5,
round_out => 7,
});
$schema->resultset('Game')->create({
id => 500,
winner => undef,
round => 1,
lower_seed => 0,
});

my $player = $schema->resultset('Player')->create({
email => 'dynamic-region-counts@example.com',
password => 'secret',
first_name => 'Dynamic',
last_name => 'Region',
});

$schema->resultset('Pick')->create({
player => $player->id,
game => 500,
pick => 500,
});

my $model = TestDBICModel->new($schema);
my $counts = Bracket::Model::DBIC::count_region_picks($model, $player->id);

ok(exists $counts->{5}, 'count map includes dynamically added region');
is($counts->{5}, 1, 'count map includes picks from dynamically added region');

for my $region_id (1 .. 4) {
is($counts->{$region_id}, 0, "existing region $region_id defaults to zero when no picks exist");
}

done_testing();
7 changes: 6 additions & 1 deletion t/model_update_points.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ my $player = $schema->resultset('Player')->create({
last_name => 'Points',
});

$schema->resultset('Region')->create({
id => 5,
name => 'Wildcard',
});

sub set_pick {
my ($player_id, $game_id, $team_id) = @_;
my ($pick) = $schema->resultset('Pick')->search({
Expand Down Expand Up @@ -70,7 +75,7 @@ ok($region1_score, 'region score created for player');
is($region1_score->get_column('points'), 47, 'player receives expected region points from current-round state');

my $all_region_scores = $schema->resultset('RegionScore')->search({ player => $player->id })->count;
is($all_region_scores, 4, 'portable path maintains all region score rows per player');
is($all_region_scores, 5, 'portable path maintains region score rows for all regions in schema');

my $player_row = $schema->resultset('Player')->find($player->id);
is($player_row->get_column('points'), 47, 'player total points updated from region scores');
Expand Down
Loading