Only send one newchange email from PostReviewer
This is a revision of change 90390 with updated tests that no longer
need to take NoteDB into account separately.
The PostReview REST endpoint translates each new reviewer in the request
into invocations of the PostReviewers REST endpoint, which would send a
newchange email each time a reviewer is successfully added. Because the
PostReview endpoint allows several Reviewers and CC'd users in batch,
prevent the PostReviewers sub-calls from sending emails, and send one
newchange email at the end.
Tests are updated to reflect fewer emails being sent and to reflect CC'd
user names no-longer appearing in the email salutation.
Bug: Issue 4563
Change-Id: I1cfd56ac1a2d107b9f6aae58ed056a1a458410af
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 b1b3f2a..1ef6337 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
@@ -451,35 +451,22 @@
// Verify emails were sent to added reviewers.
List<Message> messages = sender.getMessages();
- assertThat(messages).hasSize(3);
- // First email to user.
+ assertThat(messages).hasSize(2);
+
Message m = messages.get(0);
- if (notesMigration.readChanges()) {
- assertThat(m.rcpt()).containsExactly(user.emailAddress);
- } else {
- assertThat(m.rcpt()).containsExactly(
- user.emailAddress, observer.emailAddress);
- }
+ assertThat(m.rcpt())
+ .containsExactly(user.emailAddress,observer.emailAddress);
+ assertThat(m.body())
+ .contains(admin.fullName + " has posted comments on this change.");
+ assertThat(m.body())
+ .contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
+ assertThat(m.body()).contains("Patch Set 1: Code-Review+2");
+
+ m = messages.get(1);
+ assertThat(m.rcpt())
+ .containsExactly(user.emailAddress, observer.emailAddress);
assertThat(m.body()).contains("Hello " + user.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
- assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
- // Second email to reviewer and observer.
- m = messages.get(1);
- if (notesMigration.readChanges()) {
- assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
- assertThat(m.body()).contains(admin.fullName + " has uploaded a new change for review.");
- } else {
- assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
- assertThat(m.body()).contains("Hello " + observer.fullName + ",\n");
- assertThat(m.body()).contains("I'd like you to do a code review.");
- }
-
- // Third email is review to user and observer.
- m = messages.get(2);
- assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
- assertThat(m.body()).contains(admin.fullName + " has posted comments on this change.");
- assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
- assertThat(m.body()).contains("Patch Set 1: Code-Review+2");
}
@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 92617f3..2d0fab3 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
@@ -47,6 +47,7 @@
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.client.ReviewerState;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -57,6 +58,7 @@
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Comment;
@@ -198,6 +200,9 @@
if (input.reviewers != null) {
reviewerJsonResults = Maps.newHashMap();
for (AddReviewerInput reviewerInput : input.reviewers) {
+ // Prevent notifications because setting reviewers is batched.
+ reviewerInput.notify = NotifyHandling.NONE;
+
PostReviewers.Addition result = postReviewers.prepareApplication(
revision.getChangeResource(), reviewerInput);
reviewerJsonResults.put(reviewerInput.reviewer, result.result);
@@ -235,11 +240,27 @@
for (PostReviewers.Addition reviewerResult : reviewerResults) {
reviewerResult.gatherResults();
}
+
+ emailReviewers(revision.getChange(), reviewerResults, input.notify);
}
return Response.ok(output);
}
+ private void emailReviewers(Change change,
+ List<PostReviewers.Addition> reviewerAdditions, NotifyHandling notify) {
+ List<Account.Id> to = new ArrayList<>();
+ List<Account.Id> cc = new ArrayList<>();
+ for (PostReviewers.Addition addition : reviewerAdditions) {
+ if (addition.op.state == ReviewerState.REVIEWER) {
+ to.addAll(addition.op.reviewers.keySet());
+ } else if (addition.op.state == ReviewerState.CC) {
+ cc.addAll(addition.op.reviewers.keySet());
+ }
+ }
+ postReviewers.emailReviewers(change, to, cc, notify);
+ }
+
private RevisionResource onBehalfOf(RevisionResource rev, ReviewInput in)
throws BadRequestException, AuthException, UnprocessableEntityException,
OrmException {
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 30aa8c5..420c05c 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
@@ -364,7 +364,9 @@
if (addedCCs == null) {
addedCCs = new ArrayList<>();
}
- emailReviewers(rsrc.getChange(), addedReviewers, addedCCs, notify);
+ emailReviewers(rsrc.getChange(),
+ Lists.transform(addedReviewers, r -> r.getAccountId()), addedCCs,
+ notify);
if (!addedReviewers.isEmpty()) {
List<Account> reviewers = Lists.transform(addedReviewers,
psa -> accountCache.get(psa.getAccountId()).getAccount());
@@ -375,7 +377,7 @@
}
}
- private void emailReviewers(Change change, List<PatchSetApproval> added,
+ public void emailReviewers(Change change, Collection<Account.Id> added,
Collection<Account.Id> copied, NotifyHandling notify) {
if (added.isEmpty() && copied.isEmpty()) {
return;
@@ -386,9 +388,9 @@
// The user knows they added themselves, don't bother emailing them.
List<Account.Id> toMail = Lists.newArrayListWithCapacity(added.size());
Account.Id userId = user.get().getAccountId();
- for (PatchSetApproval psa : added) {
- if (!psa.getAccountId().equals(userId)) {
- toMail.add(psa.getAccountId());
+ for (Account.Id id : added) {
+ if (!id.equals(userId)) {
+ toMail.add(id);
}
}
List<Account.Id> toCopy = Lists.newArrayListWithCapacity(copied.size());
