opendev/gerrit
Code Issues Proposed changes

Merge changes from topic 'multidraft'

Browse Source 
* changes:
  Always publish comments on all revisions from the reply box
  PostReview: Add option to publish drafts on all revisions
  Support sending review comment emails synchronously
  Unify inline comment comparators
  Change endpoints for all draft/published comments on a change
  notedb: Support updating comments on multiple revisions
This commit is contained in:
Dave Borowitz 2015-05-04 19:01:17 +00:00 committed by Gerrit Code Review
commit 360ab3752e
33 changed files with 770 additions and 259 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

119
Documentation/rest-api-changes.txt
View File

@ -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

199
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java
View File

@ -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> postReview;
@Inject
@CanonicalWebUrl
private Provider<String> 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<String, List<CommentInfo>> actual =
gApi.changes().id(r1.getChangeId()).drafts();
assertThat((Iterable<?>) actual.keySet()).containsExactly(FILE_NAME);
List<CommentInfo> 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<String, List<CommentInfo>> actual = gApi.changes()
.id(r2.getChangeId())
.comments();
assertThat(actual.keySet()).containsExactly(FILE_NAME);
List<CommentInfo> 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<String, List<CommentInfo>> ps1Map = gApi.changes()
.id(r1.getChangeId())
.revision(r1.getCommit().name())
.comments();
assertThat(ps1Map.keySet()).containsExactly(FILE_NAME);
List<CommentInfo> 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<String, List<CommentInfo>> ps2Map = gApi.changes()
.id(r2.getChangeId())
.revision(r2.getCommit().name())
.comments();
assertThat(ps2Map.keySet()).containsExactly(FILE_NAME);
List<CommentInfo> 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<Message> 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.<String, List<CommentInput>> 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;
}

30
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@ -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<String> 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<String, List<CommentInfo>> 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<String, List<CommentInfo>> 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<String, List<CommentInfo>> comments() throws RestApiException {
throw new NotImplementedException();
}
@Override
public Map<String, List<CommentInfo>> drafts() throws RestApiException {
throw new NotImplementedException();
}
@Override
public ChangeInfo check() throws RestApiException {
throw new NotImplementedException();

12
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
View File

@ -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 {

7
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java
View File

@ -17,6 +17,13 @@ package com.google.gerrit.extensions.client;
import java.sql.Timestamp;
public abstract class Comment {
/**
* Patch set number containing this commit.
* <p>
* Only set in contexts where comments may come from multiple patch sets.
*/
public Integer patchSet;
public String id;
public String path;
public Side side;

42
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
View File

@ -908,16 +908,19 @@ public class ChangeScreen extends Screen {
}
private List<NativeMap<JsArray<CommentInfo>>> loadComments(
RevisionInfo rev, CallbackGroup group) {
final int id = rev._number();
final RevisionInfo rev, CallbackGroup group) {
final List<NativeMap<JsArray<CommentInfo>>> 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<NativeMap<JsArray<CommentInfo>>>() {
@Override
public void onSuccess(NativeMap<JsArray<CommentInfo>> 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<JsArray<CommentInfo>> filterForRevision(
NativeMap<JsArray<CommentInfo>> comments, int id) {
NativeMap<JsArray<CommentInfo>> filtered = NativeMap.create();
for (String k : comments.keySet()) {
JsArray<CommentInfo> allRevisions = comments.get(k);
JsArray<CommentInfo> 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<NativeMap<JsArray<CommentInfo>>> loadDrafts(
RevisionInfo rev, CallbackGroup group) {
final List<NativeMap<JsArray<CommentInfo>>> 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<ActionInfo> 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<ActionInfo> actionMap) {
initRevisionsAction(info, revision, actionMap);
actions.reloadRevisionActions(actionMap);
}

15
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/FileComments.java
View File

@ -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<CommentInfo> 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<CommentInfo>() {
@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));
}
}

104
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java
View File

@ -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<Integer> loaded = new HashSet<>();
private final Map<AuthorRevision, List<CommentInfo>> byAuthor =
new HashMap<>();
private final List<Integer> toLoad = new ArrayList<>(4);
private int active;
private final Map<Integer, List<CommentInfo>> 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<JsArray<CommentInfo>> map) {
loaded.add(id);
void addComments(NativeMap<JsArray<CommentInfo>> 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<CommentInfo> l = byAuthor.get(k);
int authorId = c.author()._account_id();
List<CommentInfo> 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<NativeMap<JsArray<CommentInfo>>>() {
@Override
public void onSuccess(NativeMap<JsArray<CommentInfo>> 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<CommentInfo> comments(MessageInfo msg) {
if (msg.author() == null) {
return Collections.emptyList();
}
AuthorRevision k = new AuthorRevision(msg.author(), msg._revisionNumber());
List<CommentInfo> list = byAuthor.get(k);
int authorId = msg.author()._account_id();
List<CommentInfo> 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;
}
}
}

17
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.java
View File

@ -33,14 +33,29 @@ class LineComment extends Composite {
interface Binder extends UiBinder<HTMLPanel, LineComment> {}
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;

9
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/LineComment.ui.xml
View File

@ -29,13 +29,16 @@ limitations under the License.
font-weight: bold;
}
.message {
margin-left: 111px;
margin-left: 135px;
}
</ui:style>
<g:HTMLPanel styleName='{style.box}'>
<div class='{style.location}' ui:field='fileLoc'><ui:msg>File Comment</ui:msg></div>
<div class='{style.location}' ui:field='lineLoc'><ui:msg>Line <c:InlineHyperlink ui:field='line'/>:</ui:msg></div>
<div class='{style.location}'>
<span ui:field='psLoc'><ui:msg>PS<span ui:field='psNum'/>, </ui:msg></span>
<span ui:field='fileLoc'><ui:msg>File Comment</ui:msg></span>
<span ui:field='lineLoc'><ui:msg>Line <c:InlineHyperlink ui:field='line'/>:</ui:msg></span>
</div>
<div class='{style.message}' ui:field='message'/>
</g:HTMLPanel>
</ui:UiBinder>

11
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Message.java
View File

@ -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);
}

34
gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java
View File

@ -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<NativeMap<JsArray<CommentInfo>>>() {
@Override
public void onSuccess(NativeMap<JsArray<CommentInfo>> result) {
attachComments(result);
displayComments(result);
post.setEnabled(true);
}
ChangeApi.drafts(psId.getParentKey().get())
.get(new AsyncCallback<NativeMap<JsArray<CommentInfo>>>() {
@Override
public void onSuccess(NativeMap<JsArray<CommentInfo>> 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<JsArray<CommentInfo>> result) {
in.drafts(ReviewInput.DraftHandling.KEEP);
in.comments(result);
}
private void displayComments(NativeMap<JsArray<CommentInfo>> m) {
comments.clear();

8
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java
View File

@ -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<EditInfo> cb) {
edit(id).get(cb);
}

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java
View File

@ -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();

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java
View File

@ -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() {

47
gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java
View File

@ -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<PatchLineComment> PLC_ORDER =
new Ordering<PatchLineComment>() {
@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<CommentInfo> COMMENT_INFO_ORDER =
new Ordering<CommentInfo>() {
@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<Comparable<?>> 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<PatchLineComment> comments = Lists.newArrayList();
comments.addAll(notes.getDraftComments(author).values());
@ -350,7 +393,7 @@ public class PatchLineCommentsUtil {
}
private static List<PatchLineComment> sort(List<PatchLineComment> comments) {
Collections.sort(comments, ChangeNotes.PLC_ORDER);
Collections.sort(comments, PLC_ORDER);
return comments;
}
}

28
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@ -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> 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> 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<String, List<CommentInfo>> comments() throws RestApiException {
try {
return listComments.apply(change);
} catch (OrmException e) {
throw new RestApiException("Cannot get comments", e);
}
}
@Override
public Map<String, List<CommentInfo>> 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 {

27
gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
View File

@ -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<CommentInfo> list : out.values()) {
Collections.sort(list, new Comparator<CommentInfo>() {
@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) {

40
gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java
View File

@ -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<PatchLineComment> comments);
}
private final Executor sendEmailsExecutor;
private final ExecutorService sendEmailsExecutor;
private final PatchSetInfoFactory patchSetInfoFactory;
private final CommentSender.Factory commentSenderFactory;
private final SchemaFactory<ReviewDb> 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<ReviewDb> 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<PatchLineComment>() {
@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;

58
gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeComments.java Normal file
View File

@ -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<ChangeResource> {
private final Provider<ReviewDb> db;
private final ChangeData.Factory changeDataFactory;
private final Provider<CommentJson> commentJson;
private final PatchLineCommentsUtil plcUtil;
@Inject
ListChangeComments(Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
Provider<CommentJson> commentJson,
PatchLineCommentsUtil plcUtil) {
this.db = db;
this.changeDataFactory = changeDataFactory;
this.commentJson = commentJson;
this.plcUtil = plcUtil;
}
@Override
public Map<String, List<CommentInfo>> 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()));
}
}

66
gerrit-server/src/main/java/com/google/gerrit/server/change/ListChangeDrafts.java Normal file
View File

@ -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<ChangeResource> {
private final Provider<ReviewDb> db;
private final ChangeData.Factory changeDataFactory;
private final Provider<CommentJson> commentJson;
private final PatchLineCommentsUtil plcUtil;
@Inject
ListChangeDrafts(Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
Provider<CommentJson> commentJson,
PatchLineCommentsUtil plcUtil) {
this.db = db;
this.changeDataFactory = changeDataFactory;
this.commentJson = commentJson;
this.plcUtil = plcUtil;
}
@Override
public Map<String, List<CommentInfo>> 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<PatchLineComment> drafts =
plcUtil.draftByChangeAuthor(db.get(), cd.notes(), user.getAccountId());
return commentJson.get()
.setFillAccounts(false)
.setFillPatchSet(true)
.format(drafts);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
View File

@ -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);

21
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@ -345,7 +345,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
Map<String, PatchLineComment> 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<PatchLineComment> del = Lists.newArrayList();
@ -392,6 +396,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
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<RevisionResource, ReviewInput>
return !del.isEmpty() || !ups.isEmpty();
}
private Map<String, PatchLineComment> scanDraftComments(
RevisionResource rsrc) throws OrmException {
private Map<String, PatchLineComment> changeDrafts(RevisionResource rsrc)
throws OrmException {
Map<String, PatchLineComment> drafts = Maps.newHashMap();
for (PatchLineComment c
: plcUtil.draftByChange(db.get(), rsrc.getNotes())) {
drafts.put(c.getKey().get(), c);
}
return drafts;
}
private Map<String, PatchLineComment> patchSetDrafts(RevisionResource rsrc)
throws OrmException {
Map<String, PatchLineComment> drafts = Maps.newHashMap();
for (PatchLineComment c : plcUtil.draftByPatchSetAuthor(db.get(),
rsrc.getPatchSet().getId(), rsrc.getAccountId(), rsrc.getNotes())) {

6
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsExecutorModule.java
View File

@ -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");
}

5
gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java
View File

@ -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

8
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
View File

@ -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;
}

16
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@ -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<ChangeNotes> {
}
});
public static Comparator<PatchLineComment> PLC_ORDER =
new Comparator<PatchLineComment>() {
@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 + ": "

23
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@ -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"

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java
View File

@ -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.

2
gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java
View File

@ -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();
}

44
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@ -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);
}
}

15
gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java
View File

@ -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<Message> 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<Message>() {
@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

8
gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java
View File

@ -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