opendev/gerrit
Code Issues Proposed changes

PostReview: Add option to publish drafts on all revisions

Browse Source 
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
This commit is contained in:
David Ostrovsky 2014-07-25 10:57:35 +02:00 committed by Dave Borowitz
parent e75830ed9c
commit 8a1da03928
7 changed files with 155 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

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

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

@ -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> postReview;
@Inject
@CanonicalWebUrl
private Provider<String> 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<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 @@ 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;
}

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 {

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

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

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

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