Don't change manually altered reviewer state
If users manually changes their reviewer-state, that state should not
be altered by automatically triggered reviewer-additions.
Change-Id: I0e6abc0bbde7b8e0b21ab79a5a81c1c71340f1d1
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/AddReviewers.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/AddReviewers.java
index 90f5841..43db627 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/AddReviewers.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/AddReviewers.java
@@ -28,6 +28,7 @@
import com.google.inject.assistedinject.Assisted;
import java.util.ArrayList;
import java.util.Set;
+import java.util.stream.Collectors;
/** Adds reviewers to a change. */
class AddReviewers implements Runnable {
@@ -72,14 +73,30 @@
try {
// TODO(davido): Switch back to using changes API again,
// when it supports batch mode for adding reviewers
+ Set<Account.Id> existingReviewers =
+ gApi.changes().id(changeInfo._number).reviewers().stream()
+ .map(r -> Account.id(r._accountId))
+ .collect(Collectors.toSet());
+ /* Don't add, or change state of, already existing reviewers. */
+ Set<Account.Id> reviewersToAdd =
+ reviewers.stream()
+ .filter(r -> !existingReviewers.contains(r))
+ .collect(Collectors.toSet());
+ /* If account is already configured to be added as reviewer, don't attempt to add as cc. */
+ Set<Account.Id> ccsToAdd =
+ ccs.stream()
+ .filter(c -> !existingReviewers.contains(c))
+ .filter(r -> !reviewersToAdd.contains(r))
+ .collect(Collectors.toSet());
+
ReviewInput in = new ReviewInput();
in.reviewers = new ArrayList<>(reviewers.size() + ccs.size());
- for (Account.Id account : reviewers) {
+ for (Account.Id account : reviewersToAdd) {
AddReviewerInput addReviewerInput = new AddReviewerInput();
addReviewerInput.reviewer = account.toString();
in.reviewers.add(addReviewerInput);
}
- for (Account.Id account : ccs) {
+ for (Account.Id account : ccsToAdd) {
AddReviewerInput input = new AddReviewerInput();
input.state = ReviewerState.CC;
input.reviewer = account.toString();
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
index 42548a7..348e856 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
@@ -156,10 +156,12 @@
List<ReviewerFilter> matching = findMatchingFilters(changeNumber, filters);
Set<String> reviewers = getReviewersFrom(matching);
Set<String> ccs = getCcsFrom(matching);
- ccs.removeAll(reviewers);
if (reviewers.isEmpty() && ccs.isEmpty()) {
return;
}
+ /* Remove all reviewer identifiers (account-ids, group-ids) from ccs that are present in reviewers.
+ * Further filtering of individual accounts is done in AddReviewers after the ids have been resolved into Accounts. */
+ ccs.removeAll(reviewers);
final AddReviewers addReviewers =
addReviewersFactory.create(
c,
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
index 1cb511e..02f4902 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.ReviewerState;
@@ -125,6 +126,43 @@
assertThat(reviewersFor(changeId)).containsExactlyElementsIn(ImmutableSet.of(user.id()));
}
+ @Test
+ public void dontChangeExistingStateOfReviewer() throws Exception {
+ Set<Account.Id> userSet = ImmutableSet.of(user.id());
+ TestAccount user2 = accountCreator.user2();
+ Set<Account.Id> user2Set = ImmutableSet.of(user2.id());
+
+ createFilters(filter("*").reviewer(user2).cc(user));
+ String changeId = createChange("refs/for/master").getChangeId();
+ assertThat(ccsFor(changeId)).containsExactlyElementsIn(userSet);
+ assertThat(reviewersFor(changeId)).containsExactlyElementsIn(user2Set);
+
+ addReviewer(changeId, user, ReviewerState.REVIEWER);
+ addReviewer(changeId, user2, ReviewerState.CC);
+ assertThat(reviewersFor(changeId)).containsExactlyElementsIn(userSet);
+ assertThat(ccsFor(changeId)).containsExactlyElementsIn(user2Set);
+
+ amendChange(changeId);
+ assertThat(reviewersFor(changeId)).containsExactlyElementsIn(userSet);
+ assertThat(ccsFor(changeId)).containsExactlyElementsIn(user2Set);
+ }
+
+ @Test
+ public void addAsReviewerIfConfiguredAsReviewerAndCc() throws Exception {
+ createFilters(filter("*").cc(user), filter("branch:master").reviewer(user));
+ String changeId = createChange("refs/for/master").getChangeId();
+ assertThat(reviewersFor(changeId)).containsExactlyElementsIn(ImmutableSet.of(user.id()));
+ assertThat(gApi.changes().id(changeId).get().reviewers.get(CC)).isNull();
+ }
+
+ private void addReviewer(String changeId, TestAccount user, ReviewerState state)
+ throws Exception {
+ AddReviewerInput input = new AddReviewerInput();
+ input.reviewer = user.id().toString();
+ input.state = state;
+ gApi.changes().id(changeId).addReviewer(input);
+ }
+
private Set<Account.Id> ccsFor(String changeId) throws Exception {
return reviewersFor(changeId, CC);
}
