diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 479e34b4b2..de907cac91 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/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(); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java index 1ef6337a07..c9cb06ff67 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java +++ b/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); - assertReviewers(c, CC); + if (notesMigration.readChanges()) { + 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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index ce4476679d..2decd12819 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/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 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)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index 420c05cf17..f0af5da953 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/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 {