diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 98897a5c53..c291dd4886 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -5190,6 +5190,10 @@ Value of the `tag` field from link:#review-input[ReviewInput] set while posting the review. NOTE: To apply different tags on on different votes/comments multiple invocations of the REST call are required. +|`unresolved` |optional| +Whether or not the comment must be addressed by the user. The state of +resolution of a comment thread is stored in the last comment in that thread +chronologically. |=========================== [[comment-input]] @@ -5232,6 +5236,10 @@ comment is deleted. Value of the `tag` field. Only allowed on link:#create-draft[draft comment] + inputs; for published comments, use the `tag` field in + link#review-input[ReviewInput] +|`unresolved` |optional| +Whether or not the comment must be addressed by the user. This value will +default to false if the comment is an orphan, or the value of the `in_reply_to` +comment if it is supplied. |=========================== [[comment-range]] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java index f0f4566155..5b087a4a53 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -135,7 +135,33 @@ public class CommentsIT extends AbstractDaemonTest { String changeId = r.getChangeId(); String revId = r.getCommit().getName(); ReviewInput input = new ReviewInput(); - CommentInput comment = newComment(file, Side.REVISION, line, "comment 1"); + CommentInput comment = + newComment(file, Side.REVISION, line, "comment 1", false); + input.comments = new HashMap<>(); + input.comments.put(comment.path, Lists.newArrayList(comment)); + revision(r).review(input); + Map> result = getPublishedComments(changeId, revId); + assertThat(result).isNotEmpty(); + CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path)); + assertThat(comment).isEqualTo(infoToInput(file).apply(actual)); + assertThat(comment).isEqualTo(infoToInput(file).apply( + getPublishedComment(changeId, revId, actual.id))); + } + } + + @Test + public void postCommentWithUnresolved() throws Exception { + for (Integer line : lines) { + String file = "file"; + String contents = "contents " + line; + PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo, + "first subject", file, contents); + PushOneCommit.Result r = push.to("refs/for/master"); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + ReviewInput input = new ReviewInput(); + CommentInput comment = + newComment(file, Side.REVISION, line, "comment 1", true); input.comments = new HashMap<>(); input.comments.put(comment.path, Lists.newArrayList(comment)); revision(r).review(input); @@ -156,8 +182,9 @@ public class CommentsIT extends AbstractDaemonTest { String changeId = r.getChangeId(); String revId = r.getCommit().getName(); ReviewInput input = new ReviewInput(); - CommentInput c1 = newComment(file, Side.REVISION, line, "ps-1"); - CommentInput c2 = newComment(file, Side.PARENT, line, "auto-merge of ps-1"); + CommentInput c1 = newComment(file, Side.REVISION, line, "ps-1", false); + CommentInput c2 = + newComment(file, Side.PARENT, line, "auto-merge of ps-1", false); CommentInput c3 = newCommentOnParent(file, 1, line, "parent-1 of ps-1"); CommentInput c4 = newCommentOnParent(file, 2, line, "parent-2 of ps-1"); input.comments = new HashMap<>(); @@ -176,7 +203,7 @@ public class CommentsIT extends AbstractDaemonTest { String changeId = r.getChangeId(); String revId = r.getCommit().getName(); ReviewInput input = new ReviewInput(); - CommentInput c1 = newComment(file, Side.REVISION, line, "ps-1"); + CommentInput c1 = newComment(file, Side.REVISION, line, "ps-1", false); CommentInput c2 = newCommentOnParent(file, 1, line, "parent-1 of ps-1"); CommentInput c3 = newCommentOnParent(file, 2, line, "parent-2 of ps-1"); input.comments = new HashMap<>(); @@ -193,8 +220,8 @@ public class CommentsIT extends AbstractDaemonTest { public void postCommentOnCommitMessageOnAutoMerge() throws Exception { PushOneCommit.Result r = createMergeCommitChange("refs/for/master"); ReviewInput input = new ReviewInput(); - CommentInput c = - newComment(Patch.COMMIT_MSG, Side.PARENT, 0, "comment on auto-merge"); + CommentInput c = newComment(Patch.COMMIT_MSG, Side.PARENT, 0, + "comment on auto-merge", false); input.comments = new HashMap<>(); input.comments.put(Patch.COMMIT_MSG, ImmutableList.of(c)); exception.expect(BadRequestException.class); @@ -216,7 +243,8 @@ public class CommentsIT extends AbstractDaemonTest { List expectedComments = new ArrayList<>(); for (Integer line : lines) { ReviewInput input = new ReviewInput(); - CommentInput comment = newComment(file, Side.REVISION, line, "comment " + line); + CommentInput comment = + newComment(file, Side.REVISION, line, "comment " + line, false); expectedComments.add(comment); input.comments = new HashMap<>(); input.comments.put(comment.path, Lists.newArrayList(comment)); @@ -326,7 +354,8 @@ public class CommentsIT extends AbstractDaemonTest { Timestamp origLastUpdated = r.getChange().change().getLastUpdatedOn(); ReviewInput input = new ReviewInput(); - CommentInput comment = newComment(file, Side.REVISION, line, "comment 1"); + CommentInput comment = + newComment(file, Side.REVISION, line, "comment 1", false); comment.updated = timestamp; input.comments = new HashMap<>(); input.comments.put(comment.path, Lists.newArrayList(comment)); @@ -554,12 +583,12 @@ public class CommentsIT extends AbstractDaemonTest { + url + "#/c/" + c + "/1/a.txt\n" + "File a.txt:\n" + "\n" - + url + "#/c/12/1/a.txt@a2\n" + + url + "#/c/" + c + "/1/a.txt@a2\n" + "PS1, Line 2: \n" + "what happened to this?\n" + "\n" + "\n" - + url + "#/c/12/1/a.txt@1\n" + + url + "#/c/" + c + "/1/a.txt@1\n" + "PS1, Line 1: ew\n" + "nit: trailing whitespace\n" + "\n" @@ -567,22 +596,22 @@ public class CommentsIT extends AbstractDaemonTest { + url + "#/c/" + c + "/2/a.txt\n" + "File a.txt:\n" + "\n" - + url + "#/c/12/2/a.txt@a1\n" + + url + "#/c/" + c + "/2/a.txt@a1\n" + "PS2, Line 1: \n" + "comment 1 on base\n" + "\n" + "\n" - + url + "#/c/12/2/a.txt@a2\n" + + url + "#/c/" + c + "/2/a.txt@a2\n" + "PS2, Line 2: \n" + "comment 2 on base\n" + "\n" + "\n" - + url + "#/c/12/2/a.txt@1\n" + + url + "#/c/" + c + "/2/a.txt@1\n" + "PS2, Line 1: ew\n" + "join lines\n" + "\n" + "\n" - + url + "#/c/12/2/a.txt@2\n" + + url + "#/c/" + c + "/2/a.txt@2\n" + "PS2, Line 2: nten\n" + "typo: content\n" + "\n" @@ -688,36 +717,39 @@ public class CommentsIT extends AbstractDaemonTest { } private static CommentInput newComment(String path, Side side, int line, - String message) { + String message, Boolean unresolved) { CommentInput c = new CommentInput(); - return populate(c, path, side, null, line, message); + return populate(c, path, side, null, line, message, unresolved); } private static CommentInput newCommentOnParent(String path, int parent, int line, String message) { CommentInput c = new CommentInput(); - return populate(c, path, Side.PARENT, Integer.valueOf(parent), line, message); + return populate(c, path, Side.PARENT, Integer.valueOf(parent), line, + message, false); } private DraftInput newDraft(String path, Side side, int line, String message) { DraftInput d = new DraftInput(); - return populate(d, path, side, null, line, message); + return populate(d, path, side, null, line, message, false); } private DraftInput newDraftOnParent(String path, int parent, int line, String message) { DraftInput d = new DraftInput(); - return populate(d, path, Side.PARENT, Integer.valueOf(parent), line, message); + return populate(d, path, Side.PARENT, Integer.valueOf(parent), line, + message, false); } private static C populate(C c, String path, Side side, - Integer parent, int line, String message) { + Integer parent, int line, String message, Boolean unresolved) { c.path = path; c.side = side; c.parent = parent; c.line = line != 0 ? line : null; c.message = message; + c.unresolved = unresolved; if (line != 0) { Comment.Range range = new Comment.Range(); range.startLine = line; @@ -753,5 +785,6 @@ public class CommentsIT extends AbstractDaemonTest { to.line = from.line; to.message = from.message; to.range = from.range; + to.unresolved = from.unresolved; } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index 15e5e78d31..555a81984a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java @@ -38,6 +38,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput; import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling; import com.google.gerrit.extensions.client.Side; +import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; @@ -227,7 +228,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { public void draftComment() throws Exception { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "comment"); + putDraft(user, id, 1, "comment", null); checker.rebuildAndCheckChanges(id); } @@ -235,7 +236,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { public void draftAndPublishedComment() throws Exception { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "draft comment"); + putDraft(user, id, 1, "draft comment", null); putComment(user, id, 1, "published comment"); checker.rebuildAndCheckChanges(id); } @@ -244,7 +245,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { public void publishDraftComment() throws Exception { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "draft comment"); + putDraft(user, id, 1, "draft comment", null); publishDrafts(user, id); checker.rebuildAndCheckChanges(id); } @@ -371,14 +372,14 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo( changeMetaId.name()); - putDraft(user, id, 1, "comment by user"); + putDraft(user, id, 1, "comment by user", null); ObjectId userDraftsId = getMetaRef( allUsers, refsDraftComments(id, user.getId())); assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo( changeMetaId.name() + "," + user.getId() + "=" + userDraftsId.name()); - putDraft(admin, id, 2, "comment by admin"); + putDraft(admin, id, 2, "comment by admin", null); ObjectId adminDraftsId = getMetaRef( allUsers, refsDraftComments(id, admin.getId())); assertThat(admin.getId().get()).isLessThan(user.getId().get()); @@ -387,7 +388,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { + "," + admin.getId() + "=" + adminDraftsId.name() + "," + user.getId() + "=" + userDraftsId.name()); - putDraft(admin, id, 2, "revised comment by admin"); + putDraft(admin, id, 2, "revised comment by admin", null); adminDraftsId = getMetaRef( allUsers, refsDraftComments(id, admin.getId())); assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo( @@ -565,7 +566,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "comment by user"); + putDraft(user, id, 1, "comment by user", null); assertChangeUpToDate(true, id); ObjectId oldMetaId = @@ -573,7 +574,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { // Add a draft behind NoteDb's back. setNotesMigration(false, false); - putDraft(user, id, 1, "second comment by user"); + putDraft(user, id, 1, "second comment by user", null); setInvalidNoteDbState(id); assertDraftsUpToDate(false, id, user); assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId()))) @@ -608,7 +609,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "comment by user"); + putDraft(user, id, 1, "comment by user", null); assertChangeUpToDate(true, id); ObjectId oldMetaId = @@ -616,7 +617,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { // Add a draft behind NoteDb's back. setNotesMigration(false, false); - putDraft(user, id, 1, "second comment by user"); + putDraft(user, id, 1, "second comment by user", null); ReviewDb db = getUnwrappedDb(); Change c = db.changes().get(id); @@ -668,12 +669,12 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "comment"); + putDraft(user, id, 1, "comment", null); assertDraftsUpToDate(true, id, user); // Make a ReviewDb change behind NoteDb's back and ensure it's detected. setNotesMigration(false, false); - putDraft(user, id, 1, "comment"); + putDraft(user, id, 1, "comment", null); setInvalidNoteDbState(id); assertDraftsUpToDate(false, id, user); @@ -900,7 +901,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { public void rebuildDeletesOldDraftRefs() throws Exception { PushOneCommit.Result r = createChange(); Change.Id id = r.getPatchSetId().getParentKey(); - putDraft(user, id, 1, "comment"); + putDraft(user, id, 1, "comment", null); Account.Id otherAccountId = new Account.Id(user.getId().get() + 1234); String otherDraftRef = refsDraftComments(id, otherAccountId); @@ -1162,6 +1163,23 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId1); } + @Test + public void resolveCommentsInheritsValueFromParentWhenUnspecified() + throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getPatchSetId().getParentKey(); + putDraft(user, id, 1, "comment", true); + putDraft(user, id, 1, "newComment", null); + + Map> comments = + gApi.changes().id(id.get()).current().drafts(); + for (List cList: comments.values()) { + for (CommentInfo ci: cList) { + assertThat(ci.unresolved).isEqualTo(true); + } + } + } + private void assertChangesReadOnly(RestApiException e) throws Exception { Throwable cause = e.getCause(); assertThat(cause).isInstanceOf(UpdateException.class); @@ -1213,12 +1231,13 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { } } - private void putDraft(TestAccount account, Change.Id id, int line, String msg) - throws Exception { + private void putDraft(TestAccount account, Change.Id id, int line, String msg, + Boolean unresolved) throws Exception { DraftInput in = new DraftInput(); in.line = line; in.message = msg; in.path = PushOneCommit.FILE_NAME; + in.unresolved = unresolved; AcceptanceTestRequestScope.Context old = setApiUser(account); try { gApi.changes().id(id.get()).current().createDraft(in); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java index 24e60945c6..0bf116a87b 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java @@ -34,7 +34,7 @@ public abstract class Comment { public String inReplyTo; public Timestamp updated; public String message; - public boolean unresolved; + public Boolean unresolved; public static class Range { public int startLine; diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java index accb749278..d5edb759b3 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java @@ -124,6 +124,7 @@ public final class PatchLineComment { plc.setRevId(new RevId(c.revId)); plc.setStatus(status); plc.setRealAuthor(c.getRealAuthor().getId()); + plc.setUnresolved(c.unresolved); return plc; } @@ -323,6 +324,14 @@ public final class PatchLineComment { return tag; } + public void setUnresolved(Boolean unresolved) { + this.unresolved = unresolved; + } + + public Boolean getUnresolved() { + return unresolved; + } + public Comment asComment(String serverId) { Comment c = new Comment(key.asCommentKey(), author, writtenOn, side, message, serverId, unresolved); @@ -349,7 +358,8 @@ public final class PatchLineComment { && Objects.equals(parentUuid, c.getParentUuid()) && Objects.equals(range, c.getRange()) && Objects.equals(revId, c.getRevId()) - && Objects.equals(tag, c.getTag()); + && Objects.equals(tag, c.getTag()) + && Objects.equals(unresolved, c.getUnresolved()); } return false; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java index 7349519df2..f012c2107e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java @@ -27,6 +27,7 @@ import com.google.common.collect.Ordering; import com.google.gerrit.common.Nullable; 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.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Comment; @@ -141,12 +142,33 @@ public class CommentsUtil { this.serverId = serverId; } - public Comment newComment(ChangeContext ctx, String path, PatchSet.Id psId, - short side, String message) throws OrmException { + public Comment newComment(ChangeContext ctx, + String path, + PatchSet.Id psId, + short side, + String message, + @Nullable Boolean unresolved, + @Nullable String parentUuid) + throws OrmException, UnprocessableEntityException { + if (unresolved == null) { + if (parentUuid == null) { + // Default to false if comment is not descended from another. + unresolved = false; + } else { + // Inherit unresolved value from inReplyTo comment if not specified. + Comment.Key key = new Comment.Key(parentUuid, path, psId.patchSetId); + Optional parent = get(ctx.getDb(), ctx.getNotes(), key); + if (!parent.isPresent()) { + throw new UnprocessableEntityException( + "Invalid parentUuid supplied for comment"); + } + unresolved = parent.get().unresolved; + } + } Comment c = new Comment( new Comment.Key(ChangeUtil.messageUUID(ctx.getDb()), path, psId.get()), ctx.getUser().getAccountId(), ctx.getWhen(), side, message, serverId, - false); + unresolved); ctx.getUser().updateRealAccountId(c::setRealAuthor); return c; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java index 47821f6a9a..5a7b75675e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java @@ -25,6 +25,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.PatchLineComment.Status; @@ -103,16 +104,20 @@ public class CreateDraftComment implements RestModifyView @Override public boolean updateChange(ChangeContext ctx) - throws OrmException, ResourceConflictException { + throws OrmException, ResourceConflictException, + UnprocessableEntityException { user = ctx.getIdentifiedUser(); notes = ctx.getNotes(); ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); @@ -729,7 +730,8 @@ public class PostReview implements RestModifyView approvals, oldApprovals, ctx.getWhen()); } - private boolean insertComments(ChangeContext ctx) throws OrmException { + private boolean insertComments(ChangeContext ctx) + throws OrmException, UnprocessableEntityException { Map> map = in.comments; if (map == null) { map = Collections.emptyMap(); @@ -757,16 +759,14 @@ public class PostReview implements RestModifyView String parent = Url.decode(c.inReplyTo); Comment e = drafts.remove(Url.decode(c.id)); if (e == null) { - e = commentsUtil.newComment(ctx, path, psId, c.side(), c.message); + e = commentsUtil.newComment(ctx, path, psId, c.side(), c.message, + c.unresolved, parent); } else { e.writtenOn = ctx.getWhen(); e.side = c.side(); e.message = c.message; } - if (parent != null) { - e.parentUuid = parent; - } setCommentRevId(e, patchListCache, ctx.getChange(), ps); e.setLineNbrAndRange(c.line, c.range); e.tag = in.tag; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java index 0742c6d18e..46bf6a9094 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java @@ -164,6 +164,9 @@ public class PutDraftComment implements RestModifyView