PostReview: Add option to publish drafts on all revisions Off by default, but enabled with a new draft handling option. Comments in emails now include the full set of changes, possibly prefixed with a patch set number. Bug: issue 1100 Change-Id: I8cd05d417ad210e28afd8c9d20edd07c424e9e37
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 9cd04b8..5edd956 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -4118,7 +4118,9 @@ 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 d42a078..2fa6eb2 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
@@ -28,6 +28,7 @@ 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; @@ -38,8 +39,11 @@ 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; @@ -68,6 +72,13 @@ @Inject private Provider<PostReview> postReview; + @Inject + @CanonicalWebUrl + private Provider<String> urlProvider; + + @Inject + private FakeEmailSender email; + private final Integer[] lines = {0, 1}; @Before @@ -267,6 +278,94 @@ 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(); @@ -355,9 +454,9 @@ 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/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java index dd2ce92..dc22fb0 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 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-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 93782e33..122156b 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; @@ -39,8 +40,6 @@ 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; @@ -94,7 +93,7 @@ this.patchSet = patchSet; this.authorId = authorId; this.message = message; - this.comments = comments; + this.comments = PLC_ORDER.sortedCopy(comments); } void sendAsync() { @@ -106,29 +105,6 @@ RequestContext old = requestContext.setContext(this); try { - 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); cm.setPatchSet(patchSet, patchSetInfoFactory.get(change, patchSet));
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 dfaca14..52e975d 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 @@ 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 @@ 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 @@ 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())) {
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 fc6eadd..8147cff 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 @@ 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/test/java/com/google/gerrit/testutil/FakeEmailSender.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/FakeEmailSender.java index b8d5cd2..7adf721 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 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