Merge "Never add automatic reviewers to 'private' changes"
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/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/ReviewersIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
index df25b27..77a60d4 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
@@ -26,12 +26,16 @@
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.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.inject.Inject;
import java.util.List;
import java.util.Set;
@@ -83,6 +87,44 @@
assertThat(reviewersFor(changeId)).containsExactlyElementsIn(ImmutableSet.of(user.id()));
}
+ @Test
+ public void dontAddReviewersForPrivateChange() throws Exception {
+ setReviewerFilters(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 {
+ setReviewerFilters(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 {
+ setReviewerFilters(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 void setReviewerFilters(Filter... filters) throws Exception {
fetch(testRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
testRepo.reset("refs/heads/config");
