Add draft comment creation to test API of changes

The API for draft comments mimics the one for published comments. Which
type of comment should be created is decided at the start of the fluent
API by either using newDraftComment() or newComment().

The tests for draft comments are mostly copied from the ones for
published comments albeit with some additions. Even though a lot of
the code is shared between draft and published comments, having so many
tests proved to be necessary as otherwise we wouldn't have noticed the
different code required to correctly set a tag for draft comments.

Change-Id: I413143a2fc5502efbf37406342ad42b7f6d95490

java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperations.java

@@ -27,8 +27,8 @@ public interface PerPatchsetOperations {
   TestPatchset get();
 
   /**
-   * Starts the fluent chain to create a new comment. The returned builder can be used to specify
-   * the attributes of the new comment. To create the comment for real, {@link
+   * Starts the fluent chain to create a new, published comment. The returned builder can be used to
+   * specify the attributes of the comment. To create the comment for real, {@link
    * TestCommentCreation.Builder#create()} must be called.
    *
    * <p>Example:
@@ -37,6 +37,7 @@ public interface PerPatchsetOperations {
    * String createdCommentUuid = changeOperations
    *     .change(changeId)
    *     .currentPatchset()
+   *     .newComment()
    *     .onLine(2)
    *     .ofFile("file1")
    *     .create();
@@ -45,4 +46,25 @@ public interface PerPatchsetOperations {
    * @return builder to create a new comment
    */
   TestCommentCreation.Builder newComment();
+
+  /**
+   * Starts the fluent chain to create a new draft comment. The returned builder can be used to
+   * specify the attributes of the draft comment. To create the draft comment for real, {@link
+   * TestCommentCreation.Builder#create()} must be called.
+   *
+   * <p>Example:
+   *
+   * <pre>
+   * String createdDraftCommentUuid = changeOperations
+   *     .change(changeId)
+   *     .currentPatchset()
+   *     .newDraftComment()
+   *     .onLine(2)
+   *     .ofFile("file1")
+   *     .create();
+   * </pre>
+   *
+   * @return builder to create a new comment
+   */
+  TestCommentCreation.Builder newDraftComment();
 }

java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java

@@ -18,6 +18,7 @@ import static com.google.gerrit.server.CommentsUtil.setCommentCommitId;
 
 import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation.CommentSide;
 import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Comment.Status;
 import com.google.gerrit.entities.HumanComment;
 import com.google.gerrit.entities.Patch;
 import com.google.gerrit.entities.PatchSet;
@@ -25,7 +26,6 @@ import com.google.gerrit.entities.Project;
 import com.google.gerrit.extensions.client.Comment;
 import com.google.gerrit.extensions.client.Comment.Range;
 import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.IdentifiedUser.GenericFactory;
@@ -93,7 +93,12 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
 
   @Override
   public TestCommentCreation.Builder newComment() {
-    return TestCommentCreation.builder(this::createComment);
+    return TestCommentCreation.builder(this::createComment, Status.PUBLISHED);
+  }
+
+  @Override
+  public TestCommentCreation.Builder newDraftComment() {
+    return TestCommentCreation.builder(this::createComment, Status.DRAFT);
   }
 
   private String createComment(TestCommentCreation commentCreation)
@@ -133,15 +138,16 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
     public boolean updateChange(ChangeContext context) throws Exception {
       HumanComment comment = toNewComment(context, commentCreation);
       ChangeUpdate changeUpdate = context.getUpdate(patchsetId);
-      changeUpdate.putComment(HumanComment.Status.PUBLISHED, comment);
-      // Only the tag set on the ChangeUpdate matters. The tag field of HumanComment is ignored.
+      changeUpdate.putComment(commentCreation.status(), comment);
+      // For published comments, only the tag set on the ChangeUpdate (and not on the HumanComment)
+      // matters.
       commentCreation.tag().ifPresent(changeUpdate::setTag);
       createdCommentUuid = comment.key.uuid;
       return true;
     }
 
     private HumanComment toNewComment(ChangeContext context, TestCommentCreation commentCreation)
-        throws UnprocessableEntityException, PatchListNotAvailableException {
+        throws PatchListNotAvailableException {
       String message = commentCreation.message().orElse("The text of a test comment.");
 
       String filePath = commentCreation.file().orElse(Patch.PATCHSET_LEVEL);
@@ -159,6 +165,9 @@ public class PerPatchsetOperationsImpl implements PerPatchsetOperations {
               message,
               unresolved,
               parentUuid);
+      // For draft comments, only the tag set on the HumanComment (and not on the ChangeUpdate)
+      // matters.
+      commentCreation.tag().ifPresent(tag -> newComment.tag = tag);
 
       commentCreation.line().ifPresent(line -> newComment.setLineNbrAndRange(line, null));
       // Specification of range trumps explicit line specification.

java/com/google/gerrit/acceptance/testsuite/change/TestCommentCreation.java

@@ -19,6 +19,7 @@ import com.google.gerrit.acceptance.testsuite.ThrowingFunction;
 import com.google.gerrit.acceptance.testsuite.change.TestRange.Position;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Comment;
 import com.google.gerrit.entities.Patch;
 import java.util.Optional;
 import java.util.function.Function;
@@ -46,11 +47,15 @@ public abstract class TestCommentCreation {
 
   public abstract Optional<Account.Id> author();
 
+  abstract Comment.Status status();
+
   abstract ThrowingFunction<TestCommentCreation, String> commentCreator();
 
   public static TestCommentCreation.Builder builder(
-      ThrowingFunction<TestCommentCreation, String> commentCreator) {
-    return new AutoValue_TestCommentCreation.Builder().commentCreator(commentCreator);
+      ThrowingFunction<TestCommentCreation, String> commentCreator, Comment.Status commentStatus) {
+    return new AutoValue_TestCommentCreation.Builder()
+        .commentCreator(commentCreator)
+        .status(commentStatus);
   }
 
   @AutoValue.Builder
@@ -156,7 +161,8 @@ public abstract class TestCommentCreation {
 
     /**
      * UUID of another comment to which this comment is a reply. This comment must have similar
-     * attributes (e.g. file, line, side) as the parent comment.
+     * attributes (e.g. file, line, side) as the parent comment. The parent comment must be a
+     * published comment.
      */
     public abstract Builder parentUuid(String parentUuid);
 
@@ -166,6 +172,13 @@ public abstract class TestCommentCreation {
     /** Author of the comment. Must be an existing user account. */
     public abstract Builder author(Account.Id accountId);
 
+    /**
+     * Status of the comment. Hidden in the API surface. Use {@link
+     * PerPatchsetOperations#newComment()} or {@link PerPatchsetOperations#newDraftComment()}
+     * depending on which type of comment you want to create.
+     */
+    abstract Builder status(Comment.Status value);
+
     abstract TestCommentCreation.Builder commentCreator(
         ThrowingFunction<TestCommentCreation, String> commentCreator);
 

javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java

@@ -20,6 +20,7 @@ import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.ass
 import com.google.common.truth.Correspondence;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.Patch;
@@ -36,6 +37,7 @@ public class PatchsetOperationsImplTest extends AbstractDaemonTest {
 
   @Inject private ChangeOperations changeOperations;
   @Inject private AccountOperations accountOperations;
+  @Inject private RequestScopeOperations requestScopeOperations;
 
   @Test
   public void commentCanBeCreatedWithoutSpecifyingAnyParameters() throws Exception {
@@ -310,10 +312,345 @@ public class PatchsetOperationsImplTest extends AbstractDaemonTest {
     assertThat(comment).author().id().isEqualTo(accountId.get());
   }
 
+  @Test
+  public void draftCommentCanBeCreatedWithoutSpecifyingAnyParameters() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations.change(changeId).currentPatchset().newDraftComment().create();
+
+    List<CommentInfo> comments = getDraftCommentsFromServer(changeId);
+    assertThatList(comments).comparingElementsUsing(hasUuid()).containsExactly(commentUuid);
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedOnOlderPatchset() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+    PatchSet.Id previousPatchsetId =
+        changeOperations.change(changeId).currentPatchset().get().patchsetId();
+    changeOperations.change(changeId).newPatchset().create();
+
+    String commentUuid =
+        changeOperations.change(changeId).patchset(previousPatchsetId).newDraftComment().create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).patchSet().isEqualTo(previousPatchsetId.get());
+  }
+
+  @Test
+  public void draftCommentIsCreatedWithSpecifiedMessage() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .message("Test comment message")
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).message().isEqualTo("Test comment message");
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedWithEmptyMessage() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations.change(changeId).currentPatchset().newDraftComment().noMessage().create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).message().isNull();
+  }
+
+  @Test
+  public void draftPatchsetLevelCommentCanBeCreated() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onPatchsetLevel()
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).path().isEqualTo(Patch.PATCHSET_LEVEL);
+  }
+
+  @Test
+  public void draftFileCommentCanBeCreated() throws Exception {
+    Change.Id changeId = changeOperations.newChange().file("file1").content("Line 1").create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onFileLevelOf("file1")
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).path().isEqualTo("file1");
+    assertThat(comment).line().isNull();
+    assertThat(comment).range().isNull();
+  }
+
+  @Test
+  public void draftLineCommentCanBeCreated() throws Exception {
+    Change.Id changeId =
+        changeOperations
+            .newChange()
+            .file("file1")
+            .content("Line 1\nLine 2\nLine 3\nLine 4\n")
+            .create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onLine(3)
+            .ofFile("file1")
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).line().isEqualTo(3);
+    assertThat(comment).range().isNull();
+  }
+
+  @Test
+  public void draftRangeCommentCanBeCreated() throws Exception {
+    Change.Id changeId =
+        changeOperations
+            .newChange()
+            .file("file1")
+            .content("Line 1\nLine 2\nLine 3\nLine 4\n")
+            .create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .fromLine(2)
+            .charOffset(4)
+            .toLine(3)
+            .charOffset(5)
+            .ofFile("file1")
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).range().startLine().isEqualTo(2);
+    assertThat(comment).range().startCharacter().isEqualTo(4);
+    assertThat(comment).range().endLine().isEqualTo(3);
+    assertThat(comment).range().endCharacter().isEqualTo(5);
+    // Line is automatically filled from specified range. It's the end line.
+    assertThat(comment).line().isEqualTo(3);
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedOnPatchsetCommit() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onPatchsetCommit()
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    // Null is often used instead of Side.REVISION as Side.REVISION is the default.
+    assertThat(comment).side().isAnyOf(Side.REVISION, null);
+    assertThat(comment).parent().isNull();
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedOnParentCommit() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onParentCommit()
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).side().isEqualTo(Side.PARENT);
+    assertThat(comment).parent().isEqualTo(1);
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedOnSecondParentCommit() throws Exception {
+    // Second parents only exist for merge commits. The test API currently doesn't support the
+    // creation of changes with merge commits yet, though. As there's no explicit validation keeping
+    // us from adding comments on the non-existing second parent of a regular commit, just use the
+    // latter. That's still better than not having this test at all.
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onSecondParentCommit()
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).side().isEqualTo(Side.PARENT);
+    assertThat(comment).parent().isEqualTo(2);
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedOnAutoMergeCommit() throws Exception {
+    // Second parents only exist for merge commits. The test API currently doesn't support the
+    // creation of changes with merge commits yet, though. As there's no explicit validation keeping
+    // us from adding comments on the non-existing second parent of a regular commit, just use the
+    // latter. That's still better than not having this test at all.
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .onAutoMergeCommit()
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).side().isEqualTo(Side.PARENT);
+    assertThat(comment).parent().isNull();
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedAsResolved() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations.change(changeId).currentPatchset().newDraftComment().resolved().create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).unresolved().isFalse();
+  }
+
+  @Test
+  public void draftCommentCanBeCreatedAsUnresolved() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations.change(changeId).currentPatchset().newDraftComment().unresolved().create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).unresolved().isTrue();
+  }
+
+  @Test
+  public void draftReplyToDraftCommentCanBeCreated() {
+    Change.Id changeId = changeOperations.newChange().create();
+    String parentCommentUuid =
+        changeOperations.change(changeId).currentPatchset().newDraftComment().create();
+
+    // Gerrit's other APIs shouldn't support the creation of a draft reply to a draft comment but
+    // there's currently no reason to not support such a comment via the test API if a test really
+    // wants to create such a comment.
+    changeOperations
+        .change(changeId)
+        .currentPatchset()
+        .newDraftComment()
+        .parentUuid(parentCommentUuid)
+        .create();
+  }
+
+  @Test
+  public void draftReplyToPublishedCommentCanBeCreated() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+    String parentCommentUuid =
+        changeOperations.change(changeId).currentPatchset().newComment().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .parentUuid(parentCommentUuid)
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).inReplyTo().isEqualTo(parentCommentUuid);
+  }
+
+  @Test
+  public void tagCanBeAttachedToADraftComment() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .tag("my special tag")
+            .create();
+
+    CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+    assertThat(comment).tag().isEqualTo("my special tag");
+  }
+
+  @Test
+  public void draftCommentIsCreatedWithSpecifiedAuthor() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+    Account.Id accountId = accountOperations.newAccount().create();
+
+    String commentUuid =
+        changeOperations
+            .change(changeId)
+            .currentPatchset()
+            .newDraftComment()
+            .author(accountId)
+            .create();
+
+    // A user can only retrieve their own draft comments.
+    requestScopeOperations.setApiUser(accountId);
+    List<CommentInfo> comments = getDraftCommentsFromServer(changeId);
+    // Draft comments never have the author field set. As a user can only retrieve their own draft
+    // comments, we implicitly know that the author was correctly set when we find the created
+    // comment in the draft comments of that user.
+    assertThatList(comments).comparingElementsUsing(hasUuid()).containsExactly(commentUuid);
+  }
+
+  @Test
+  public void noDraftCommentsAreCreatedOnCreationOfPublishedComment() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    changeOperations.change(changeId).currentPatchset().newComment().create();
+
+    List<CommentInfo> comments = getDraftCommentsFromServer(changeId);
+    assertThatList(comments).isEmpty();
+  }
+
+  @Test
+  public void noPublishedCommentsAreCreatedOnCreationOfDraftComment() throws Exception {
+    Change.Id changeId = changeOperations.newChange().create();
+
+    changeOperations.change(changeId).currentPatchset().newDraftComment().create();
+
+    List<CommentInfo> comments = getCommentsFromServer(changeId);
+    assertThatList(comments).isEmpty();
+  }
+
   private List<CommentInfo> getCommentsFromServer(Change.Id changeId) throws RestApiException {
     return gApi.changes().id(changeId.get()).commentsAsList();
   }
 
+  private List<CommentInfo> getDraftCommentsFromServer(Change.Id changeId) throws RestApiException {
+    return gApi.changes().id(changeId.get()).draftsAsList();
+  }
+
   private CommentInfo getCommentFromServer(Change.Id changeId, String uuid)
       throws RestApiException {
     return gApi.changes().id(changeId.get()).commentsAsList().stream()
@@ -325,6 +662,18 @@ public class PatchsetOperationsImplTest extends AbstractDaemonTest {
                     String.format("Comment %s not found on change %d", uuid, changeId.get())));
   }
 
+  private CommentInfo getDraftCommentFromServer(Change.Id changeId, String uuid)
+      throws RestApiException {
+    return gApi.changes().id(changeId.get()).draftsAsList().stream()
+        .filter(comment -> comment.id.equals(uuid))
+        .findAny()
+        .orElseThrow(
+            () ->
+                new IllegalStateException(
+                    String.format(
+                        "Draft comment %s not found on change %d", uuid, changeId.get())));
+  }
+
   private Correspondence<CommentInfo, String> hasUuid() {
     return NullAwareCorrespondence.transforming(comment -> comment.id, "hasUuid");
   }