Merge "Allow to move reviewers to CC"

Documentation/rest-api-changes.txt

@@ -3088,6 +3088,12 @@ Adds one user or all members of one group as reviewer to the change.
 The reviewer to be added to the change must be provided in the request
 body as a link:#reviewer-input[ReviewerInput] entity.
 
+Users can be moved from reviewer to CC and vice versa. This means if a
+user is added as CC that is already a reviewer on the change, the
+reviewer state of that user is updated to CC. If a user that is already
+a CC on the change is added as reviewer, the reviewer state of that
+user is updated to reviewer.
+
 .Request
 ----
   POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/reviewers HTTP/1.0
@@ -3131,6 +3137,12 @@ members are added as reviewer.
 If a group with many members is added as reviewer a confirmation may be
 required.
 
+If a group is added as CC and members of this group are already
+reviewers on the change, these members stay reviewers on the change
+(they are not downgraded to CC). However if a group is added as
+reviewer, all group members become reviewer of the change, even if they
+have been added as CC before.
+
 .Request
 ----
   POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/reviewers HTTP/1.0

java/com/google/gerrit/server/ApprovalsUtil.java

@@ -239,17 +239,28 @@ public class ApprovalsUtil {
    * @param notes change notes.
    * @param update change update.
    * @param wantCCs accounts to CC.
+   * @param keepExistingReviewers whether provided accounts that are already reviewer should be kept
+   *     as reviewer or be downgraded to CC
    * @return whether a change was made.
    */
   public Collection<Account.Id> addCcs(
-      ChangeNotes notes, ChangeUpdate update, Collection<Account.Id> wantCCs) {
-    return addCcs(update, wantCCs, notes.load().getReviewers());
+      ChangeNotes notes,
+      ChangeUpdate update,
+      Collection<Account.Id> wantCCs,
+      boolean keepExistingReviewers) {
+    return addCcs(update, wantCCs, notes.load().getReviewers(), keepExistingReviewers);
   }
 
   private Collection<Account.Id> addCcs(
-      ChangeUpdate update, Collection<Account.Id> wantCCs, ReviewerSet existingReviewers) {
+      ChangeUpdate update,
+      Collection<Account.Id> wantCCs,
+      ReviewerSet existingReviewers,
+      boolean keepExistingReviewers) {
     Set<Account.Id> need = new LinkedHashSet<>(wantCCs);
-    need.removeAll(existingReviewers.all());
+    need.removeAll(existingReviewers.byState(CC));
+    if (keepExistingReviewers) {
+      need.removeAll(existingReviewers.byState(REVIEWER));
+    }
     need.removeAll(update.getReviewers().keySet());
     for (Account.Id account : need) {
       update.putReviewer(account, CC);

java/com/google/gerrit/server/change/AddReviewersOp.java

@@ -64,10 +64,14 @@ public class AddReviewersOp implements BatchUpdateOp {
      * @param accountIds account IDs to add.
      * @param addresses email addresses to add.
      * @param state resulting reviewer state.
+     * @param forGroup whether this reviewer addition adds accounts for a group
      * @return batch update operation.
      */
     AddReviewersOp create(
-        Set<Account.Id> accountIds, Collection<Address> addresses, ReviewerState state);
+        Set<Account.Id> accountIds,
+        Collection<Address> addresses,
+        ReviewerState state,
+        boolean forGroup);
   }
 
   @AutoValue
@@ -107,6 +111,7 @@ public class AddReviewersOp implements BatchUpdateOp {
   private final Set<Account.Id> accountIds;
   private final Collection<Address> addresses;
   private final ReviewerState state;
+  private final boolean forGroup;
 
   // Unlike addedCCs, addedReviewers is a PatchSetApproval because the AddReviewerResult returned
   // via the REST API is supposed to include vote information.
@@ -130,7 +135,8 @@ public class AddReviewersOp implements BatchUpdateOp {
       AddReviewersEmail addReviewersEmail,
       @Assisted Set<Account.Id> accountIds,
       @Assisted Collection<Address> addresses,
-      @Assisted ReviewerState state) {
+      @Assisted ReviewerState state,
+      @Assisted boolean forGroup) {
     checkArgument(state == REVIEWER || state == CC, "must be %s or %s: %s", REVIEWER, CC, state);
     this.approvalsUtil = approvalsUtil;
     this.psUtil = psUtil;
@@ -142,6 +148,7 @@ public class AddReviewersOp implements BatchUpdateOp {
     this.accountIds = accountIds;
     this.addresses = addresses;
     this.state = state;
+    this.forGroup = forGroup;
   }
 
   // TODO(dborowitz): This mutable setter is ugly, but a) it's less ugly than adding boolean args
@@ -162,7 +169,7 @@ public class AddReviewersOp implements BatchUpdateOp {
       if (state == CC) {
         addedCCs =
             approvalsUtil.addCcs(
-                ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), accountIds);
+                ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), accountIds, forGroup);
       } else {
         addedReviewers =
             approvalsUtil.addReviewers(

java/com/google/gerrit/server/change/ReviewerAdder.java

@@ -237,7 +237,8 @@ public class ReviewerAdder {
         revision.getUser(),
         ImmutableSet.of(user.getAccountId()),
         null,
-        true);
+        true,
+        false);
   }
 
   @Nullable
@@ -260,7 +261,13 @@ public class ReviewerAdder {
 
     if (isValidReviewer(notes.getChange().getDest(), reviewerUser.getAccount())) {
       return new ReviewerAddition(
-          input, notes, user, ImmutableSet.of(reviewerUser.getAccountId()), null, exactMatchFound);
+          input,
+          notes,
+          user,
+          ImmutableSet.of(reviewerUser.getAccountId()),
+          null,
+          exactMatchFound,
+          false);
     }
     return fail(
         input,
@@ -344,7 +351,7 @@ public class ReviewerAdder {
       }
     }
 
-    return new ReviewerAddition(input, notes, user, reviewers, null, true);
+    return new ReviewerAddition(input, notes, user, reviewers, null, true, true);
   }
 
   @Nullable
@@ -366,7 +373,7 @@ public class ReviewerAdder {
           FailureType.NOT_FOUND,
           MessageFormat.format(ChangeMessages.get().reviewerInvalid, input.reviewer));
     }
-    return new ReviewerAddition(input, notes, user, null, ImmutableList.of(adr), true);
+    return new ReviewerAddition(input, notes, user, null, ImmutableList.of(adr), true, false);
   }
 
   private boolean isValidReviewer(BranchNameKey branch, Account member)
@@ -421,7 +428,8 @@ public class ReviewerAdder {
         CurrentUser caller,
         @Nullable Iterable<Account.Id> reviewers,
         @Nullable Iterable<Address> reviewersByEmail,
-        boolean exactMatchFound) {
+        boolean exactMatchFound,
+        boolean forGroup) {
       checkArgument(
           reviewers != null || reviewersByEmail != null,
           "must have either reviewers or reviewersByEmail");
@@ -435,7 +443,7 @@ public class ReviewerAdder {
       this.reviewersByEmail =
           reviewersByEmail == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewersByEmail);
       this.caller = caller.asIdentifiedUser();
-      op = addReviewersOpFactory.create(this.reviewers, this.reviewersByEmail, state());
+      op = addReviewersOpFactory.create(this.reviewers, this.reviewersByEmail, state(), forGroup);
       this.exactMatchFound = exactMatchFound;
     }
 

javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.rest.change;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
 import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
 import static com.google.gerrit.extensions.client.ReviewerState.CC;
 import static com.google.gerrit.extensions.client.ReviewerState.REMOVED;
@@ -755,6 +756,81 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
     assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).ccs).isEmpty();
   }
 
+  @Test
+  public void moveCcToReviewer() throws Exception {
+    // Create a change and add 'user' as CC.
+    String changeId = createChange().getChangeId();
+    AddReviewerInput reviewerInput = new AddReviewerInput();
+    reviewerInput.reviewer = user.email();
+    reviewerInput.state = ReviewerState.CC;
+    gApi.changes().id(changeId).addReviewer(reviewerInput);
+
+    // Verify that 'user' is a CC on the change and that there are no reviewers.
+    ChangeInfo c = gApi.changes().id(changeId).get();
+    Collection<AccountInfo> ccs = c.reviewers.get(CC);
+    assertThat(ccs).isNotNull();
+    assertThat(ccs).hasSize(1);
+    assertThat(ccs.iterator().next()._accountId).isEqualTo(user.id().get());
+    assertThat(c.reviewers.get(REVIEWER)).isNull();
+
+    // Move 'user' from CC to reviewer.
+    gApi.changes().id(changeId).addReviewer(user.id().toString());
+
+    // Verify that 'user' is a reviewer on the change now and that there are no CCs.
+    c = gApi.changes().id(changeId).get();
+    Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
+    assertThat(reviewers).isNotNull();
+    assertThat(reviewers).hasSize(1);
+    assertThat(reviewers.iterator().next()._accountId).isEqualTo(user.id().get());
+    assertThat(c.reviewers.get(CC)).isNull();
+  }
+
+  @Test
+  public void moveReviewerToCc() throws Exception {
+    // Allow everyone to approve changes.
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, 2))
+        .update();
+
+    // Create a change and add 'user' as reviewer.
+    String changeId = createChange().getChangeId();
+    gApi.changes().id(changeId).addReviewer(user.id().toString());
+
+    // Verify that 'user' is a reviewer on the change and that there are no CCs.
+    ChangeInfo c = gApi.changes().id(changeId).get();
+    Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
+    assertThat(reviewers).isNotNull();
+    assertThat(reviewers).hasSize(1);
+    assertThat(reviewers.iterator().next()._accountId).isEqualTo(user.id().get());
+    assertThat(c.reviewers.get(CC)).isNull();
+
+    // Let 'user' approve the change and verify that the change has the approval.
+    requestScopeOperations.setApiUser(user.id());
+    approve(changeId);
+    c = gApi.changes().id(changeId).get();
+    assertThat(c.labels.get("Code-Review").approved._accountId).isEqualTo(user.id().get());
+
+    // Move 'user' from reviewer to CC.
+    requestScopeOperations.setApiUser(admin.id());
+    AddReviewerInput reviewerInput = new AddReviewerInput();
+    reviewerInput.reviewer = user.id().toString();
+    reviewerInput.state = CC;
+    gApi.changes().id(changeId).addReviewer(reviewerInput);
+
+    // Verify that 'user' is a CC on the change now and that there are no reviewers.
+    c = gApi.changes().id(changeId).get();
+    Collection<AccountInfo> ccs = c.reviewers.get(CC);
+    assertThat(ccs).isNotNull();
+    assertThat(ccs).hasSize(1);
+    assertThat(ccs.iterator().next()._accountId).isEqualTo(user.id().get());
+    assertThat(c.reviewers.get(REVIEWER)).isNull();
+
+    // Verify that the approval of 'user' is still there.
+    assertThat(c.labels.get("Code-Review").approved._accountId).isEqualTo(user.id().get());
+  }
+
   private void assertThatUserIsOnlyReviewer(String changeId) throws Exception {
     AccountInfo userInfo = new AccountInfo(user.fullName(), user.getEmailAddress().getEmail());
     userInfo._accountId = user.id().get();