PostReviewOp: check all comments for stream events, including drafts

When checking what type of comments notification to perform to
stream-events, consider *ALL* published comments and not just the
ones passed as input.

This allows to fix a regression in v3.7 where the GUI stores
the patchset-level comments first as drafts and then publishes them.

Bug: Issue 16475
Release-Notes: Fix stream events message including comments created as draft
Change-Id: I2ac40bd8235fea5c0059e8ca4111d3b6bf266f5a

2 files changed, 49 insertions, 10 deletions

diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index 5ff0968fc6..9274f5246b 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -193,14 +193,14 @@ public class PostReviewOp implements BatchUpdateOp {
       // TODO(davido): Remove this workaround when patch set level comments are exposed in comment
       // added event. For backwards compatibility, patchset level comment has a higher priority
       // than change message and should be used as comment in comment added event.
-      if (in.comments != null && in.comments.containsKey(PATCHSET_LEVEL)) {
-        List<CommentInput> patchSetLevelComments = in.comments.get(PATCHSET_LEVEL);
-        if (patchSetLevelComments != null && !patchSetLevelComments.isEmpty()) {
-          CommentInput firstComment = patchSetLevelComments.get(0);
-          if (!Strings.isNullOrEmpty(firstComment.message)) {
-            comment = String.format("Patch Set %s:\n\n%s", psId.get(), firstComment.message);
-          }
-        }
+      String patchSetLevelComment =
+          comments.stream()
+              .filter(c -> c.key.filename.equals(PATCHSET_LEVEL))
+              .map(c -> Strings.nullToEmpty(c.message))
+              .collect(Collectors.joining("\n"))
+              .trim();
+      if (!patchSetLevelComment.isEmpty()) {
+        comment = String.format("Patch Set %s:\n\n%s", psId.get(), patchSetLevelComment);
       }
     }
     commentAdded.fire(
diff --git a/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java b/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java
index 13a9e0c1a0..80b8ff074b 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/StreamEventsIT.java
@@ -15,6 +15,7 @@
 package com.google.gerrit.acceptance.ssh;
 
 import static com.google.gerrit.acceptance.WaitUtil.waitUntil;
+import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
 
 import com.google.common.base.Splitter;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -22,13 +23,17 @@ import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.Sandboxed;
 import com.google.gerrit.acceptance.UseSsh;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
+import com.google.gerrit.server.query.change.ChangeData;
 import java.io.IOException;
 import java.io.Reader;
 import java.time.Duration;
 import java.util.List;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 import org.junit.After;
 import org.junit.Before;
@@ -40,7 +45,9 @@ import org.junit.Test;
 public class StreamEventsIT extends AbstractDaemonTest {
   private static final Duration MAX_DURATION_FOR_RECEIVING_EVENTS = Duration.ofSeconds(2);
   private static final String TEST_REVIEW_COMMENT = "any comment";
+  private static final String TEST_REVIEW_DRAFT_COMMENT = "any draft comment";
   private Reader streamEventsReader;
+  private ChangeData change;
 
   @Before
   public void setup() throws Exception {
@@ -59,6 +66,23 @@ public class StreamEventsIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void publishedDraftPatchSetLevelCommentShowsUpInStreamEvents() throws Exception {
+    change = createChange().getChange();
+
+    String firstDraftComment = String.format("%s 1", TEST_REVIEW_DRAFT_COMMENT);
+    String secondDraftComment = String.format("%s 2", TEST_REVIEW_DRAFT_COMMENT);
+
+    draftReviewChange(PATCHSET_LEVEL, firstDraftComment);
+    draftReviewChange(PATCHSET_LEVEL, secondDraftComment);
+    publishDraftReviews();
+
+    waitForEvent(
+        () ->
+            pollEventsContaining("comment-added", firstDraftComment, secondDraftComment).size()
+                == 1);
+  }
+
+  @Test
   public void batchRefsUpdatedShowSeparatelyInStreamEvents() throws Exception {
     String refName = createChange().getChange().currentPatchSet().refName();
     waitForEvent(
@@ -77,7 +101,22 @@ public class StreamEventsIT extends AbstractDaemonTest {
     changeApi.current().review(reviewInput);
   }
 
-  private List<String> pollEventsContaining(String eventType, String expectedContent) {
+  private void draftReviewChange(String path, String reviewMessage) throws Exception {
+    DraftInput draftInput = new DraftInput();
+    draftInput.message = reviewMessage;
+    draftInput.path = path;
+    ChangeApi changeApi = gApi.changes().id(change.getId().get());
+    changeApi.current().createDraft(draftInput).get();
+  }
+
+  private void publishDraftReviews() throws Exception {
+    ReviewInput reviewInput = new ReviewInput();
+    reviewInput.tag = "new_tag";
+    reviewInput.drafts = DraftHandling.PUBLISH;
+    gApi.changes().id(change.getId().get()).current().review(reviewInput);
+  }
+
+  private List<String> pollEventsContaining(String eventType, String... expectedContent) {
     try {
       char[] cbuf = new char[2048];
       StringBuilder eventsOutput = new StringBuilder();
@@ -90,7 +129,7 @@ public class StreamEventsIT extends AbstractDaemonTest {
           .filter(
               event ->
                   event.contains(String.format("\"type\":\"%s\"", eventType))
-                      && event.contains(expectedContent))
+                      && Stream.of(expectedContent).allMatch(event::contains))
           .collect(Collectors.toList());
     } catch (IOException e) {
       throw new IllegalStateException(e);