Merge "Update attention set behaviour when commenting on a thread." into stable-3.11
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index bc47adc..2803c0e 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -14,9 +14,11 @@
package com.google.gerrit.server.restapi.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.Objects.requireNonNull;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
@@ -25,6 +27,8 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -48,15 +52,17 @@
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.util.AttentionSetUtil;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
-import java.util.Collection;
import java.util.HashSet;
+import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
@@ -76,6 +82,7 @@
private final ServiceUserClassifier serviceUserClassifier;
private final CommentsUtil commentsUtil;
private final DraftCommentsReader draftCommentsReader;
+ private final ProjectCache projectCache;
@Inject
ReplyAttentionSetUpdates(
@@ -86,7 +93,8 @@
AccountResolver accountResolver,
ServiceUserClassifier serviceUserClassifier,
CommentsUtil commentsUtil,
- DraftCommentsReader draftCommentsReader) {
+ DraftCommentsReader draftCommentsReader,
+ ProjectCache projectCache) {
this.permissionBackend = permissionBackend;
this.addToAttentionSetOpFactory = addToAttentionSetOpFactory;
this.removeFromAttentionSetOpFactory = removeFromAttentionSetOpFactory;
@@ -95,6 +103,7 @@
this.serviceUserClassifier = serviceUserClassifier;
this.commentsUtil = commentsUtil;
this.draftCommentsReader = draftCommentsReader;
+ this.projectCache = projectCache;
}
/** Adjusts the attention set but only based on the automatic rules. */
@@ -235,7 +244,7 @@
addOwnerAndUploaderToAttentionSetIfSomeoneElseReplied(
bu, postReviewOp, changeNotes, currentUser, readyForReview, allNewComments);
- addAllAuthorsOfCommentThreads(bu, changeNotes, allNewComments);
+ addAllAuthorsOfCommentThreads(bu, changeNotes, allNewComments, currentUser);
}
/**
@@ -329,17 +338,71 @@
/** Adds all authors of all comment threads that received a reply during this update */
private void addAllAuthorsOfCommentThreads(
- BatchUpdate bu, ChangeNotes changeNotes, ImmutableSet<HumanComment> allNewComments) {
- List<HumanComment> publishedComments = commentsUtil.publishedHumanCommentsByChange(changeNotes);
- ImmutableSet<CommentThread<HumanComment>> repliedToCommentThreads =
- CommentThreads.forComments(publishedComments).getThreadsForChildren(allNewComments);
+ BatchUpdate bu,
+ ChangeNotes changeNotes,
+ ImmutableSet<HumanComment> allNewComments,
+ CurrentUser currentUser) {
+ boolean isOwnerOrUploader =
+ currentUser.getAccountId().equals(changeNotes.getChange().getOwner())
+ || currentUser.getAccountId().equals(changeNotes.getCurrentPatchSet().uploader());
- ImmutableSet<Account.Id> repliedToUsers =
- repliedToCommentThreads.stream()
- .map(CommentThread::comments)
- .flatMap(Collection::stream)
- .map(comment -> comment.author.getId())
- .collect(toImmutableSet());
+ boolean noCRLabel = false;
+ Optional<LabelValue> maxCRValue =
+ projectCache
+ .get(changeNotes.getChange().getProject())
+ .orElseThrow(
+ () ->
+ new IllegalStateException(
+ String.format(
+ "Couldn't find project \"%s\" for a change \"%s\"",
+ changeNotes.getChange().getProject(), changeNotes.getChangeId())))
+ .getLabelTypes(changeNotes)
+ .byLabel(LabelId.CODE_REVIEW)
+ .map(l -> l.getMax());
+
+ ImmutableSet<Account.Id> maxCrApprovers;
+ if (maxCRValue.isPresent()) {
+ maxCrApprovers =
+ changeNotes.getApprovals().all().get(changeNotes.getCurrentPatchSet().id()).stream()
+ .filter(
+ a ->
+ a.label().equals(LabelId.CODE_REVIEW)
+ && a.value() == maxCRValue.get().getValue())
+ .map(a -> a.accountId())
+ .collect(toImmutableSet());
+ } else {
+ noCRLabel = true;
+ maxCrApprovers = ImmutableSet.of();
+ }
+
+ // Include newly published comments, when building threads.
+ ImmutableList<HumanComment> relevantComments =
+ Stream.concat(
+ commentsUtil.publishedHumanCommentsByChange(changeNotes).stream(),
+ allNewComments.stream())
+ .collect(toImmutableList());
+ ImmutableSet<CommentThread<HumanComment>> repliedToCommentThreads =
+ CommentThreads.forComments(relevantComments).getThreadsForChildren(allNewComments);
+
+ LinkedHashSet<Account.Id> repliedToUsers = new LinkedHashSet<>();
+ for (CommentThread<HumanComment> thread : repliedToCommentThreads) {
+ // If thread is resolved, we only bring back the commenters who have not yet left max
+ // Code-Review vote.
+ // If Owner replied but didn't resolve, we assume clarification was asked add everyone on the
+ // thread to attention set.
+ boolean ignoreVoteCheck = noCRLabel || (thread.unresolved() && isOwnerOrUploader);
+ if (thread.unresolved() && !isOwnerOrUploader) {
+ // Reviewer replied. Owner is still the one to act. No need to add commenters.
+ continue;
+ }
+ thread.comments().stream()
+ .map(comment -> comment.author.getId())
+ .filter(
+ a ->
+ !a.equals(currentUser.getAccountId())
+ && (ignoreVoteCheck || !maxCrApprovers.contains(a)))
+ .forEach(repliedToUsers::add);
+ }
ImmutableSet<Account.Id> possibleUsersToAdd = approvalsUtil.getReviewers(changeNotes).all();
SetView<Account.Id> usersToAdd = Sets.intersection(possibleUsersToAdd, repliedToUsers);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 92da64e..0f245bd 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -130,6 +130,11 @@
@Before
public void setUp() {
TimeUtil.setCurrentMillisSupplier(fakeClock);
+ projectOperations
+ .project(allProjects)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
}
@Test
@@ -1402,10 +1407,51 @@
}
@Test
- public void reviewAddsAllUsersInCommentThread() throws Exception {
+ public void ownerReplyResolvedAddsNonVotedInCommentThread() throws Exception {
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
- change(r).current().review(reviewWithComment());
+ ReviewInput ri = reviewWithComment();
+ ri.label("Code-Review", 2);
+ change(r).current().review(ri);
+
+ TestAccount user2 = accountCreator.user2();
+
+ requestScopeOperations.setApiUser(user2.id());
+ change(r)
+ .current()
+ .review(
+ reviewInReplyToComment(
+ Iterables.getOnlyElement(
+ gApi.changes().id(r.getChangeId()).current().commentsAsList())
+ .id));
+
+ change(r).attention(user.email()).remove(new AttentionSetInput("removal"));
+ requestScopeOperations.setApiUser(admin.id());
+ ri =
+ reviewInReplyToComment(
+ gApi.changes().id(r.getChangeId()).current().commentsAsList().get(1).id);
+ ri.comments.get(Patch.COMMIT_MSG).get(0).unresolved = false;
+ change(r).current().review(ri);
+
+ // First user already voted, no need to bring them back.
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user2));
+ assertThat(attentionSet).hasAccountIdThat().isEqualTo(user2.id());
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet)
+ .hasReasonThat()
+ .isEqualTo("Someone else replied on a comment you posted");
+ }
+
+ @Test
+ public void ownerReplyUnresolvedAddsAllUsersInCommentThread() throws Exception {
+ PushOneCommit.Result r = createChange();
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput ri = reviewWithComment();
+ ri.label("Code-Review", 2);
+ change(r).current().review(ri);
TestAccount user2 = accountCreator.user2();
@@ -1443,6 +1489,71 @@
}
@Test
+ public void reviewerReplyUnresolvedAddsOnlyOwner() throws Exception {
+ PushOneCommit.Result r = createChange();
+ requestScopeOperations.setApiUser(user.id());
+ change(r).current().review(reviewWithComment());
+
+ TestAccount user2 = accountCreator.user2();
+ change(r).attention(admin.email()).remove(new AttentionSetInput("removal"));
+ requestScopeOperations.setApiUser(user2.id());
+ change(r)
+ .current()
+ .review(
+ reviewInReplyToComment(
+ Iterables.getOnlyElement(
+ gApi.changes().id(r.getChangeId()).current().commentsAsList())
+ .id));
+
+ // First user is not needed, owner haven't addressed their comment yet.
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasAccountIdThat().isEqualTo(admin.id());
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Someone else replied on the change");
+ }
+
+ @Test
+ public void reviewerReplyResolvedAddsNonVotedInCommentThread() throws Exception {
+ PushOneCommit.Result r = createChange();
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput ri = reviewWithComment();
+ ri.label("Code-Review", 2);
+ change(r).current().review(ri);
+
+ TestAccount user2 = accountCreator.user2();
+ requestScopeOperations.setApiUser(user2.id());
+ change(r)
+ .current()
+ .review(
+ reviewInReplyToComment(
+ Iterables.getOnlyElement(
+ gApi.changes().id(r.getChangeId()).current().commentsAsList())
+ .id));
+
+ TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
+ requestScopeOperations.setApiUser(user3.id());
+ ri =
+ reviewInReplyToComment(
+ gApi.changes().id(r.getChangeId()).current().commentsAsList().get(1).id);
+ ri.comments.get(Patch.COMMIT_MSG).get(0).unresolved = false;
+ change(r).current().review(ri);
+
+ // First user already voted, no need to bring them back.
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user2));
+ assertThat(attentionSet).hasAccountIdThat().isEqualTo(user2.id());
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet)
+ .hasReasonThat()
+ .isEqualTo("Someone else replied on a comment you posted");
+ }
+
+ @Test
public void reviewAddsAllUsersInCommentThreadWhenOriginalCommentIsARobotComment()
throws Exception {
PushOneCommit.Result result = createChange();
@@ -3278,6 +3389,7 @@
comment.message = "comment";
comment.setUpdated(TimeUtil.now());
comment.inReplyTo = id;
+ comment.unresolved = true;
ReviewInput reviewInput = new ReviewInput();
reviewInput.comments = ImmutableMap.of(Patch.COMMIT_MSG, ImmutableList.of(comment));
return reviewInput;
