diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8c10bc7bbc..5edd95634a 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -1146,6 +1146,109 @@ Adds or updates the change in the secondary index. HTTP/1.1 204 No Content ---- +[[list-change-comments]] +=== List Change Comments +-- +'GET /changes/link:#change-id[\{change-id\}]/comments' +-- + +Lists the published comments of all revisions of the change. + +Returns a map of file paths to lists of link:#comment-info[CommentInfo] +entries. The entries in the map are sorted by file path, and the +comments for each path are sorted by patch set number. Each comment has +the `patch_set` and `author` fields set. + +.Request +---- + GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0 +---- + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": [ + { + "patch_set": 1, + "id": "TvcXrmjM", + "line": 23, + "message": "[nit] trailing whitespace", + "updated": "2013-02-26 15:40:43.986000000" + "author": { + "_account_id": 1000096, + "name": "John Doe", + "email": "john.doe@example.com" + } + }, + { + "patch_set": 2, + "id": "TveXwFiA", + "line": 49, + "in_reply_to": "TfYX-Iuo", + "message": "Done", + "updated": "2013-02-26 15:40:45.328000000" + "author": { + "_account_id": 1000097, + "name": "Jane Roe", + "email": "jane.roe@example.com" + } + } + ] + } +---- + +[[list-change-drafts]] +=== List Change Drafts +-- +'GET /changes/link:#change-id[\{change-id\}]/drafts' +-- + +Lists the draft comments of all revisions of the change that belong to +the calling user. + +Returns a map of file paths to lists of link:#comment-info[CommentInfo] +entries. The entries in the map are sorted by file path, and the +comments for each path are sorted by patch set number. Each comment has +the `patch_set` field set, and no `author`. + +.Request +---- + GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/drafts HTTP/1.0 +---- + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + "gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java": [ + { + "patch_set": 1, + "id": "TvcXrmjM", + "line": 23, + "message": "[nit] trailing whitespace", + "updated": "2013-02-26 15:40:43.986000000" + }, + { + "patch_set": 2, + "id": "TveXwFiA", + "line": 49, + "in_reply_to": "TfYX-Iuo", + "message": "Done", + "updated": "2013-02-26 15:40:45.328000000" + } + ] + } +---- + [[check-change]] === Check change -- @@ -2586,7 +2689,7 @@ all LFs are included in the Prolog code: ---- [[list-drafts]] -=== List Drafts +=== List Revision Drafts -- 'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/drafts/' -- @@ -2594,9 +2697,8 @@ all LFs are included in the Prolog code: Lists the draft comments of a revision that belong to the calling user. -As result a map is returned that maps the file path to a list of -link:#comment-info[CommentInfo] entries. The entries in the map are -sorted by file path. +Returns a map of file paths to lists of link:#comment-info[CommentInfo] +entries. The entries in the map are sorted by file path. .Request ---- @@ -2765,7 +2867,7 @@ Deletes a draft comment from a revision. ---- [[list-comments]] -=== List Comments +=== List Revision Comments -- 'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/comments/' -- @@ -3494,6 +3596,9 @@ The `CommentInfo` entity contains information about an inline comment. [options="header",cols="1,^1,5"] |=========================== |Field Name ||Description +|`patch_set` |optional| +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. + @@ -4013,7 +4118,9 @@ will be modified to be the "best" value allowed by the access controls. Draft handling that defines how draft comments are handled that are already in the database but that were not also described in this input. + -Allowed values are `DELETE`, `PUBLISH` and `KEEP`. + +Allowed values are `DELETE`, `PUBLISH`, `PUBLISH_ALL_REVISIONS` and +`KEEP`. All values except `PUBLISH_ALL_REVISIONS` operate only on drafts +for a single revision. + If not set, the default is `DELETE`. |`notify` |optional| Notify handling that defines to whom email notifications should be sent 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 c05b56f124..2fa6eb2e92 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 @@ -15,7 +15,11 @@ package com.google.gerrit.acceptance.server.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; +import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -24,6 +28,7 @@ import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.extensions.api.changes.DraftInput; 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.Comment; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.CommentInfo; @@ -34,8 +39,11 @@ import com.google.gerrit.server.change.ChangesCollection; import com.google.gerrit.server.change.PostReview; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Revisions; +import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.testutil.ConfigSuite; +import com.google.gerrit.testutil.FakeEmailSender; +import com.google.gerrit.testutil.FakeEmailSender.Message; import com.google.inject.Inject; import com.google.inject.Provider; @@ -64,6 +72,13 @@ public class CommentsIT extends AbstractDaemonTest { @Inject private Provider postReview; + @Inject + @CanonicalWebUrl + private Provider urlProvider; + + @Inject + private FakeEmailSender email; + private final Integer[] lines = {0, 1}; @Before @@ -186,6 +201,186 @@ public class CommentsIT extends AbstractDaemonTest { } } + @Test + public void listChangeDrafts() throws Exception { + PushOneCommit.Result r1 = createChange(); + + PushOneCommit.Result r2 = pushFactory.create( + db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent", + r1.getChangeId()) + .to("refs/for/master"); + + + setApiUser(admin); + addDraft(r1.getChangeId(), r1.getCommit().getName(), + newDraft(FILE_NAME, Side.REVISION, 1, "nit: trailing whitespace")); + addDraft(r2.getChangeId(), r2.getCommit().getName(), + newDraft(FILE_NAME, Side.REVISION, 1, "typo: content")); + + setApiUser(user); + addDraft(r2.getChangeId(), r2.getCommit().getName(), + newDraft(FILE_NAME, Side.REVISION, 1, "+1, please fix")); + + setApiUser(admin); + Map> actual = + gApi.changes().id(r1.getChangeId()).drafts(); + assertThat((Iterable) actual.keySet()).containsExactly(FILE_NAME); + List comments = actual.get(FILE_NAME); + assertThat(comments).hasSize(2); + + CommentInfo c1 = comments.get(0); + assertThat(c1.author).isNull(); + assertThat(c1.patchSet).isEqualTo(1); + assertThat(c1.message).isEqualTo("nit: trailing whitespace"); + assertThat(c1.side).isNull(); + assertThat(c1.line).isEqualTo(1); + + CommentInfo c2 = comments.get(1); + assertThat(c2.author).isNull(); + assertThat(c2.patchSet).isEqualTo(2); + assertThat(c2.message).isEqualTo("typo: content"); + assertThat(c2.side).isNull(); + assertThat(c2.line).isEqualTo(1); + } + + @Test + public void listChangeComments() throws Exception { + PushOneCommit.Result r1 = createChange(); + + PushOneCommit.Result r2 = pushFactory.create( + db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent", + r1.getChangeId()) + .to("refs/for/master"); + + addComment(r1, "nit: trailing whitespace"); + addComment(r2, "typo: content"); + + Map> actual = gApi.changes() + .id(r2.getChangeId()) + .comments(); + assertThat(actual.keySet()).containsExactly(FILE_NAME); + + List comments = actual.get(FILE_NAME); + assertThat(comments).hasSize(2); + + CommentInfo c1 = comments.get(0); + assertThat(c1.author._accountId).isEqualTo(user.getId().get()); + assertThat(c1.patchSet).isEqualTo(1); + assertThat(c1.message).isEqualTo("nit: trailing whitespace"); + assertThat(c1.side).isNull(); + assertThat(c1.line).isEqualTo(1); + + CommentInfo c2 = comments.get(1); + assertThat(c2.author._accountId).isEqualTo(user.getId().get()); + assertThat(c2.patchSet).isEqualTo(2); + assertThat(c2.message).isEqualTo("typo: content"); + assertThat(c2.side).isNull(); + assertThat(c2.line).isEqualTo(1); + } + + @Test + public void publishCommentsAllRevisions() throws Exception { + PushOneCommit.Result r1 = createChange(); + + PushOneCommit.Result r2 = pushFactory.create( + db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new\ncntent\n", + r1.getChangeId()) + .to("refs/for/master"); + + addDraft(r1.getChangeId(), r1.getCommit().getName(), + newDraft(FILE_NAME, Side.REVISION, 1, "nit: trailing whitespace")); + addDraft(r2.getChangeId(), r2.getCommit().getName(), + newDraft(FILE_NAME, Side.REVISION, 1, "join lines")); + addDraft(r2.getChangeId(), r2.getCommit().getName(), + newDraft(FILE_NAME, Side.REVISION, 2, "typo: content")); + + ReviewInput reviewInput = new ReviewInput(); + reviewInput.drafts = DraftHandling.PUBLISH_ALL_REVISIONS; + reviewInput.message = "comments"; + gApi.changes() + .id(r2.getChangeId()) + .current() + .review(reviewInput); + + assertThat(gApi.changes() + .id(r1.getChangeId()) + .revision(r1.getCommit().name()) + .drafts()) + .isEmpty(); + Map> ps1Map = gApi.changes() + .id(r1.getChangeId()) + .revision(r1.getCommit().name()) + .comments(); + assertThat(ps1Map.keySet()).containsExactly(FILE_NAME); + List ps1List = ps1Map.get(FILE_NAME); + assertThat(ps1List).hasSize(1); + assertThat(ps1List.get(0).message).isEqualTo("nit: trailing whitespace"); + + assertThat(gApi.changes() + .id(r2.getChangeId()) + .revision(r2.getCommit().name()) + .drafts()) + .isEmpty(); + Map> ps2Map = gApi.changes() + .id(r2.getChangeId()) + .revision(r2.getCommit().name()) + .comments(); + assertThat(ps2Map.keySet()).containsExactly(FILE_NAME); + List ps2List = ps2Map.get(FILE_NAME); + assertThat(ps2List).hasSize(2); + assertThat(ps2List.get(0).message).isEqualTo("join lines"); + assertThat(ps2List.get(1).message).isEqualTo("typo: content"); + + ImmutableList messages = + email.getMessages(r2.getChangeId(), "comment"); + assertThat(messages).hasSize(1); + String url = urlProvider.get(); + int c = r1.getChange().getId().get(); + assertThat(messages.get(0).body()).contains( + "\n" + + "Patch Set 2:\n" + + "\n" + + "(3 comments)\n" + + "\n" + + "comments\n" + + "\n" + + url + "#/c/" + c + "/1/a.txt\n" + + "File a.txt:\n" + + "\n" + + "PS1, Line 1: ew\n" + + "nit: trailing whitespace\n" + + "\n" + + "\n" + + url + "#/c/" + c + "/2/a.txt\n" + + "File a.txt:\n" + + "\n" + + "PS2, Line 1: ew\n" + + "join lines\n" + + "\n" + + "\n" + + "PS2, Line 2: nten\n" + + "typo: content\n" + + "\n" + + "\n" + + "-- \n"); + } + + + private void addComment(PushOneCommit.Result r, String message) + throws Exception { + CommentInput c = new CommentInput(); + c.line = 1; + c.message = message; + c.path = FILE_NAME; + ReviewInput in = new ReviewInput(); + in.comments = ImmutableMap.> of( + FILE_NAME, ImmutableList.of(c)); + gApi.changes() + .id(r.getChangeId()) + .revision(r.getCommit().name()) + .review(in); + } + private CommentInfo addDraft(String changeId, String revId, DraftInput in) throws Exception { return gApi.changes().id(changeId).revision(revId).createDraft(in).get(); @@ -259,9 +454,9 @@ public class CommentsIT extends AbstractDaemonTest { c.message = message; if (line != 0) { Comment.Range range = new Comment.Range(); - range.startLine = 1; + range.startLine = line; range.startCharacter = 1; - range.endLine = 1; + range.endLine = line; range.endCharacter = 5; c.range = range; } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 06f0a75e31..6781af275a 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -16,6 +16,7 @@ package com.google.gerrit.extensions.api.changes; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.SuggestedReviewerInfo; import com.google.gerrit.extensions.restapi.NotImplementedException; @@ -23,6 +24,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import java.util.EnumSet; import java.util.List; +import java.util.Map; import java.util.Set; public interface ChangeApi { @@ -106,6 +108,24 @@ public interface ChangeApi { */ Set getHashtags() throws RestApiException; + /** + * Get all published comments on a change. + * + * @return comments in a map keyed by path; comments have the {@code revision} + * field set to indicate their patch set. + * @throws RestApiException + */ + Map> comments() throws RestApiException; + + /** + * Get all draft comments for the current user on a change. + * + * @return drafts in a map keyed by path; comments have the {@code revision} + * field set to indicate their patch set. + * @throws RestApiException + */ + Map> drafts() throws RestApiException; + ChangeInfo check() throws RestApiException; ChangeInfo check(FixInput fix) throws RestApiException; @@ -249,6 +269,16 @@ public interface ChangeApi { throw new NotImplementedException(); } + @Override + public Map> comments() throws RestApiException { + throw new NotImplementedException(); + } + + @Override + public Map> drafts() throws RestApiException { + throw new NotImplementedException(); + } + @Override public ChangeInfo check() throws RestApiException { throw new NotImplementedException(); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java index dd2ce9250c..dc22fb053f 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java @@ -61,7 +61,17 @@ public class ReviewInput { public String onBehalfOf; public static enum DraftHandling { - DELETE, PUBLISH, KEEP + /** Delete pending drafts on this revision only. */ + DELETE, + + /** Publish pending drafts on this revision only. */ + PUBLISH, + + /** Leave pending drafts alone. */ + KEEP, + + /** Publish pending drafts on all revisions. */ + PUBLISH_ALL_REVISIONS; } public static enum NotifyHandling { 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 e79df1c026..b9863d7652 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 @@ -17,6 +17,13 @@ package com.google.gerrit.extensions.client; import java.sql.Timestamp; public abstract class Comment { + /** + * Patch set number containing this commit. + *

+ * Only set in contexts where comments may come from multiple patch sets. + */ + public Integer patchSet; + public String id; public String path; public Side side; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java index 6d237938a6..9545b84548 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java @@ -908,16 +908,19 @@ public class ChangeScreen extends Screen { } private List>> loadComments( - RevisionInfo rev, CallbackGroup group) { - final int id = rev._number(); + final RevisionInfo rev, CallbackGroup group) { final List>> r = new ArrayList<>(1); - ChangeApi.revision(changeId.get(), rev.name()) - .view("comments") + // TODO(dborowitz): Could eliminate this call by adding an option to include + // inline comments in the change detail. + ChangeApi.comments(changeId.get()) .get(group.add(new AsyncCallback>>() { @Override public void onSuccess(NativeMap> result) { - r.add(result); - history.addComments(id, result); + // Return value is used for populating the file table, so only count + // comments for the current revision. Still include all comments in + // the history table. + r.add(filterForRevision(result, rev._number())); + history.addComments(result); } @Override @@ -927,6 +930,23 @@ public class ChangeScreen extends Screen { return r; } + private static NativeMap> filterForRevision( + NativeMap> comments, int id) { + NativeMap> filtered = NativeMap.create(); + for (String k : comments.keySet()) { + JsArray allRevisions = comments.get(k); + JsArray thisRevision = JsArray.createArray().cast(); + for (int i = 0; i < allRevisions.length(); i++) { + CommentInfo c = allRevisions.get(i); + if (c.patch_set() == id) { + thisRevision.push(c); + } + } + filtered.put(k, thisRevision); + } + return filtered; + } + private List>> loadDrafts( RevisionInfo rev, CallbackGroup group) { final List>> r = new ArrayList<>(1); @@ -1129,10 +1149,7 @@ public class ChangeScreen extends Screen { initRevisionsAction(info, revision, emptyMap); quickApprove.setVisible(false); actions.reloadRevisionActions(emptyMap); - } - private void renderRevisionInfo(ChangeInfo info, - NativeMap actionMap) { RevisionInfo revisionInfo = info.revision(revision); boolean current = revision.equals(info.current_revision()) && !revisionInfo.is_edit(); @@ -1146,8 +1163,6 @@ public class ChangeScreen extends Screen { statusText.setInnerText(Util.toLongString(info.status())); } - initRevisionsAction(info, revision, actionMap); - if (Gerrit.isSignedIn()) { replyAction = new ReplyAction(info, revision, hasDraftComments, style, commentLinkProcessor, reply, quickApprove); @@ -1160,6 +1175,11 @@ public class ChangeScreen extends Screen { } else { quickApprove.setVisible(false); } + } + + private void renderRevisionInfo(ChangeInfo info, + NativeMap actionMap) { + initRevisionsAction(info, revision, actionMap); actions.reloadRevisionActions(actionMap); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java index 71720119ec..e73c70a0ba 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java @@ -26,8 +26,6 @@ import com.google.gwt.user.client.ui.Composite; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.HTMLPanel; -import java.util.Collections; -import java.util.Comparator; import java.util.List; class FileComments extends Composite { @@ -38,22 +36,15 @@ class FileComments extends Composite { @UiField FlowPanel comments; FileComments(CommentLinkProcessor clp, - PatchSet.Id ps, + PatchSet.Id defaultPs, String title, List list) { initWidget(uiBinder.createAndBindUi(this)); - path.setTargetHistoryToken(url(ps, list.get(0))); + path.setTargetHistoryToken(url(defaultPs, list.get(0))); path.setText(title); - - Collections.sort(list, new Comparator() { - @Override - public int compare(CommentInfo a, CommentInfo b) { - return a.line() - b.line(); - } - }); for (CommentInfo c : list) { - comments.add(new LineComment(clp, ps, c)); + comments.add(new LineComment(clp, defaultPs, c)); } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java index 7635d81a1b..d1f6f2e4fc 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java @@ -14,8 +14,6 @@ package com.google.gerrit.client.change; -import com.google.gerrit.client.account.AccountInfo; -import com.google.gerrit.client.changes.ChangeApi; import com.google.gerrit.client.changes.ChangeInfo; import com.google.gerrit.client.changes.ChangeInfo.MessageInfo; import com.google.gerrit.client.changes.CommentInfo; @@ -23,9 +21,7 @@ import com.google.gerrit.client.rpc.NativeMap; import com.google.gerrit.client.rpc.Natives; import com.google.gerrit.client.ui.CommentLinkProcessor; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gwt.core.client.JsArray; -import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.Widget; @@ -33,22 +29,15 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; class History extends FlowPanel { private CommentLinkProcessor clp; private ReplyAction replyAction; private Change.Id changeId; - private final Set loaded = new HashSet<>(); - private final Map> byAuthor = - new HashMap<>(); - - private final List toLoad = new ArrayList<>(4); - private int active; + private final Map> byAuthor = new HashMap<>(); void set(CommentLinkProcessor clp, ReplyAction ra, Change.Id id, ChangeInfo info) { @@ -60,9 +49,7 @@ class History extends FlowPanel { if (messages != null) { for (MessageInfo msg : Natives.asList(messages)) { Message ui = new Message(this, msg); - if (loaded.contains(msg._revisionNumber())) { - ui.addComments(comments(msg)); - } + ui.addComments(comments(msg)); add(ui); } autoOpen(ChangeScreen.myLastReply(info)); @@ -99,18 +86,16 @@ class History extends FlowPanel { replyAction.onReply(info); } - void addComments(int id, NativeMap> map) { - loaded.add(id); - + void addComments(NativeMap> map) { for (String path : map.keySet()) { for (CommentInfo c : Natives.asList(map.get(path))) { c.path(path); if (c.author() != null) { - AuthorRevision k = new AuthorRevision(c.author(), id); - List l = byAuthor.get(k); + int authorId = c.author()._account_id(); + List l = byAuthor.get(authorId); if (l == null) { l = new ArrayList<>(); - byAuthor.put(k, l); + byAuthor.put(authorId, l); } l.add(c); } @@ -118,58 +103,13 @@ class History extends FlowPanel { } } - void load(int revisionNumber) { - if (revisionNumber > 0 && loaded.add(revisionNumber)) { - toLoad.add(revisionNumber); - start(); - } - } - - private void start() { - if (active >= 2 || toLoad.isEmpty() || !isAttached()) { - return; - } - - final int revisionNumber = toLoad.remove(0); - active++; - ChangeApi.revision(new PatchSet.Id(changeId, revisionNumber)) - .view("comments") - .get(new AsyncCallback>>() { - @Override - public void onSuccess(NativeMap> result) { - addComments(revisionNumber, result); - update(revisionNumber); - --active; - start(); - } - - @Override - public void onFailure(Throwable caught) { - loaded.remove(revisionNumber); - loaded.removeAll(toLoad); - toLoad.clear(); - active--; - } - }); - } - - private void update(int revisionNumber) { - for (Widget child : getChildren()) { - Message ui = (Message) child; - MessageInfo info = ui.getMessageInfo(); - if (info._revisionNumber() == revisionNumber) { - ui.addComments(comments(info)); - } - } - } - private List comments(MessageInfo msg) { if (msg.author() == null) { return Collections.emptyList(); } - AuthorRevision k = new AuthorRevision(msg.author(), msg._revisionNumber()); - List list = byAuthor.get(k); + int authorId = msg.author()._account_id(); + List list = byAuthor.get(authorId); if (list == null) { return Collections.emptyList(); } @@ -187,34 +127,10 @@ class History extends FlowPanel { if (match.isEmpty()) { return Collections.emptyList(); } else if (other.isEmpty()) { - byAuthor.remove(k); + byAuthor.remove(authorId); } else { - byAuthor.put(k, other); + byAuthor.put(authorId, other); } return match; } - - private static final class AuthorRevision { - final int author; - final int revision; - - AuthorRevision(AccountInfo author, int revision) { - this.author = author._account_id(); - this.revision = revision; - } - - @Override - public int hashCode() { - return author * 31 + revision; - } - - @Override - public boolean equals(Object o) { - if (!(o instanceof AuthorRevision)) { - return false; - } - AuthorRevision b = (AuthorRevision) o; - return author == b.author && revision == b.revision; - } - } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java index 8fa5a68e15..7b3c310954 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java @@ -33,14 +33,29 @@ class LineComment extends Composite { interface Binder extends UiBinder {} private static final Binder uiBinder = GWT.create(Binder.class); + @UiField Element psLoc; + @UiField Element psNum; @UiField Element fileLoc; @UiField Element lineLoc; @UiField InlineHyperlink line; @UiField Element message; - LineComment(CommentLinkProcessor clp, PatchSet.Id ps, CommentInfo info) { + LineComment(CommentLinkProcessor clp, + PatchSet.Id defaultPs, + CommentInfo info) { initWidget(uiBinder.createAndBindUi(this)); + PatchSet.Id ps; + if (info.patch_set() != defaultPs.get()) { + ps = new PatchSet.Id(defaultPs.getParentKey(), info.patch_set()); + psNum.setInnerText(Integer.toString(info.patch_set())); + } else { + ps = defaultPs; + psLoc.removeFromParent(); + psLoc = null; + psNum= null; + } + if (info.has_line()) { fileLoc.removeFromParent(); fileLoc = null; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml index 4926a07788..2890832739 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml @@ -29,13 +29,16 @@ limitations under the License. font-weight: bold; } .message { - margin-left: 111px; + margin-left: 135px; } -

File Comment
-
Line :
+
+ PS, + File Comment + Line : +
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java index 22d39a7dea..bc1ac3034b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java @@ -121,13 +121,9 @@ class Message extends Composite { } void setOpen(boolean open) { - if (open && info._revisionNumber() > 0) { - if (commentList == null) { - history.load(info._revisionNumber()); - } else if (!commentList.isEmpty()) { - renderComments(commentList); - commentList = Collections.emptyList(); - } + if (open && info._revisionNumber() > 0 && !commentList.isEmpty()) { + renderComments(commentList); + commentList = Collections.emptyList(); } setName(open); @@ -156,7 +152,6 @@ class Message extends Composite { void autoOpen() { if (commentList == null) { autoOpen = true; - history.load(info._revisionNumber()); } else if (!commentList.isEmpty()) { setOpen(true); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java index f67860eac4..d6e7159248 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java @@ -21,9 +21,9 @@ import com.google.gerrit.client.changes.ChangeApi; import com.google.gerrit.client.changes.ChangeInfo.ApprovalInfo; import com.google.gerrit.client.changes.ChangeInfo.LabelInfo; import com.google.gerrit.client.changes.ChangeInfo.MessageInfo; -import com.google.gerrit.client.changes.CommentApi; import com.google.gerrit.client.changes.CommentInfo; import com.google.gerrit.client.changes.ReviewInput; +import com.google.gerrit.client.changes.ReviewInput.DraftHandling; import com.google.gerrit.client.changes.Util; import com.google.gerrit.client.rpc.GerritCallback; import com.google.gerrit.client.rpc.NativeMap; @@ -140,19 +140,19 @@ class ReplyBox extends Composite { protected void onLoad() { commentsPanel.setVisible(false); post.setEnabled(false); - CommentApi.drafts(psId, new AsyncCallback>>() { - @Override - public void onSuccess(NativeMap> result) { - attachComments(result); - displayComments(result); - post.setEnabled(true); - } + ChangeApi.drafts(psId.getParentKey().get()) + .get(new AsyncCallback>>() { + @Override + public void onSuccess(NativeMap> result) { + displayComments(result); + post.setEnabled(true); + } - @Override - public void onFailure(Throwable caught) { - post.setEnabled(true); - } - }); + @Override + public void onFailure(Throwable caught) { + post.setEnabled(true); + } + }); Scheduler.get().scheduleDeferred(new ScheduledCommand() { @Override @@ -186,6 +186,9 @@ class ReplyBox extends Composite { private void postReview() { in.message(message.getText().trim()); + // Don't send any comments in the request; just publish everything, even if + // e.g. a draft was modified in another tab since we last looked it up. + in.drafts(DraftHandling.PUBLISH_ALL_REVISIONS); in.prePost(); ChangeApi.revision(psId.getParentKey().get(), revision) .view("review") @@ -379,11 +382,6 @@ class ReplyBox extends Composite { && values.contains((short) 1); } - private void attachComments(NativeMap> result) { - in.drafts(ReviewInput.DraftHandling.KEEP); - in.comments(result); - } - private void displayComments(NativeMap> m) { comments.clear(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java index 1135491d4d..652e4bd39e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java @@ -102,6 +102,14 @@ public class ChangeApi { return call(id, revision, "actions"); } + public static RestApi comments(int id) { + return call(id, "comments"); + } + + public static RestApi drafts(int id) { + return call(id, "drafts"); + } + public static void edit(int id, AsyncCallback cb) { edit(id).get(cb); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java index 52e5d40837..fed041ca66 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java @@ -82,6 +82,7 @@ public class CommentInfo extends JavaScriptObject { public final native String path() /*-{ return this.path }-*/; public final native String id() /*-{ return this.id }-*/; public final native String in_reply_to() /*-{ return this.in_reply_to }-*/; + public final native int patch_set() /*-{ return this.patch_set }-*/; public final Side side() { String s = sideRaw(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java index 7651495015..096dbd0df7 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java @@ -24,7 +24,7 @@ public class ReviewInput extends JavaScriptObject { } public static enum DraftHandling { - DELETE, PUBLISH, KEEP + DELETE, PUBLISH, KEEP, PUBLISH_ALL_REVISIONS } public static ReviewInput create() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java index f2b4fdc8e1..88f034eb67 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -14,12 +14,18 @@ package com.google.gerrit.server; +import static com.google.common.base.MoreObjects.firstNonNull; + import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.base.Predicate; +import com.google.common.collect.ComparisonChain; import com.google.common.collect.FluentIterable; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Ordering; +import com.google.gerrit.extensions.client.Side; +import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchLineComment; @@ -62,10 +68,47 @@ import java.util.Set; */ @Singleton public class PatchLineCommentsUtil { + public static Ordering PLC_ORDER = + new Ordering() { + @Override + public int compare(PatchLineComment c1, PatchLineComment c2) { + String filename1 = c1.getKey().getParentKey().get(); + String filename2 = c2.getKey().getParentKey().get(); + return ComparisonChain.start() + .compare(filename1, filename2) + .compare(getCommentPsId(c1).get(), getCommentPsId(c2).get()) + .compare(c1.getSide(), c2.getSide()) + .compare(c1.getLine(), c2.getLine()) + .compare(c1.getWrittenOn(), c2.getWrittenOn()) + .result(); + } + }; + + public static final Ordering COMMENT_INFO_ORDER = + new Ordering() { + @Override + public int compare(CommentInfo a, CommentInfo b) { + return ComparisonChain.start() + .compare(a.path, b.path, NULLS_FIRST) + .compare(a.patchSet, b.patchSet, NULLS_FIRST) + .compare(side(a), side(b)) + .compare(a.line, b.line, NULLS_FIRST) + .compare(a.id, b.id) + .result(); + } + + private int side(CommentInfo c) { + return firstNonNull(c.side, Side.REVISION).ordinal(); + } + }; + public static PatchSet.Id getCommentPsId(PatchLineComment plc) { return plc.getKey().getParentKey().getParentKey(); } + private static final Ordering> NULLS_FIRST = + Ordering.natural().nullsFirst(); + private final GitRepositoryManager repoManager; private final AllUsersName allUsers; private final DraftCommentNotes.Factory draftFactory; @@ -219,7 +262,7 @@ public class PatchLineCommentsUtil { in.getKey().getParentKey().getParentKey().getParentKey(); return changeId.equals(matchId); } - }).toSortedList(ChangeNotes.PLC_ORDER); + }).toSortedList(PLC_ORDER); } List comments = Lists.newArrayList(); comments.addAll(notes.getDraftComments(author).values()); @@ -350,7 +393,7 @@ public class PatchLineCommentsUtil { } private static List sort(List comments) { - Collections.sort(comments, ChangeNotes.PLC_ORDER); + Collections.sort(comments, PLC_ORDER); return comments; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index 06c59eebfb..78b3f10c00 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -26,6 +26,7 @@ import com.google.gerrit.extensions.api.changes.RevertInput; import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.SuggestedReviewerInfo; import com.google.gerrit.extensions.restapi.IdString; @@ -40,6 +41,8 @@ import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.Check; import com.google.gerrit.server.change.GetHashtags; import com.google.gerrit.server.change.GetTopic; +import com.google.gerrit.server.change.ListChangeComments; +import com.google.gerrit.server.change.ListChangeDrafts; import com.google.gerrit.server.change.PostHashtags; import com.google.gerrit.server.change.PostReviewers; import com.google.gerrit.server.change.PutTopic; @@ -56,6 +59,7 @@ import com.google.inject.assistedinject.Assisted; import java.io.IOException; import java.util.EnumSet; import java.util.List; +import java.util.Map; import java.util.Set; class ChangeApiImpl implements ChangeApi { @@ -78,6 +82,8 @@ class ChangeApiImpl implements ChangeApi { private final Provider changeJson; private final PostHashtags postHashtags; private final GetHashtags getHashtags; + private final ListChangeComments listComments; + private final ListChangeDrafts listDrafts; private final Check check; private final ChangeEdits.Detail editDetail; @@ -96,6 +102,8 @@ class ChangeApiImpl implements ChangeApi { Provider changeJson, PostHashtags postHashtags, GetHashtags getHashtags, + ListChangeComments listComments, + ListChangeDrafts listDrafts, Check check, ChangeEdits.Detail editDetail, @Assisted ChangeResource change) { @@ -113,6 +121,8 @@ class ChangeApiImpl implements ChangeApi { this.changeJson = changeJson; this.postHashtags = postHashtags; this.getHashtags = getHashtags; + this.listComments = listComments; + this.listDrafts = listDrafts; this.check = check; this.editDetail = editDetail; this.change = change; @@ -297,6 +307,24 @@ class ChangeApiImpl implements ChangeApi { } } + @Override + public Map> comments() throws RestApiException { + try { + return listComments.apply(change); + } catch (OrmException e) { + throw new RestApiException("Cannot get comments", e); + } + } + + @Override + public Map> drafts() throws RestApiException { + try { + return listDrafts.apply(change); + } catch (OrmException e) { + throw new RestApiException("Cannot get drafts", e); + } + } + @Override public ChangeInfo check() throws RestApiException { try { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java index 57ae5eaa2b..46fabd5007 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java @@ -14,7 +14,7 @@ package com.google.gerrit.server.change; -import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.gerrit.server.PatchLineCommentsUtil.COMMENT_INFO_ORDER; import com.google.common.base.Strings; import com.google.gerrit.extensions.client.Comment.Range; @@ -29,7 +29,6 @@ import com.google.inject.Inject; import java.util.ArrayList; import java.util.Collections; -import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.TreeMap; @@ -39,6 +38,7 @@ class CommentJson { private final AccountLoader.Factory accountLoaderFactory; private boolean fillAccounts = true; + private boolean fillPatchSet; @Inject CommentJson(AccountLoader.Factory accountLoaderFactory) { @@ -50,6 +50,11 @@ class CommentJson { return this; } + CommentJson setFillPatchSet(boolean fillPatchSet) { + this.fillPatchSet = fillPatchSet; + return this; + } + CommentInfo format(PatchLineComment c) throws OrmException { AccountLoader loader = null; if (fillAccounts) { @@ -81,20 +86,7 @@ class CommentJson { } for (List list : out.values()) { - Collections.sort(list, new Comparator() { - @Override - public int compare(CommentInfo a, CommentInfo b) { - int c = firstNonNull(a.side, Side.REVISION).ordinal() - - firstNonNull(b.side, Side.REVISION).ordinal(); - if (c == 0) { - c = firstNonNull(a.line, 0) - firstNonNull(b.line, 0); - } - if (c == 0) { - c = a.id.compareTo(b.id); - } - return c; - } - }); + Collections.sort(list, COMMENT_INFO_ORDER); } if (accountLoader != null) { @@ -106,6 +98,9 @@ class CommentJson { private CommentInfo toCommentInfo(PatchLineComment c, AccountLoader loader) { CommentInfo r = new CommentInfo(); + if (fillPatchSet) { + r.patchSet = c.getKey().getParentKey().getParentKey().get(); + } r.id = Url.encode(c.getKey().get()); r.path = c.getKey().getParentKey().getFileName(); if (c.getSide() == 0) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java index d75b6c6ef2..122156b3b3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java @@ -14,7 +14,8 @@ package com.google.gerrit.server.change; -import com.google.common.collect.Lists; +import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER; + import com.google.gerrit.extensions.api.changes.ReviewInput.NotifyHandling; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -24,7 +25,6 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.EmailReviewCommentsExecutor; -import com.google.gerrit.server.git.WorkQueue.Executor; import com.google.gerrit.server.mail.CommentSender; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.util.RequestContext; @@ -40,9 +40,8 @@ import com.google.inject.assistedinject.Assisted; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.Collections; -import java.util.Comparator; import java.util.List; +import java.util.concurrent.ExecutorService; public class EmailReviewComments implements Runnable, RequestContext { private static final Logger log = LoggerFactory.getLogger(EmailReviewComments.class); @@ -57,7 +56,7 @@ public class EmailReviewComments implements Runnable, RequestContext { List comments); } - private final Executor sendEmailsExecutor; + private final ExecutorService sendEmailsExecutor; private final PatchSetInfoFactory patchSetInfoFactory; private final CommentSender.Factory commentSenderFactory; private final SchemaFactory schemaFactory; @@ -73,7 +72,7 @@ public class EmailReviewComments implements Runnable, RequestContext { @Inject EmailReviewComments ( - @EmailReviewCommentsExecutor final Executor executor, + @EmailReviewCommentsExecutor ExecutorService executor, PatchSetInfoFactory patchSetInfoFactory, CommentSender.Factory commentSenderFactory, SchemaFactory schemaFactory, @@ -94,7 +93,7 @@ public class EmailReviewComments implements Runnable, RequestContext { this.patchSet = patchSet; this.authorId = authorId; this.message = message; - this.comments = comments; + this.comments = PLC_ORDER.sortedCopy(comments); } void sendAsync() { @@ -103,31 +102,8 @@ public class EmailReviewComments implements Runnable, RequestContext { @Override public void run() { + RequestContext old = requestContext.setContext(this); try { - requestContext.setContext(this); - - comments = Lists.newArrayList(comments); - Collections.sort(comments, new Comparator() { - @Override - public int compare(PatchLineComment a, PatchLineComment b) { - int cmp = path(a).compareTo(path(b)); - if (cmp != 0) { - return cmp; - } - - // 0 is ancestor, 1 is revision. Sort ancestor first. - cmp = a.getSide() - b.getSide(); - if (cmp != 0) { - return cmp; - } - - return a.getLine() - b.getLine(); - } - - private String path(PatchLineComment c) { - return c.getKey().getParentKey().getFileName(); - } - }); CommentSender cm = commentSenderFactory.create(notify, change.getId()); cm.setFrom(authorId); @@ -138,7 +114,7 @@ public class EmailReviewComments implements Runnable, RequestContext { } catch (Exception e) { log.error("Cannot email comments for " + patchSet.getId(), e); } finally { - requestContext.setContext(null); + requestContext.setContext(old); if (db != null) { db.close(); db = null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java new file mode 100644 index 0000000000..97befa097c --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java @@ -0,0 +1,58 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.PatchLineCommentsUtil; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +import java.util.List; +import java.util.Map; + +@Singleton +public class ListChangeComments implements RestReadView { + private final Provider db; + private final ChangeData.Factory changeDataFactory; + private final Provider commentJson; + private final PatchLineCommentsUtil plcUtil; + + @Inject + ListChangeComments(Provider db, + ChangeData.Factory changeDataFactory, + Provider commentJson, + PatchLineCommentsUtil plcUtil) { + this.db = db; + this.changeDataFactory = changeDataFactory; + this.commentJson = commentJson; + this.plcUtil = plcUtil; + } + + @Override + public Map> apply( + ChangeResource rsrc) throws AuthException, OrmException { + ChangeData cd = changeDataFactory.create(db.get(), rsrc.getControl()); + return commentJson.get() + .setFillAccounts(true) + .setFillPatchSet(true) + .format(plcUtil.publishedByChange(db.get(), cd.notes())); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java new file mode 100644 index 0000000000..2b5d7d908a --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java @@ -0,0 +1,66 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.RestReadView; +import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.PatchLineCommentsUtil; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +import java.util.List; +import java.util.Map; + +@Singleton +public class ListChangeDrafts implements RestReadView { + private final Provider db; + private final ChangeData.Factory changeDataFactory; + private final Provider commentJson; + private final PatchLineCommentsUtil plcUtil; + + @Inject + ListChangeDrafts(Provider db, + ChangeData.Factory changeDataFactory, + Provider commentJson, + PatchLineCommentsUtil plcUtil) { + this.db = db; + this.changeDataFactory = changeDataFactory; + this.commentJson = commentJson; + this.plcUtil = plcUtil; + } + + @Override + public Map> apply( + ChangeResource rsrc) throws AuthException, OrmException { + if (!rsrc.getControl().getCurrentUser().isIdentifiedUser()) { + throw new AuthException("Authentication required"); + } + IdentifiedUser user = (IdentifiedUser) rsrc.getControl().getCurrentUser(); + ChangeData cd = changeDataFactory.create(db.get(), rsrc.getControl()); + List drafts = + plcUtil.draftByChangeAuthor(db.get(), cd.notes(), user.getAccountId()); + return commentJson.get() + .setFillAccounts(false) + .setFillPatchSet(true) + .format(drafts); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index d0e4c99c94..b73af9318b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -52,6 +52,8 @@ public class Module extends RestApiModule { get(CHANGE_KIND, "topic").to(GetTopic.class); get(CHANGE_KIND, "in").to(IncludedIn.class); get(CHANGE_KIND, "hashtags").to(GetHashtags.class); + get(CHANGE_KIND, "comments").to(ListChangeComments.class); + get(CHANGE_KIND, "drafts").to(ListChangeDrafts.class); get(CHANGE_KIND, "check").to(Check.class); post(CHANGE_KIND, "check").to(Check.class); put(CHANGE_KIND, "topic").to(PutTopic.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index dfaca14fe2..52e975d7ab 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -345,7 +345,11 @@ public class PostReview implements RestModifyView Map drafts = Collections.emptyMap(); if (!in.isEmpty() || draftsHandling != DraftHandling.KEEP) { - drafts = scanDraftComments(rsrc); + if (draftsHandling == DraftHandling.PUBLISH_ALL_REVISIONS) { + drafts = changeDrafts(rsrc); + } else { + drafts = patchSetDrafts(rsrc); + } } List del = Lists.newArrayList(); @@ -392,6 +396,7 @@ public class PostReview implements RestModifyView del.addAll(drafts.values()); break; case PUBLISH: + case PUBLISH_ALL_REVISIONS: for (PatchLineComment e : drafts.values()) { e.setStatus(PatchLineComment.Status.PUBLISHED); e.setWrittenOn(timestamp); @@ -406,8 +411,18 @@ public class PostReview implements RestModifyView return !del.isEmpty() || !ups.isEmpty(); } - private Map scanDraftComments( - RevisionResource rsrc) throws OrmException { + private Map changeDrafts(RevisionResource rsrc) + throws OrmException { + Map drafts = Maps.newHashMap(); + for (PatchLineComment c + : plcUtil.draftByChange(db.get(), rsrc.getNotes())) { + drafts.put(c.getKey().get(), c); + } + return drafts; + } + + private Map patchSetDrafts(RevisionResource rsrc) + throws OrmException { Map drafts = Maps.newHashMap(); for (PatchLineComment c : plcUtil.draftByPatchSetAuthor(db.get(), rsrc.getPatchSet().getId(), rsrc.getAccountId(), rsrc.getNotes())) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsExecutorModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsExecutorModule.java index 25fbfb982a..365000b2c1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsExecutorModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsExecutorModule.java @@ -25,6 +25,7 @@ import com.google.inject.Singleton; import org.eclipse.jgit.lib.Config; import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.ExecutorService; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; @@ -48,9 +49,12 @@ public class ReceiveCommitsExecutorModule extends AbstractModule { @Provides @Singleton @EmailReviewCommentsExecutor - public WorkQueue.Executor createEmailReviewCommentsExecutor( + public ExecutorService createEmailReviewCommentsExecutor( @GerritServerConfig Config config, WorkQueue queues) { int poolSize = config.getInt("sendemail", null, "threadPoolSize", 1); + if (poolSize == 0) { + return MoreExecutors.newDirectExecutorService(); + } return queues.createQueue(poolSize, "EmailReviewComments"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index fc6eaddb13..8147cff2f7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.mail; +import static com.google.gerrit.server.PatchLineCommentsUtil.getCommentPsId; + import com.google.common.base.Optional; import com.google.common.base.Strings; import com.google.common.collect.Ordering; @@ -175,7 +177,8 @@ public class CommentSender extends ReplyToChangeSender { short side = comment.getSide(); CommentRange range = comment.getRange(); if (range != null) { - String prefix = String.format("Line %d: ", range.getStartLine()); + String prefix = "PS" + getCommentPsId(comment).get() + + ", Line " + range.getStartLine() + ": "; for (int n = range.getStartLine(); n <= range.getEndLine(); n++) { out.append(n == range.getStartLine() ? prefix diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 0df9b908db..47c1731984 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; -import static com.google.gerrit.server.PatchLineCommentsUtil.getCommentPsId; import static com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap; import com.google.common.collect.ListMultimap; @@ -170,11 +169,6 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { } private void verifyComment(PatchLineComment comment) { - checkState(psId != null, - "setPatchSetId must be called first"); - checkArgument(getCommentPsId(comment).equals(psId), - "Comment on %s does not match configured patch set %s", - getCommentPsId(comment), psId); if (migration.writeChanges()) { checkArgument(comment.getRevId() != null); } @@ -290,7 +284,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { } commit.setAuthor(newIdent(getUser().getAccount(), when)); commit.setCommitter(new PersonIdent(serverIdent, when)); - commit.setMessage(String.format("Comment on patch set %d", psId.get())); + commit.setMessage("Update draft comments"); return true; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index 279e0132a2..063ff5a48c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -18,7 +18,6 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; -import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; @@ -51,7 +50,6 @@ import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; import java.sql.Timestamp; -import java.util.Comparator; import java.util.Map; /** View of a single {@link Change} based on the log of its notes branch. */ @@ -74,20 +72,6 @@ public class ChangeNotes extends AbstractChangeNotes { } }); - public static Comparator PLC_ORDER = - new Comparator() { - @Override - public int compare(PatchLineComment c1, PatchLineComment c2) { - String filename1 = c1.getKey().getParentKey().get(); - String filename2 = c2.getKey().getParentKey().get(); - return ComparisonChain.start() - .compare(filename1, filename2) - .compare(c1.getLine(), c2.getLine()) - .compare(c1.getWrittenOn(), c2.getWrittenOn()) - .result(); - } - }; - public static ConfigInvalidException parseException(Change.Id changeId, String fmt, Object... args) { return new ConfigInvalidException("Change " + changeId + ": " diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 41202bd53f..43c232be6d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.gerrit.server.PatchLineCommentsUtil.getCommentPsId; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET; @@ -241,7 +240,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private void insertDraftComment(PatchLineComment c) throws OrmException { - createDraftUpdateIfNull(c); + createDraftUpdateIfNull(); draftUpdate.insertComment(c); } @@ -262,7 +261,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private void upsertDraftComment(PatchLineComment c) { - createDraftUpdateIfNull(c); + createDraftUpdateIfNull(); draftUpdate.upsertComment(c); } @@ -281,38 +280,28 @@ public class ChangeUpdate extends AbstractChangeUpdate { } private void updateDraftComment(PatchLineComment c) throws OrmException { - createDraftUpdateIfNull(c); + createDraftUpdateIfNull(); draftUpdate.updateComment(c); } private void deleteDraftComment(PatchLineComment c) throws OrmException { - createDraftUpdateIfNull(c); + createDraftUpdateIfNull(); draftUpdate.deleteComment(c); } private void deleteDraftCommentIfPresent(PatchLineComment c) throws OrmException { - createDraftUpdateIfNull(c); + createDraftUpdateIfNull(); draftUpdate.deleteCommentIfPresent(c); } - private void createDraftUpdateIfNull(PatchLineComment c) { + private void createDraftUpdateIfNull() { if (draftUpdate == null) { draftUpdate = draftUpdateFactory.create(ctl, when); - if (psId != null) { - draftUpdate.setPatchSetId(psId); - } else { - draftUpdate.setPatchSetId(getCommentPsId(c)); - } } } private void verifyComment(PatchLineComment c) { - checkArgument(psId != null, - "setPatchSetId must be called first"); - checkArgument(getCommentPsId(c).equals(psId), - "Comment on %s doesn't match previous patch set %s", - getCommentPsId(c), psId); checkArgument(c.getRevId() != null); checkArgument(c.getStatus() == Status.PUBLISHED, "Cannot add a draft comment to a ChangeUpdate. Use a ChangeDraftUpdate" diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java index be50162c68..149325d90b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER; import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST; import static com.google.gerrit.server.notedb.ChangeNotes.parseException; import static java.nio.charset.StandardCharsets.UTF_8; @@ -541,7 +542,7 @@ public class CommentsInNotesUtil { noteMap.remove(commit); continue; } - Collections.sort(comments, ChangeNotes.PLC_ORDER); + Collections.sort(comments, PLC_ORDER); // We allow comments for multiple commits to be written in the same // update, even though the rest of the metadata update is associated with // a single patch set. diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java index f3addbfb67..59549ac0bd 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java @@ -371,7 +371,7 @@ public class CommentsTest { @Test public void testPatchLineCommentsUtilByCommentStatus() throws OrmException { assertThat(plcUtil.publishedByChange(db, revRes2.getNotes())) - .containsExactly(plc1, plc2, plc3).inOrder(); + .containsExactly(plc3, plc1, plc2).inOrder(); assertThat(plcUtil.draftByChange(db, revRes2.getNotes())) .containsExactly(plc4, plc5).inOrder(); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 93792806a3..6067442b7b 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -1211,4 +1211,48 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertThat(newNotes(c).getComments()).containsExactly( ImmutableMultimap.of(new RevId(rev), comment)); } + + @Test + public void updateCommentsForMultipleRevisions() throws Exception { + Change c = newChange(); + String uuid = "uuid"; + String rev1 = "abcd1234abcd1234abcd1234abcd1234abcd1234"; + String rev2 = "abcd4567abcd4567abcd4567abcd4567abcd4567"; + CommentRange range = new CommentRange(1, 1, 2, 1); + PatchSet.Id ps1 = c.currentPatchSetId(); + String filename = "filename1"; + short side = (short) 1; + + incrementPatchSet(c); + PatchSet.Id ps2 = c.currentPatchSetId(); + + ChangeUpdate update = newUpdate(c, otherUser); + update.setPatchSetId(ps2); + Timestamp now = TimeUtil.nowTs(); + PatchLineComment comment1 = newComment(ps1, filename, + uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps1", + side, rev1, Status.DRAFT); + PatchLineComment comment2 = newComment(ps2, filename, + uuid, range, range.getEndLine(), otherUser, null, now, "comment on ps2", + side, rev2, Status.DRAFT); + update.upsertComment(comment1); + update.upsertComment(comment2); + update.commit(); + + ChangeNotes notes = newNotes(c); + assertThat(notes.getDraftComments(otherUserId)).hasSize(2); + assertThat(notes.getComments()).isEmpty(); + + update = newUpdate(c, otherUser); + update.setPatchSetId(ps2); + comment1.setStatus(Status.PUBLISHED); + comment2.setStatus(Status.PUBLISHED); + update.upsertComment(comment1); + update.upsertComment(comment2); + update.commit(); + + notes = newNotes(c); + assertThat(notes.getDraftComments(otherUserId)).isEmpty(); + assertThat(notes.getComments()).hasSize(2); + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java index b8d5cd2f21..7adf721a2a 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java @@ -15,6 +15,8 @@ package com.google.gerrit.testutil; import com.google.auto.value.AutoValue; +import com.google.common.base.Predicate; +import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.gerrit.common.errors.EmailException; @@ -102,6 +104,19 @@ public class FakeEmailSender implements EmailSender { } } + public ImmutableList getMessages(String changeId, String type) { + final String idFooter = "\nGerrit-Change-Id: " + changeId + "\n"; + final String typeFooter = "\nGerrit-MessageType: " + type + "\n"; + return FluentIterable.from(getMessages()) + .filter(new Predicate() { + @Override + public boolean apply(Message in) { + return in.body().contains(idFooter) + && in.body().contains(typeFooter); + } + }).toList(); + } + private void waitForEmails() { // TODO(dborowitz): This is brittle; consider forcing emails to use // a single thread in tests (tricky because most callers just use the diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java index a68f71539a..2219d285c7 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java @@ -44,7 +44,6 @@ import com.google.gerrit.server.git.EmailReviewCommentsExecutor; import com.google.gerrit.server.git.GarbageCollection; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.PerThreadRequestScope; -import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.index.ChangeSchemas; import com.google.gerrit.server.index.IndexModule.IndexType; import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier; @@ -94,6 +93,7 @@ public class InMemoryModule extends FactoryModule { cfg.setBoolean("index", "lucene", "testInmemory", true); cfg.setInt("index", "lucene", "testVersion", ChangeSchemas.getLatest().getVersion()); + cfg.setInt("sendemail", null, "threadPoolSize", 0); } private final Config cfg; @@ -205,10 +205,8 @@ public class InMemoryModule extends FactoryModule { @Provides @Singleton @EmailReviewCommentsExecutor - public WorkQueue.Executor createEmailReviewCommentsExecutor( - @GerritServerConfig Config config, WorkQueue queues) { - int poolSize = config.getInt("sendemail", null, "threadPoolSize", 1); - return queues.createQueue(poolSize, "EmailReviewComments"); + public ExecutorService createEmailReviewCommentsExecutor() { + return MoreExecutors.newDirectExecutorService(); } @Provides