Only port unresolved comment threads
Porting all comments from previous patchsets to the requested patchset
could be a large amount of comments especially if there was a lot of
activity (conversations + code adjustments) on the change in the past.
In addition, some of the comment threads might not even apply to the
later patchsets anymore as they were already addressed. To increase
the overview for users, we decide to port only unresolved comment
threads. We filter the comments already in the backend as we can thus
achieve the best performance (less comments to port -> less diffs to
compute, which can be a big amount otherwise for higher patchset
numbers).
We apply that filtering approach both to published and draft comments.
However, we don't factor in the unresolved state of draft comments which
are a reply to published comments when deciding whether to port the
published comment thread. We could have done this but it would have
added additional complexity. Furthermore, this would have introduced
a dependency of published comments on draft comments which we typically
don't have for endpoints of published comments. There's also always
the risk that Gerrit's state changes between the calls to the two
different endpoints for published and draft comments (e.g. draft
comment which changed the unresolved state is discarded), in which case
the published comment endpoint would return the wrong result.
For the frontend, this behavior means that it might get a ported draft
comment for which the published comment thread was not ported (or the
other way around). In such a case, the frontend should simply re-use the
original comment thread (which it loaded anyway as the frontend loads
all comments on a change) and put it to the file/range indicated by the
ported draft comment. The frontend needs such an approach anyway as we
don't port robot comments to which a published/draft comment can also
be a reply.
There is one corner case not implemented right now: If a robot comment
is in the middle of a comment thread, we might derive the wrong
unresolved state for the thread as we internally consider the thread
as two threads (-> one before the robot comment, one afterwards).
In theory, comment threads involving robot comments in the middle
are possible with the current APIs. However, we don't know of anybody
using such a bot at the moment. If somebody does, they might see
other issues as well as this corner case is not explicitly supported in
other places of Gerrit either. If we wanted to fix this corner case,
we'd need to load robot comments too and then filter them out before
porting the comments (or returning them). We didn't want to add this
additional complexity at the moment but can do so whenever we need it.
The same situation shouldn't happen for draft comments as drafts will
be considered to be at the end of a comment thread. Even if a draft
comment was added prior to another reply on the thread, its date will
be adjusted when published and hence it will move to the end of the
thread. In addition, published comments can't be a reply to a draft
comment.
Filtering out all resolved comment threads might be too aggressive in
some situations. Reviewers might especially want to see comment threads
which were resolved for the target patchset. The current fields on
comments don't allow us to identify this situation, though, as the
patchset field of a comment always points to the patchset on which the
root of the comment thread was left. We also shouldn't change this,
especially as it might break other tools. We'd need another solution
which we can add when we see that the current, simpler approach isn't
sufficient enough.
Change-Id: I4af5946ce34b9f4998d4922c6b51c585bc687c66
diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
index 3effa8c..05241e2 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentPorter.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentPorter.java
@@ -29,6 +29,8 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.CommentsUtil;
+import com.google.gerrit.server.change.CommentThread;
+import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.DiffMappings;
import com.google.gerrit.server.patch.GitPositionTransformer;
@@ -43,6 +45,7 @@
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
@@ -102,8 +105,18 @@
private ImmutableList<HumanComment> filterToRelevant(
List<HumanComment> allComments, PatchSet targetPatchset) {
- return allComments.stream()
- .filter(comment -> comment.key.patchSetId < targetPatchset.number())
+ ImmutableList<HumanComment> previousPatchsetsComments =
+ allComments.stream()
+ .filter(comment -> comment.key.patchSetId < targetPatchset.number())
+ .collect(toImmutableList());
+
+ ImmutableSet<CommentThread<HumanComment>> commentThreads =
+ CommentThreads.forComments(previousPatchsetsComments).getThreads();
+
+ return commentThreads.stream()
+ .filter(CommentThread::unresolved)
+ .map(CommentThread::comments)
+ .flatMap(Collection::stream)
.collect(toImmutableList());
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
index 9274a3a..a6301f9 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java
@@ -27,6 +27,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.acceptance.testsuite.change.TestCommentCreation;
import com.google.gerrit.acceptance.testsuite.change.TestPatchset;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
@@ -41,7 +42,6 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import org.junit.Ignore;
import org.junit.Test;
public class PortedCommentsIT extends AbstractDaemonTest {
@@ -58,9 +58,9 @@
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- changeOps.change(changeId).patchset(patchset2Id).newComment().create();
- changeOps.change(changeId).patchset(patchset3Id).newComment().create();
+ String comment1Uuid = newComment(patchset1Id).create();
+ newComment(patchset2Id).create();
+ newComment(patchset3Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -76,8 +76,8 @@
PatchSet.Id patchset3Id = changeOps.change(changeId).newPatchset().create();
PatchSet.Id patchset4Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- String comment3Uuid = changeOps.change(changeId).patchset(patchset3Id).newComment().create();
+ String comment1Uuid = newComment(patchset1Id).create();
+ String comment3Uuid = newComment(patchset3Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset4Id));
@@ -93,8 +93,8 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String comment1Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- String comment2Uuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String comment1Uuid = newComment(patchset1Id).create();
+ String comment2Uuid = newComment(patchset1Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -110,21 +110,9 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String rootCommentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
- String child1CommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(rootCommentUuid)
- .create();
- String child2CommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(child1CommentUuid)
- .create();
+ String rootCommentUuid = newComment(patchset1Id).create();
+ String child1CommentUuid = newComment(patchset1Id).parentUuid(rootCommentUuid).create();
+ String child2CommentUuid = newComment(patchset1Id).parentUuid(child1CommentUuid).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -134,17 +122,14 @@
}
@Test
- // TODO(aliceks): Filter out unresolved comment threads.
- @Ignore
- public void onlyUnresolvedCommentsArePorted() throws Exception {
+ public void onlyUnresolvedPublishedCommentsArePorted() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
- String comment2Uuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create();
+ newComment(patchset1Id).resolved().create();
+ String comment2Uuid = newComment(patchset1Id).unresolved().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -152,33 +137,34 @@
}
@Test
- // TODO(aliceks): Filter out unresolved comment threads.
- @Ignore
+ public void onlyUnresolvedDraftCommentsArePorted() throws Exception {
+ Account.Id accountId = accountOps.newAccount().create();
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().create();
+ PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
+ // Add comments.
+ newDraftComment(patchset1Id).author(accountId).resolved().create();
+ String comment2Uuid = newDraftComment(patchset1Id).author(accountId).unresolved().create();
+
+ List<CommentInfo> portedComments =
+ flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
+
+ assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(comment2Uuid);
+ }
+
+ @Test
public void unresolvedStateOfLastCommentInThreadMatters() throws Exception {
// Set up change and patchsets.
Change.Id changeId = changeOps.newChange().create();
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String rootComment1Uuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().resolved().create();
+ String rootComment1Uuid = newComment(patchset1Id).resolved().create();
String childComment1Uuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(rootComment1Uuid)
- .unresolved()
- .create();
- String rootComment2Uuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().unresolved().create();
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .parentUuid(rootComment2Uuid)
- .resolved()
- .create();
+ newComment(patchset1Id).parentUuid(rootComment1Uuid).unresolved().create();
+ String rootComment2Uuid = newComment(patchset1Id).unresolved().create();
+ newComment(patchset1Id).parentUuid(rootComment2Uuid).resolved().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -196,28 +182,15 @@
// Add comments. Comments should be more than 1 second apart as NoteDb only supports second
// precision.
LocalDateTime now = LocalDateTime.now();
- String rootCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .resolved()
- .createdOn(now)
- .create();
+ String rootCommentUuid = newComment(patchset1Id).resolved().createdOn(now).create();
String childComment1Uuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.parentUuid(rootCommentUuid)
.resolved()
.createdOn(now.plusSeconds(5))
.create();
String childComment2Uuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.parentUuid(rootCommentUuid)
.unresolved()
.createdOn(now.plusSeconds(10))
@@ -231,6 +204,30 @@
}
@Test
+ public void unresolvedStateOfDraftCommentsIsIgnoredForPublishedComments() throws Exception {
+ Account.Id accountId = accountOps.newAccount().create();
+ // Set up change and patchsets.
+ Change.Id changeId = changeOps.newChange().create();
+ PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
+ PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
+ // Add comments.
+ String rootComment1Uuid = newComment(patchset1Id).resolved().create();
+ newDraftComment(patchset1Id)
+ .author(accountId)
+ .parentUuid(rootComment1Uuid)
+ .unresolved()
+ .create();
+ String rootComment2Uuid = newComment(patchset1Id).unresolved().create();
+ newDraftComment(patchset1Id).author(accountId).parentUuid(rootComment2Uuid).resolved().create();
+
+ // Draft comments are only visible to their author.
+ requestScopeOps.setApiUser(accountId);
+ List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
+
+ assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(rootComment2Uuid);
+ }
+
+ @Test
public void draftCommentsAreNotPortedViaApiForPublishedComments() throws Exception {
Account.Id accountId = accountOps.newAccount().create();
// Set up change and patchsets.
@@ -238,7 +235,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
- changeOps.change(changeId).patchset(patchset1Id).newDraftComment().author(accountId).create();
+ newDraftComment(patchset1Id).author(accountId).create();
// Draft comments are only visible to their author.
requestScopeOps.setApiUser(accountId);
@@ -255,7 +252,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(accountId).create();
+ newComment(patchset1Id).author(accountId).create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
@@ -271,7 +268,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(accountId).create();
+ newComment(patchset1Id).author(accountId).create();
List<CommentInfo> portedComments =
flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId));
@@ -288,7 +285,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add draft comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(otherUserId).create();
+ newComment(patchset1Id).author(otherUserId).create();
List<CommentInfo> portedComments = flatten(getPortedDraftCommentsOfUser(patchset2Id, userId));
@@ -303,10 +300,7 @@
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
String rangeCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.message("Range comment")
.fromLine(1)
.charOffset(2)
@@ -315,30 +309,11 @@
.ofFile("myFile")
.create();
String lineCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .message("Line comment")
- .onLine(1)
- .ofFile("myFile")
- .create();
+ newComment(patchset1Id).message("Line comment").onLine(1).ofFile("myFile").create();
String fileCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .message("File comment")
- .onFileLevelOf("myFile")
- .create();
+ newComment(patchset1Id).message("File comment").onFileLevelOf("myFile").create();
String patchsetLevelCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .message("Patchset-level comment")
- .onPatchsetLevel()
- .create();
+ newComment(patchset1Id).message("Patchset-level comment").onPatchsetLevel().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -355,8 +330,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String commentUuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().onParentCommit().create();
+ String commentUuid = newComment(patchset1Id).onParentCommit().create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -370,7 +344,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
List<CommentInfo> portedComments = flatten(getPortedComments(patchset2Id));
@@ -384,7 +358,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -399,13 +373,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newDraftComment()
- .author(authorId)
- .create();
+ String commentUuid = newDraftComment(patchset1Id).author(authorId).create();
Map<String, List<CommentInfo>> portedComments =
getPortedDraftCommentsOfUser(patchset2Id, authorId);
@@ -424,8 +392,7 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create();
+ String commentUuid = newComment(patchset1.patchsetId()).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -439,13 +406,7 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1.patchsetId())
- .newComment()
- .message("My comment text")
- .create();
+ String commentUuid = newComment(patchset1.patchsetId()).message("My comment text").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -459,15 +420,9 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comments.
- String rootCommentUuid =
- changeOps.change(changeId).patchset(patchset1.patchsetId()).newComment().create();
+ String rootCommentUuid = newComment(patchset1.patchsetId()).create();
String childCommentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1.patchsetId())
- .newComment()
- .parentUuid(rootCommentUuid)
- .create();
+ newComment(patchset1.patchsetId()).parentUuid(rootCommentUuid).create();
CommentInfo portedComment = getPortedComment(patchset2Id, childCommentUuid);
@@ -482,8 +437,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps.change(changeId).patchset(patchset1Id).newComment().author(authorId).create();
+ String commentUuid = newComment(patchset1Id).author(authorId).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -498,13 +452,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newDraftComment()
- .author(authorId)
- .create();
+ String commentUuid = newDraftComment(patchset1Id).author(authorId).create();
Map<String, List<CommentInfo>> portedComments =
getPortedDraftCommentsOfUser(patchset2Id, authorId);
@@ -521,13 +469,7 @@
TestPatchset patchset1 = changeOps.change(changeId).currentPatchset().get();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1.patchsetId())
- .newComment()
- .tag("My comment tag")
- .create();
+ String commentUuid = newComment(patchset1.patchsetId()).tag("My comment tag").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -541,7 +483,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -555,7 +497,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid = changeOps.change(changeId).patchset(patchset1Id).newComment().create();
+ String commentUuid = newComment(patchset1Id).create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -572,13 +514,7 @@
PatchSet.Id patchset1Id = changeOps.change(changeId).currentPatchset().get().patchsetId();
PatchSet.Id patchset2Id = changeOps.change(changeId).newPatchset().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -602,10 +538,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -636,10 +569,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -665,10 +595,7 @@
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -703,10 +630,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -746,10 +670,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -789,10 +710,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -821,10 +739,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(3)
@@ -855,10 +770,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(2)
@@ -889,10 +801,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -923,10 +832,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(1)
.charOffset(2)
.toLine(3)
@@ -955,10 +861,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(3)
@@ -988,10 +891,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1020,10 +920,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1060,10 +957,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(2)
.charOffset(2)
.toLine(4)
@@ -1087,10 +981,7 @@
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
.fromLine(3)
.charOffset(2)
.toLine(4)
@@ -1119,14 +1010,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1147,14 +1031,7 @@
.content("Line 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1170,14 +1047,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1201,14 +1071,7 @@
PatchSet.Id patchset3Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset3Id);
@@ -1238,14 +1101,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1272,14 +1128,7 @@
.content("Line 1\nLine two\nLine three\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(2)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1302,14 +1151,7 @@
.content("Line 1\nLine 2\nLine three\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(2)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(2).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1330,14 +1172,7 @@
.content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1358,14 +1193,7 @@
.content("Line 1\nLine 2\nSome completely\ndifferent\ncontent\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(4)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(4).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1386,14 +1214,7 @@
.content("Line 1\nLine 2\nLine three\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(4)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(4).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1409,14 +1230,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onLine(3)
- .ofFile("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onLine(3).ofFile("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
@@ -1439,13 +1253,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
CommentInfo portedComment = getPortedComment(patchset2Id, commentUuid);
@@ -1461,13 +1269,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1492,7 +1294,7 @@
.content("Line 1\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().onFileLevelOf("myFile").create();
+ newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1510,13 +1312,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").delete().create();
// Add comment.
- String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
- .onFileLevelOf("myFile")
- .create();
+ String commentUuid = newComment(patchset1Id).onFileLevelOf("myFile").create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
assertThatMap(portedComments).keys().containsExactly(Patch.PATCHSET_LEVEL);
@@ -1539,7 +1335,7 @@
.content("Line 1\nLine 1.1\nLine 1.2\nLine 2\nLine 3\nLine 4\n")
.create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create();
+ newComment(patchset1Id).onPatchsetLevel().create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1555,7 +1351,7 @@
PatchSet.Id patchset2Id =
changeOps.change(changeId).newPatchset().file("myFile").renameTo("newFileName").create();
// Add comment.
- changeOps.change(changeId).patchset(patchset1Id).newComment().onPatchsetLevel().create();
+ newComment(patchset1Id).onPatchsetLevel().create();
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchset2Id);
@@ -1576,10 +1372,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(changeId)
- .patchset(patchset1Id)
- .newComment()
+ newComment(patchset1Id)
// The /COMMIT_MSG file has a header of 6 lines, so the summary line is in line 7.
// Place comment on 'Text 2' which is line 10.
.onLine(10)
@@ -1616,14 +1409,7 @@
changeOps.change(childChangeId).newPatchset().parent().patchset(parentPatchset2Id).create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onParentCommit()
- .onLine(1)
- .ofFile("myFile")
- .create();
+ newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("myFile").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1665,14 +1451,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onParentCommit()
- .onLine(1)
- .ofFile("file1")
- .create();
+ newComment(childPatchset1Id).onParentCommit().onLine(1).ofFile("file1").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1714,14 +1493,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onSecondParentCommit()
- .onLine(1)
- .ofFile("file2")
- .create();
+ newComment(childPatchset1Id).onSecondParentCommit().onLine(1).ofFile("file2").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1769,14 +1541,7 @@
.create();
// Add comment.
String commentUuid =
- changeOps
- .change(childChangeId)
- .patchset(childPatchset1Id)
- .newComment()
- .onAutoMergeCommit()
- .onLine(1)
- .ofFile("file1")
- .create();
+ newComment(childPatchset1Id).onAutoMergeCommit().onLine(1).ofFile("file1").create();
CommentInfo portedComment = getPortedComment(childPatchset2Id, commentUuid);
@@ -1787,6 +1552,22 @@
assertThat(portedComment).line().isGreaterThan(2);
}
+ private TestCommentCreation.Builder newComment(PatchSet.Id patchsetId) {
+ // Create unresolved comments by default as only those are ported. Tests get override the
+ // unresolved state by explicitly setting it.
+ return changeOps.change(patchsetId.changeId()).patchset(patchsetId).newComment().unresolved();
+ }
+
+ private TestCommentCreation.Builder newDraftComment(PatchSet.Id patchsetId) {
+ // Create unresolved comments by default as only those are ported. Tests get override the
+ // unresolved state by explicitly setting it.
+ return changeOps
+ .change(patchsetId.changeId())
+ .patchset(patchsetId)
+ .newDraftComment()
+ .unresolved();
+ }
+
private CommentInfo getPortedComment(PatchSet.Id patchsetId, String commentUuid)
throws RestApiException {
Map<String, List<CommentInfo>> portedComments = getPortedComments(patchsetId);
diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
index cb29315..9c30fc9 100644
--- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
+++ b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java
@@ -57,6 +57,8 @@
@Mock private PatchListCache patchListCache;
+ private int uuidCounter = 0;
+
@Test
public void commentsAreNotDroppedWhenDiffNotAvailable() throws Exception {
Project.NameKey project = Project.nameKey("myProject");
@@ -206,7 +208,7 @@
private HumanComment createComment(PatchSet.Id patchsetId, String filePath) {
return new HumanComment(
- new Comment.Key("commentUuid", filePath, patchsetId.get()),
+ new Comment.Key(getUniqueUuid(), filePath, patchsetId.get()),
Account.id(100),
new Timestamp(1234),
(short) 1,
@@ -215,6 +217,10 @@
true);
}
+ private String getUniqueUuid() {
+ return "commentUuid" + uuidCounter++;
+ }
+
private Correspondence<HumanComment, String> hasFilePath() {
return NullAwareCorrespondence.transforming(comment -> comment.key.filename, "hasFilePath");
}
