Bug 2003867 - reviewer rotation seems to always assign patches to the same reviewers within the group

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,26 @@ 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');
+  }
+
+  # 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'});
+  }
+}
+
 sub install_filesystem {
   my ($self, $args) = @_;
   my $files = $args->{'files'};

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'));
 
@@ -434,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 ($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.
@@ -501,8 +498,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.
@@ -520,10 +517,16 @@ sub rotate_reviewer_list {
     = grep { $project_members[$_]->phid eq $last_reviewer_phid }
     0..$#project_members;
 
-  # Rotate list
+  # 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 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(
@@ -533,25 +536,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 = ?',
+    undef, $project->phid, $author_phid, $reviewer->phid, $reviewer->phid);
 }
 
 sub get_stack_reviewers {