From 92cba85254d4b2977b05ae4c836d19cbbfd93d16 Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Fri, 28 Oct 2016 14:31:39 -0700 Subject: [PATCH] Only send one newchange email from PostReviewer 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 --- .../rest/change/ChangeReviewersIT.java | 39 +++++++------------ .../gerrit/server/change/PostReview.java | 22 +++++++++++ .../gerrit/server/change/PostReviewers.java | 18 ++++++--- 3 files changed, 48 insertions(+), 31 deletions(-) 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 aa7e8647f7..8bb5d0269b 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 @@ -288,35 +288,22 @@ public class ChangeReviewersIT extends AbstractDaemonTest { // Verify emails were sent to added reviewers. List 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 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 aa35da861f..6e2d51a613 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 @@ -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 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 for (PostReviewers.Addition reviewerResult : reviewerResults) { reviewerResult.gatherResults(); } + + emailReviewers(revision.getChange(), reviewerResults, input.notify); } return Response.ok(output); } + private void emailReviewers(Change change, + List reviewerAdditions, NotifyHandling notify) { + List to = new ArrayList<>(); + List 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 89e8e11818..16c0dd6473 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 @@ -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 accounts = Lists.transform(addedReviewers, + new Function() { + 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 added, + public void emailReviewers(Change change, Collection added, Collection 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 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 toCopy = Lists.newArrayListWithCapacity(copied.size());