Add api portion of resolvable comments

With this change, the unresolved field of a comment is now set by the
value supplied in the request body. If no value is supplied, the value
of the parent is used, and if no parent exists then comments default to
unresolved=false.

Change-Id: I4d2e16e42a78473b78b4e5b044687df4c5554963
This commit is contained in:
Kasper Nilsson
2016-12-20 14:13:21 -08:00
parent e6bb34fbf0
commit 7ec3036ab1
11 changed files with 156 additions and 52 deletions

View File

@@ -5190,6 +5190,10 @@ Value of the `tag` field from link:#review-input[ReviewInput] set
while posting the review. while posting the review.
NOTE: To apply different tags on on different votes/comments multiple NOTE: To apply different tags on on different votes/comments multiple
invocations of the REST call are required. 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]] [[comment-input]]
@@ -5232,6 +5236,10 @@ comment is deleted.
Value of the `tag` field. Only allowed on link:#create-draft[draft comment] + Value of the `tag` field. Only allowed on link:#create-draft[draft comment] +
inputs; for published comments, use the `tag` field in + inputs; for published comments, use the `tag` field in +
link#review-input[ReviewInput] 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]] [[comment-range]]

View File

@@ -135,7 +135,33 @@ public class CommentsIT extends AbstractDaemonTest {
String changeId = r.getChangeId(); String changeId = r.getChangeId();
String revId = r.getCommit().getName(); String revId = r.getCommit().getName();
ReviewInput input = new ReviewInput(); 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<String, List<CommentInfo>> 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 = new HashMap<>();
input.comments.put(comment.path, Lists.newArrayList(comment)); input.comments.put(comment.path, Lists.newArrayList(comment));
revision(r).review(input); revision(r).review(input);
@@ -156,8 +182,9 @@ public class CommentsIT extends AbstractDaemonTest {
String changeId = r.getChangeId(); String changeId = r.getChangeId();
String revId = r.getCommit().getName(); String revId = r.getCommit().getName();
ReviewInput input = new ReviewInput(); 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 = newComment(file, Side.PARENT, line, "auto-merge of ps-1"); 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 c3 = newCommentOnParent(file, 1, line, "parent-1 of ps-1");
CommentInput c4 = newCommentOnParent(file, 2, line, "parent-2 of ps-1"); CommentInput c4 = newCommentOnParent(file, 2, line, "parent-2 of ps-1");
input.comments = new HashMap<>(); input.comments = new HashMap<>();
@@ -176,7 +203,7 @@ public class CommentsIT extends AbstractDaemonTest {
String changeId = r.getChangeId(); String changeId = r.getChangeId();
String revId = r.getCommit().getName(); String revId = r.getCommit().getName();
ReviewInput input = new ReviewInput(); 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 c2 = newCommentOnParent(file, 1, line, "parent-1 of ps-1");
CommentInput c3 = newCommentOnParent(file, 2, line, "parent-2 of ps-1"); CommentInput c3 = newCommentOnParent(file, 2, line, "parent-2 of ps-1");
input.comments = new HashMap<>(); input.comments = new HashMap<>();
@@ -193,8 +220,8 @@ public class CommentsIT extends AbstractDaemonTest {
public void postCommentOnCommitMessageOnAutoMerge() throws Exception { public void postCommentOnCommitMessageOnAutoMerge() throws Exception {
PushOneCommit.Result r = createMergeCommitChange("refs/for/master"); PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
ReviewInput input = new ReviewInput(); ReviewInput input = new ReviewInput();
CommentInput c = CommentInput c = newComment(Patch.COMMIT_MSG, Side.PARENT, 0,
newComment(Patch.COMMIT_MSG, Side.PARENT, 0, "comment on auto-merge"); "comment on auto-merge", false);
input.comments = new HashMap<>(); input.comments = new HashMap<>();
input.comments.put(Patch.COMMIT_MSG, ImmutableList.of(c)); input.comments.put(Patch.COMMIT_MSG, ImmutableList.of(c));
exception.expect(BadRequestException.class); exception.expect(BadRequestException.class);
@@ -216,7 +243,8 @@ public class CommentsIT extends AbstractDaemonTest {
List<CommentInput> expectedComments = new ArrayList<>(); List<CommentInput> expectedComments = new ArrayList<>();
for (Integer line : lines) { for (Integer line : lines) {
ReviewInput input = new ReviewInput(); 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); expectedComments.add(comment);
input.comments = new HashMap<>(); input.comments = new HashMap<>();
input.comments.put(comment.path, Lists.newArrayList(comment)); input.comments.put(comment.path, Lists.newArrayList(comment));
@@ -326,7 +354,8 @@ public class CommentsIT extends AbstractDaemonTest {
Timestamp origLastUpdated = r.getChange().change().getLastUpdatedOn(); Timestamp origLastUpdated = r.getChange().change().getLastUpdatedOn();
ReviewInput input = new ReviewInput(); 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; comment.updated = timestamp;
input.comments = new HashMap<>(); input.comments = new HashMap<>();
input.comments.put(comment.path, Lists.newArrayList(comment)); input.comments.put(comment.path, Lists.newArrayList(comment));
@@ -554,12 +583,12 @@ public class CommentsIT extends AbstractDaemonTest {
+ url + "#/c/" + c + "/1/a.txt\n" + url + "#/c/" + c + "/1/a.txt\n"
+ "File a.txt:\n" + "File a.txt:\n"
+ "\n" + "\n"
+ url + "#/c/12/1/a.txt@a2\n" + url + "#/c/" + c + "/1/a.txt@a2\n"
+ "PS1, Line 2: \n" + "PS1, Line 2: \n"
+ "what happened to this?\n" + "what happened to this?\n"
+ "\n" + "\n"
+ "\n" + "\n"
+ url + "#/c/12/1/a.txt@1\n" + url + "#/c/" + c + "/1/a.txt@1\n"
+ "PS1, Line 1: ew\n" + "PS1, Line 1: ew\n"
+ "nit: trailing whitespace\n" + "nit: trailing whitespace\n"
+ "\n" + "\n"
@@ -567,22 +596,22 @@ public class CommentsIT extends AbstractDaemonTest {
+ url + "#/c/" + c + "/2/a.txt\n" + url + "#/c/" + c + "/2/a.txt\n"
+ "File a.txt:\n" + "File a.txt:\n"
+ "\n" + "\n"
+ url + "#/c/12/2/a.txt@a1\n" + url + "#/c/" + c + "/2/a.txt@a1\n"
+ "PS2, Line 1: \n" + "PS2, Line 1: \n"
+ "comment 1 on base\n" + "comment 1 on base\n"
+ "\n" + "\n"
+ "\n" + "\n"
+ url + "#/c/12/2/a.txt@a2\n" + url + "#/c/" + c + "/2/a.txt@a2\n"
+ "PS2, Line 2: \n" + "PS2, Line 2: \n"
+ "comment 2 on base\n" + "comment 2 on base\n"
+ "\n" + "\n"
+ "\n" + "\n"
+ url + "#/c/12/2/a.txt@1\n" + url + "#/c/" + c + "/2/a.txt@1\n"
+ "PS2, Line 1: ew\n" + "PS2, Line 1: ew\n"
+ "join lines\n" + "join lines\n"
+ "\n" + "\n"
+ "\n" + "\n"
+ url + "#/c/12/2/a.txt@2\n" + url + "#/c/" + c + "/2/a.txt@2\n"
+ "PS2, Line 2: nten\n" + "PS2, Line 2: nten\n"
+ "typo: content\n" + "typo: content\n"
+ "\n" + "\n"
@@ -688,36 +717,39 @@ public class CommentsIT extends AbstractDaemonTest {
} }
private static CommentInput newComment(String path, Side side, int line, private static CommentInput newComment(String path, Side side, int line,
String message) { String message, Boolean unresolved) {
CommentInput c = new CommentInput(); 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, private static CommentInput newCommentOnParent(String path, int parent,
int line, String message) { int line, String message) {
CommentInput c = new CommentInput(); 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, private DraftInput newDraft(String path, Side side, int line,
String message) { String message) {
DraftInput d = new DraftInput(); 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, private DraftInput newDraftOnParent(String path, int parent, int line,
String message) { String message) {
DraftInput d = new DraftInput(); 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 extends Comment> C populate(C c, String path, Side side, private static <C extends Comment> 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.path = path;
c.side = side; c.side = side;
c.parent = parent; c.parent = parent;
c.line = line != 0 ? line : null; c.line = line != 0 ? line : null;
c.message = message; c.message = message;
c.unresolved = unresolved;
if (line != 0) { if (line != 0) {
Comment.Range range = new Comment.Range(); Comment.Range range = new Comment.Range();
range.startLine = line; range.startLine = line;
@@ -753,5 +785,6 @@ public class CommentsIT extends AbstractDaemonTest {
to.line = from.line; to.line = from.line;
to.message = from.message; to.message = from.message;
to.range = from.range; to.range = from.range;
to.unresolved = from.unresolved;
} }
} }

View File

@@ -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.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling; import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.client.Side; 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.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
@@ -227,7 +228,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public void draftComment() throws Exception { public void draftComment() throws Exception {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); Change.Id id = r.getPatchSetId().getParentKey();
putDraft(user, id, 1, "comment"); putDraft(user, id, 1, "comment", null);
checker.rebuildAndCheckChanges(id); checker.rebuildAndCheckChanges(id);
} }
@@ -235,7 +236,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public void draftAndPublishedComment() throws Exception { public void draftAndPublishedComment() throws Exception {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); 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"); putComment(user, id, 1, "published comment");
checker.rebuildAndCheckChanges(id); checker.rebuildAndCheckChanges(id);
} }
@@ -244,7 +245,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public void publishDraftComment() throws Exception { public void publishDraftComment() throws Exception {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); Change.Id id = r.getPatchSetId().getParentKey();
putDraft(user, id, 1, "draft comment"); putDraft(user, id, 1, "draft comment", null);
publishDrafts(user, id); publishDrafts(user, id);
checker.rebuildAndCheckChanges(id); checker.rebuildAndCheckChanges(id);
} }
@@ -371,14 +372,14 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo( assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo(
changeMetaId.name()); changeMetaId.name());
putDraft(user, id, 1, "comment by user"); putDraft(user, id, 1, "comment by user", null);
ObjectId userDraftsId = getMetaRef( ObjectId userDraftsId = getMetaRef(
allUsers, refsDraftComments(id, user.getId())); allUsers, refsDraftComments(id, user.getId()));
assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo( assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo(
changeMetaId.name() changeMetaId.name()
+ "," + user.getId() + "=" + userDraftsId.name()); + "," + user.getId() + "=" + userDraftsId.name());
putDraft(admin, id, 2, "comment by admin"); putDraft(admin, id, 2, "comment by admin", null);
ObjectId adminDraftsId = getMetaRef( ObjectId adminDraftsId = getMetaRef(
allUsers, refsDraftComments(id, admin.getId())); allUsers, refsDraftComments(id, admin.getId()));
assertThat(admin.getId().get()).isLessThan(user.getId().get()); assertThat(admin.getId().get()).isLessThan(user.getId().get());
@@ -387,7 +388,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
+ "," + admin.getId() + "=" + adminDraftsId.name() + "," + admin.getId() + "=" + adminDraftsId.name()
+ "," + user.getId() + "=" + userDraftsId.name()); + "," + user.getId() + "=" + userDraftsId.name());
putDraft(admin, id, 2, "revised comment by admin"); putDraft(admin, id, 2, "revised comment by admin", null);
adminDraftsId = getMetaRef( adminDraftsId = getMetaRef(
allUsers, refsDraftComments(id, admin.getId())); allUsers, refsDraftComments(id, admin.getId()));
assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo( assertThat(getUnwrappedDb().changes().get(id).getNoteDbState()).isEqualTo(
@@ -565,7 +566,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); Change.Id id = r.getPatchSetId().getParentKey();
putDraft(user, id, 1, "comment by user"); putDraft(user, id, 1, "comment by user", null);
assertChangeUpToDate(true, id); assertChangeUpToDate(true, id);
ObjectId oldMetaId = ObjectId oldMetaId =
@@ -573,7 +574,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
// Add a draft behind NoteDb's back. // Add a draft behind NoteDb's back.
setNotesMigration(false, false); setNotesMigration(false, false);
putDraft(user, id, 1, "second comment by user"); putDraft(user, id, 1, "second comment by user", null);
setInvalidNoteDbState(id); setInvalidNoteDbState(id);
assertDraftsUpToDate(false, id, user); assertDraftsUpToDate(false, id, user);
assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId()))) assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
@@ -608,7 +609,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); Change.Id id = r.getPatchSetId().getParentKey();
putDraft(user, id, 1, "comment by user"); putDraft(user, id, 1, "comment by user", null);
assertChangeUpToDate(true, id); assertChangeUpToDate(true, id);
ObjectId oldMetaId = ObjectId oldMetaId =
@@ -616,7 +617,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
// Add a draft behind NoteDb's back. // Add a draft behind NoteDb's back.
setNotesMigration(false, false); setNotesMigration(false, false);
putDraft(user, id, 1, "second comment by user"); putDraft(user, id, 1, "second comment by user", null);
ReviewDb db = getUnwrappedDb(); ReviewDb db = getUnwrappedDb();
Change c = db.changes().get(id); Change c = db.changes().get(id);
@@ -668,12 +669,12 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); Change.Id id = r.getPatchSetId().getParentKey();
putDraft(user, id, 1, "comment"); putDraft(user, id, 1, "comment", null);
assertDraftsUpToDate(true, id, user); assertDraftsUpToDate(true, id, user);
// Make a ReviewDb change behind NoteDb's back and ensure it's detected. // Make a ReviewDb change behind NoteDb's back and ensure it's detected.
setNotesMigration(false, false); setNotesMigration(false, false);
putDraft(user, id, 1, "comment"); putDraft(user, id, 1, "comment", null);
setInvalidNoteDbState(id); setInvalidNoteDbState(id);
assertDraftsUpToDate(false, id, user); assertDraftsUpToDate(false, id, user);
@@ -900,7 +901,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
public void rebuildDeletesOldDraftRefs() throws Exception { public void rebuildDeletesOldDraftRefs() throws Exception {
PushOneCommit.Result r = createChange(); PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey(); 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); Account.Id otherAccountId = new Account.Id(user.getId().get() + 1234);
String otherDraftRef = refsDraftComments(id, otherAccountId); String otherDraftRef = refsDraftComments(id, otherAccountId);
@@ -1162,6 +1163,23 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertThat(notes.getChange().currentPatchSetId()).isEqualTo(psId1); 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<String, List<CommentInfo>> comments =
gApi.changes().id(id.get()).current().drafts();
for (List<CommentInfo> cList: comments.values()) {
for (CommentInfo ci: cList) {
assertThat(ci.unresolved).isEqualTo(true);
}
}
}
private void assertChangesReadOnly(RestApiException e) throws Exception { private void assertChangesReadOnly(RestApiException e) throws Exception {
Throwable cause = e.getCause(); Throwable cause = e.getCause();
assertThat(cause).isInstanceOf(UpdateException.class); 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) private void putDraft(TestAccount account, Change.Id id, int line, String msg,
throws Exception { Boolean unresolved) throws Exception {
DraftInput in = new DraftInput(); DraftInput in = new DraftInput();
in.line = line; in.line = line;
in.message = msg; in.message = msg;
in.path = PushOneCommit.FILE_NAME; in.path = PushOneCommit.FILE_NAME;
in.unresolved = unresolved;
AcceptanceTestRequestScope.Context old = setApiUser(account); AcceptanceTestRequestScope.Context old = setApiUser(account);
try { try {
gApi.changes().id(id.get()).current().createDraft(in); gApi.changes().id(id.get()).current().createDraft(in);

View File

@@ -34,7 +34,7 @@ public abstract class Comment {
public String inReplyTo; public String inReplyTo;
public Timestamp updated; public Timestamp updated;
public String message; public String message;
public boolean unresolved; public Boolean unresolved;
public static class Range { public static class Range {
public int startLine; public int startLine;

View File

@@ -124,6 +124,7 @@ public final class PatchLineComment {
plc.setRevId(new RevId(c.revId)); plc.setRevId(new RevId(c.revId));
plc.setStatus(status); plc.setStatus(status);
plc.setRealAuthor(c.getRealAuthor().getId()); plc.setRealAuthor(c.getRealAuthor().getId());
plc.setUnresolved(c.unresolved);
return plc; return plc;
} }
@@ -323,6 +324,14 @@ public final class PatchLineComment {
return tag; return tag;
} }
public void setUnresolved(Boolean unresolved) {
this.unresolved = unresolved;
}
public Boolean getUnresolved() {
return unresolved;
}
public Comment asComment(String serverId) { public Comment asComment(String serverId) {
Comment c = new Comment(key.asCommentKey(), author, writtenOn, side, Comment c = new Comment(key.asCommentKey(), author, writtenOn, side,
message, serverId, unresolved); message, serverId, unresolved);
@@ -349,7 +358,8 @@ public final class PatchLineComment {
&& Objects.equals(parentUuid, c.getParentUuid()) && Objects.equals(parentUuid, c.getParentUuid())
&& Objects.equals(range, c.getRange()) && Objects.equals(range, c.getRange())
&& Objects.equals(revId, c.getRevId()) && Objects.equals(revId, c.getRevId())
&& Objects.equals(tag, c.getTag()); && Objects.equals(tag, c.getTag())
&& Objects.equals(unresolved, c.getUnresolved());
} }
return false; return false;
} }

View File

@@ -27,6 +27,7 @@ import com.google.common.collect.Ordering;
import com.google.gerrit.common.Nullable; import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo; 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.Account;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.Comment;
@@ -141,12 +142,33 @@ public class CommentsUtil {
this.serverId = serverId; this.serverId = serverId;
} }
public Comment newComment(ChangeContext ctx, String path, PatchSet.Id psId, public Comment newComment(ChangeContext ctx,
short side, String message) throws OrmException { 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<Comment> 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( Comment c = new Comment(
new Comment.Key(ChangeUtil.messageUUID(ctx.getDb()), path, psId.get()), new Comment.Key(ChangeUtil.messageUUID(ctx.getDb()), path, psId.get()),
ctx.getUser().getAccountId(), ctx.getWhen(), side, message, serverId, ctx.getUser().getAccountId(), ctx.getWhen(), side, message, serverId,
false); unresolved);
ctx.getUser().updateRealAccountId(c::setRealAuthor); ctx.getUser().updateRealAccountId(c::setRealAuthor);
return c; return c;
} }

View File

@@ -25,6 +25,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView; 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.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Comment; import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchLineComment.Status; import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
@@ -103,16 +104,20 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
@Override @Override
public boolean updateChange(ChangeContext ctx) public boolean updateChange(ChangeContext ctx)
throws ResourceNotFoundException, OrmException { throws ResourceNotFoundException, OrmException,
UnprocessableEntityException {
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (ps == null) { if (ps == null) {
throw new ResourceNotFoundException("patch set not found: " + psId); throw new ResourceNotFoundException("patch set not found: " + psId);
} }
String parentUuid = Url.decode(in.inReplyTo);
comment = commentsUtil.newComment( comment = commentsUtil.newComment(
ctx, in.path, ps.getId(), in.side(), in.message.trim()); ctx, in.path, ps.getId(), in.side(), in.message.trim(),
comment.parentUuid = Url.decode(in.inReplyTo); in.unresolved, parentUuid);
comment.setLineNbrAndRange(in.line, in.range); comment.setLineNbrAndRange(in.line, in.range);
comment.tag = in.tag; comment.tag = in.tag;
setCommentRevId( setCommentRevId(
comment, patchListCache, ctx.getChange(), ps); comment, patchListCache, ctx.getChange(), ps);

View File

@@ -694,7 +694,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
@Override @Override
public boolean updateChange(ChangeContext ctx) public boolean updateChange(ChangeContext ctx)
throws OrmException, ResourceConflictException { throws OrmException, ResourceConflictException,
UnprocessableEntityException {
user = ctx.getIdentifiedUser(); user = ctx.getIdentifiedUser();
notes = ctx.getNotes(); notes = ctx.getNotes();
ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
@@ -729,7 +730,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
approvals, oldApprovals, ctx.getWhen()); approvals, oldApprovals, ctx.getWhen());
} }
private boolean insertComments(ChangeContext ctx) throws OrmException { private boolean insertComments(ChangeContext ctx)
throws OrmException, UnprocessableEntityException {
Map<String, List<CommentInput>> map = in.comments; Map<String, List<CommentInput>> map = in.comments;
if (map == null) { if (map == null) {
map = Collections.emptyMap(); map = Collections.emptyMap();
@@ -757,16 +759,14 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
String parent = Url.decode(c.inReplyTo); String parent = Url.decode(c.inReplyTo);
Comment e = drafts.remove(Url.decode(c.id)); Comment e = drafts.remove(Url.decode(c.id));
if (e == null) { 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 { } else {
e.writtenOn = ctx.getWhen(); e.writtenOn = ctx.getWhen();
e.side = c.side(); e.side = c.side();
e.message = c.message; e.message = c.message;
} }
if (parent != null) {
e.parentUuid = parent;
}
setCommentRevId(e, patchListCache, ctx.getChange(), ps); setCommentRevId(e, patchListCache, ctx.getChange(), ps);
e.setLineNbrAndRange(c.line, c.range); e.setLineNbrAndRange(c.line, c.range);
e.tag = in.tag; e.tag = in.tag;

View File

@@ -164,6 +164,9 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
// TODO(dborowitz): Can we support changing tags via PUT? // TODO(dborowitz): Can we support changing tags via PUT?
e.tag = in.tag; e.tag = in.tag;
} }
if (in.unresolved != null) {
e.unresolved = in.unresolved;
}
return e; return e;
} }
} }

View File

@@ -18,6 +18,7 @@ import com.google.common.base.Strings;
import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -179,7 +180,8 @@ public class MailProcessor {
} }
@Override @Override
public boolean updateChange(ChangeContext ctx) throws OrmException { public boolean updateChange(ChangeContext ctx) throws OrmException,
UnprocessableEntityException {
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (ps == null) { if (ps == null) {
throw new OrmException("patch set not found: " + psId); throw new OrmException("patch set not found: " + psId);
@@ -224,12 +226,14 @@ public class MailProcessor {
} }
Comment comment = commentsUtil.newComment(ctx, fileName, Comment comment = commentsUtil.newComment(ctx, fileName,
psForComment.getId(), (short) side.ordinal(), c.message); psForComment.getId(), (short) side.ordinal(), c.message,
false, null);
comment.tag = tag; comment.tag = tag;
if (c.inReplyTo != null) { if (c.inReplyTo != null) {
comment.parentUuid = c.inReplyTo.key.uuid; comment.parentUuid = c.inReplyTo.key.uuid;
comment.lineNbr = c.inReplyTo.lineNbr; comment.lineNbr = c.inReplyTo.lineNbr;
comment.range = c.inReplyTo.range; comment.range = c.inReplyTo.range;
comment.unresolved = c.inReplyTo.unresolved;
} }
CommentsUtil.setCommentRevId(comment, patchListCache, CommentsUtil.setCommentRevId(comment, patchListCache,
ctx.getChange(), psForComment); ctx.getChange(), psForComment);

View File

@@ -434,7 +434,7 @@ public class CommentSender extends ReplyToChangeSender {
} }
/** /**
* Retrieve the file lines refered to by a comment. * Retrieve the file lines referred to by a comment.
* @param comment The comment that refers to some file contents. The comment * @param comment The comment that refers to some file contents. The comment
* may be a line comment or a ranged comment. * may be a line comment or a ranged comment.
* @param fileData The file on which the comment appears. * @param fileData The file on which the comment appears.