opendev/gerrit
Code Issues Proposed changes

Clear from attention set when abandoning, submitting, or marking WIP

Browse Source 
Currently, we don't add users to the attention set when some events
occur, e.g adding or removing reviewers, replying on comments, etc.

However, it makes sense that some events cause the change to no longer
require attention from anyone. In those cases, we should remove all
users from the attention set.

Those cases are: closing a change, and marking work in progress.

Change-Id: I645219c1a928f34e5eace688806e39e748cfcfae
This commit is contained in:
Gal Paikin
2020-05-25 15:24:09 +02:00
parent 9be1b62bd1
commit cecfa724aa
2 changed files with 89 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -65,6 +65,7 @@ import com.google.gerrit.mail.Address;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.AttentionSetUtil;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.assistedinject.Assisted;
@@ -80,6 +81,7 @@ import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
@@ -583,6 +585,12 @@ public class ChangeUpdate extends AbstractChangeUpdate {
if (status != null) {
addFooter(msg, FOOTER_STATUS, status.name().toLowerCase());
if (status.equals(Change.Status.ABANDONED)) {
clearAttentionSet(msg, "Change was abandoned");
}
if (status.equals(Change.Status.MERGED)) {
clearAttentionSet(msg, "Change was submitted");
}
}
if (topic != null) {
@@ -686,6 +694,9 @@ public class ChangeUpdate extends AbstractChangeUpdate {
if (workInProgress != null) {
addFooter(msg, FOOTER_WORK_IN_PROGRESS, workInProgress);
if (workInProgress) {
clearAttentionSet(msg, "Change was marked work in progress");
}
}
if (revertOf != null) {
@@ -709,6 +720,23 @@ public class ChangeUpdate extends AbstractChangeUpdate {
return cb;
}
private void clearAttentionSet(StringBuilder msg, String reason) {
if (getNotes().getAttentionSet() == null) {
return;
}
Set<AttentionSetUpdate> toClear =
AttentionSetUtil.additionsOnly(getNotes().getAttentionSet()).stream()
.map(
a ->
AttentionSetUpdate.createForWrite(
a.account(), AttentionSetUpdate.Operation.REMOVE, reason))
.collect(Collectors.toSet());
for (AttentionSetUpdate attentionSetUpdate : toClear) {
addFooter(msg, FOOTER_ATTENTION, noteUtil.attentionSetUpdateToJson(attentionSetUpdate));
}
}
private void addPatchSetFooter(StringBuilder sb, int ps) {
addFooter(sb, FOOTER_PATCH_SET).append(ps);
if (psState != null) {

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

@@ -16,6 +16,8 @@ package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.Iterables;
import com.google.common.collect.UnmodifiableIterator;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -23,6 +25,7 @@ import com.google.gerrit.acceptance.UseClockStep;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.extensions.api.changes.AddToAttentionSetInput;
import com.google.gerrit.extensions.api.changes.RemoveFromAttentionSetInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.server.util.time.TimeUtil;
import java.time.Duration;
import java.time.Instant;
@@ -133,4 +136,62 @@ public class AttentionSetIT extends AbstractDaemonTest {
change(r).attention(user.id().toString()).remove(new RemoveFromAttentionSetInput("foo"));
assertThat(r.getChange().attentionSet()).isEmpty();
}
@Test
public void abandonRemovesUsers() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addToAttentionSet(new AddToAttentionSetInput(user.email(), "user"));
change(r).addToAttentionSet(new AddToAttentionSetInput(admin.email(), "admin"));
change(r).abandon();
UnmodifiableIterator<AttentionSetUpdate> attentionSets =
r.getChange().attentionSet().iterator();
AttentionSetUpdate userUpdate = attentionSets.next();
assertThat(userUpdate.account()).isEqualTo(user.id());
assertThat(userUpdate.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(userUpdate.reason()).isEqualTo("Change was abandoned");
AttentionSetUpdate adminUpdate = attentionSets.next();
assertThat(adminUpdate.account()).isEqualTo(admin.id());
assertThat(adminUpdate.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(adminUpdate.reason()).isEqualTo("Change was abandoned");
}
@Test
public void workInProgressRemovesUsers() throws Exception {
PushOneCommit.Result r = createChange();
change(r).addToAttentionSet(new AddToAttentionSetInput(user.email(), "reason"));
change(r).setWorkInProgress();
AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("Change was marked work in progress");
}
@Test
public void submitRemovesUsersForAllSubmittedChanges() throws Exception {
PushOneCommit.Result r1 = createChange("refs/heads/master", "file1", "content");
change(r1).addToAttentionSet(new AddToAttentionSetInput(user.email(), "reason"));
change(r1).current().review(ReviewInput.approve());
PushOneCommit.Result r2 = createChange("refs/heads/master", "file2", "content");
change(r2).addToAttentionSet(new AddToAttentionSetInput(user.email(), "reason"));
change(r2).current().review(ReviewInput.approve());
change(r2).current().submit();
AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r1.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("Change was submitted");
attentionSet = Iterables.getOnlyElement(r2.getChange().attentionSet());
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("Change was submitted");
}
}