Revert "Only send one newchange email from PostReviewer"

This change has broken Gerrit master build in NoteDB mode

This reverts commit d64564bafd.

Change-Id: Ic26165d8a017778c93c31af14021606b4895e5f3

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

@@ -451,29 +451,35 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
 
     // Verify emails were sent to added reviewers.
     List<Message> messages = sender.getMessages();
-    assertThat(messages).hasSize(2);
+    assertThat(messages).hasSize(3);
-
+    // First email to user.
     Message m = messages.get(0);
-    assertThat(m.rcpt())
+    if (notesMigration.readChanges()) {
-        .containsExactly(user.emailAddress,observer.emailAddress);
+      assertThat(m.rcpt()).containsExactly(user.emailAddress);
-    assertThat(m.body())
+    } else {
-        .contains(admin.fullName + " has posted comments on this change.");
+      assertThat(m.rcpt()).containsExactly(
-    assertThat(m.body())
+          user.emailAddress, observer.emailAddress);
-        .contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
+    }
-    assertThat(m.body()).contains("Patch Set 1: Code-Review+2");
+    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())
+      assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
-          .containsExactly(user.emailAddress, observer.emailAddress);
+      assertThat(m.body()).contains(admin.fullName + " has uploaded a new change for review.");
-      assertThat(m.body())
-          .contains(admin.fullName + " has uploaded a new change for review.");
     } else {
-      assertThat(m.rcpt())
+      assertThat(m.rcpt()).containsExactly(user.emailAddress, observer.emailAddress);
-          .containsExactly(user.emailAddress, observer.emailAddress);
+      assertThat(m.body()).contains("Hello " + observer.fullName + ",\n");
-      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

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

@@ -47,7 +47,6 @@ 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;
@@ -58,7 +57,6 @@ 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;
@@ -200,9 +198,6 @@ 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);
@@ -240,27 +235,11 @@ 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 {

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

@@ -364,9 +364,7 @@ public class PostReviewers
         if (addedCCs == null) {
           addedCCs = new ArrayList<>();
         }
-        emailReviewers(rsrc.getChange(),
+        emailReviewers(rsrc.getChange(), addedReviewers, addedCCs, notify);
-            Lists.transform(addedReviewers, r -> r.getAccountId()), addedCCs,
-            notify);
         if (!addedReviewers.isEmpty()) {
           List<Account> reviewers = Lists.transform(addedReviewers,
               psa -> accountCache.get(psa.getAccountId()).getAccount());
@@ -377,7 +375,7 @@ public class PostReviewers
     }
   }
 
-  public void emailReviewers(Change change, Collection<Account.Id> added,
+  private void emailReviewers(Change change, List<PatchSetApproval> added,
       Collection<Account.Id> copied, NotifyHandling notify) {
     if (added.isEmpty() && copied.isEmpty()) {
       return;
@@ -388,9 +386,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 (Account.Id id : added) {
+    for (PatchSetApproval psa : added) {
-      if (!id.equals(userId)) {
+      if (!psa.getAccountId().equals(userId)) {
-        toMail.add(id);
+        toMail.add(psa.getAccountId());
       }
     }
     List<Account.Id> toCopy = Lists.newArrayListWithCapacity(copied.size());