opendev/gerrit
Code Issues Proposed changes

Add option to omit duplicate comments.

Browse Source 
Change-Id: I6afc938ad72afbcfd5cb653b88f7e65431d2018c
This commit is contained in:
Bill Wendling 2015-10-19 15:40:57 -07:00 committed by Jonathan Nieder
parent d218e4078c
commit 692b4ec251
6 changed files with 114 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -4482,23 +4482,23 @@ revision.
[options="header",cols="1,^1,5"]
|============================
|Field Name ||Description
|`message` |optional|
|Field Name ||Description
|`message` |optional|
The message to be added as review comment.
|`labels` |optional|
|`labels` |optional|
The votes that should be added to the revision as a map that maps the
label names to the voting values.
|`comments` |optional|
|`comments` |optional|
The comments that should be added as a map that maps a file path to a
list of link:#comment-input[CommentInput] entities.
|`strict_labels`|`true` if not set|
|`strict_labels` |`true` if not set|
Whether all labels are required to be within the user's permitted ranges
based on access controls. +
If `true`, attempting to use a label not granted to the user will fail
the entire modify operation early. +
If `false`, the operation will execute anyway, but the proposed labels
will be modified to be the "best" value allowed by the access controls.
|`drafts` |optional|
|`drafts` |optional|
Draft handling that defines how draft comments are handled that are
already in the database but that were not also described in this
input. +
@ -4506,12 +4506,14 @@ 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` |optional|
Notify handling that defines to whom email notifications should be sent
after the review is stored. +
Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
If not set, the default is `ALL`.
|`on_behalf_of`|optional|
|`omit_duplicate_comments`|optional|
If `true`, comments with the same content at the same place will be omitted.
|`on_behalf_of` |optional|
link:rest-api-accounts.html#account-id[\{account-id\}] the review
should be posted on behalf of. To use this option the caller must
have been granted `labelAs-NAME` permission for all keys of labels.

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

@ -188,12 +188,35 @@ public class CommentsIT extends AbstractDaemonTest {
}
}
@Test
public void addDuplicateComments() throws Exception {
PushOneCommit.Result r1 = createChange();
String changeId = r1.getChangeId();
String revId = r1.getCommit().getName();
addComment(r1, "nit: trailing whitespace");
addComment(r1, "nit: trailing whitespace");
Map<String, List<CommentInfo>> result = getPublishedComments(changeId, revId);
assertThat(result.get(FILE_NAME)).hasSize(2);
addComment(r1, "nit: trailing whitespace", true);
result = getPublishedComments(changeId, revId);
assertThat(result.get(FILE_NAME)).hasSize(2);
PushOneCommit.Result r2 = pushFactory.create(
db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "content")
.to("refs/for/master");
changeId = r2.getChangeId();
revId = r2.getCommit().getName();
addComment(r2, "nit: trailing whitespace", true);
result = getPublishedComments(changeId, revId);
assertThat(result.get(FILE_NAME)).hasSize(1);
}
@Test
public void listChangeDrafts() throws Exception {
PushOneCommit.Result r1 = createChange();
PushOneCommit.Result r2 = pushFactory.create(
db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent",
db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new content",
r1.getChangeId())
.to("refs/for/master");
@ -363,9 +386,13 @@ public class CommentsIT extends AbstractDaemonTest {
+ "-- \n");
}
private void addComment(PushOneCommit.Result r, String message)
throws Exception {
addComment(r, message, false);
}
private void addComment(PushOneCommit.Result r, String message,
boolean omitDuplicateComments) throws Exception {
CommentInput c = new CommentInput();
c.line = 1;
c.message = message;
@ -373,6 +400,7 @@ public class CommentsIT extends AbstractDaemonTest {
ReviewInput in = new ReviewInput();
in.comments = ImmutableMap.<String, List<CommentInput>> of(
FILE_NAME, ImmutableList.of(c));
in.omitDuplicateComments = omitDuplicateComments;
gApi.changes()
.id(r.getChangeId())
.revision(r.getCommit().name())

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

@ -48,6 +48,12 @@ public class ReviewInput {
/** Who to send email notifications to after review is stored. */
public NotifyHandling notify = NotifyHandling.ALL;
/**
* If true check to make sure that the comments being posted aren't already
* present.
*/
public boolean omitDuplicateComments;
/**
* Account ID, name, email address or username of another user. The review
* will be posted/updated on behalf of this named user instead of the

15
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java
View File

@ -15,5 +15,16 @@
package com.google.gerrit.extensions.client;
public enum Side {
PARENT, REVISION
}
PARENT,
REVISION;
public static Side fromShort(short s) {
switch (s) {
case 0:
return PARENT;
case 1:
return REVISION;
}
return null;
}
}

2
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java
View File

@ -52,7 +52,7 @@ public final class PatchLineComment {
}
@Override
protected void set(String newValue) {
public void set(String newValue) {
uuid = newValue;
}

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

@ -17,12 +17,17 @@ package com.google.gerrit.server.change;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.hash.HashCode;
import com.google.common.hash.Hashing;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
@ -75,9 +80,11 @@ import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
@Singleton
@ -299,6 +306,32 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
/**
* Used to compare PatchLineComments with CommentInput comments.
*/
@AutoValue
abstract static class CommentSetEntry {
private static CommentSetEntry create(Patch.Key key,
Integer line, Side side, HashCode message, CommentRange range) {
return new AutoValue_PostReview_CommentSetEntry(key, line, side, message,
range);
}
public static CommentSetEntry create(PatchLineComment comment) {
return create(comment.getKey().getParentKey(),
comment.getLine(),
Side.fromShort(comment.getSide()),
Hashing.sha1().hashString(comment.getMessage(), UTF_8),
comment.getRange());
}
abstract Patch.Key key();
@Nullable abstract Integer line();
abstract Side side();
abstract HashCode message();
@Nullable abstract CommentRange range();
}
private class Op extends BatchUpdate.Op {
private final PatchSet.Id psId;
private final ReviewInput in;
@ -374,6 +407,10 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
List<PatchLineComment> del = Lists.newArrayList();
List<PatchLineComment> ups = Lists.newArrayList();
Set<CommentSetEntry> existingIds = in.omitDuplicateComments
? readExistingComments(ctx)
: Collections.<CommentSetEntry>emptySet();
for (Map.Entry<String, List<CommentInput>> ent : map.entrySet()) {
String path = ent.getKey();
for (CommentInput c : ent.getValue()) {
@ -381,9 +418,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
PatchLineComment e = drafts.remove(Url.decode(c.id));
if (e == null) {
e = new PatchLineComment(
new PatchLineComment.Key(
new Patch.Key(psId, path),
ChangeUtil.messageUUID(ctx.getDb())),
new PatchLineComment.Key(new Patch.Key(psId, path), null),
c.line != null ? c.line : 0,
user.getAccountId(),
parent, ctx.getWhen());
@ -403,6 +438,12 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
c.range.endCharacter));
e.setLine(c.range.endLine);
}
if (existingIds.contains(CommentSetEntry.create(e))) {
continue;
}
if (e.getKey().get() == null) {
e.getKey().set(ChangeUtil.messageUUID(ctx.getDb()));
}
ups.add(e);
}
}
@ -430,6 +471,16 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
return !del.isEmpty() || !ups.isEmpty();
}
private Set<CommentSetEntry> readExistingComments(ChangeContext ctx)
throws OrmException {
Set<CommentSetEntry> r = new HashSet<>();
for (PatchLineComment c : plcUtil.publishedByChange(ctx.getDb(),
ctx.getChangeNotes())) {
r.add(CommentSetEntry.create(c));
}
return r;
}
private Map<String, PatchLineComment> changeDrafts(ChangeContext ctx)
throws OrmException {
Map<String, PatchLineComment> drafts = Maps.newHashMap();