Assume a comment is a robot comment when not found

Currently, when we don't find a comment, we throw IllegalStateException.
Unfortunately, this is not true since in those cases, the comment
that is missing is actually just a robot comment.

This was needed in CommentsUtil#newHumanComment, which always had a bug
that was just not noticed since we usually create new comments using
draft comments. When a comment has a parent robot comment, and
unresolved is unset, we wouldn't find any comment (although the id is
valid, since it belongs to a robot comment).

Also, needed in CommentsUtil#getAllCommentsInCommentThreads, since going
through all the user's comments descendants, we may get into a robot
comment, which we should ignore.

As a follow-up change, in some of those places it's reasonable to also
search for the robot comment, and then throw IllegalStateException if
the comment is neither human nor robot comment.

Change-Id: I06db2d8f3ab0fa01c4962f8d1b081f6811d6982a

java/com/google/gerrit/server/CommentsUtil.java

@@ -37,7 +37,6 @@ import com.google.gerrit.entities.RobotComment;
 import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.client.Side;
 import com.google.gerrit.extensions.common.CommentInfo;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.GerritServerId;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -132,8 +131,7 @@ public class CommentsUtil {
       short side,
       String message,
       @Nullable Boolean unresolved,
-      @Nullable String parentUuid)
+      @Nullable String parentUuid) {
-      throws UnprocessableEntityException {
     if (unresolved == null) {
       if (parentUuid == null) {
         // Default to false if comment is not descended from another.
@@ -142,10 +140,10 @@ public class CommentsUtil {
         // Inherit unresolved value from inReplyTo comment if not specified.
         Comment.Key key = new Comment.Key(parentUuid, path, psId.get());
         Optional<HumanComment> parent = getPublishedHumanComment(changeNotes, key);
-        if (!parent.isPresent()) {
+
-          throw new UnprocessableEntityException("Invalid parentUuid supplied for comment");
+        // If the comment was not found, it is descended from a robot comment, or the UUID is
-        }
+        // invalid. Either way, we use the default.
-        unresolved = parent.get().unresolved;
+        unresolved = parent.map(p -> p.unresolved).orElse(false);
       }
     }
     HumanComment c =
@@ -352,7 +350,7 @@ public class CommentsUtil {
    * @param newComments set of all the new comments added on the change by the current user.
    * @return set of all comments in the comments thread that received a reply.
    */
-  public Set<HumanComment> getAllCommentsInCommentThreads(
+  public Set<HumanComment> getAllHumanCommentsInCommentThreads(
       ChangeNotes changeNotes, ImmutableSet<HumanComment> newComments) {
     Map<String, HumanComment> uuidToComment =
         publishedHumanCommentsByChange(changeNotes).stream()
@@ -369,8 +367,9 @@ public class CommentsUtil {
       if (current.parentUuid != null) {
         HumanComment parent = uuidToComment.get(current.parentUuid);
         if (parent == null) {
-          throw new IllegalStateException(
+          // If we can't find the parent within the human comments, the parent must be a robot
-              String.format("Comment %s not found", current.parentUuid));
+          // comment and can be ignored.
+          continue;
         }
         if (!seen.contains(current.parentUuid)) {
           toTraverse.add(parent);

java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java

@@ -210,7 +210,7 @@ public class ReplyAttentionSetUpdates {
   private void addAllAuthorsOfCommentThreads(
       BatchUpdate bu, ChangeNotes changeNotes, ImmutableSet<HumanComment> allNewComments) {
     Set<HumanComment> allCommentsInCommentThreads =
-        commentsUtil.getAllCommentsInCommentThreads(changeNotes, allNewComments);
+        commentsUtil.getAllHumanCommentsInCommentThreads(changeNotes, allNewComments);
     // Copy the set to make it mutable, so that we can delete users that were already added.
     Set<Account.Id> possibleUsersToAdd =
         new HashSet<>(approvalsUtil.getReviewers(changeNotes).all());

javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java

@@ -832,6 +832,25 @@ public class AttentionSetIT extends AbstractDaemonTest {
     assertThat(attentionSet.reason()).isEqualTo("Someone else replied on the change");
   }
 
+  @Test
+  public void reviewIgnoresRobotCommentsForAttentionSet() throws Exception {
+    PushOneCommit.Result r = createChange();
+    requestScopeOperations.setApiUser(user.id());
+    testCommentHelper.addRobotComment(
+        r.getChangeId(),
+        testCommentHelper.createRobotCommentInputWithMandatoryFields(Patch.COMMIT_MSG));
+
+    requestScopeOperations.setApiUser(admin.id());
+    change(r)
+        .current()
+        .review(
+            reviewInReplyToComment(
+                Iterables.getOnlyElement(
+                        gApi.changes().id(r.getChangeId()).current().robotCommentsAsList())
+                    .id));
+    assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+  }
+
   @Test
   public void reviewAddsAllUsersInCommentThread() throws Exception {
     PushOneCommit.Result r = createChange();
@@ -869,6 +888,42 @@ public class AttentionSetIT extends AbstractDaemonTest {
     assertThat(attentionSet.reason()).isEqualTo("Someone else replied on a comment you posted");
   }
 
+  @Test
+  public void reviewAddsAllUsersInCommentThreadWhenOriginalCommentIsARobotComment()
+      throws Exception {
+    PushOneCommit.Result result = createChange();
+    testCommentHelper.addRobotComment(
+        result.getChangeId(),
+        testCommentHelper.createRobotCommentInputWithMandatoryFields(Patch.COMMIT_MSG));
+
+    requestScopeOperations.setApiUser(user.id());
+    // Reply to the robot comment.
+    change(result)
+        .current()
+        .review(
+            reviewInReplyToComment(
+                Iterables.getOnlyElement(
+                        gApi.changes().id(result.getChangeId()).current().robotCommentsAsList())
+                    .id));
+
+    requestScopeOperations.setApiUser(admin.id());
+    // Reply to the human comment. which was a reply to the robot comment.
+    change(result)
+        .current()
+        .review(
+            reviewInReplyToComment(
+                Iterables.getOnlyElement(
+                        gApi.changes().id(result.getChangeId()).current().commentsAsList())
+                    .id));
+
+    // The user which replied to the robot comment was added to the attention set.
+    AttentionSetUpdate attentionSet =
+        Iterables.getOnlyElement(getAttentionSetUpdatesForUser(result, user));
+    assertThat(attentionSet.account()).isEqualTo(user.id());
+    assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
+    assertThat(attentionSet.reason()).isEqualTo("Someone else replied on a comment you posted");
+  }
+
   @Test
   public void reviewAddsAllUsersInCommentThreadWhenPostedAsDraft() throws Exception {
     PushOneCommit.Result r = createChange();

javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java

@@ -46,6 +46,7 @@ import com.google.gerrit.extensions.client.Comment;
 import com.google.gerrit.extensions.client.Side;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.RobotCommentInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.IdString;
@@ -60,6 +61,7 @@ import com.google.gerrit.server.restapi.change.ChangesCollection;
 import com.google.gerrit.server.restapi.change.PostReview;
 import com.google.gerrit.testing.FakeEmailSender;
 import com.google.gerrit.testing.FakeEmailSender.Message;
+import com.google.gerrit.testing.TestCommentHelper;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import java.sql.Timestamp;
@@ -90,6 +92,7 @@ public class CommentsIT extends AbstractDaemonTest {
   @Inject private Provider<PostReview> postReview;
   @Inject private RequestScopeOperations requestScopeOperations;
   @Inject private CommentsUtil commentsUtil;
+  @Inject private TestCommentHelper testCommentHelper;
 
   private final Integer[] lines = {0, 1};
 
@@ -1431,6 +1434,30 @@ public class CommentsIT extends AbstractDaemonTest {
     assertThat(getChangeSortedComments(id.get())).hasSize(3);
   }
 
+  @Test
+  public void canCreateHumanCommentWithRobotCommentAsParentAndUnsetUnresolved() throws Exception {
+    PushOneCommit.Result result = createChange();
+    String changeId = result.getChangeId();
+    String ps1 = result.getCommit().name();
+
+    testCommentHelper.addRobotComment(
+        result.getChangeId(),
+        testCommentHelper.createRobotCommentInputWithMandatoryFields(FILE_NAME));
+    RobotCommentInfo robotCommentInfo =
+        Iterables.getOnlyElement(gApi.changes().id(changeId).current().robotCommentsAsList());
+
+    CommentInput comment = newComment(FILE_NAME, "comment 1 reply");
+    comment.inReplyTo = robotCommentInfo.id;
+    comment.unresolved = null;
+    addComments(changeId, ps1, comment);
+
+    CommentInfo resultComment = Iterables.getOnlyElement(getPublishedCommentsAsList(changeId));
+    assertThat(resultComment.inReplyTo).isEqualTo(robotCommentInfo.id);
+
+    // Default unresolved is false.
+    assertThat(resultComment.unresolved).isFalse();
+  }
+
   private List<CommentInfo> getRevisionComments(String changeId, String revId) throws Exception {
     return getPublishedComments(changeId, revId).values().stream()
         .flatMap(List::stream)