From 8a1da03928ab4fa6e67ac36f716b7eb3ffa941d0 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Fri, 25 Jul 2014 10:57:35 +0200 Subject: [PATCH] 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 --- Documentation/rest-api-changes.txt | 4 +- .../acceptance/server/change/CommentsIT.java | 103 +++++++++++++++++- .../extensions/api/changes/ReviewInput.java | 12 +- .../server/change/EmailReviewComments.java | 30 +---- .../gerrit/server/change/PostReview.java | 21 +++- .../gerrit/server/mail/CommentSender.java | 5 +- .../gerrit/testutil/FakeEmailSender.java | 15 +++ 7 files changed, 155 insertions(+), 35 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 9cd04b899b..5edd95634a 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -4118,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 d42a078d2f..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 @@ -28,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; @@ -38,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; @@ -68,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 @@ -267,6 +278,94 @@ public class CommentsIT extends AbstractDaemonTest { 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(); @@ -355,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/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-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 93782e3381..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; @@ -39,8 +40,6 @@ 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; @@ -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() { @@ -106,29 +105,6 @@ public class EmailReviewComments implements Runnable, RequestContext { RequestContext old = requestContext.setContext(this); try { - 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); 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 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/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/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