From b3256183e1761aa6dbff82fed1a7feac7dcab3bc Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Thu, 20 Aug 2020 12:09:12 +0300 Subject: [PATCH] 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 --- .../google/gerrit/server/CommentsUtil.java | 19 +++---- .../change/ReplyAttentionSetUpdates.java | 2 +- .../rest/change/AttentionSetIT.java | 55 +++++++++++++++++++ .../acceptance/server/change/CommentsIT.java | 27 +++++++++ 4 files changed, 92 insertions(+), 11 deletions(-) diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java index f3dfd63627..81969f0c0e 100644 --- a/java/com/google/gerrit/server/CommentsUtil.java +++ b/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) - throws UnprocessableEntityException { + @Nullable String parentUuid) { 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 parent = getPublishedHumanComment(changeNotes, key); - if (!parent.isPresent()) { - throw new UnprocessableEntityException("Invalid parentUuid supplied for comment"); - } - unresolved = parent.get().unresolved; + + // 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.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 getAllCommentsInCommentThreads( + public Set getAllHumanCommentsInCommentThreads( ChangeNotes changeNotes, ImmutableSet newComments) { Map 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( - String.format("Comment %s not found", current.parentUuid)); + // If we can't find the parent within the human comments, the parent must be a robot + // comment and can be ignored. + continue; } if (!seen.contains(current.parentUuid)) { toTraverse.add(parent); diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java index 629e773ce8..b7a03d192d 100644 --- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java +++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java @@ -210,7 +210,7 @@ public class ReplyAttentionSetUpdates { private void addAllAuthorsOfCommentThreads( BatchUpdate bu, ChangeNotes changeNotes, ImmutableSet allNewComments) { Set 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 possibleUsersToAdd = new HashSet<>(approvalsUtil.getReviewers(changeNotes).all()); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java index b86a1a6bf5..017061800c 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java +++ b/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(); diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index 7389cfeaaa..7faae6bceb 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/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; @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 getRevisionComments(String changeId, String revId) throws Exception { return getPublishedComments(changeId, revId).values().stream() .flatMap(List::stream)