opendev/gerrit
Code Issues Proposed changes

Add reviewers to the attention set if marking a change ready for review

Browse Source 
Another useful default for adding users to the attention set is about
marking a change ready for review. This change ensures that all
reviewers are added to the attention set when a change exits "work in
progress" state and becomes "ready for review".

Via the "review" endpoint, it is currently possible to add or remove
reviewers while marking a change ready for review. In this case, we
ensure that those new reviewers are handled correctly: New reviewers
that are added while marking a change ready for review will also be
added to the attention set, while reviewers that are removed while
marking the change ready for review will not be added.

Change-Id: I2442f167bee31040bab85c19116f6308a0873694
This commit is contained in:
Gal Paikin
2020-05-28 12:12:45 +02:00
parent fb6642911c
commit 8bf2a18575
4 changed files with 96 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/change/AddToAttentionSetOp.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.change;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.AttentionSetUpdate.Operation;
@@ -81,9 +80,8 @@ public class AddToAttentionSetOp implements BatchUpdateOp {
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
update.addToPlannedAttentionSetUpdates(
ImmutableSet.of(
AttentionSetUpdate.createForWrite(
attentionUserId, AttentionSetUpdate.Operation.ADD, reason)));
attentionUserId, AttentionSetUpdate.Operation.ADD, reason));
if (withChangeMessage) {
addChangeMessage(ctx, update);
}

4
java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.change;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.AttentionSetUpdate.Operation;
@@ -81,8 +80,7 @@ public class RemoveFromAttentionSetOp implements BatchUpdateOp {
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
update.addToPlannedAttentionSetUpdates(
ImmutableSet.of(
AttentionSetUpdate.createForWrite(attentionUserId, Operation.REMOVE, reason)));
AttentionSetUpdate.createForWrite(attentionUserId, Operation.REMOVE, reason));
if (withChangeMessage) {
addChangeMessage(ctx, update);
}

26
java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -50,6 +50,7 @@ import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Table;
import com.google.common.collect.TreeBasedTable;
import com.google.gerrit.common.data.SubmitRecord;
@@ -406,6 +407,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
.forEach(u -> plannedAttentionSetUpdates.putIfAbsent(u.account(), u));
}
public void addToPlannedAttentionSetUpdates(AttentionSetUpdate update) {
addToPlannedAttentionSetUpdates(ImmutableSet.of(update));
}
public void setAssignee(Account.Id assignee) {
checkArgument(assignee != null, "use removeAssignee");
this.assignee = Optional.of(assignee);
@@ -647,7 +652,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addIdent(msg, e.getKey()).append('\n');
}
applyReviewerUpdatesToAttentionSet();
for (Map.Entry<Address, ReviewerStateInternal> e : reviewersByEmail.entrySet()) {
addFooter(msg, e.getValue().getByEmailFooterKey(), e.getKey().toString());
}
@@ -710,6 +714,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, FOOTER_WORK_IN_PROGRESS, workInProgress);
if (workInProgress) {
clearAttentionSet("Change was marked work in progress");
} else {
addAllReviewersToAttentionSet();
}
}
@@ -742,23 +748,20 @@ public class ChangeUpdate extends AbstractChangeUpdate {
if (getNotes().getAttentionSet() == null) {
return;
}
Set<AttentionSetUpdate> removeAll =
AttentionSetUtil.additionsOnly(getNotes().getAttentionSet()).stream()
.map(
a ->
AttentionSetUpdate.createForWrite(
a.account(), AttentionSetUpdate.Operation.REMOVE, reason))
.collect(Collectors.toSet());
addToPlannedAttentionSetUpdates(removeAll);
.forEach(a -> addToPlannedAttentionSetUpdates(a));
}
private void applyReviewerUpdatesToAttentionSet() {
if (workInProgress != null || getNotes().getChange().isWorkInProgress()) {
if ((workInProgress != null && workInProgress == true)
|| getNotes().getChange().isWorkInProgress()) {
// Users shouldn't be added to the attention set if the change is work in progress.
return;
}
Set<Account.Id> currentReviewers =
getNotes().getReviewers().byState(ReviewerStateInternal.REVIEWER);
Set<AttentionSetUpdate> updates = new HashSet();
@@ -781,6 +784,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addToPlannedAttentionSetUpdates(updates);
}
private void addAllReviewersToAttentionSet() {
getNotes().getReviewers().byState(ReviewerStateInternal.REVIEWER).stream()
.map(
r ->
AttentionSetUpdate.createForWrite(
r, AttentionSetUpdate.Operation.ADD, "Change was marked ready for review"))
.forEach(a -> addToPlannedAttentionSetUpdates(a));
}
/**
* Any updates to the attention set must be done in {@link #addToPlannedAttentionSetUpdates}. This
* method is called after all the updates are finished to do the updates once and for real.

71
javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.UnmodifiableIterator;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -26,6 +27,7 @@ import com.google.gerrit.acceptance.UseClockStep;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.AddToAttentionSetInput;
import com.google.gerrit.extensions.api.changes.HashtagsInput;
import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ReviewerState;
@@ -279,12 +281,11 @@ public class AttentionSetIT extends AbstractDaemonTest {
@Test
public void addingReviewerWhileMarkingWorkInprogressDoesntAddToAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
ReviewInput reviewInput = new ReviewInput();
ReviewInput reviewInput = new ReviewInput().setWorkInProgress(true);
AddReviewerInput addReviewerInput = new AddReviewerInput();
addReviewerInput.state = ReviewerState.REVIEWER;
addReviewerInput.reviewer = user.email();
reviewInput.reviewers = ImmutableList.of(addReviewerInput);
reviewInput.workInProgress = true;
change(r).current().review(reviewInput);
@@ -337,4 +338,70 @@ public class AttentionSetIT extends AbstractDaemonTest {
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("Reviewer was removed");
}
@Test
public void readyForReviewAddsAllReviewersToAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
change(r).setWorkInProgress();
change(r).addReviewer(user.email());
change(r).setReadyForReview();
AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
assertThat(attentionSet.reason()).isEqualTo("Change was marked ready for review");
}
@Test
public void readyForReviewWhileRemovingReviewerDoesntAddThemToAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
change(r).setWorkInProgress();
change(r).addReviewer(user.email());
ReviewInput reviewInput = new ReviewInput().setReady(true);
AddReviewerInput addReviewerInput = new AddReviewerInput();
addReviewerInput.state = ReviewerState.CC;
addReviewerInput.reviewer = user.email();
reviewInput.reviewers = ImmutableList.of(addReviewerInput);
change(r).current().review(reviewInput);
AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("Reviewer was removed");
}
@Test
public void readyForReviewWhileAddingReviewerAddsThemToAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
change(r).setWorkInProgress();
ReviewInput reviewInput = new ReviewInput().setReady(true);
AddReviewerInput addReviewerInput = new AddReviewerInput();
addReviewerInput.state = ReviewerState.REVIEWER;
addReviewerInput.reviewer = user.email();
reviewInput.reviewers = ImmutableList.of(addReviewerInput);
change(r).current().review(reviewInput);
AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
assertThat(attentionSet.reason()).isEqualTo("Reviewer was added");
}
@Test
public void reviewersAreNotAddedForNoReasonBecauseOfAnUpdate() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addToAttentionSet(new AddToAttentionSetInput(user.email(), "user"));
change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("removed"));
HashtagsInput hashtagsInput = new HashtagsInput();
hashtagsInput.add = ImmutableSet.of("tag");
change(r).setHashtags(hashtagsInput);
AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("removed");
}
}