opendev/gerrit
Code Issues Proposed changes

Only send one newchange email from PostReviewer

Browse Source 
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: If45290b875b0f5c7f52db9a824ab853b63c8f7a9
This commit is contained in:
Wyatt Allen
2016-10-28 14:31:39 -07:00
committed by David Pursehouse
parent e858c54e37
commit d64564bafd
3 changed files with 45 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -451,35 +451,29 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
// 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.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.
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);
if (notesMigration.readChanges()) {
assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
assertThat(m.body()).contains(admin.fullName + " has uploaded a new change for review.");
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.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.");
}
// 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

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

@@ -47,6 +47,7 @@ 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.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.RestApiException;
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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
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 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
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 {

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

@@ -364,7 +364,9 @@ public class PostReviewers
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 @@ public class PostReviewers
}
}
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 @@ public class PostReviewers
// 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());