Merge "Migrate reviewers plugin to polymer 3"
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java
index bcf586e..60bcb88 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Module.java
@@ -77,7 +77,6 @@
             protected void configure() {
               get(PROJECT_KIND, "reviewers").to(GetReviewers.class);
               put(PROJECT_KIND, "reviewers").to(PutReviewers.class);
-              get(PROJECT_KIND, "suggest_reviewers").to(SuggestProjectReviewers.class);
             }
           });
     }
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 1aea95e..04e301b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
@@ -133,10 +133,11 @@
 
   private void onEvent(ChangeEvent event) {
     ChangeInfo c = event.getChange();
-    if (config.ignoreWip() && (c.workInProgress != null && c.workInProgress)) {
+    /* Never add reviewers automatically to private changes. */
+    if (Boolean.TRUE.equals(c.isPrivate)) {
       return;
     }
-    if (config.ignorePrivate() && (c.isPrivate != null && c.isPrivate)) {
+    if (config.ignoreWip() && Boolean.TRUE.equals(c.workInProgress)) {
       return;
     }
     Project.NameKey projectName = Project.nameKey(c.project);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfig.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfig.java
index 02f8872..96a4f6b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfig.java
@@ -45,7 +45,6 @@
   private static final String KEY_ENABLE_REST = "enableREST";
   private static final String KEY_SUGGEST_ONLY = "suggestOnly";
   private static final String KEY_IGNORE_WIP = "ignoreWip";
-  private static final String KEY_IGNORE_PRIVATE = "ignorePrivate";
 
   private final PluginConfigFactory cfgFactory;
   private final String pluginName;
@@ -53,7 +52,6 @@
   private final boolean enableREST;
   private final boolean suggestOnly;
   private final boolean ignoreWip;
-  private final boolean ignorePrivate;
 
   @Inject
   ReviewersConfig(PluginConfigFactory cfgFactory, @PluginName String pluginName) {
@@ -63,7 +61,6 @@
     this.enableREST = cfg.getBoolean(pluginName, null, KEY_ENABLE_REST, true);
     this.suggestOnly = cfg.getBoolean(pluginName, null, KEY_SUGGEST_ONLY, false);
     this.ignoreWip = cfg.getBoolean(pluginName, null, KEY_IGNORE_WIP, true);
-    this.ignorePrivate = cfg.getBoolean(pluginName, null, KEY_IGNORE_PRIVATE, true);
   }
 
   public ForProject forProject(Project.NameKey projectName) {
@@ -89,10 +86,6 @@
     return ignoreWip;
   }
 
-  public boolean ignorePrivate() {
-    return ignorePrivate;
-  }
-
   static class ForProject extends VersionedMetaData {
     private Config cfg;
 
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/SuggestProjectReviewers.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/SuggestProjectReviewers.java
deleted file mode 100644
index d73d5fb..0000000
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/SuggestProjectReviewers.java
+++ /dev/null
@@ -1,74 +0,0 @@
-// Copyright (C) 2016 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.googlesource.gerrit.plugins.reviewers;
-
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.client.ReviewerState;
-import com.google.gerrit.extensions.common.AccountVisibility;
-import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
-import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.Response;
-import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
-import com.google.gerrit.server.project.ProjectResource;
-import com.google.gerrit.server.restapi.change.ReviewersUtil;
-import com.google.gerrit.server.restapi.change.ReviewersUtil.VisibilityControl;
-import com.google.gerrit.server.restapi.change.SuggestReviewers;
-import com.google.inject.Inject;
-import java.io.IOException;
-import java.util.List;
-import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
-
-/** Implements a project suggest REST end-point. */
-public class SuggestProjectReviewers extends SuggestReviewers
-    implements RestReadView<ProjectResource> {
-  private final PermissionBackend permissionBackend;
-
-  @Inject
-  SuggestProjectReviewers(
-      AccountVisibility av,
-      @GerritServerConfig Config cfg,
-      ReviewersUtil reviewersUtil,
-      PermissionBackend permissionBackend) {
-    super(av, cfg, reviewersUtil);
-    this.permissionBackend = permissionBackend;
-  }
-
-  @Override
-  public Response<List<SuggestedReviewerInfo>> apply(ProjectResource rsrc)
-      throws BadRequestException, StorageException, IOException, ConfigInvalidException,
-          PermissionBackendException {
-    return Response.ok(
-        reviewersUtil.suggestReviewers(
-            ReviewerState.REVIEWER, null, this, rsrc.getProjectState(), getVisibility(rsrc), true));
-  }
-
-  private VisibilityControl getVisibility(final ProjectResource rsrc) {
-    return new VisibilityControl() {
-      @Override
-      public boolean isVisibleTo(Account.Id account) {
-        return permissionBackend
-            .absentUser(account)
-            .project(rsrc.getNameKey())
-            .testOrFalse(ProjectPermission.ACCESS);
-      }
-    };
-  }
-}
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md
index b678ff5..9de1838 100644
--- a/src/main/resources/Documentation/about.md
+++ b/src/main/resources/Documentation/about.md
@@ -3,6 +3,11 @@
 The configuration for adding reviewers to submitted changes can be
 [configured per project](config.md).
 
+__NOTE__:
+Private changes are ignored, which means that this plugin will never add reviewers
+to a change that is in `private` state. Reviewers will only be added to such a
+change if it transitions out of `private` state.
+
 SEE ALSO
 --------
 
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 0b94f0e..1088535 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -9,7 +9,6 @@
     enableREST = true
     suggestOnly = false
     ignoreWip = false
-    ignorePrivate = false
 ```
 
 reviewers.enableREST
@@ -29,10 +28,6 @@
 	considered when adding reviewers. Defaults to true. To enable adding
 	reviewers on changes in WIP state set this value to false.
 
-reviewers.ignorePrivate
-:	Ignore changes in private state. When set to true changes in private state
-	are not considered when adding reviewers. Defaults to true. To enable
-	adding reviewers on changes in Private state set this value to false.
 Per project configuration of the @PLUGIN@ plugin is done in the
 `reviewers.config` file of the project. Missing values are inherited
 from the parent projects. This means a global default configuration can
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java
new file mode 100644
index 0000000..383dcfd
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/AbstractReviewersPluginTest.java
@@ -0,0 +1,88 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.reviewers;
+
+import static com.google.gerrit.acceptance.GitUtil.fetch;
+import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.FILENAME;
+import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.KEY_REVIEWER;
+import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.SECTION_FILTER;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.RefNames;
+import com.google.inject.Inject;
+import java.util.Arrays;
+import java.util.List;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+
+/** Base class for reviewer plugin tests. */
+public class AbstractReviewersPluginTest extends LightweightPluginDaemonTest {
+  @Inject protected ProjectOperations projectOperations;
+
+  protected void createFilters(FilterData... filters) throws Exception {
+    createFiltersFor(testRepo, filters);
+  }
+
+  protected void createFiltersFor(TestRepository<?> repo, FilterData... filters) throws Exception {
+    String previousHead = repo.getRepository().getBranch();
+    checkoutRefsMetaConfig(repo);
+    Config cfg = new Config();
+    Arrays.stream(filters)
+        .forEach(f -> cfg.setStringList(SECTION_FILTER, f.filter, KEY_REVIEWER, f.reviewers));
+    pushFactory
+        .create(admin.newIdent(), repo, "Add reviewers", FILENAME, cfg.toText())
+        .to(RefNames.REFS_CONFIG)
+        .assertOkStatus();
+    repo.reset(previousHead);
+  }
+
+  protected TestRepository<?> checkoutRefsMetaConfig(TestRepository<?> repo) throws Exception {
+    fetch(repo, RefNames.REFS_CONFIG + ":refs/heads/config");
+    repo.reset("refs/heads/config");
+    return repo;
+  }
+
+  protected FilterData filter(String filter) {
+    return new FilterData(filter);
+  }
+
+  /** Assists tests to define a filter. */
+  protected static class FilterData {
+    List<String> reviewers;
+    String filter;
+
+    FilterData(String filter) {
+      this.filter = filter;
+      this.reviewers = Lists.newArrayList();
+    }
+
+    FilterData reviewer(TestAccount reviewer) {
+      return reviewer(reviewer.email());
+    }
+
+    FilterData reviewer(String reviewer) {
+      reviewers.add(reviewer);
+      return this;
+    }
+
+    public ReviewerFilterSection asSection() {
+      return new ReviewerFilterSection(filter, ImmutableSet.copyOf(reviewers));
+    }
+  }
+}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfigIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfigIT.java
index 1a280e2..19bb268 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfigIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersConfigIT.java
@@ -14,97 +14,60 @@
 
 package com.googlesource.gerrit.plugins.reviewers;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.GitUtil.fetch;
-import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.FILENAME;
-import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.KEY_REVIEWER;
-import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.SECTION_FILTER;
 
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
-import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.TestPlugin;
-import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.RefNames;
-import com.google.inject.Inject;
+import java.util.Arrays;
 import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
 import org.junit.Test;
 
 @NoHttpd
 @TestPlugin(name = "reviewers", sysModule = "com.googlesource.gerrit.plugins.reviewers.Module")
-public class ReviewersConfigIT extends LightweightPluginDaemonTest {
+public class ReviewersConfigIT extends AbstractReviewersPluginTest {
   private static final String BRANCH_MAIN = "branch:main";
   private static final String NO_FILTER = "*";
   private static final String JANE_DOE = "jane.doe@example.com";
   private static final String JOHN_DOE = "john.doe@example.com";
 
-  @Inject private ProjectOperations projectOperations;
-
   @Before
   public void setUp() throws Exception {
-    fetch(testRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
-    testRepo.reset("refs/heads/config");
+    checkoutRefsMetaConfig(testRepo);
   }
 
   @Test
   public void reviewersConfigSingle() throws Exception {
-    Config cfg = new Config();
-    cfg.setString(SECTION_FILTER, NO_FILTER, KEY_REVIEWER, JOHN_DOE);
-    cfg.setString(SECTION_FILTER, BRANCH_MAIN, KEY_REVIEWER, JANE_DOE);
+    createFilters(filter(NO_FILTER).reviewer(JOHN_DOE), filter(BRANCH_MAIN).reviewer(JANE_DOE));
 
-    pushFactory
-        .create(admin.newIdent(), testRepo, "Add reviewers", FILENAME, cfg.toText())
-        .to(RefNames.REFS_CONFIG)
-        .assertOkStatus();
-
-    assertThat(reviewersConfig().forProject(project).getReviewerFilterSections())
-        .containsExactlyElementsIn(
-            ImmutableList.of(
-                new ReviewerFilterSection(NO_FILTER, ImmutableSet.of(JOHN_DOE)),
-                new ReviewerFilterSection(BRANCH_MAIN, ImmutableSet.of(JANE_DOE))))
-        .inOrder();
+    assertProjectHasFilters(
+        project, filter(NO_FILTER).reviewer(JOHN_DOE), filter(BRANCH_MAIN).reviewer(JANE_DOE));
   }
 
   @Test
   public void reviewersConfigWithMergedInheritance() throws Exception {
-    Config parentCfg = new Config();
-    parentCfg.setString(SECTION_FILTER, NO_FILTER, KEY_REVIEWER, JOHN_DOE);
-    parentCfg.setString(SECTION_FILTER, BRANCH_MAIN, KEY_REVIEWER, JOHN_DOE);
-
-    pushFactory
-        .create(
-            admin.newIdent(),
-            testRepo,
-            "Add reviewers parent project",
-            FILENAME,
-            parentCfg.toText())
-        .to(RefNames.REFS_CONFIG)
-        .assertOkStatus();
-
     Project.NameKey childProject = projectOperations.newProject().parent(project).create();
-    TestRepository<?> childTestRepo = cloneProject(childProject);
-    fetch(childTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
-    childTestRepo.reset("refs/heads/config");
+    TestRepository<?> childTestRepo = checkoutRefsMetaConfig(cloneProject(childProject));
 
-    Config cfg = new Config();
-    cfg.setString(SECTION_FILTER, NO_FILTER, KEY_REVIEWER, JANE_DOE);
-    cfg.setString(SECTION_FILTER, BRANCH_MAIN, KEY_REVIEWER, JANE_DOE);
+    createFiltersFor(
+        childTestRepo,
+        filter(NO_FILTER).reviewer(JANE_DOE),
+        filter(BRANCH_MAIN).reviewer(JANE_DOE));
 
-    pushFactory
-        .create(
-            admin.newIdent(), childTestRepo, "Add reviewers child project", FILENAME, cfg.toText())
-        .to(RefNames.REFS_CONFIG)
-        .assertOkStatus();
+    createFilters(filter(NO_FILTER).reviewer(JOHN_DOE), filter(BRANCH_MAIN).reviewer(JOHN_DOE));
 
-    assertThat(reviewersConfig().forProject(childProject).getReviewerFilterSections())
+    assertProjectHasFilters(
+        childProject,
+        filter(NO_FILTER).reviewer(JOHN_DOE).reviewer(JANE_DOE),
+        filter(BRANCH_MAIN).reviewer(JOHN_DOE).reviewer(JANE_DOE));
+  }
+
+  private void assertProjectHasFilters(Project.NameKey project, FilterData... filters) {
+    assertThat(reviewersConfig().forProject(project).getReviewerFilterSections())
         .containsExactlyElementsIn(
-            ImmutableList.of(
-                new ReviewerFilterSection(NO_FILTER, ImmutableSet.of(JOHN_DOE, JANE_DOE)),
-                new ReviewerFilterSection(BRANCH_MAIN, ImmutableSet.of(JOHN_DOE, JANE_DOE))))
+            Arrays.stream(filters).map(f -> f.asSection()).collect(toImmutableList()))
         .inOrder();
   }
 
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 df25b27..a2a7035 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
@@ -15,38 +15,30 @@
 package com.googlesource.gerrit.plugins.reviewers;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.GitUtil.fetch;
 import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
-import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.FILENAME;
-import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.KEY_REVIEWER;
-import static com.googlesource.gerrit.plugins.reviewers.ReviewersConfig.SECTION_FILTER;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Lists;
-import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.acceptance.TestPlugin;
-import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.entities.RefNames;
-import com.google.inject.Inject;
-import java.util.List;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.common.ChangeInfo;
 import java.util.Set;
-import org.eclipse.jgit.lib.Config;
 import org.junit.Test;
 
 @NoHttpd
 @TestPlugin(name = "reviewers", sysModule = "com.googlesource.gerrit.plugins.reviewers.TestModule")
-public class ReviewersIT extends LightweightPluginDaemonTest {
-  @Inject private ProjectOperations projectOperations;
+public class ReviewersIT extends AbstractReviewersPluginTest {
 
   @Test
   public void addReviewers() throws Exception {
     TestAccount user2 = accountCreator.user2();
-    setReviewerFilters(filter("*").reviewer(user).reviewer(user2));
+    createFilters(filter("*").reviewer(user).reviewer(user2));
     String changeId = createChange().getChangeId();
     assertThat(reviewersFor(changeId))
         .containsExactlyElementsIn(ImmutableSet.of(user.id(), user2.id()));
@@ -55,7 +47,7 @@
   @Test
   public void addReviewersMatchMultipleSections() throws Exception {
     TestAccount user2 = accountCreator.user2();
-    setReviewerFilters(filter("*").reviewer(user), filter("\"^a.txt\"").reviewer(user2));
+    createFilters(filter("*").reviewer(user), filter("\"^a.txt\"").reviewer(user2));
     String changeId = createChange().getChangeId();
     assertThat(reviewersFor(changeId))
         .containsExactlyElementsIn(ImmutableSet.of(user.id(), user2.id()));
@@ -63,7 +55,7 @@
 
   @Test
   public void doNotAddReviewersFromNonMatchingFilters() throws Exception {
-    setReviewerFilters(filter("branch:master").reviewer(user));
+    createFilters(filter("branch:master").reviewer(user));
     createBranch(BranchNameKey.create(project, "other-branch"));
     // Create a change that matches the filter section.
     createChange("refs/for/master");
@@ -74,7 +66,7 @@
 
   @Test
   public void addReviewersFromMatchingFilters() throws Exception {
-    setReviewerFilters(filter("branch:other-branch").reviewer(user));
+    createFilters(filter("branch:other-branch").reviewer(user));
     // Create a change that doesn't match the filter section.
     createChange("refs/for/master");
     // The actual change we want to test
@@ -83,18 +75,42 @@
     assertThat(reviewersFor(changeId)).containsExactlyElementsIn(ImmutableSet.of(user.id()));
   }
 
-  private void setReviewerFilters(Filter... filters) throws Exception {
-    fetch(testRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
-    testRepo.reset("refs/heads/config");
-    Config cfg = new Config();
-    for (Filter f : filters) {
-      cfg.setStringList(SECTION_FILTER, f.filter, KEY_REVIEWER, f.reviewers);
-    }
-    pushFactory
-        .create(admin.newIdent(), testRepo, "Add reviewers", FILENAME, cfg.toText())
-        .to(RefNames.REFS_CONFIG)
-        .assertOkStatus();
-    testRepo.reset(projectOperations.project(project).getHead("master"));
+  @Test
+  public void dontAddReviewersForPrivateChange() throws Exception {
+    createFilters(filter("*").reviewer(user));
+    PushOneCommit.Result r = createChange("refs/for/master%private");
+    assertThat(r.getChange().change().isPrivate()).isTrue();
+    assertNoReviewersAddedFor(r.getChangeId());
+  }
+
+  @Test
+  public void privateBitFlippedAndReviewersAddedOnSubmit() throws Exception {
+    createFilters(filter("*").reviewer(user));
+    PushOneCommit.Result r = createChange("refs/for/master%private");
+    assertThat(r.getChange().change().isPrivate()).isTrue();
+    String changeId = r.getChangeId();
+    assertNoReviewersAddedFor(changeId);
+    // This adds admin as reviewer
+    gApi.changes().id(changeId).current().review(ReviewInput.approve());
+    gApi.changes().id(changeId).current().submit();
+    assertThat(reviewersFor(changeId))
+        .containsExactlyElementsIn(ImmutableSet.of(admin.id(), user.id()));
+    ChangeInfo info = gApi.changes().id(changeId).get();
+    assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
+    assertThat(info.isPrivate).isNull();
+  }
+
+  @Test
+  public void reviewerAddedOnPrivateBitFlip() throws Exception {
+    createFilters(filter("*").reviewer(user));
+    PushOneCommit.Result r = createChange("refs/for/master%private");
+    assertThat(r.getChange().change().isPrivate()).isTrue();
+    String changeId = r.getChangeId();
+    assertNoReviewersAddedFor(changeId);
+    gApi.changes().id(changeId).setPrivate(false);
+    ChangeInfo info = gApi.changes().id(changeId).get();
+    assertThat(info.isPrivate).isNull();
+    assertThat(reviewersFor(changeId)).containsExactlyElementsIn(ImmutableSet.of(user.id()));
   }
 
   private Set<Account.Id> reviewersFor(String changeId) throws Exception {
@@ -106,23 +122,4 @@
   private void assertNoReviewersAddedFor(String changeId) throws Exception {
     assertThat(gApi.changes().id(changeId).get().reviewers.get(REVIEWER)).isNull();
   }
-
-  private Filter filter(String filter) {
-    return new Filter(filter);
-  }
-
-  private class Filter {
-    List<String> reviewers;
-    String filter;
-
-    Filter(String filter) {
-      this.filter = filter;
-      this.reviewers = Lists.newArrayList();
-    }
-
-    Filter reviewer(TestAccount reviewer) {
-      reviewers.add(reviewer.email());
-      return this;
-    }
-  }
 }