opendev/gerrit
Code Issues Proposed changes

Only send one newchange email from PostReviewer

Browse Source 
This is a revision of change If45290b87 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
This commit is contained in:
Wyatt Allen
2016-10-28 14:31:39 -07:00
committed by David Ostrovsky
parent 21fcf7d567
commit 92cba85254
3 changed files with 48 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -288,35 +288,22 @@ 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.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\n");
}
@Test

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

@@ -45,6 +45,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
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.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;
@@ -54,6 +55,8 @@ 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.CommentRange;
import com.google.gerrit.reviewdb.client.Patch;
@@ -181,6 +184,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);
@@ -218,11 +224,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 {

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
import com.google.common.base.Function;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
@@ -359,7 +360,14 @@ public class PostReviewers
if (addedCCs == null) {
addedCCs = new ArrayList<>();
}
emailReviewers(rsrc.getChange(), addedReviewers, addedCCs, notify);
List<Account.Id> accounts = Lists.transform(addedReviewers,
new Function<PatchSetApproval, Account.Id>() {
public Account.Id apply(PatchSetApproval psa) {
return psa.getAccountId();
}
});
emailReviewers(rsrc.getChange(), accounts, addedCCs, notify);
if (!addedReviewers.isEmpty()) {
for (PatchSetApproval psa : addedReviewers) {
Account account = accountCache.get(psa.getAccountId()).getAccount();
@@ -371,7 +379,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;
@@ -382,9 +390,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());