From 3edc14264791d3f667dc0f65a5dcca53d0b7bb83 Mon Sep 17 00:00:00 2001 From: Gal Paikin Date: Thu, 19 Mar 2020 12:04:52 +0100 Subject: [PATCH] Allow patchset-level comments As a sub-requirement of the following: https://www.gerritcodereview.com/design-docs/change-log-threaded-feedback.html adding comments that are not attached to a file are needed for creating threaded change messages. Also, adding robot comments that are not a part of a file is needed as part of an effort to improve robot comments. More specifically, some robots might want to complain about a missing file and hence can't attach such a robot comment to an existing file. When adding a new comment in PostReview, the backend determines whether a comment's file is legal by checking if the file exists, or if it's a magic file (e.g commit message and merge list). Same applies for creating and updating draft comments. This change adds "/PATCHSET_LEVEL" to the list of legal file names. That way, it will be possible to create patchset-level comments on the backend by specifying "/PATCHSET_LEVEL" as the name of the file. After rolling out this change, it will be possible to create patchset-level comments but it will not be possible to create them via the UI. Another modification is a link to file-id (path) from CommentInput and CommentInfo to better document the possible magic files (such as /PATCHSET_LEVEL). Another backend change that's required is to fix email notifications for patchset-level comments, since currently /PATCHSET_LEVEL file path is used. Also, the email links to the file /PATCHSET_LEVEL, which is against the planned user experience. It's not possible to create those patchset level comments via the UI, and it's only possible if you're specifically trying to do so by naming the file name "/PATCHSET_LEVEL". Also, even if comments were created it would not break the UI, but just make it look slightly off (e.g, it would have a file called /PATCHSET_LEVEL). Therefore, it's not a problem to merge this change now. Some frontend adjustments are required: 1. Make patchset-level robot comments behave correctly in the UI. They are currently planned to be visible at the top of the findings section. 2. Make patchset-level human comments behave correctly in the UI, and allow creating them via the UI. Implementation TBD. Change-Id: Ia84feb593c9ff03613752b8db59d8dfd30de638d --- Documentation/rest-api-changes.txt | 9 +- java/com/google/gerrit/entities/Patch.java | 8 +- .../testing/RobotCommentInfoSubject.java | 6 + .../google/gerrit/server/patch/PatchList.java | 19 +- .../restapi/change/CreateDraftComment.java | 4 + .../server/restapi/change/PostReview.java | 22 +- .../restapi/change/PutDraftComment.java | 4 + .../gerrit/testing/TestCommentHelper.java | 1 - .../acceptance/api/revision/GetBlameIT.java | 27 +++ .../api/revision/RevisionDiffIT.java | 19 ++ .../acceptance/api/revision/RevisionIT.java | 14 ++ .../api/revision/RobotCommentsIT.java | 53 +++++ .../acceptance/server/change/CommentsIT.java | 201 +++++++++++++++++- .../com/google/gerrit/entities/PatchTest.java | 1 + .../gerrit/server/patch/PatchListTest.java | 24 ++- 15 files changed, 382 insertions(+), 30 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8930bc9a95..ef6ef767be 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -5723,6 +5723,11 @@ information and the committer information. The list of commits that are being integrated into the destination branch by submitting the merge commit. +* `/PATCHSET_LEVEL`: ++ +This file path is used exclusively for posting and indicating +patchset-level comments, thus not relevant for other use-cases. + [[fix-id]] === \{fix-id\} UUID of a suggested fix. @@ -6218,7 +6223,7 @@ The patch set number for the comment; only set in contexts where + comments may be returned for multiple patch sets. |`id` ||The URL encoded UUID of the comment. |`path` |optional| -The path of the file for which the inline comment was done. + +link:#file-id[The file path] for which the inline comment was done. + Not set if returned in a map where the key is the file path. |`side` |optional| The side on which the comment was added. + @@ -6271,7 +6276,7 @@ comment. The URL encoded UUID of the comment if an existing draft comment should be updated. |`path` |optional| -The path of the file for which the inline comment should be added. + +link:#file-id[The file path] for which the inline comment should be added. + Doesn't need to be set if contained in a map where the key is the file path. |`side` |optional| diff --git a/java/com/google/gerrit/entities/Patch.java b/java/com/google/gerrit/entities/Patch.java index 50f86fea4d..3926d4787a 100644 --- a/java/com/google/gerrit/entities/Patch.java +++ b/java/com/google/gerrit/entities/Patch.java @@ -34,6 +34,12 @@ public final class Patch { /** Magical file name which represents the merge list of a merge commit. */ public static final String MERGE_LIST = "/MERGE_LIST"; + /** + * Magical file name which doesn't represent a file. Used specifically for patchset-level + * comments. + */ + public static final String PATCHSET_LEVEL = "/PATCHSET_LEVEL"; + /** * Checks if the given path represents a magic file. A magic file is a generated file that is * automatically included into changes. It does not exist in the commit of the patch set. @@ -42,7 +48,7 @@ public final class Patch { * @return {@code true} if the path represents a magic file, otherwise {@code false}. */ public static boolean isMagic(String path) { - return COMMIT_MSG.equals(path) || MERGE_LIST.equals(path); + return COMMIT_MSG.equals(path) || MERGE_LIST.equals(path) || PATCHSET_LEVEL.equals(path); } public static Key key(PatchSet.Id patchSetId, String fileName) { diff --git a/java/com/google/gerrit/extensions/common/testing/RobotCommentInfoSubject.java b/java/com/google/gerrit/extensions/common/testing/RobotCommentInfoSubject.java index 0698735acf..dd226ed65c 100644 --- a/java/com/google/gerrit/extensions/common/testing/RobotCommentInfoSubject.java +++ b/java/com/google/gerrit/extensions/common/testing/RobotCommentInfoSubject.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertAbout; import static com.google.gerrit.truth.ListSubject.elements; import com.google.common.truth.FailureMetadata; +import com.google.common.truth.StringSubject; import com.google.common.truth.Subject; import com.google.gerrit.extensions.common.FixSuggestionInfo; import com.google.gerrit.extensions.common.RobotCommentInfo; @@ -53,6 +54,11 @@ public class RobotCommentInfoSubject extends Subject { .thatCustom(robotCommentInfo.fixSuggestions, FixSuggestionInfoSubject.fixSuggestions()); } + public StringSubject path() { + isNotNull(); + return check("path").that(robotCommentInfo.path); + } + public FixSuggestionInfoSubject onlyFixSuggestion() { return fixSuggestions().onlyElement(); } diff --git a/java/com/google/gerrit/server/patch/PatchList.java b/java/com/google/gerrit/server/patch/PatchList.java index c9e45bac2d..28f61d3a52 100644 --- a/java/com/google/gerrit/server/patch/PatchList.java +++ b/java/com/google/gerrit/server/patch/PatchList.java @@ -45,23 +45,12 @@ import org.eclipse.jgit.lib.ObjectId; public class PatchList implements Serializable { private static final long serialVersionUID = PatchListKey.serialVersionUID; - private static final Comparator PATCH_CMP = - Comparator.comparing(PatchListEntry::getNewName, PatchList::comparePaths); - @VisibleForTesting - static int comparePaths(String a, String b) { - int m1 = Patch.isMagic(a) ? (a.equals(Patch.MERGE_LIST) ? 2 : 1) : 3; - int m2 = Patch.isMagic(b) ? (b.equals(Patch.MERGE_LIST) ? 2 : 1) : 3; + static final Comparator FILE_PATH_CMP = + Comparator.comparing(Patch::isMagic).reversed().thenComparing(Comparator.naturalOrder()); - if (m1 != m2) { - return m1 - m2; - } else if (m1 < 3) { - return 0; - } - - // m1 == m2 == 3: normal names. - return a.compareTo(b); - } + private static final Comparator PATCH_CMP = + Comparator.comparing(PatchListEntry::getNewName, FILE_PATH_CMP); @Nullable private transient ObjectId oldId; private transient ObjectId newId; diff --git a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java index 5b7245d407..42032f7d5f 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java +++ b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.restapi.change; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.server.CommentsUtil.setCommentCommitId; import com.google.common.base.Strings; @@ -73,6 +74,9 @@ public class CreateDraftComment implements RestModifyView= 0"); } else if (in.line != null && in.range != null && in.line != in.range.endLine) { diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java index 7008bb9019..1e2e644030 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReview.java +++ b/java/com/google/gerrit/server/restapi/change/PostReview.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.restapi.change; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.server.CommentsUtil.setCommentCommitId; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF; @@ -606,6 +607,7 @@ public class PostReview implements RestModifyView ensureLineIsNonNegative(comment.line, path); ensureCommentNotOnMagicFilesOfAutoMerge(path, comment); ensureRangeIsValid(path, comment.range); + ensureValidPatchsetLevelComment(path, comment); } } } @@ -644,6 +646,14 @@ public class PostReview implements RestModifyView } } + private static void ensureValidPatchsetLevelComment( + String path, T comment) throws BadRequestException { + if (path.equals(PATCHSET_LEVEL) + && (comment.side != null || comment.range != null || comment.line != null)) { + throw new BadRequestException("Patchset-level comments can't have side, range, or line"); + } + } + private void checkRobotComments( RevisionResource revision, Map> in) throws BadRequestException, PatchListNotAvailableException { @@ -703,7 +713,7 @@ public class PostReview implements RestModifyView ensureReplacementsArePresent(commentPath, fixReplacementInfos); for (FixReplacementInfo fixReplacementInfo : fixReplacementInfos) { - ensureReplacementPathIsSet(commentPath, fixReplacementInfo.path); + ensureReplacementPathIsSetAndNotPatchsetLevel(commentPath, fixReplacementInfo.path); ensureRangeIsSet(commentPath, fixReplacementInfo.range); ensureRangeIsValid(commentPath, fixReplacementInfo.range); ensureReplacementStringIsSet(commentPath, fixReplacementInfo.replacement); @@ -727,14 +737,20 @@ public class PostReview implements RestModifyView } } - private static void ensureReplacementPathIsSet(String commentPath, String replacementPath) - throws BadRequestException { + private static void ensureReplacementPathIsSetAndNotPatchsetLevel( + String commentPath, String replacementPath) throws BadRequestException { if (replacementPath == null) { throw new BadRequestException( String.format( "A file path must be given for the replacement of the robot comment on %s", commentPath)); } + if (replacementPath.equals(PATCHSET_LEVEL)) { + throw new BadRequestException( + String.format( + "A file path must not be %s for the replacement of the robot comment on %s", + PATCHSET_LEVEL, commentPath)); + } } private static void ensureRangeIsSet(String commentPath, Range range) throws BadRequestException { diff --git a/java/com/google/gerrit/server/restapi/change/PutDraftComment.java b/java/com/google/gerrit/server/restapi/change/PutDraftComment.java index 63cd7a349c..ea5836536b 100644 --- a/java/com/google/gerrit/server/restapi/change/PutDraftComment.java +++ b/java/com/google/gerrit/server/restapi/change/PutDraftComment.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.restapi.change; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.server.CommentsUtil.setCommentCommitId; import com.google.gerrit.entities.Comment; @@ -79,6 +80,9 @@ public class PutDraftComment implements RestModifyView= 0"); + } else if (in.path.equals(PATCHSET_LEVEL) + && (in.side != null || in.range != null || in.line != null)) { + throw new BadRequestException("patchset-level comments can't have side, range, or line"); } else if (in.line != null && in.range != null && in.line != in.range.endLine) { throw new BadRequestException("range endLine must be on the same line as the comment"); } diff --git a/java/com/google/gerrit/testing/TestCommentHelper.java b/java/com/google/gerrit/testing/TestCommentHelper.java index dfa4601bd6..bd859db438 100644 --- a/java/com/google/gerrit/testing/TestCommentHelper.java +++ b/java/com/google/gerrit/testing/TestCommentHelper.java @@ -117,7 +117,6 @@ public class TestCommentHelper { RobotCommentInput in = new RobotCommentInput(); in.robotId = "happyRobot"; in.robotRunId = "1"; - in.line = 1; in.message = "nit: trailing whitespace"; in.path = path; return in; diff --git a/javatests/com/google/gerrit/acceptance/api/revision/GetBlameIT.java b/javatests/com/google/gerrit/acceptance/api/revision/GetBlameIT.java index 8dfebad802..62140ed4c3 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/GetBlameIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/GetBlameIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.api.revision; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; @@ -34,6 +35,16 @@ public class GetBlameIT extends AbstractDaemonTest { assertThat(blameInfos).isEmpty(); } + @Test + public void forPatchsetLevelFile() throws Exception { + PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO"); + List blameInfos = + gApi.changes().id(r.getChangeId()).current().file(PATCHSET_LEVEL).blameRequest().get(); + + // File doesn't exist in commit. + assertThat(blameInfos).isEmpty(); + } + @Test public void forNonExistingFileFromBase() throws Exception { PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO"); @@ -50,6 +61,22 @@ public class GetBlameIT extends AbstractDaemonTest { assertThat(blameInfos).isEmpty(); } + @Test + public void forPatchsetLevelFileFromBase() throws Exception { + PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO"); + List blameInfos = + gApi.changes() + .id(r.getChangeId()) + .current() + .file(PATCHSET_LEVEL) + .blameRequest() + .forBase(true) + .get(); + + // File doesn't exist in base commit. + assertThat(blameInfos).isEmpty(); + } + @Test public void forNewlyAddedFile() throws Exception { PushOneCommit.Result r = createChange("Test Change", "foo.txt", "FOO"); diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index 717d3cc4f4..001f6bce71 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.entities.Patch.COMMIT_MSG; import static com.google.gerrit.entities.Patch.MERGE_LIST; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.extensions.common.testing.DiffInfoSubject.assertThat; import static com.google.gerrit.extensions.common.testing.FileInfoSubject.assertThat; import static com.google.gerrit.git.ObjectIds.abbreviateName; @@ -120,6 +121,24 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertDiffForNewFile(result, COMMIT_MSG, result.getCommit().getFullMessage()); } + @Test + public void patchsetLevelFileDiffIsEmpty() throws Exception { + PushOneCommit.Result result = createChange(); + DiffInfo diffForPatchsetLevelFile = + gApi.changes() + .id(result.getChangeId()) + .revision(result.getCommit().name()) + .file(PATCHSET_LEVEL) + .diff(); + // This behavior is the same as the behavior for non-existent files. + assertThat(diffForPatchsetLevelFile).binary().isNull(); + assertThat(diffForPatchsetLevelFile).content().isEmpty(); + assertThat(diffForPatchsetLevelFile).diffHeader().isNull(); + assertThat(diffForPatchsetLevelFile).metaA().isNull(); + assertThat(diffForPatchsetLevelFile).metaB().isNull(); + assertThat(diffForPatchsetLevelFile).webLinks().isNull(); + } + @Test public void deletedFileIsIncludedInDiff() throws Exception { gApi.changes().id(changeId).edit().deleteFile(FILE_NAME); diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java index 74f9134c2d..dd13643ed8 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -24,6 +24,7 @@ import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow; import static com.google.gerrit.entities.Patch.COMMIT_MSG; import static com.google.gerrit.entities.Patch.MERGE_LIST; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS; import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS; import static com.google.gerrit.git.ObjectIds.abbreviateName; @@ -1499,6 +1500,19 @@ public class RevisionIT extends AbstractDaemonTest { assertContent(r, COMMIT_MSG, r.getCommit().getFullMessage()); } + @Test + public void patchsetLevelContentDoesNotExist() throws Exception { + PushOneCommit.Result change = createChange(); + assertThrows( + ResourceNotFoundException.class, + () -> + gApi.changes() + .id(change.getChangeId()) + .revision(change.getCommit().name()) + .file(PATCHSET_LEVEL) + .content()); + } + @Test public void cannotGetContentOfDirectory() throws Exception { Map files = ImmutableMap.of("dir/file1.txt", "content 1"); diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java index 986c8c4441..27b866b132 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java @@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.api.revision; import static com.google.common.collect.MoreCollectors.onlyElement; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES; import static com.google.gerrit.extensions.common.testing.DiffInfoSubject.assertThat; import static com.google.gerrit.extensions.common.testing.EditInfoSubject.assertThat; @@ -35,6 +36,7 @@ import com.google.gerrit.entities.Patch; import com.google.gerrit.extensions.api.changes.PublishChangeEditInput; import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; 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.ChangeMessageInfo; import com.google.gerrit.extensions.common.ChangeType; @@ -266,6 +268,57 @@ public class RobotCommentsIT extends AbstractDaemonTest { assertRobotComment(comment, in, false); } + @Test + public void patchsetLevelRobotCommentCanBeAddedAndRetrieved() throws Exception { + RobotCommentInput input = TestCommentHelper.createRobotCommentInput(PATCHSET_LEVEL); + testCommentHelper.addRobotComment(changeId, input); + + List results = getRobotComments(); + assertThatList(results).onlyElement().path().isEqualTo(PATCHSET_LEVEL); + } + + @Test + public void patchsetLevelRobotCommentCantHaveLine() throws Exception { + RobotCommentInput input = TestCommentHelper.createRobotCommentInput(PATCHSET_LEVEL); + input.line = 1; + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> testCommentHelper.addRobotComment(changeId, input)); + assertThat(ex.getMessage()).contains("line"); + } + + @Test + public void patchsetLevelRobotCommentCantHaveRange() throws Exception { + RobotCommentInput input = TestCommentHelper.createRobotCommentInput(PATCHSET_LEVEL); + input.range = createRange(2, 9, 5, 10); + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> testCommentHelper.addRobotComment(changeId, input)); + assertThat(ex.getMessage()).contains("range"); + } + + @Test + public void patchsetLevelRobotCommentCantHaveSide() throws Exception { + RobotCommentInput input = TestCommentHelper.createRobotCommentInput(PATCHSET_LEVEL); + input.side = Side.REVISION; + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> testCommentHelper.addRobotComment(changeId, input)); + assertThat(ex.getMessage()).contains("side"); + } + + @Test + public void fixSuggestionCannotPointToPatchsetLevel() throws Exception { + RobotCommentInput input = TestCommentHelper.createRobotCommentInput(FILE_NAME); + FixReplacementInfo brokenFixReplacement = createFixReplacementInfo(); + brokenFixReplacement.path = PATCHSET_LEVEL; + input.fixSuggestions = ImmutableList.of(createFixSuggestionInfo(brokenFixReplacement)); + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> testCommentHelper.addRobotComment(changeId, input)); + assertThat(ex.getMessage()).contains("file path must not be " + PATCHSET_LEVEL); + } + @Test public void hugeRobotCommentIsRejected() { int defaultSizeLimit = 1 << 20; diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index ecd4025b97..4fac821cad 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -18,7 +18,9 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; +import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import static com.google.gerrit.truth.MapSubject.assertThatMap; import static java.util.stream.Collectors.groupingBy; import static java.util.stream.Collectors.toList; @@ -175,6 +177,189 @@ public class CommentsIT extends AbstractDaemonTest { } } + @Test + public void patchsetLevelCommentCanBeAddedAndRetrieved() throws Exception { + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + String ps1 = result.getCommit().name(); + + CommentInput comment = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + addComments(changeId, ps1, comment); + + Map> results = getPublishedComments(changeId, ps1); + assertThatMap(results).keys().containsExactly(PATCHSET_LEVEL); + } + + @Test + public void deletePatchsetLevelComment() throws Exception { + requestScopeOperations.setApiUser(admin.id()); + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + String commentMessage = "to be deleted"; + CommentInput comment = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, commentMessage); + addComments(changeId, revId, comment); + + Map> results = getPublishedComments(changeId, revId); + CommentInfo oldComment = Iterables.getOnlyElement(results.get(PATCHSET_LEVEL)); + + DeleteCommentInput input = new DeleteCommentInput("reason"); + gApi.changes().id(changeId).revision(revId).comment(oldComment.id).delete(input); + CommentInfo updatedComment = + Iterables.getOnlyElement(getPublishedComments(changeId, revId).get(PATCHSET_LEVEL)); + + assertThat(updatedComment.message).doesNotContain(commentMessage); + } + + @Test + public void patchsetLevelCommentCantHaveLine() throws Exception { + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + String ps1 = result.getCommit().name(); + + CommentInput input = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + input.line = 1; + BadRequestException ex = + assertThrows(BadRequestException.class, () -> addComments(changeId, ps1, input)); + assertThat(ex.getMessage()).contains("line"); + } + + @Test + public void patchsetLevelCommentCantHaveRange() throws Exception { + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + String ps1 = result.getCommit().name(); + + CommentInput input = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + input.range = createLineRange(1, 3); + BadRequestException ex = + assertThrows(BadRequestException.class, () -> addComments(changeId, ps1, input)); + assertThat(ex.getMessage()).contains("range"); + } + + @Test + public void patchsetLevelCommentCantHaveSide() throws Exception { + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + String ps1 = result.getCommit().name(); + + CommentInput input = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + input.side = Side.REVISION; + BadRequestException ex = + assertThrows(BadRequestException.class, () -> addComments(changeId, ps1, input)); + assertThat(ex.getMessage()).contains("side"); + } + + @Test + public void patchsetLevelDraftCommentCanBeAddedAndRetrieved() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + addDraft(changeId, revId, comment); + Map> results = getDraftComments(changeId, revId); + assertThatMap(results).keys().containsExactly(PATCHSET_LEVEL); + } + + @Test + public void deletePatchsetLevelDraft() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput draft = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment 1"); + CommentInfo returned = addDraft(changeId, revId, draft); + deleteDraft(changeId, revId, returned.id); + Map> drafts = getDraftComments(changeId, revId); + assertThat(drafts).isEmpty(); + } + + @Test + public void patchsetLevelDraftCommentCantHaveLine() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + comment.line = 1; + BadRequestException ex = + assertThrows(BadRequestException.class, () -> addDraft(changeId, revId, comment)); + assertThat(ex.getMessage()).contains("line"); + } + + @Test + public void patchsetLevelDraftCommentCantHaveRange() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + comment.range = createLineRange(1, 3); + BadRequestException ex = + assertThrows(BadRequestException.class, () -> addDraft(changeId, revId, comment)); + assertThat(ex.getMessage()).contains("range"); + } + + @Test + public void patchsetLevelDraftCommentCantHaveSide() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + comment.side = Side.REVISION; + BadRequestException ex = + assertThrows(BadRequestException.class, () -> addDraft(changeId, revId, comment)); + assertThat(ex.getMessage()).contains("range"); + } + + @Test + public void patchsetLevelDraftCommentCantBeUpdatedToHaveLine() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + addDraft(changeId, revId, comment); + Map> results = getDraftComments(changeId, revId); + DraftInput update = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + update.id = Iterables.getOnlyElement(results.get(PATCHSET_LEVEL)).id; + update.line = 1; + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> updateDraft(changeId, revId, update, update.id)); + assertThat(ex.getMessage()).contains("line"); + } + + @Test + public void patchsetLevelDraftCommentCantBeUpdatedToHaveRange() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + addDraft(changeId, revId, comment); + Map> results = getDraftComments(changeId, revId); + DraftInput update = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + update.id = Iterables.getOnlyElement(results.get(PATCHSET_LEVEL)).id; + update.range = createLineRange(1, 3); + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> updateDraft(changeId, revId, update, update.id)); + assertThat(ex.getMessage()).contains("range"); + } + + @Test + public void patchsetLevelDraftCommentCantBeUpdatedToHaveSide() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + DraftInput comment = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + addDraft(changeId, revId, comment); + Map> results = getDraftComments(changeId, revId); + DraftInput update = newDraftWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment"); + update.id = Iterables.getOnlyElement(results.get(PATCHSET_LEVEL)).id; + update.side = Side.REVISION; + BadRequestException ex = + assertThrows( + BadRequestException.class, () -> updateDraft(changeId, revId, update, update.id)); + assertThat(ex.getMessage()).contains("side"); + } + @Test public void postCommentWithReply() throws Exception { for (Integer line : lines) { @@ -1083,7 +1268,7 @@ public class CommentsIT extends AbstractDaemonTest { addComments(changeId, ps4, c7, c8); // 11th commit: Add (c9) to PS2. - CommentInput c9 = newComment("b.txt", "comment 9"); + CommentInput c9 = newCommentWithOnlyMandatoryFields("b.txt", "comment 9"); addComments(changeId, ps2, c9); List commentsBeforeDelete = getChangeSortedComments(id.get()); @@ -1340,6 +1525,11 @@ public class CommentsIT extends AbstractDaemonTest { return newComment(file, Side.REVISION, 0, message, false); } + private static CommentInput newCommentWithOnlyMandatoryFields(String path, String message) { + CommentInput c = new CommentInput(); + return populate(c, path, null, null, null, null, message, false); + } + private static CommentInput newComment( String path, Side side, int line, String message, Boolean unresolved) { CommentInput c = new CommentInput(); @@ -1367,19 +1557,24 @@ public class CommentsIT extends AbstractDaemonTest { return populate(d, path, Side.PARENT, parent, line, message, false); } + private DraftInput newDraftWithOnlyMandatoryFields(String path, String message) { + DraftInput d = new DraftInput(); + return populate(d, path, null, null, null, null, message, false); + } + private static C populate( C c, String path, Side side, Integer parent, - int line, + Integer line, Comment.Range range, String message, Boolean unresolved) { c.path = path; c.side = side; c.parent = parent; - c.line = line != 0 ? line : null; + c.line = line != null && line != 0 ? line : null; c.message = message; c.unresolved = unresolved; if (range != null) { diff --git a/javatests/com/google/gerrit/entities/PatchTest.java b/javatests/com/google/gerrit/entities/PatchTest.java index 9f906a92ef..dce1b3ee88 100644 --- a/javatests/com/google/gerrit/entities/PatchTest.java +++ b/javatests/com/google/gerrit/entities/PatchTest.java @@ -24,6 +24,7 @@ public class PatchTest { public void isMagic() { assertThat(Patch.isMagic("/COMMIT_MSG")).isTrue(); assertThat(Patch.isMagic("/MERGE_LIST")).isTrue(); + assertThat(Patch.isMagic("/PATCHSET_LEVEL")).isTrue(); assertThat(Patch.isMagic("/COMMIT_MSG/")).isFalse(); assertThat(Patch.isMagic("COMMIT_MSG")).isFalse(); diff --git a/javatests/com/google/gerrit/server/patch/PatchListTest.java b/javatests/com/google/gerrit/server/patch/PatchListTest.java index e224191a5a..56adefaad6 100644 --- a/javatests/com/google/gerrit/server/patch/PatchListTest.java +++ b/javatests/com/google/gerrit/server/patch/PatchListTest.java @@ -26,16 +26,30 @@ import java.util.Arrays; import org.junit.Test; public class PatchListTest { + @Test public void fileOrder() { String[] names = { - "zzz", "def/g", "/!xxx", "abc", Patch.MERGE_LIST, "qrx", Patch.COMMIT_MSG, + "zzz", + "def/g", + "/!xxx", + "abc", + Patch.MERGE_LIST, + "qrx", + Patch.COMMIT_MSG, + Patch.PATCHSET_LEVEL }; String[] want = { - Patch.COMMIT_MSG, Patch.MERGE_LIST, "/!xxx", "abc", "def/g", "qrx", "zzz", + Patch.COMMIT_MSG, + Patch.MERGE_LIST, + Patch.PATCHSET_LEVEL, + "/!xxx", + "abc", + "def/g", + "qrx", + "zzz", }; - - Arrays.sort(names, 0, names.length, PatchList::comparePaths); + Arrays.sort(names, 0, names.length, PatchList.FILE_PATH_CMP); assertThat(names).isEqualTo(want); } @@ -48,7 +62,7 @@ public class PatchListTest { Patch.COMMIT_MSG, "/!xxx", "abc", "def/g", "qrx", "zzz", }; - Arrays.sort(names, 0, names.length, PatchList::comparePaths); + Arrays.sort(names, 0, names.length, PatchList.FILE_PATH_CMP); assertThat(names).isEqualTo(want); }