Implicitly CC user when leaving non-voting review

If a reviewing user isn't already in the ReviewerSet of a change, and
they post a message without voting, they expect to be notified if anyone
replies to their review. In this circumstance we can simply add the user
to the CC list.

Bug: Issue 4638
Change-Id: Ia5e6b9791e5b341bbc524dd421fa7eb32dbc733d

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -28,6 +28,7 @@ import static com.google.gerrit.server.project.Util.blockLabel;
 import static com.google.gerrit.server.project.Util.category;
 import static com.google.gerrit.server.project.Util.value;
 import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.toList;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
@@ -54,6 +55,7 @@ import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ApprovalInfo;
@@ -1001,6 +1003,65 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
   }
 
+  @Test
+  public void implicitlyCcOnNonVotingReview() throws Exception {
+    PushOneCommit.Result r = createChange();
+    setApiUser(user);
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(new ReviewInput());
+
+    ChangeInfo c = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    // If we're not reading from NoteDb, then the CCed user will be returned
+    // in the REVIEWER state.
+    ReviewerState state = notesMigration.readChanges() ? CC : REVIEWER;
+    assertThat(c.reviewers.get(state).stream().map(ai -> ai._accountId)
+        .collect(toList())).containsExactly(user.id.get());
+  }
+
+  @Test
+  public void implicitlyAddReviewerOnVotingReview() throws Exception {
+    PushOneCommit.Result r = createChange();
+    setApiUser(user);
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(ReviewInput.recommend().message("LGTM"));
+
+    ChangeInfo c = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(c.reviewers.get(REVIEWER).stream().map(ai -> ai._accountId)
+        .collect(toList())).containsExactly(user.id.get());
+
+    // Further test: remove the vote, then comment again. The user should be
+    // implicitly re-added to the ReviewerSet, as a CC if we're using NoteDb.
+    setApiUser(admin);
+    gApi.changes()
+        .id(r.getChangeId())
+        .reviewer(user.getId().toString())
+        .remove();
+    c = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    assertThat(c.reviewers.values()).isEmpty();
+
+    setApiUser(user);
+    gApi.changes()
+        .id(r.getChangeId())
+        .revision(r.getCommit().name())
+        .review(new ReviewInput().message("hi"));
+    c = gApi.changes()
+        .id(r.getChangeId())
+        .get();
+    ReviewerState state = notesMigration.readChanges() ? CC : REVIEWER;
+    assertThat(c.reviewers.get(state).stream().map(ai -> ai._accountId)
+        .collect(toList())).containsExactly(user.id.get());
+  }
+
   @Test
   public void addReviewerToClosedChange() throws Exception {
     PushOneCommit.Result r = createChange();

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java

@@ -277,12 +277,19 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
     assertThat(result.labels).isNull();
     assertThat(result.reviewers).isNull();
 
-    // Verify user is not added as reviewer.
+    // Verify user is added to CC list.
     ChangeInfo c = gApi.changes()
         .id(r.getChangeId())
         .get();
-    assertReviewers(c, REVIEWER);
+    if (notesMigration.readChanges()) {
-    assertReviewers(c, CC);
+      assertReviewers(c, REVIEWER);
+      assertReviewers(c, CC, user);
+    } else {
+      // If we aren't reading from NoteDb, the user will appear as a
+      // reviewer.
+      assertReviewers(c, REVIEWER, user);
+      assertReviewers(c, CC);
+    }
   }
 
   @Test

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -48,8 +48,10 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
 import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
 import com.google.gerrit.extensions.api.changes.ReviewResult;
+import com.google.gerrit.extensions.api.changes.ReviewerInfo;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
@@ -229,11 +231,48 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
 
     try (BatchUpdate bu = batchUpdateFactory.create(db.get(),
           revision.getChange().getProject(), revision.getUser(), ts)) {
+      Account.Id id = bu.getUser().getAccountId();
+      boolean ccOrReviewer = input.labels != null;
+
+      if (!ccOrReviewer) {
+        // Check if user was already CCed or reviewing prior to this review.
+        ReviewerSet currentReviewers = approvalsUtil.getReviewers(
+            db.get(), revision.getChangeResource().getNotes());
+        ccOrReviewer = currentReviewers.all().contains(id);
+      }
+
       // Apply reviewer changes first. Revision emails should be sent to the
-      // updated set of reviewers.
+      // updated set of reviewers. Also keep track of whether the user added
+      // themselves as a reviewer or to the CC list.
       for (PostReviewers.Addition reviewerResult : reviewerResults) {
         bu.addOp(revision.getChange().getId(), reviewerResult.op);
+        if (!ccOrReviewer && reviewerResult.result.reviewers != null) {
+          for (ReviewerInfo reviewerInfo : reviewerResult.result.reviewers) {
+            if (id.equals(reviewerInfo._accountId)) {
+              ccOrReviewer = true;
+              break;
+            }
+          }
+        }
+        if (!ccOrReviewer && reviewerResult.result.ccs != null) {
+          for (AccountInfo accountInfo : reviewerResult.result.ccs) {
+            if (id.equals(accountInfo._accountId)) {
+              ccOrReviewer = true;
+              break;
+            }
+          }
+        }
       }
+
+      if (!ccOrReviewer) {
+        // User posting this review isn't currently in the reviewer or CC list,
+        // isn't being explicitly added, and isn't voting on any label.
+        // Automatically CC them on this change so they receive replies.
+        PostReviewers.Addition selfAddition =
+            postReviewers.ccCurrentUser(bu.getUser(), revision);
+        bu.addOp(revision.getChange().getId(), selfAddition.op);
+      }
+
       bu.addOp(
           revision.getChange().getId(),
           new Op(revision.getPatchSet().getId(), input, reviewerResults));

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java

@@ -39,6 +39,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.account.AccountCache;
@@ -177,6 +178,13 @@ public class PostReviewers
         input.state(), input.notify);
   }
 
+  Addition ccCurrentUser(CurrentUser user, RevisionResource revision) {
+    return new Addition(
+        user.getUserName(), revision.getChangeResource(),
+        ImmutableMap.of(user.getAccountId(), revision.getControl()),
+        CC, NotifyHandling.NONE);
+  }
+
   private Addition putAccount(String reviewer, ReviewerResource rsrc,
       ReviewerState state, NotifyHandling notify)
       throws UnprocessableEntityException {