Add owner to attention set only when a reply requires the attention of the owner
So far the change owner (and the uploader) have been added to the
attention set whenever someone replied on the change, even if the reply
was on an outdated patch set and didn't impact the current change state
(e.g. if someone voted on an outdated patch set and none of the votes
were copied to the current patch set).
With this change we update the attention set update logic to:
* Only add the change owner and uploader to the attention set if a reply
requires their attention which is the case if votes on the current
patch set were updated (either directly or through vote copying), a
change message was posted, new comments were posted or a
work-in-progress change was marked as ready.
* Keep the replying user in the attention set (or add them to the
attention set if they are not in the attention set yet) if they voted
on an outdated patch set and not all of their votes got copied to the
current patch set, so that they are made aware that they need to
re-apply their vote on the current patch set. In this case the reason
for the user to be in the attention set is set to "Some votes were not
copied to the current patch set".
* For bot replies: Only add the change owner and uploader to the
attention set if negative votes got updated on the current patch set
(either directly or through vote copying).
This means to decide whether or not the change owner and uploader should
be added to the attention set and whether or not the replying user
should be kept in the attention set or be added to it, depends on the
result of the approval copying which is done in PostReviewOp (whether or
not approvals that were applied on outdated patch sets were copied to
the current patch set).
This means which attention set updates need to be performed depends on
the result of PostReviewOp, which is not available yet at the moment
when we create the BatchUpdate ops to update the attention set, but only
when the attention update ops are being executed through BatchUpdate (at
this point the preceding PostReviewOp which is part of the same
BatchUpdate has been executed and can be asked for its result).
To make attention set updates that are based on the result of the
preceding PostReviewOp, we add attention set updates with conditions
when creating the ops for the BatchUpdate, that are only being checked
when the ops are being executed (also see change Iba9e5617f which added
support for conditional attention set updates).
The new behaviour for attention set updates on reply is this:
1. If the reply of a human user changes the current change state (it
updates votes on the current patch set, posts a change message, posts
new comments or marks a work-in-progress as ready), the change owner
and the uploader are added to the attention set and the replying user
is removed from the attention set. This means no-op (empty) replies
that do not change the current change state no longer update the
attention set.
2. If the reply of a human user only applies votes on an outdated patch
set and none of these votes is copied to the current patch set, the
change owner and uploader are not added to the attention set and the
replying user is kept in the attention set or is added to it to make
them aware that they need to re-apply votes on the current patch set
(the reason for being in the attention set is updated accordingly).
3. If the reply of a bot user results in a negative vote on the current
patch set, the change owner and uploader are added to the attention
set.
4. If the reply of a bot doesn't result in a negative vote on the
current patch set (e.g. because the bot voted on an outdated patch
set and the vote was not copied to the current patch set), the change
owner and uploader are not added to the attention set.
Note, the behaviour for bot users is different than for human users
since we have simplied rules for them (the change owner and uploader are
only added to the attention set if the bot voted negatively).
Bug: Google b/262017722
Release-Notes: Changed attention set logic to add owner and uploader to attention set only when a reply requires the attention of the m
Change-Id: I1c87439c1aa612535c7270cec9ea4a8e4ba3534a
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 50cd2bc..f54df5b 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -378,8 +378,8 @@
bu.addOp(revision.getChange().getId(), postReviewOp);
// Adjust the attention set based on the input
- replyAttentionSetUpdates.updateAttentionSet(
- bu, revision.getNotes(), input, revision.getUser());
+ replyAttentionSetUpdates.updateAttentionSetOnPostReview(
+ bu, postReviewOp, revision.getNotes(), input, revision.getUser());
bu.execute();
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index c017981..490ff490 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.restapi.change;
import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -171,6 +172,54 @@
}
}
+ @AutoValue
+ public abstract static class Result {
+ /**
+ * Whether this {@code PostReviewOp} updated any vote on the current patch set.
+ *
+ * @return returns {@code true} if a) ReviewInput contained votes and b) ReviewInput was applied
+ * on the current patch set or any votes got copied to the current patch set.
+ */
+ abstract boolean updatedAnyVoteOnCurrentPatchSet();
+
+ /**
+ * Whether this {@code PostReviewOp} applied any negative vote on the current patch set.
+ *
+ * @return returns {@code true} if a) ReviewInput contained negative votes and b) ReviewInput
+ * was applied on the current patch set or any of the negative votes got copied to the
+ * current patch set.
+ */
+ abstract boolean updatedAnyNegativeVoteOnCurrentPatchSet();
+
+ /**
+ * Whether this {@code PostReviewOp} applied votes on an outdated patch set that were not copied
+ * to the current patch set.
+ *
+ * @return returns {@code true} if a) ReviewInput contained votes, b) ReviewInput was applied on
+ * an outdated patch set and c) not all of the votes got copied to the current patch set
+ */
+ abstract boolean appliedVotesOnOutdatedPatchSetThatWereNotCopiedToCurrentPatchSet();
+
+ /**
+ * Whether this {@code PostReviewOp} posted a change message.
+ *
+ * @return returns {@code true} if ReviewInput contained a message.
+ */
+ abstract boolean postedChangeMessage();
+
+ static Result create(
+ boolean updatedAnyVoteOnCurrentPatchSet,
+ boolean updatedAnyNegativeVoteOnCurrentPatchSet,
+ boolean appliedVotesOnOutdatedPatchSetThatWereNotCopiedToCurrentPatchSet,
+ boolean postedChangeMessage) {
+ return new AutoValue_PostReviewOp_Result(
+ updatedAnyVoteOnCurrentPatchSet,
+ updatedAnyNegativeVoteOnCurrentPatchSet,
+ appliedVotesOnOutdatedPatchSetThatWereNotCopiedToCurrentPatchSet,
+ postedChangeMessage);
+ }
+ }
+
@VisibleForTesting
public static final String START_REVIEW_MESSAGE = "This change is ready for review.";
@@ -203,6 +252,8 @@
private Map<String, Short> approvals = new HashMap<>();
private Map<String, Short> oldApprovals = new HashMap<>();
+ private Result result;
+
@Inject
PostReviewOp(
@GerritServerConfig Config gerritConfig,
@@ -266,6 +317,14 @@
try (TraceContext.TraceTimer ignored = newTimer("insertMessage")) {
dirty |= insertMessage(ctx);
}
+
+ result =
+ Result.create(
+ updatedAnyVoteOnCurrentPatchSet(),
+ updatedAnyNegativeVoteOnCurrentPatchSet(),
+ appliedVotesOnOutdatedPatchSetThatWereNotCopiedToCurrentPatchSet(),
+ postedChangeMessage());
+
return dirty;
}
@@ -1127,6 +1186,98 @@
labelDelta.add(LabelVote.create(name, value));
}
+ /**
+ * Gets the result of running this {@code PostReviewOp}.
+ *
+ * <p>Must only be invoked after this {@code PostReviewOp} has been executed with {@link
+ * com.google.gerrit.server.update.BatchUpdate}.
+ *
+ * @throws IllegalStateException thrown if invoked before this {@code PostReviewOp} has been
+ * executed
+ */
+ public Result getResult() {
+ checkState(result != null, "cannot retrieve result, change update has not been executed yet");
+ return result;
+ }
+
+ /**
+ * Whether this {@code PostReviewOp} updated any vote on the current patch set.
+ *
+ * <p>Must only be invoked after this {@code PostReviewOp} has been executed with {@link
+ * com.google.gerrit.server.update.BatchUpdate}.
+ *
+ * @return returns {@code true} if a) ReviewInput contained votes and b) ReviewInput was applied
+ * on the current patch set or any votes got copied to the current patch set.
+ */
+ private boolean updatedAnyVoteOnCurrentPatchSet() {
+ return in.labels != null
+ && !in.labels.isEmpty()
+ && (notes.getCurrentPatchSet().id().equals(psId)
+ || labelUpdatesOnFollowUpPatchSets.values().stream()
+ .anyMatch(
+ copiedLabelUpdate ->
+ copiedLabelUpdate.patchSetId().equals(notes.getCurrentPatchSet().id())));
+ }
+
+ /**
+ * Whether this {@code PostReviewOp} applied any negative vote on the current patch set.
+ *
+ * <p>Must only be invoked after this {@code PostReviewOp} has been executed with {@link
+ * com.google.gerrit.server.update.BatchUpdate}.
+ *
+ * @return returns {@code true} if a) ReviewInput contained negative votes and b) ReviewInput was
+ * applied on the current patch set or any of the negative votes got copied to the current
+ * patch set.
+ */
+ private boolean updatedAnyNegativeVoteOnCurrentPatchSet() {
+ return in.labels != null
+ && in.labels.values().stream().anyMatch(vote -> vote < 0)
+ && (notes.getCurrentPatchSet().id().equals(psId)
+ || labelUpdatesOnFollowUpPatchSets.entries().stream()
+ .filter(e -> e.getKey().value() < 0)
+ .anyMatch(e -> e.getValue().patchSetId().equals(notes.getCurrentPatchSet().id())));
+ }
+
+ /**
+ * Whether this {@code PostReviewOp} applied votes on an outdated patch set that were not copied
+ * to the current patch set.
+ *
+ * <p>Must only be invoked after this {@code PostReviewOp} has been executed with {@link
+ * com.google.gerrit.server.update.BatchUpdate}.
+ *
+ * @return returns {@code true} if a) ReviewInput contained votes, b) ReviewInput was applied on
+ * an outdated patch set and c) not all of the votes got copied to the current patch set
+ */
+ private boolean appliedVotesOnOutdatedPatchSetThatWereNotCopiedToCurrentPatchSet() {
+ if (in.labels == null || notes.getCurrentPatchSet().id().equals(psId)) {
+ return false;
+ }
+
+ for (Map.Entry<String, Short> labelEntry : in.labels.entrySet()) {
+ if (labelUpdatesOnFollowUpPatchSets
+ .get(LabelVote.create(labelEntry.getKey(), labelEntry.getValue())).stream()
+ .anyMatch(
+ copiedLabelUpdate ->
+ copiedLabelUpdate.patchSetId().equals(notes.getCurrentPatchSet().id()))) {
+ continue;
+ }
+
+ // vote was not copied to current patch set
+ return true;
+ }
+
+ return false;
+ }
+
+ /**
+ * Whether this {@code PostReviewOp} posted a change message.
+ *
+ * @return returns {@code true} if ReviewInput contained a message.
+ */
+ private boolean postedChangeMessage() {
+ return !Strings.isNullOrEmpty(in.message);
+ }
+
private TraceContext.TraceTimer newTimer(String method) {
return TraceContext.newTimer(getClass().getSimpleName() + "#" + method, Metadata.empty());
}
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index 9adf963..bc47adc 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -15,11 +15,13 @@
package com.google.gerrit.server.restapi.change;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.HumanComment;
@@ -37,6 +39,7 @@
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.change.AddToAttentionSetOp;
import com.google.gerrit.server.change.AttentionSetUnchangedOp;
+import com.google.gerrit.server.change.AttentionSetUpdateCondition;
import com.google.gerrit.server.change.CommentThread;
import com.google.gerrit.server.change.CommentThreads;
import com.google.gerrit.server.change.RemoveFromAttentionSetOp;
@@ -106,6 +109,7 @@
}
processRules(
bu,
+ /* postReviewOp= */ null,
changeNotes,
readyForReview,
currentUser,
@@ -113,14 +117,24 @@
}
/**
- * Adjusts the attention set by adding and removing users. If the same user should be added and
- * removed or added/removed twice, the user will only be added/removed once, based on first
- * addition/removal.
+ * Adjusts the attention set when a review is posted.
+ *
+ * <p>If the same user should be added and removed or added/removed twice, the user will only be
+ * added/removed once, based on first addition/removal.
+ *
+ * @param postReviewOp the {@link PostReviewOp} that is being executed before the attention set
+ * updates
*/
- public void updateAttentionSet(
- BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input, CurrentUser currentUser)
+ public void updateAttentionSetOnPostReview(
+ BatchUpdate bu,
+ PostReviewOp postReviewOp,
+ ChangeNotes changeNotes,
+ ReviewInput input,
+ CurrentUser currentUser)
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
+ requireNonNull(postReviewOp, "postReviewOp must not be null");
+
processManualUpdates(bu, changeNotes, input);
if (input.ignoreAutomaticAttentionSetRules) {
@@ -133,12 +147,13 @@
boolean isReadyForReview = isReadyForReview(changeNotes, input);
if (isReadyForReview && serviceUserClassifier.isServiceUser(currentUser.getAccountId())) {
- botsWithNegativeLabelsAddOwnerAndUploader(bu, changeNotes, input);
+ botsWithNegativeLabelsAddOwnerAndUploader(bu, postReviewOp, changeNotes);
return;
}
processRules(
bu,
+ postReviewOp,
changeNotes,
isReadyForReview,
currentUser,
@@ -182,27 +197,33 @@
}
/**
- * Process the automatic rules of the attention set. All of the automatic rules except
- * adding/removing reviewers and entering/exiting WIP state are done here, and the rest are done
- * in {@link ChangeUpdate}
+ * Process the automatic rules of the attention set.
+ *
+ * <p>All of the automatic rules except adding/removing reviewers and entering/exiting WIP state
+ * are done here, and the rest are done in {@link ChangeUpdate}.
+ *
+ * @param postReviewOp {@link PostReviewOp} that is being executed before the attention set
+ * updates, may be {@code null}
*/
private void processRules(
BatchUpdate bu,
+ @Nullable PostReviewOp postReviewOp,
ChangeNotes changeNotes,
boolean readyForReview,
CurrentUser currentUser,
ImmutableSet<HumanComment> allNewComments) {
- // Replying removes the publishing user from the attention set.
- removeFromAttentionSet(bu, changeNotes, currentUser.getAccountId(), "removed on reply", false);
-
- Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
- Account.Id owner = changeNotes.getChange().getOwner();
+ updateAttentionSetForCurrentUser(bu, postReviewOp, changeNotes, currentUser);
// The rest of the conditions only apply if the change is open.
if (changeNotes.getChange().getStatus().isClosed()) {
// We still add the owner if a new comment thread was created, on closed changes.
if (allNewComments.stream().anyMatch(c -> c.parentUuid == null)) {
- addToAttentionSet(bu, changeNotes, owner, "A new comment thread was created", false);
+ addToAttentionSet(
+ bu,
+ changeNotes,
+ changeNotes.getChange().getOwner(),
+ "A new comment thread was created",
+ false);
}
return;
}
@@ -212,14 +233,98 @@
return;
}
- if (!currentUser.getAccountId().equals(owner)) {
- addToAttentionSet(bu, changeNotes, owner, "Someone else replied on the change", false);
+ addOwnerAndUploaderToAttentionSetIfSomeoneElseReplied(
+ bu, postReviewOp, changeNotes, currentUser, readyForReview, allNewComments);
+ addAllAuthorsOfCommentThreads(bu, changeNotes, allNewComments);
+ }
+
+ /**
+ * Updates the attention set for the current user.
+ *
+ * <p>Removes the current user from the attention set (since they replied) unless they voted on an
+ * outdated patch set and some of the votes were not copied to the current patch set (in this case
+ * they should be in the attention set to re-apply their votes).
+ *
+ * <p>If the current user voted on an outdated patch set and some of the votes were not copied to
+ * the current patch set:
+ *
+ * <ul>
+ * <li>the current user is added to the attention set (if they are not in the attention set yet)
+ * or
+ * <li>the reason for the current user to be in the attention set is updated (if they are
+ * already in the attention set).
+ * </ul>
+ */
+ private void updateAttentionSetForCurrentUser(
+ BatchUpdate bu,
+ @Nullable PostReviewOp postReviewOp,
+ ChangeNotes changeNotes,
+ CurrentUser currentUser) {
+ if (postReviewOp == null) {
+ // Replying removes the current user from the attention set.
+ removeFromAttentionSet(
+ bu, changeNotes, currentUser.getAccountId(), "removed on reply", false);
+ } else {
+ // If the current user voted on an outdated patch set and some of the votes were not copied to
+ // the current patch set the current user should stay in the attention set, or be added to the
+ // attention set. In case the user stays in the attention set, this updates the reason for
+ // being in the attention set.
+ AttentionSetUpdateCondition addOrKeepCondition =
+ () ->
+ postReviewOp
+ .getResult()
+ .appliedVotesOnOutdatedPatchSetThatWereNotCopiedToCurrentPatchSet();
+ maybeAddToAttentionSet(
+ bu,
+ addOrKeepCondition,
+ changeNotes,
+ currentUser.getAccountId(),
+ "Some votes were not copied to the current patch set",
+ false);
+
+ // Otherwise replying removes the current user from the attention set.
+ AttentionSetUpdateCondition removeCondition = () -> !addOrKeepCondition.check();
+ maybeRemoveFromAttentionSet(
+ bu, removeCondition, changeNotes, currentUser.getAccountId(), "removed on reply", false);
}
- if (!owner.equals(uploader) && !currentUser.getAccountId().equals(uploader)) {
- addToAttentionSet(bu, changeNotes, uploader, "Someone else replied on the change", false);
+ }
+
+ /**
+ * Adds the owner and uploader to the attention set if someone else replied.
+ *
+ * <p>Replying means they either updated the votes on the current patch set (either directly on
+ * the current patch set or the votes were copied to the current patch set), they posted a change
+ * message, they marked the change as ready or they posted new comments.
+ */
+ private void addOwnerAndUploaderToAttentionSetIfSomeoneElseReplied(
+ BatchUpdate bu,
+ @Nullable PostReviewOp postReviewOp,
+ ChangeNotes changeNotes,
+ CurrentUser currentUser,
+ boolean readyForReview,
+ ImmutableSet<HumanComment> allNewComments) {
+ AttentionSetUpdateCondition condition =
+ postReviewOp != null
+ ? () ->
+ postReviewOp.getResult().updatedAnyVoteOnCurrentPatchSet()
+ || postReviewOp.getResult().postedChangeMessage()
+ || (changeNotes.getChange().isWorkInProgress() && readyForReview)
+ || !allNewComments.isEmpty()
+ : () ->
+ (changeNotes.getChange().isWorkInProgress() && readyForReview)
+ || !allNewComments.isEmpty();
+
+ Account.Id owner = changeNotes.getChange().getOwner();
+ if (!currentUser.getAccountId().equals(owner)) {
+ maybeAddToAttentionSet(
+ bu, condition, changeNotes, owner, "Someone else replied on the change", false);
}
- addAllAuthorsOfCommentThreads(bu, changeNotes, allNewComments);
+ Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
+ if (!owner.equals(uploader) && !currentUser.getAccountId().equals(uploader)) {
+ maybeAddToAttentionSet(
+ bu, condition, changeNotes, uploader, "Someone else replied on the change", false);
+ }
}
/** Adds all authors of all comment threads that received a reply during this update */
@@ -267,20 +372,26 @@
/**
* Bots don't process automatic rules, the only attention set change they do is this rule: Add
- * owner and uploader when a bot votes negatively, but only if the change is open.
+ * owner and uploader when a bot votes negatively on the current patch set, but only if the change
+ * is open.
*/
private void botsWithNegativeLabelsAddOwnerAndUploader(
- BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input) {
+ BatchUpdate bu, PostReviewOp postReviewOp, ChangeNotes changeNotes) {
if (changeNotes.getChange().isClosed()) {
return;
}
- if (input.labels != null && input.labels.values().stream().anyMatch(vote -> vote < 0)) {
- Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
- Account.Id owner = changeNotes.getChange().getOwner();
- addToAttentionSet(bu, changeNotes, owner, "A robot voted negatively on a label", false);
- if (!owner.equals(uploader)) {
- addToAttentionSet(bu, changeNotes, uploader, "A robot voted negatively on a label", false);
- }
+
+ AttentionSetUpdateCondition condition =
+ () -> postReviewOp.getResult().updatedAnyNegativeVoteOnCurrentPatchSet();
+
+ Account.Id owner = changeNotes.getChange().getOwner();
+ maybeAddToAttentionSet(
+ bu, condition, changeNotes, owner, "A robot voted negatively on a label", false);
+
+ Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
+ if (!owner.equals(uploader)) {
+ maybeAddToAttentionSet(
+ bu, condition, changeNotes, uploader, "A robot voted negatively on a label", false);
}
}
@@ -300,6 +411,28 @@
}
/**
+ * Adds the user to the attention set if the given condition is true.
+ *
+ * @param bu BatchUpdate to perform the updates to the attention set
+ * @param condition condition that decides whether the attention set update should be performed
+ * @param changeNotes current change
+ * @param user user to add to the attention set
+ * @param reason reason for adding
+ * @param notify whether or not to notify about this addition
+ */
+ private void maybeAddToAttentionSet(
+ BatchUpdate bu,
+ AttentionSetUpdateCondition condition,
+ ChangeNotes changeNotes,
+ Account.Id user,
+ String reason,
+ boolean notify) {
+ AddToAttentionSetOp addToAttentionSet =
+ addToAttentionSetOpFactory.create(user, reason, notify).setCondition(condition);
+ bu.addOp(changeNotes.getChangeId(), addToAttentionSet);
+ }
+
+ /**
* Removes the user from the attention set
*
* @param bu BatchUpdate to perform the updates to the attention set.
@@ -315,6 +448,28 @@
bu.addOp(changeNotes.getChangeId(), removeFromAttentionSetOp);
}
+ /**
+ * Removes the user from the attention set if the given condition is true.
+ *
+ * @param bu BatchUpdate to perform the updates to the attention set.
+ * @param condition condition that decides whether the attention set update should be performed
+ * @param changeNotes current change.
+ * @param user user to add remove from the attention set.
+ * @param reason reason for removing.
+ * @param notify whether or not to notify about this removal.
+ */
+ private void maybeRemoveFromAttentionSet(
+ BatchUpdate bu,
+ AttentionSetUpdateCondition condition,
+ ChangeNotes changeNotes,
+ Account.Id user,
+ String reason,
+ boolean notify) {
+ RemoveFromAttentionSetOp removeFromAttentionSetOp =
+ removeFromAttentionSetOpFactory.create(user, reason, notify).setCondition(condition);
+ bu.addOp(changeNotes.getChangeId(), removeFromAttentionSetOp);
+ }
+
private static boolean isReadyForReview(ChangeNotes changeNotes, ReviewInput input) {
return (!changeNotes.getChange().isWorkInProgress() && !input.workInProgress) || input.ready;
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 1edea05..c7672d1 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -48,6 +48,7 @@
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Patch;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.RefNames;
@@ -1030,14 +1031,13 @@
}
@Test
- public void repliesAddsOwner() throws Exception {
+ public void repliesAddsOwner_voteAdded() throws Exception {
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
+ change(r).current().review(ReviewInput.dislike());
- ReviewInput reviewInput = new ReviewInput();
- change(r).current().review(reviewInput);
-
+ // The change owner has been added to the attention set
AttentionSetUpdate attentionSet =
Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
assertThat(attentionSet).hasAccountIdThat().isEqualTo(admin.id());
@@ -1046,6 +1046,224 @@
}
@Test
+ public void repliesAddsOwner_voteCopied() throws Exception {
+ // Define a label with a copy condition that copies all votes.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder fooReview =
+ labelBuilder(
+ "Foo-Review",
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"))
+ .setCopyCondition("is:ANY");
+ u.getConfig().upsertLabelType(fooReview.build());
+ u.save();
+ }
+
+ // Allow voting on the label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("Foo-Review")
+ .ref(RefNames.REFS_HEADS + "*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Create a change with 2 patch sets.
+ PushOneCommit.Result r = createChange();
+ PatchSet patchSet1 = r.getChange().currentPatchSet();
+ r = amendChange(r.getChangeId());
+ r.assertOkStatus();
+
+ // Apply a negative vote on the first patch set which is copied since the label has a copy
+ // condition that copies all votes
+ requestScopeOperations.setApiUser(user.id());
+ change(r).revision(patchSet1.number()).review(new ReviewInput().label("Foo-Review", -1));
+
+ // The change owner has been added to the attention set
+ 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 repliesDoesntAddOwner_voteNotCopied_userNotRemovedReasonUpdated() throws Exception {
+ // Define a label without any copy condition.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder fooReview =
+ labelBuilder(
+ "Foo-Review",
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"));
+ u.getConfig().upsertLabelType(fooReview.build());
+ u.save();
+ }
+
+ // Allow voting on the label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("Foo-Review")
+ .ref(RefNames.REFS_HEADS + "*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Create a change with 2 patch sets.
+ PushOneCommit.Result r = createChange();
+ PatchSet patchSet1 = r.getChange().currentPatchSet();
+ r = amendChange(r.getChangeId());
+ r.assertOkStatus();
+
+ // Add user as a reviewer so that the user is in the attention set
+ change(r).addReviewer(user.email());
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isNotEmpty();
+
+ // Apply a negative vote on the first patch set which is not copied since the label doesn't have
+ // a copy condition
+ requestScopeOperations.setApiUser(user.id());
+ change(r).revision(patchSet1.number()).review(new ReviewInput().label("Foo-Review", -1));
+
+ // The change owner has not been added to the attention set
+ assertThat(getAttentionSetUpdatesForUser(r, admin)).isEmpty();
+
+ // The reason for the user to be in the attention set has been updated
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user));
+ assertThat(attentionSet).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet)
+ .hasReasonThat()
+ .isEqualTo("Some votes were not copied to the current patch set");
+ }
+
+ @Test
+ public void repliesDoesntAddOwner_voteNotCopied_userAdded() throws Exception {
+ // Define a label without any copy condition.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder fooReview =
+ labelBuilder(
+ "Foo-Review",
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"));
+ u.getConfig().upsertLabelType(fooReview.build());
+ u.save();
+ }
+
+ // Allow voting on the label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("Foo-Review")
+ .ref(RefNames.REFS_HEADS + "*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Create a change with 2 patch sets.
+ PushOneCommit.Result r = createChange();
+ PatchSet patchSet1 = r.getChange().currentPatchSet();
+ r = amendChange(r.getChangeId());
+ r.assertOkStatus();
+
+ // User is not in the attention set yet
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+
+ // Apply a negative vote on the first patch set which is not copied since the label doesn't have
+ // a copy condition
+ requestScopeOperations.setApiUser(user.id());
+ change(r).revision(patchSet1.number()).review(new ReviewInput().label("Foo-Review", -1));
+
+ // The change owner has not been added to the attention set
+ assertThat(getAttentionSetUpdatesForUser(r, admin)).isEmpty();
+
+ // The user has been added to the attention set because the vote was not copied
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user));
+ assertThat(attentionSet).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet)
+ .hasReasonThat()
+ .isEqualTo("Some votes were not copied to the current patch set");
+ }
+
+ @Test
+ public void repliesAddsOwner_changeMessagePosted() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message = "A message";
+ change(r).current().review(reviewInput);
+
+ // The change owner has been added to the attention set
+ 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 repliesAddsOwner_commentAdded() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput reviewInput = new ReviewInput();
+ ReviewInput.CommentInput comment = new ReviewInput.CommentInput();
+ comment.side = Side.REVISION;
+ comment.path = Patch.COMMIT_MSG;
+ comment.message = "comment";
+ reviewInput.comments = ImmutableMap.of(Patch.COMMIT_MSG, ImmutableList.of(comment));
+ change(r).current().review(reviewInput);
+
+ // The change owner has been added to the attention set
+ 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 repliesAddsOwner_markedAsReady() throws Exception {
+ PushOneCommit.Result r = createChange();
+ change(r).setWorkInProgress();
+
+ requestScopeOperations.setApiUser(accountCreator.admin2().id());
+ ReviewInput reviewInput = ReviewInput.create().setReady(true);
+ change(r).current().review(reviewInput);
+
+ // The change owner has been added to the attention set
+ 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 noOpRepliesDontAddOwner() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ // post a no-op reply (a reply that does neither add any vote, change message, comment nor
+ // changes the change to ready)
+ requestScopeOperations.setApiUser(user.id());
+ change(r).current().review(new ReviewInput());
+
+ // The change owner has not been added to the attention set
+ assertThat(getAttentionSetUpdatesForUser(r, admin)).isEmpty();
+ }
+
+ @Test
public void robotRepliesDoNotAddToAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addReviewer(user.email());
@@ -1119,18 +1337,16 @@
@Test
public void repliesAddsOwnerAndUploader() throws Exception {
- // Create change with owner: admin
+ // Create change with admin as the owner and user as the uploader
PushOneCommit.Result r = createChange();
r = amendChangeWithUploader(r, project, user);
+ change(r).attention(user.email()).remove(new AttentionSetInput("reason"));
+ // Post a comment by another user
TestAccount user2 = accountCreator.user2();
requestScopeOperations.setApiUser(user2.id());
-
- change(r).attention(user.email()).remove(new AttentionSetInput("reason"));
ReviewInput reviewInput = new ReviewInput();
- change(r).current().review(reviewInput);
-
- reviewInput = new ReviewInput();
+ reviewInput.message = "A message";
change(r).current().review(reviewInput);
// Uploader added
@@ -1148,6 +1364,25 @@
}
@Test
+ public void noOpRepliesDontAddOwnerAndUploader() throws Exception {
+ // Create change with admin as the owner and user as the uploader
+ PushOneCommit.Result r = createChange();
+ r = amendChangeWithUploader(r, project, user);
+ change(r).attention(user.email()).remove(new AttentionSetInput("reason"));
+
+ // Post a no-op reply by another user (a reply that does neither add any vote, change message,
+ // comment nor changes the change to ready)
+ TestAccount user2 = accountCreator.user2();
+ requestScopeOperations.setApiUser(user2.id());
+ ReviewInput reviewInput = new ReviewInput();
+ change(r).current().review(reviewInput);
+
+ // Neither the change owner nor the uploader have been added to the attention set
+ assertThat(getAttentionSetUpdatesForUser(r, admin)).isEmpty();
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+ }
+
+ @Test
public void reviewIgnoresRobotCommentsForAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
@@ -1382,7 +1617,9 @@
requestScopeOperations.setApiUser(user.id());
- change(r).current().review(new ReviewInput());
+ reviewInput = new ReviewInput();
+ reviewInput.message = "A message";
+ change(r).current().review(reviewInput);
// Reviewer and CC not added since the uploader didn't reply to their comments
assertThat(getAttentionSetUpdatesForUser(r, cc)).isEmpty();
@@ -1573,6 +1810,99 @@
}
@Test
+ public void robotReviewWithNegativeLabelOnOutdatedPatchSetAddsOwnerIfVoteWasCopied()
+ throws Exception {
+ // Define a label with a copy condition that copies all votes.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder fooReview =
+ labelBuilder(
+ "Foo-Review",
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"))
+ .setCopyCondition("is:ANY");
+ u.getConfig().upsertLabelType(fooReview.build());
+ u.save();
+ }
+
+ // Allow voting on the label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("Foo-Review")
+ .ref(RefNames.REFS_HEADS + "*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Create a change with 2 patch sets.
+ PushOneCommit.Result r = createChange();
+ PatchSet patchSet1 = r.getChange().currentPatchSet();
+ r = amendChange(r.getChangeId());
+ r.assertOkStatus();
+
+ // Create a service user and apply a negative vote on the first patch set which is copied since
+ // the label has a copy condition that copies all votes
+ TestAccount robot =
+ accountCreator.create(
+ "robot2", "robot2@example.com", "Ro Bot", "Ro", ServiceUserClassifier.SERVICE_USERS);
+ requestScopeOperations.setApiUser(robot.id());
+ change(r).revision(patchSet1.number()).review(new ReviewInput().label("Foo-Review", -1));
+
+ // The change owner has been added to the attention set
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasAccountIdThat().isEqualTo(admin.id());
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("A robot voted negatively on a label");
+ }
+
+ @Test
+ public void robotReviewWithNegativeLabelOnOutdatedPatchSetDoesntAddOwnerIfVoteWasNotCopied()
+ throws Exception {
+ // Define a label without any copy condition.
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder fooReview =
+ labelBuilder(
+ "Foo-Review",
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"));
+ u.getConfig().upsertLabelType(fooReview.build());
+ u.save();
+ }
+
+ // Allow voting on the label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("Foo-Review")
+ .ref(RefNames.REFS_HEADS + "*")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Create a change with 2 patch sets.
+ PushOneCommit.Result r = createChange();
+ PatchSet patchSet1 = r.getChange().currentPatchSet();
+ r = amendChange(r.getChangeId());
+ r.assertOkStatus();
+
+ // Create a service user and apply a negative vote on the first patch set which is not copied
+ // since the label doesn't have a copy condition
+ TestAccount robot =
+ accountCreator.create(
+ "robot2", "robot2@example.com", "Ro Bot", "Ro", ServiceUserClassifier.SERVICE_USERS);
+ requestScopeOperations.setApiUser(robot.id());
+ change(r).revision(patchSet1.number()).review(new ReviewInput().label("Foo-Review", -1));
+
+ // The change owner has not been added to the attention set
+ assertThat(getAttentionSetUpdatesForUser(r, admin)).isEmpty();
+ }
+
+ @Test
public void robotReviewWithNegativeLabelDoesntAddOwnerIfChangeIsMerged() throws Exception {
TestAccount robot =
accountCreator.create(
