When added as author, be added as CC and not reviewer
Adding people as reviewer creates more noise, since they are
automatically added to the attention set, and they are also receiving an
email with expectation to review the change (although the owner might
not have that expectation). In the case owner does, they can still add
them as reviewer manually.
Had to fix some outdated tests.
Change-Id: I908f79315b109075ab0196d2ffadc7fd354cc126
diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java
index 3d986d2..5d55b4d 100644
--- a/java/com/google/gerrit/server/change/ReviewerAdder.java
+++ b/java/com/google/gerrit/server/change/ReviewerAdder.java
@@ -19,7 +19,6 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
-import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
@@ -140,12 +139,12 @@
}
logger.atFine().log(
- "Adding account %d from author/committer identity of commit %s as reviewer to change %d",
+ "Adding account %d from author/committer identity of commit %s as cc to change %d",
accountId.get(), commitId.name(), change.getChangeId());
InternalAddReviewerInput in = new InternalAddReviewerInput();
in.reviewer = accountId.toString();
- in.state = REVIEWER;
+ in.state = CC;
in.notify = notify;
in.otherFailureBehavior = FailureBehavior.IGNORE;
return Optional.of(in);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 496a2b4..ccfa60e 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -1499,19 +1499,19 @@
assertThat(commit.author.email).isEqualTo(user.email());
assertThat(commit.committer.email).isEqualTo(user.email());
- // check that the author/committer was added as reviewer
- Collection<AccountInfo> reviewers = change.reviewers.get(REVIEWER);
+ // check that the author/committer was added as cc
+ Collection<AccountInfo> reviewers = change.reviewers.get(CC);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId).isEqualTo(user.id().get());
- assertThat(change.reviewers.get(CC)).isNull();
+ assertThat(change.reviewers.get(REVIEWER)).isNull();
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
Message m = messages.get(0);
assertThat(m.from().name()).isEqualTo("Administrator (Code Review)");
assertThat(m.rcpt()).containsExactly(user.getNameEmail());
- assertThat(m.body()).contains("I'd like you to do a code review");
+ assertThat(m.body()).contains("has uploaded this change for review");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertMailReplyTo(m, admin.email());
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 2f9530c..839b051 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -872,7 +872,7 @@
String changeId = project.get() + "~master~" + result.getChangeId();
// 'user' cherry-picks the change to a new branch, the source change's author/committer('admin')
- // will be added as a reviewer of the newly created change.
+ // will be added as cc of the newly created change.
requestScopeOperations.setApiUser(user.id());
CherryPickInput input = new CherryPickInput();
input.message = "it goes to a new branch";
@@ -882,7 +882,7 @@
input.notify = NotifyHandling.ALL;
sender.clear();
gApi.changes().id(changeId).current().cherryPick(input);
- assertNotifyTo(admin);
+ assertNotifyCc(admin);
// Disable the notification. 'admin' as a reviewer should not be notified any more.
input.destination = "branch-2";
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index c526e31..763e7b1 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -87,6 +87,7 @@
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.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CommentInfo;
@@ -1123,7 +1124,7 @@
String changeId = GitUtil.getChangeId(testRepo, c).get();
assertThat(getOwnerEmail(changeId)).isEqualTo(admin.email());
- assertThat(getReviewerEmails(changeId, ReviewerState.REVIEWER))
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC))
.containsExactly(user.email(), user2.email());
assertThat(sender.getMessages()).hasSize(1);
@@ -1148,7 +1149,7 @@
pushHead(testRepo, "refs/for/master");
assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email());
- assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER))
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.CC))
.containsExactly(user.email(), user2.email());
assertThat(sender.getMessages()).hasSize(1);
@@ -1180,27 +1181,20 @@
// Push this commit as "Administrator" (requires Forge Committer Identity)
pushHead(testRepo, "refs/for/master%l=Code-Review+1", false);
- // Expected Code-Review votes:
- // 1. 0 from User (committer):
- // When the committer is forged, the committer is automatically added as
- // reviewer, hence we expect a dummy 0 vote for the committer.
- // 2. +1 from Administrator (uploader):
- // On push Code-Review+1 was specified, hence we expect a +1 vote from
- // the uploader.
+ // Expected Code-Review vote:
+ // +1 from Administrator (uploader):
+ // On push Code-Review+1 was specified, hence we expect a +1 vote from the uploader. When the
+ // committer is forged, the committer is automatically added as cc, but that doesn't add votes
+ // (as opposted to being added as reviewer that adds a dummy +0 vote). We ensure there are no
+ // votes from the committer.
ChangeInfo ci =
get(GitUtil.getChangeId(testRepo, c).get(), DETAILED_LABELS, MESSAGES, DETAILED_ACCOUNTS);
LabelInfo cr = ci.labels.get("Code-Review");
- assertThat(cr.all).hasSize(2);
- int indexAdmin = admin.fullName().equals(cr.all.get(0).name) ? 0 : 1;
- int indexUser = indexAdmin == 0 ? 1 : 0;
- assertThat(cr.all.get(indexAdmin).name).isEqualTo(admin.fullName());
- assertThat(cr.all.get(indexAdmin).value.intValue()).isEqualTo(1);
- assertThat(cr.all.get(indexUser).name).isEqualTo(user.fullName());
- assertThat(cr.all.get(indexUser).value.intValue()).isEqualTo(0);
+ ApprovalInfo approvalInfo = Iterables.getOnlyElement(cr.all);
+ assertThat(approvalInfo.name).isEqualTo(admin.fullName());
+ assertThat(approvalInfo.value.intValue()).isEqualTo(1);
assertThat(Iterables.getLast(ci.messages).message)
.isEqualTo("Uploaded patch set 1: Code-Review+1.");
- // Check that the user who pushed the change was added as a reviewer since they added a vote
- assertThatUserIsOnlyReviewer(ci, admin);
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 9b12f29..b2a349e 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -1714,7 +1714,7 @@
.sent("newpatchset", sc)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer)
- .to(other)
+ .cc(other)
.cc(sc.ccer)
.cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL)
.noOneElse();
@@ -1730,7 +1730,7 @@
.sent("newpatchset", sc)
.notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner.
.to(sc.reviewer)
- .to(other)
+ .cc(other)
.cc(sc.ccer)
.cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL)
.noOneElse();
