Also port resolved draft comments (in addition to unresolved ones)

In I4af5946ce, we switched to only porting unresolved comments to
avoid overloading users and decided to keep all unresolved comment
threads as we guessed that those would still be relevant for users.
For draft comments, the situation is a bit different, though. All
draft comments of a user are very likely relevant for that user.
In addition, the risk of overloading users is very much reduced as
users are unlikely to keep tons of draft comments around on a change.

Hence, we decide to port draft comments irrespective of their
unresolved state.

This will help to better support users during several use cases. For
instance, change authors sometimes upload another patchset while
someone else is reviewing the change. Such reviewers will then be able
to see all of their previous draft comments even when they continue the
review on the later patchset.

Change-Id: Ifeac90995a695d6ceaedf928f7aadf1e19e3f24f

java/com/google/gerrit/server/restapi/change/CommentPorter.java

@@ -29,8 +29,6 @@ import com.google.gerrit.entities.PatchSet;
 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;
@@ -45,11 +43,11 @@ import com.google.gerrit.server.patch.PatchListKey;
 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;
 import java.util.Optional;
+import java.util.stream.Stream;
 import org.eclipse.jgit.lib.ObjectId;
 
 /**
@@ -96,32 +94,40 @@ public class CommentPorter {
    * @param changeNotes the {@link ChangeNotes} of the change to which the comments belong
    * @param targetPatchset the patchset to which the comments should be ported
    * @param comments the original comments
+   * @param filters additional filters to apply to the comments before porting. Only the remaining
+   *     comments will be ported.
    * @return the ported comments, in no particular order
    */
   public ImmutableList<HumanComment> portComments(
-      ChangeNotes changeNotes, PatchSet targetPatchset, List<HumanComment> comments) {
+      ChangeNotes changeNotes,
+      PatchSet targetPatchset,
+      List<HumanComment> comments,
+      List<HumanCommentFilter> filters) {
 
-    ImmutableList<HumanComment> relevantComments = filterToRelevant(comments, targetPatchset);
+    ImmutableList<HumanCommentFilter> allFilters = addDefaultFilters(filters, targetPatchset);
+    ImmutableList<HumanComment> relevantComments = filter(comments, allFilters);
     return port(changeNotes, targetPatchset, relevantComments);
   }
 
-  private ImmutableList<HumanComment> filterToRelevant(
-      List<HumanComment> allComments, PatchSet targetPatchset) {
-    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)
+  private ImmutableList<HumanCommentFilter> addDefaultFilters(
+      List<HumanCommentFilter> filters, PatchSet targetPatchset) {
+    // Apply the EarlierPatchsetCommentFilter first as it reduces the amount of comments before
+    // more expensive filters are applied.
+    HumanCommentFilter earlierPatchsetFilter =
+        new EarlierPatchsetCommentFilter(targetPatchset.id());
+    return Stream.concat(Stream.of(earlierPatchsetFilter), filters.stream())
         .collect(toImmutableList());
   }
 
+  private ImmutableList<HumanComment> filter(
+      List<HumanComment> allComments, ImmutableList<HumanCommentFilter> filters) {
+    ImmutableList<HumanComment> filteredComments = ImmutableList.copyOf(allComments);
+    for (HumanCommentFilter filter : filters) {
+      filteredComments = filter.filter(filteredComments);
+    }
+    return filteredComments;
+  }
+
   private ImmutableList<HumanComment> port(
       ChangeNotes notes, PatchSet targetPatchset, List<HumanComment> comments) {
     Map<Integer, ImmutableList<HumanComment>> commentsPerPatchset =
@@ -309,4 +315,21 @@ public class CommentPorter {
     int adjustedEndLine = originalEndChar > 0 ? lineRange.end() : lineRange.end() + 1;
     return new Range(lineRange.start() + 1, originalStartChar, adjustedEndLine, originalEndChar);
   }
+
+  /** A filter which just keeps those comments which are before the given patchset. */
+  private static class EarlierPatchsetCommentFilter implements HumanCommentFilter {
+
+    private final PatchSet.Id patchsetId;
+
+    public EarlierPatchsetCommentFilter(PatchSet.Id patchsetId) {
+      this.patchsetId = patchsetId;
+    }
+
+    @Override
+    public ImmutableList<HumanComment> filter(ImmutableList<HumanComment> comments) {
+      return comments.stream()
+          .filter(comment -> comment.key.patchSetId < patchsetId.get())
+          .collect(toImmutableList());
+    }
+  }
 }

java/com/google/gerrit/server/restapi/change/HumanCommentFilter.java

@@ -0,0 +1,32 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.restapi.change;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.HumanComment;
+
+/** Filter for human comments. */
+public interface HumanCommentFilter {
+
+  /**
+   * Filters the given comments. The returned comments are the ones which still remain after this
+   * filter was applied.
+   *
+   * @param comments comments which should be filtered
+   * @return remaining comments. Must not include comments which weren't included in the given
+   *     comments.
+   */
+  ImmutableList<HumanComment> filter(ImmutableList<HumanComment> comments);
+}

java/com/google/gerrit/server/restapi/change/ListPortedComments.java

@@ -52,7 +52,11 @@ public class ListPortedComments implements RestReadView<RevisionResource> {
     List<HumanComment> allComments =
         commentsUtil.publishedHumanCommentsByChange(revisionResource.getNotes());
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(revisionResource.getNotes(), targetPatchset, allComments);
+        commentPorter.portComments(
+            revisionResource.getNotes(),
+            targetPatchset,
+            allComments,
+            ImmutableList.of(new UnresolvedCommentFilter()));
     return Response.ok(format(portedComments));
   }
 

java/com/google/gerrit/server/restapi/change/ListPortedDrafts.java

@@ -53,7 +53,8 @@ public class ListPortedDrafts implements RestReadView<RevisionResource> {
         commentsUtil.draftByChangeAuthor(
             revisionResource.getNotes(), revisionResource.getAccountId());
     ImmutableList<HumanComment> portedDraftComments =
-        commentPorter.portComments(revisionResource.getNotes(), targetPatchset, draftComments);
+        commentPorter.portComments(
+            revisionResource.getNotes(), targetPatchset, draftComments, ImmutableList.of());
     return Response.ok(format(portedDraftComments));
   }
 

java/com/google/gerrit/server/restapi/change/UnresolvedCommentFilter.java

@@ -0,0 +1,40 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.restapi.change;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.server.change.CommentThread;
+import com.google.gerrit.server.change.CommentThreads;
+import java.util.Collection;
+
+/** A filter which only keeps comments which are part of an unresolved {@link CommentThread}. */
+public class UnresolvedCommentFilter implements HumanCommentFilter {
+
+  @Override
+  public ImmutableList<HumanComment> filter(ImmutableList<HumanComment> comments) {
+    ImmutableSet<CommentThread<HumanComment>> commentThreads =
+        CommentThreads.forComments(comments).getThreads();
+
+    return commentThreads.stream()
+        .filter(CommentThread::unresolved)
+        .map(CommentThread::comments)
+        .flatMap(Collection::stream)
+        .collect(toImmutableList());
+  }
+}

javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java

@@ -141,20 +141,22 @@ public class PortedCommentsIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void onlyUnresolvedDraftCommentsArePorted() throws Exception {
+  public void resolvedAndUnresolvedDraftCommentsArePorted() 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 comment1Uuid = 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);
+    assertThat(portedComments)
+        .comparingElementsUsing(hasUuid())
+        .containsExactly(comment1Uuid, comment2Uuid);
   }
 
   @Test

javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java

@@ -79,7 +79,8 @@ public class CommentPorterTest {
     when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
         .thenThrow(PatchListNotAvailableException.class);
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+        commentPorter.portComments(
+            changeNotes, patchset2, ImmutableList.of(comment), ImmutableList.of());
 
     assertThat(portedComments).isNotEmpty();
   }
@@ -100,7 +101,8 @@ public class CommentPorterTest {
     when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
         .thenThrow(IllegalStateException.class);
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+        commentPorter.portComments(
+            changeNotes, patchset2, ImmutableList.of(comment), ImmutableList.of());
 
     assertThat(portedComments).isNotEmpty();
   }
@@ -119,7 +121,8 @@ public class CommentPorterTest {
     when(commentsUtil.determineCommitId(any(), any(), anyShort()))
         .thenThrow(IllegalStateException.class);
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+        commentPorter.portComments(
+            changeNotes, patchset2, ImmutableList.of(comment), ImmutableList.of());
 
     assertThat(portedComments).isNotEmpty();
   }
@@ -140,7 +143,8 @@ public class CommentPorterTest {
     when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
         .thenThrow(IllegalStateException.class);
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+        commentPorter.portComments(
+            changeNotes, patchset2, ImmutableList.of(comment), ImmutableList.of());
 
     assertThat(portedComments)
         .comparingElementsUsing(hasFilePath())
@@ -169,7 +173,8 @@ public class CommentPorterTest {
         .thenThrow(IllegalStateException.class)
         .thenReturn(emptyDiff);
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(changeNotes, patchset3, ImmutableList.of(comment1, comment2));
+        commentPorter.portComments(
+            changeNotes, patchset3, ImmutableList.of(comment1, comment2), ImmutableList.of());
 
     // One of the comments should still be ported as usual. -> Keeps its file name as the diff was
     // empty.
@@ -194,7 +199,8 @@ public class CommentPorterTest {
     when(patchListCache.get(any(PatchListKey.class), any(Project.NameKey.class)))
         .thenReturn(emptyDiff);
     ImmutableList<HumanComment> portedComments =
-        commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment));
+        commentPorter.portComments(
+            changeNotes, patchset2, ImmutableList.of(comment), ImmutableList.of());
 
     assertThat(portedComments).isEmpty();
   }