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);
   }