1 files changed, 34 insertions, 21 deletions
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index 653481048a..c5c864db82 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -25,19 +25,16 @@ import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.Sandboxed;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.common.data.GlobalCapability;
-import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupDescriptions;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.common.GroupInfo;
 import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.extensions.restapi.Url;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.group.CreateGroup;
-import com.google.gerrit.server.group.GroupsCollection;
+import com.google.gerrit.server.group.InternalGroup;
 import com.google.inject.Inject;
 import java.util.Arrays;
 import java.util.List;
@@ -48,11 +45,9 @@ import org.junit.Test;
 public class SuggestReviewersIT extends AbstractDaemonTest {
   @Inject private CreateGroup.Factory createGroupFactory;
 
-  @Inject private GroupsCollection groups;
-
-  private AccountGroup group1;
-  private AccountGroup group2;
-  private AccountGroup group3;
+  private InternalGroup group1;
+  private InternalGroup group2;
+  private InternalGroup group3;
 
   private TestAccount user1;
   private TestAccount user2;
@@ -144,8 +139,8 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     List<SuggestedReviewerInfo> reviewers;
 
     setApiUser(user3);
-    block("read", ANONYMOUS_USERS, "refs/*");
-    allow("read", group1.getGroupUUID(), "refs/*");
+    block("refs/*", "read", ANONYMOUS_USERS);
+    allow("refs/*", "read", group1.getGroupUUID());
     reviewers = suggestReviewers(changeId, user2.username, 2);
     assertThat(reviewers).isEmpty();
   }
@@ -240,8 +235,8 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
   @GerritConfig(name = "addreviewer.maxAllowed", value = "2")
   @GerritConfig(name = "addreviewer.maxWithoutConfirmation", value = "1")
   public void suggestReviewersGroupSizeConsiderations() throws Exception {
-    AccountGroup largeGroup = group("large");
-    AccountGroup mediumGroup = group("medium");
+    InternalGroup largeGroup = group("large");
+    InternalGroup mediumGroup = group("medium");
 
     // Both groups have Administrator as a member. Add two users to large
     // group to push it past maxAllowed, and one to medium group to push it
@@ -402,6 +397,24 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
         .inOrder();
   }
 
+  @Test
+  public void suggestNoInactiveAccounts() throws Exception {
+    String name = name("foo");
+    TestAccount foo1 = accountCreator.create(name + "-1");
+    assertThat(gApi.accounts().id(foo1.username).getActive()).isTrue();
+
+    TestAccount foo2 = accountCreator.create(name + "-2");
+    assertThat(gApi.accounts().id(foo2.username).getActive()).isTrue();
+
+    String changeId = createChange().getChangeId();
+    assertReviewers(
+        suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
+
+    gApi.accounts().id(foo2.username).setActive(false);
+    assertThat(gApi.accounts().id(foo2.username).getActive()).isFalse();
+    assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(foo1), ImmutableList.of());
+  }
+
   private List<SuggestedReviewerInfo> suggestReviewers(String changeId, String query)
       throws Exception {
     return gApi.changes().id(changeId).suggestReviewers(query).get();
@@ -412,19 +425,19 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
     return gApi.changes().id(changeId).suggestReviewers(query).withLimit(n).get();
   }
 
-  private AccountGroup group(String name) throws Exception {
+  private InternalGroup group(String name) throws Exception {
     GroupInfo group = createGroupFactory.create(name(name)).apply(TopLevelResource.INSTANCE, null);
-    GroupDescription.Basic d = groups.parseInternal(Url.decode(group.id));
-    return GroupDescriptions.toAccountGroup(d);
+    return groupCache.get(new AccountGroup.UUID(group.id)).orElse(null);
   }
 
-  private TestAccount user(String name, String fullName, String emailName, AccountGroup... groups)
+  private TestAccount user(String name, String fullName, String emailName, InternalGroup... groups)
       throws Exception {
-    String[] groupNames = Arrays.stream(groups).map(AccountGroup::getName).toArray(String[]::new);
-    return accounts.create(name(name), name(emailName) + "@example.com", fullName, groupNames);
+    String[] groupNames = Arrays.stream(groups).map(InternalGroup::getName).toArray(String[]::new);
+    return accountCreator.create(
+        name(name), name(emailName) + "@example.com", fullName, groupNames);
   }
 
-  private TestAccount user(String name, String fullName, AccountGroup... groups) throws Exception {
+  private TestAccount user(String name, String fullName, InternalGroup... groups) throws Exception {
     return user(name, fullName, name, groups);
   }
 
@@ -449,7 +462,7 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
   private void assertReviewers(
       List<SuggestedReviewerInfo> actual,
       List<TestAccount> expectedUsers,
-      List<AccountGroup> expectedGroups) {
+      List<InternalGroup> expectedGroups) {
     List<Integer> actualAccountIds =
         actual.stream()
             .filter(i -> i.account != null)