From b8e1456fdbce1d97d43b97f15110b90fb6abb239 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 25 Sep 2020 14:13:50 +0200 Subject: [PATCH] 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 --- .../server/restapi/change/CommentPorter.java | 61 +++++++++++++------ .../restapi/change/HumanCommentFilter.java | 32 ++++++++++ .../restapi/change/ListPortedComments.java | 6 +- .../restapi/change/ListPortedDrafts.java | 3 +- .../change/UnresolvedCommentFilter.java | 40 ++++++++++++ .../api/revision/PortedCommentsIT.java | 8 ++- .../restapi/change/CommentPorterTest.java | 18 ++++-- 7 files changed, 138 insertions(+), 30 deletions(-) create mode 100644 java/com/google/gerrit/server/restapi/change/HumanCommentFilter.java create mode 100644 java/com/google/gerrit/server/restapi/change/UnresolvedCommentFilter.java diff --git a/java/com/google/gerrit/server/restapi/change/CommentPorter.java b/java/com/google/gerrit/server/restapi/change/CommentPorter.java index e2abf29f47..681509cd55 100644 --- a/java/com/google/gerrit/server/restapi/change/CommentPorter.java +++ b/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 portComments( - ChangeNotes changeNotes, PatchSet targetPatchset, List comments) { + ChangeNotes changeNotes, + PatchSet targetPatchset, + List comments, + List filters) { - ImmutableList relevantComments = filterToRelevant(comments, targetPatchset); + ImmutableList allFilters = addDefaultFilters(filters, targetPatchset); + ImmutableList relevantComments = filter(comments, allFilters); return port(changeNotes, targetPatchset, relevantComments); } - private ImmutableList filterToRelevant( - List allComments, PatchSet targetPatchset) { - ImmutableList previousPatchsetsComments = - allComments.stream() - .filter(comment -> comment.key.patchSetId < targetPatchset.number()) - .collect(toImmutableList()); - - ImmutableSet> commentThreads = - CommentThreads.forComments(previousPatchsetsComments).getThreads(); - - return commentThreads.stream() - .filter(CommentThread::unresolved) - .map(CommentThread::comments) - .flatMap(Collection::stream) + private ImmutableList addDefaultFilters( + List 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 filter( + List allComments, ImmutableList filters) { + ImmutableList filteredComments = ImmutableList.copyOf(allComments); + for (HumanCommentFilter filter : filters) { + filteredComments = filter.filter(filteredComments); + } + return filteredComments; + } + private ImmutableList port( ChangeNotes notes, PatchSet targetPatchset, List comments) { Map> 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 filter(ImmutableList comments) { + return comments.stream() + .filter(comment -> comment.key.patchSetId < patchsetId.get()) + .collect(toImmutableList()); + } + } } diff --git a/java/com/google/gerrit/server/restapi/change/HumanCommentFilter.java b/java/com/google/gerrit/server/restapi/change/HumanCommentFilter.java new file mode 100644 index 0000000000..01800420bd --- /dev/null +++ b/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 filter(ImmutableList comments); +} diff --git a/java/com/google/gerrit/server/restapi/change/ListPortedComments.java b/java/com/google/gerrit/server/restapi/change/ListPortedComments.java index ec1dc1d0f2..6d6a02a01a 100644 --- a/java/com/google/gerrit/server/restapi/change/ListPortedComments.java +++ b/java/com/google/gerrit/server/restapi/change/ListPortedComments.java @@ -52,7 +52,11 @@ public class ListPortedComments implements RestReadView { List allComments = commentsUtil.publishedHumanCommentsByChange(revisionResource.getNotes()); ImmutableList portedComments = - commentPorter.portComments(revisionResource.getNotes(), targetPatchset, allComments); + commentPorter.portComments( + revisionResource.getNotes(), + targetPatchset, + allComments, + ImmutableList.of(new UnresolvedCommentFilter())); return Response.ok(format(portedComments)); } diff --git a/java/com/google/gerrit/server/restapi/change/ListPortedDrafts.java b/java/com/google/gerrit/server/restapi/change/ListPortedDrafts.java index b5542bf63c..9b254f1b9a 100644 --- a/java/com/google/gerrit/server/restapi/change/ListPortedDrafts.java +++ b/java/com/google/gerrit/server/restapi/change/ListPortedDrafts.java @@ -53,7 +53,8 @@ public class ListPortedDrafts implements RestReadView { commentsUtil.draftByChangeAuthor( revisionResource.getNotes(), revisionResource.getAccountId()); ImmutableList portedDraftComments = - commentPorter.portComments(revisionResource.getNotes(), targetPatchset, draftComments); + commentPorter.portComments( + revisionResource.getNotes(), targetPatchset, draftComments, ImmutableList.of()); return Response.ok(format(portedDraftComments)); } diff --git a/java/com/google/gerrit/server/restapi/change/UnresolvedCommentFilter.java b/java/com/google/gerrit/server/restapi/change/UnresolvedCommentFilter.java new file mode 100644 index 0000000000..e75ccca156 --- /dev/null +++ b/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 filter(ImmutableList comments) { + ImmutableSet> commentThreads = + CommentThreads.forComments(comments).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 d76d114f0e..d3d84579d1 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/PortedCommentsIT.java +++ b/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 portedComments = flatten(getPortedDraftCommentsOfUser(patchset2Id, accountId)); - assertThat(portedComments).comparingElementsUsing(hasUuid()).containsExactly(comment2Uuid); + assertThat(portedComments) + .comparingElementsUsing(hasUuid()) + .containsExactly(comment1Uuid, comment2Uuid); } @Test diff --git a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java b/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java index 8891cd2082..d0398e94f1 100644 --- a/javatests/com/google/gerrit/server/restapi/change/CommentPorterTest.java +++ b/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 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 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 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 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 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 portedComments = - commentPorter.portComments(changeNotes, patchset2, ImmutableList.of(comment)); + commentPorter.portComments( + changeNotes, patchset2, ImmutableList.of(comment), ImmutableList.of()); assertThat(portedComments).isEmpty(); }