opendev/gerrit
Code Issues Proposed changes

Ensure reviewers are are not added to attention set on submit

Browse Source 
Currently, there is a bug that adds the person who submitted the change
as a reviewer, and in turn to the attention set if they are not a
reviewer.

This change ensures nobody is added on submit by becoming a reviewer.

Change-Id: If708dc98f4b195976ccd9cd1862810c215b21244
This commit is contained in:
Gal Paikin
2020-07-01 18:23:53 +03:00
parent 098bcf24a1
commit 2806e4ccee
2 changed files with 50 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -659,7 +659,9 @@ public class ChangeUpdate extends AbstractChangeUpdate {
addFooter(msg, e.getValue().getFooterKey());
noteUtil.appendAccountIdIdentString(msg, e.getKey()).append('\n');
}
applyReviewerUpdatesToAttentionSet();
for (Map.Entry<Address, ReviewerStateInternal> e : reviewersByEmail.entrySet()) {
addFooter(msg, e.getValue().getByEmailFooterKey(), e.getKey().toString());
}
@@ -766,8 +768,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private void applyReviewerUpdatesToAttentionSet() {
if ((workInProgress != null && workInProgress == true)
|| getNotes().getChange().isWorkInProgress()) {
// Users shouldn't be added to the attention set if the change is work in progress.
|| getNotes().getChange().isWorkInProgress()
|| status == Change.Status.MERGED) {
// Attention set shouldn't change here for changes that are work in progress or are about to
// be submitted.
return;
}
Set<Account.Id> currentReviewers =

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

@@ -240,6 +240,50 @@ public class AttentionSetIT extends AbstractDaemonTest {
assertThat(attentionSet.reason()).isEqualTo("Change was submitted");
}
/**
* There is currently a bug that adds the person who submitted the change as reviewer, which in
* turn adds them to the attention set. This test ensures this doesn't happen.
*/
@Test
public void submitDoesNotAddReviewersToAttentionSet() throws Exception {
PushOneCommit.Result r = createChange("refs/heads/master", "file1", "content");
// Someone else approves, because if admin reviews, they will be added to the reviewers (and the
// bug won't be reproduced).
requestScopeOperations.setApiUser(accountCreator.admin2().id());
change(r).current().review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
requestScopeOperations.setApiUser(admin.id());
change(r).attention(admin.email()).remove(new AttentionSetInput("remove"));
change(r).current().submit();
// Attention set updates that relate to the admin (the person who replied) are filtered out.
AttentionSetUpdate attentionSet =
Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
assertThat(attentionSet.account()).isEqualTo(admin.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
assertThat(attentionSet.reason()).isEqualTo("remove");
change(r).addReviewer(user.email());
}
@Test
public void addedReviewersAreAddedToAttentionSetOnMergedChanges() throws Exception {
PushOneCommit.Result r = createChange();
change(r).current().review(ReviewInput.approve());
change(r).current().submit();
change(r).addReviewer(user.email());
AttentionSetUpdate attentionSet =
Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user));
assertThat(attentionSet.account()).isEqualTo(user.id());
assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
assertThat(attentionSet.reason()).isEqualTo("Reviewer was added");
}
@Test
public void reviewersAddedAndRemovedFromAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();