From fc74006086f8847264ab6a171d6b8be4fc80cb9d Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 12 Mar 2026 15:07:39 -0400 Subject: [PATCH 1/3] Bug 2003867 - reviewer rotation seems to always assign patches to the same reviewers within the group --- extensions/PhabBugz/Extension.pm | 23 +++++++++++++- extensions/PhabBugz/lib/Util.pm | 52 ++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index 50c881c9d3..8aff17ce30 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -167,8 +167,15 @@ sub db_schema_abstract_schema { FIELDS => [ id => {TYPE => 'INTSERIAL', NOTNULL => 1, PRIMARYKEY => 1,}, project_phid => {TYPE => 'VARCHAR(255)', NOTNULL => 1,}, + author_phid => {TYPE => 'VARCHAR(255)', NOTNULL => 1,}, user_phid => {TYPE => 'VARCHAR(255)', NOTNULL => 1,}, - ] + ], + INDEXES => [ + phab_reviewer_rotation_idx => { + FIELDS => ['project_phid', 'author_phid'], + TYPE => 'UNIQUE', + }, + ], }; $args->{'schema'}->{'phab_uplift_form_state'} = { FIELDS => [ @@ -179,6 +186,20 @@ sub db_schema_abstract_schema { }; } +sub install_update_db { + my $dbh = Bugzilla->dbh; + + # Bug 2003867 - Make reviewer rotation per-author instead of global. + # Clear existing rows since they lack author context. + if (!$dbh->bz_column_info('phab_reviewer_rotation', 'author_phid')) { + $dbh->bz_add_column('phab_reviewer_rotation', 'author_phid', + {TYPE => 'VARCHAR(255)', NOTNULL => 1, DEFAULT => "''"}); + $dbh->do('DELETE FROM phab_reviewer_rotation'); + $dbh->bz_add_index('phab_reviewer_rotation', 'phab_reviewer_rotation_idx', + {FIELDS => ['project_phid', 'author_phid'], TYPE => 'UNIQUE'}); + } +} + sub install_filesystem { my ($self, $args) = @_; my $files = $args->{'files'}; diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index f1d459ca69..81f7f1f142 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -384,8 +384,9 @@ PROJECT: foreach my $project (@review_projects) { INFO('Sorted project members found: ' . (join ', ', map { $_->name } @project_members)); - # Find the last selected reviewer for the rotation group, if one exists. - my $last_reviewer_phid = find_last_reviewer_phid($project); + # Find the last selected reviewer for the rotation group and author, if one exists. + my $author_phid = $revision->author->phid; + my $last_reviewer_phid = find_last_reviewer_phid($project, $author_phid); INFO('Last reviewer phid found: ' . ($last_reviewer_phid ? $last_reviewer_phid : 'None')); @@ -439,7 +440,7 @@ PROJECT: foreach my $project (@review_projects) { INFO('Considering candidate reviewer: ' . $member->name); # Skip this member if they were the last one picked - if ($member->phid eq $last_reviewer_phid) { + if ($last_reviewer_phid && $member->phid eq $last_reviewer_phid) { INFO('Already the last reviewer picked, skipping: ' . $member->name); next; } @@ -457,6 +458,24 @@ PROJECT: foreach my $project (@review_projects) { } } + # Fallback: retry without skipping the last reviewer. Being the last + # reviewer is a preference for rotation, not a hard exclusion. This + # prevents the "not found" error when the group is small and other + # members are unavailable (Bug 2003867). + if ($last_reviewer_phid) { + INFO('No reviewer found, retrying without skipping last reviewer'); + foreach my $member (@project_members) { + if ($member->phid ne $revision->author->phid + && $member->bugzilla_user->can_see_bug($revision->bug->id) + && $member->bugzilla_user->settings->{block_reviews}->{value} ne 'on') + { + INFO('Fallback: promoting member to reviewer: ' . $member->name); + set_new_reviewer($revision, $project, $member, $is_blocking, \@review_users); + next PROJECT; + } + } + } + # If we got to this point, we did not find a suitable reviewer so # we will leave a comment in the revision with explanation. First # we need to check to make sure we have not already added this comment. @@ -501,8 +520,8 @@ sub set_new_reviewer { $revision->add_subscriber($project->phid); # Store the data in the phab_reviewer_rotation table so they will be - # next time. - update_last_reviewer_phid($project, $member); + # next time. Track per author so each author gets independent rotation. + update_last_reviewer_phid($project, $revision->author->phid, $member); # Add new reviewer to review users list in case they are also # a member of the next review rotation group. @@ -533,25 +552,26 @@ sub rotate_reviewer_list { } sub find_last_reviewer_phid { - my ($project) = @_; - INFO('Retrieving last reviewer for project ' . $project->phid); + my ($project, $author_phid) = @_; + INFO('Retrieving last reviewer for project ' . $project->phid + . ' and author ' . $author_phid); return Bugzilla->dbh->selectrow_array( - 'SELECT user_phid FROM phab_reviewer_rotation WHERE project_phid = ?', - undef, $project->phid); + 'SELECT user_phid FROM phab_reviewer_rotation WHERE project_phid = ? AND author_phid = ?', + undef, $project->phid, $author_phid); } sub update_last_reviewer_phid { - my ($project, $reviewer) = @_; + my ($project, $author_phid, $reviewer) = @_; my $dbh = Bugzilla->dbh; - INFO( - 'Updating last reviewer ' . $reviewer->name . ' for project ' . $project->name); + INFO('Updating last reviewer ' . $reviewer->name + . ' for project ' . $project->name + . ' and author ' . $author_phid); - $dbh->do('DELETE FROM phab_reviewer_rotation WHERE project_phid = ?', - undef, $project->phid); $dbh->do( - 'INSERT INTO phab_reviewer_rotation (project_phid, user_phid) VALUES (?, ?)', - undef, $project->phid, $reviewer->phid); + 'INSERT INTO phab_reviewer_rotation (project_phid, author_phid, user_phid) VALUES (?, ?, ?) ' + . 'ON DUPLICATE KEY UPDATE user_phid = VALUES(user_phid)', + undef, $project->phid, $author_phid, $reviewer->phid); } sub get_stack_reviewers { From 9f2dc4d32feb8ba70657eb7ccec44fa545e47bcc Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 12 Mar 2026 23:01:13 -0400 Subject: [PATCH 2/3] Copilot suggested fixes --- extensions/PhabBugz/Extension.pm | 6 ++++++ extensions/PhabBugz/lib/Util.pm | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index 8aff17ce30..676c4ad66c 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -195,6 +195,12 @@ sub install_update_db { $dbh->bz_add_column('phab_reviewer_rotation', 'author_phid', {TYPE => 'VARCHAR(255)', NOTNULL => 1, DEFAULT => "''"}); $dbh->do('DELETE FROM phab_reviewer_rotation'); + } + + # Add unique index separately so it is also created if a previous run + # added the column but crashed before reaching the bz_add_index call. + if (!$dbh->bz_index_info('phab_reviewer_rotation', 'phab_reviewer_rotation_idx')) { + $dbh->do('DELETE FROM phab_reviewer_rotation'); $dbh->bz_add_index('phab_reviewer_rotation', 'phab_reviewer_rotation_idx', {FIELDS => ['project_phid', 'author_phid'], TYPE => 'UNIQUE'}); } diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 81f7f1f142..533d69669c 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -539,6 +539,11 @@ sub rotate_reviewer_list { = grep { $project_members[$_]->phid eq $last_reviewer_phid } 0..$#project_members; + # If the last reviewer is no longer a member of the group (e.g. left the + # project), treat it as if there is no previous reviewer and return the + # list unrotated. + return @project_members if !defined $index; + # Rotate list my @rotated_members = ( @project_members[$index..$#project_members], @@ -570,8 +575,8 @@ sub update_last_reviewer_phid { $dbh->do( 'INSERT INTO phab_reviewer_rotation (project_phid, author_phid, user_phid) VALUES (?, ?, ?) ' - . 'ON DUPLICATE KEY UPDATE user_phid = VALUES(user_phid)', - undef, $project->phid, $author_phid, $reviewer->phid); + . 'ON DUPLICATE KEY UPDATE user_phid = ?', + undef, $project->phid, $author_phid, $reviewer->phid, $reviewer->phid); } sub get_stack_reviewers { From 34fdd8b0f8d7232379966d24a7f09fc82bec29b6 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Fri, 13 Mar 2026 16:38:37 -0400 Subject: [PATCH 3/3] Changed based on connors comments in Slack --- extensions/PhabBugz/lib/Util.pm | 35 +++++++-------------------------- 1 file changed, 7 insertions(+), 28 deletions(-) diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 533d69669c..a5dac133b8 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -435,16 +435,12 @@ PROJECT: foreach my $project (@review_projects) { } } - # Loop through all members and pick the next one in line after last selected + # Loop through all members and pick the next eligible reviewer. + # The list has been rotated so the last reviewer is at the end, + # meaning they are only selected if no other eligible reviewer is available. foreach my $member (@project_members) { INFO('Considering candidate reviewer: ' . $member->name); - # Skip this member if they were the last one picked - if ($last_reviewer_phid && $member->phid eq $last_reviewer_phid) { - INFO('Already the last reviewer picked, skipping: ' . $member->name); - next; - } - # Here we look to see if they can see the bug, and they are not set to away # (not accepting reviews). If both are positive, we have found our reviewer # and exit the loop. @@ -458,24 +454,6 @@ PROJECT: foreach my $project (@review_projects) { } } - # Fallback: retry without skipping the last reviewer. Being the last - # reviewer is a preference for rotation, not a hard exclusion. This - # prevents the "not found" error when the group is small and other - # members are unavailable (Bug 2003867). - if ($last_reviewer_phid) { - INFO('No reviewer found, retrying without skipping last reviewer'); - foreach my $member (@project_members) { - if ($member->phid ne $revision->author->phid - && $member->bugzilla_user->can_see_bug($revision->bug->id) - && $member->bugzilla_user->settings->{block_reviews}->{value} ne 'on') - { - INFO('Fallback: promoting member to reviewer: ' . $member->name); - set_new_reviewer($revision, $project, $member, $is_blocking, \@review_users); - next PROJECT; - } - } - } - # If we got to this point, we did not find a suitable reviewer so # we will leave a comment in the revision with explanation. First # we need to check to make sure we have not already added this comment. @@ -544,10 +522,11 @@ sub rotate_reviewer_list { # list unrotated. return @project_members if !defined $index; - # Rotate list + # Rotate the list so the last reviewer is at the end, meaning they will + # only be selected if no other eligible reviewer is available. my @rotated_members = ( - @project_members[$index..$#project_members], - @project_members[0..$index - 1] + @project_members[$index + 1..$#project_members], + @project_members[0..$index] ); INFO(