Cleanup classes
Remove the complicated constructor SimpleSubmitRulesModule,
it is error-prone and Guice might not be entirely ready at this
point.
Remove #getConfig, which was only used once from
NoUnresolvedCommentsRule. Directly call the right method.
Qualify sub-classes imports.
Change-Id: Ic5a6a4875861279ae237ca6438adf5fd7694a29e
diff --git a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/SimpleSubmitRulesModule.java b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/SimpleSubmitRulesModule.java
index 821f794..874f6d5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/SimpleSubmitRulesModule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/SimpleSubmitRulesModule.java
@@ -14,42 +14,19 @@
package com.googlesource.gerrit.plugins.simplesubmitrules;
-import com.google.common.annotations.VisibleForTesting;
-import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.RestApiModule;
-import com.google.gerrit.server.config.PluginConfig;
-import com.google.gerrit.server.config.PluginConfigFactory;
-import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectResource;
-import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.AbstractModule;
-import com.google.inject.Inject;
import com.googlesource.gerrit.plugins.simplesubmitrules.config.ConfigServlet;
import com.googlesource.gerrit.plugins.simplesubmitrules.config.ConfigTranslator;
import com.googlesource.gerrit.plugins.simplesubmitrules.rules.NoUnresolvedCommentsRule;
import com.googlesource.gerrit.plugins.simplesubmitrules.rules.RequireNonAuthorApprovalRule;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/** Bootstraps the Simple Submit Rules plugin */
public class SimpleSubmitRulesModule extends AbstractModule {
- private static final Logger log = LoggerFactory.getLogger(SimpleSubmitRulesModule.class);
-
- /** Name of this plugin, as defined in the BUILD file */
- @VisibleForTesting protected static final String PLUGIN_NAME = "simple-submit-rules";
-
- private static final String API_ENDPOINT = PLUGIN_NAME;
- private final PluginConfigFactory pluginConfigFactory;
- private final String pluginName;
-
- @Inject
- public SimpleSubmitRulesModule(
- PluginConfigFactory pluginConfigFactory, @PluginName String pluginName) {
- this.pluginConfigFactory = pluginConfigFactory;
- this.pluginName = pluginName;
- }
+ private static final String API_ENDPOINT = "simple-submit-rules";
@Override
protected void configure() {
@@ -67,13 +44,4 @@
DynamicSet.bind(binder(), SubmitRule.class).to(RequireNonAuthorApprovalRule.class);
DynamicSet.bind(binder(), SubmitRule.class).to(NoUnresolvedCommentsRule.class);
}
-
- public PluginConfig getConfig(ChangeData changeData) {
- try {
- return pluginConfigFactory.getFromProjectConfig(changeData.project(), pluginName);
- } catch (NoSuchProjectException e) {
- log.error("Could not load plugin configuration", e);
- return null;
- }
- }
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServlet.java b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServlet.java
index 468969b..bedff8d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServlet.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServlet.java
@@ -15,7 +15,7 @@
package com.googlesource.gerrit.plugins.simplesubmitrules.config;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.IdentifiedUser;
@@ -59,10 +59,8 @@
}
@Override
- public Object apply(
- ProjectResource resource,
- com.googlesource.gerrit.plugins.simplesubmitrules.config.SubmitConfig inConfig)
- throws PermissionBackendException, RestApiException, IOException {
+ public Object apply(ProjectResource resource, SubmitConfig inConfig)
+ throws PermissionBackendException, AuthException, BadRequestException, IOException {
permissionBackend
.user(resource.getUser())
diff --git a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/SubmitConfig.java b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/SubmitConfig.java
index 20712a9..9c05ffc 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/SubmitConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/SubmitConfig.java
@@ -22,8 +22,8 @@
import java.util.Set;
public class SubmitConfig {
- public Map<String, LabelDefinition> labels = new HashMap<>();
- public CommentsRules comments = new CommentsRules();
+ public Map<String, SubmitConfig.LabelDefinition> labels = new HashMap<>();
+ public SubmitConfig.CommentsRules comments = new SubmitConfig.CommentsRules();
@Override
public String toString() {
@@ -53,7 +53,7 @@
}
static class CommentsRules {
- boolean blockIfUnresolvedComments = false;
+ boolean blockIfUnresolvedComments;
@Override
public String toString() {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRule.java b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRule.java
index 6712782..bd6451c 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRule.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRule.java
@@ -16,16 +16,17 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.data.SubmitRecord;
-import com.google.gerrit.common.data.SubmitRecord.Status;
import com.google.gerrit.common.data.SubmitRequirement;
+import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.server.config.PluginConfig;
+import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.googlesource.gerrit.plugins.simplesubmitrules.SimpleSubmitRulesConfig;
-import com.googlesource.gerrit.plugins.simplesubmitrules.SimpleSubmitRulesModule;
import java.util.Collection;
import java.util.Collections;
import org.slf4j.Logger;
@@ -39,20 +40,31 @@
.setType("unresolved_comments")
.setFallbackText("Resolve all comments")
.build();
- private final SimpleSubmitRulesModule plugin;
+ private final PluginConfigFactory pluginConfigFactory;
+ private final String pluginName;
@Inject
- public NoUnresolvedCommentsRule(SimpleSubmitRulesModule plugin) {
- this.plugin = plugin;
+ public NoUnresolvedCommentsRule(
+ PluginConfigFactory pluginConfigFactory, @PluginName String pluginName) {
+ this.pluginConfigFactory = pluginConfigFactory;
+ this.pluginName = pluginName;
}
@Override
public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions options) {
- PluginConfig config = plugin.getConfig(cd);
- boolean blockIfUnresolvedComments =
+ PluginConfig config;
+ try {
+ config = pluginConfigFactory.getFromProjectConfig(cd.project(), pluginName);
+ } catch (NoSuchProjectException e) {
+ log.error("Error when fetching config of change {}'s project", cd.getId(), e);
+
+ return error("Error when fetching configuration");
+ }
+
+ boolean ruleEnabled =
config.getBoolean(SimpleSubmitRulesConfig.KEY_BLOCK_IF_UNRESOLVED_COMMENTS, false);
- if (!blockIfUnresolvedComments) {
+ if (!ruleEnabled) {
return Collections.emptyList();
}
@@ -62,20 +74,23 @@
} catch (OrmException e) {
log.error("Error when counting unresolved comments for change {}", cd.getId(), e);
- SubmitRecord sr = new SubmitRecord();
- sr.status = Status.RULE_ERROR;
- sr.errorMessage = "Error when counting unresolved comments";
- return ImmutableList.of(sr);
+ return error("Error when counting unresolved comments");
}
SubmitRecord sr = new SubmitRecord();
sr.requirements = Collections.singletonList(REQUIREMENT);
- if (unresolvedComments == null || unresolvedComments > 0) {
- sr.status = Status.NOT_READY;
- } else {
- sr.status = Status.OK;
- }
+ sr.status =
+ unresolvedComments == null || unresolvedComments > 0
+ ? SubmitRecord.Status.NOT_READY
+ : SubmitRecord.Status.OK;
return ImmutableList.of(sr);
}
+
+ private static Collection<SubmitRecord> error(String errorMessage) {
+ SubmitRecord sr = new SubmitRecord();
+ sr.status = SubmitRecord.Status.RULE_ERROR;
+ sr.errorMessage = errorMessage;
+ return ImmutableList.of(sr);
+ }
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/AbstractSimpleSubmitRulesIT.java b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/AbstractSimpleSubmitRulesIT.java
index d1f4f45..e73172f 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/AbstractSimpleSubmitRulesIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/AbstractSimpleSubmitRulesIT.java
@@ -26,12 +26,11 @@
import org.eclipse.jgit.lib.Config;
import org.junit.Ignore;
-@TestPlugin(
- name = SimpleSubmitRulesModule.PLUGIN_NAME,
- sysModule = "com.googlesource.gerrit.plugins.simplesubmitrules.SimpleSubmitRulesModule"
-)
-@Ignore
/** Base class used by IT tests, loads the Simple Submit Rules plugin. */
+@TestPlugin(
+ name = "my-plugin",
+ sysModule = "com.googlesource.gerrit.plugins.simplesubmitrules.SimpleSubmitRulesModule")
+@Ignore
public abstract class AbstractSimpleSubmitRulesIT extends LightweightPluginDaemonTest {
/** Helper method to change the project.config file using a provided consumer. */
protected void changeProjectConfig(Consumer<Config> callback) throws Exception {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServletIT.java b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServletIT.java
index 6b1e2ba..e83b0e9 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServletIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigServletIT.java
@@ -21,7 +21,7 @@
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.extensions.restapi.RawInput;
-import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.reviewdb.client.Project;
import com.googlesource.gerrit.plugins.simplesubmitrules.AbstractSimpleSubmitRulesIT;
import org.junit.Before;
import org.junit.Test;
@@ -86,7 +86,7 @@
"application/json");
}
- private static String endpointUrl(NameKey project) {
+ private static String endpointUrl(Project.NameKey project) {
return "/projects/" + project.get() + "/simple-submit-rules";
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslatorTest.java b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslatorTest.java
index f3b95ca..332afb5 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslatorTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslatorTest.java
@@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.data.LabelType;
-import com.googlesource.gerrit.plugins.simplesubmitrules.config.SubmitConfig.LabelDefinition;
import java.util.function.BiConsumer;
import java.util.function.Predicate;
import org.junit.Test;
@@ -67,7 +66,7 @@
String copyScoreName, BiConsumer<LabelType, Boolean> functionToSet) {
LabelType label = LabelType.withDefaultValues("Verified");
- LabelDefinition labelDefinition = new SubmitConfig.LabelDefinition();
+ SubmitConfig.LabelDefinition labelDefinition = new SubmitConfig.LabelDefinition();
functionToSet.accept(label, false);
ConfigTranslator.extractLabelCopyScores(label, labelDefinition);
@@ -81,7 +80,7 @@
String copyScoreName, BiConsumer<LabelType, Boolean> functionToSet) {
LabelType label = LabelType.withDefaultValues("Verified");
- LabelDefinition labelDefinition = new SubmitConfig.LabelDefinition();
+ SubmitConfig.LabelDefinition labelDefinition = new SubmitConfig.LabelDefinition();
functionToSet.accept(label, true);
ConfigTranslator.extractLabelCopyScores(label, labelDefinition);
diff --git a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRuleIT.java b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRuleIT.java
index 3ae48f1..8e80861 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRuleIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/NoUnresolvedCommentsRuleIT.java
@@ -22,10 +22,9 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.server.project.SubmitRuleOptions;
-import com.google.inject.Inject;
+import com.google.gerrit.server.query.change.ChangeData;
import com.googlesource.gerrit.plugins.simplesubmitrules.AbstractSimpleSubmitRulesIT;
import com.googlesource.gerrit.plugins.simplesubmitrules.SimpleSubmitRulesConfig;
import java.util.Collection;
@@ -35,7 +34,6 @@
@NoHttpd
public class NoUnresolvedCommentsRuleIT extends AbstractSimpleSubmitRulesIT {
private static final String FILENAME = "my.file";
- @Inject private NoUnresolvedCommentsRule rule;
@Before
public void enableRuleBeforeTest() throws Exception {
@@ -44,12 +42,11 @@
@Test
public void blocksWithUnresolvedComments() throws Exception {
- CommentInput comment = newFileComment();
+ ReviewInput.CommentInput comment = newFileComment();
comment.unresolved = true;
PushOneCommit.Result r = createChangeWithComment(comment);
- Collection<SubmitRecord> submitRecords =
- rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
+ Collection<SubmitRecord> submitRecords = evaluate(r.getChange(), SubmitRuleOptions.defaults());
assertThat(submitRecords).hasSize(1);
SubmitRecord result = submitRecords.iterator().next();
@@ -60,12 +57,11 @@
@Test
public void doesNotBlockWithNoComments() throws Exception {
- CommentInput comment = newFileComment();
+ ReviewInput.CommentInput comment = newFileComment();
comment.unresolved = false;
PushOneCommit.Result r = createChangeWithComment(comment);
- Collection<SubmitRecord> submitRecords =
- rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
+ Collection<SubmitRecord> submitRecords = evaluate(r.getChange(), SubmitRuleOptions.defaults());
assertThat(submitRecords).hasSize(1);
SubmitRecord result = submitRecords.iterator().next();
@@ -79,7 +75,7 @@
PushOneCommit.Result change = createChange("refs/for/master");
Collection<SubmitRecord> submitRecords =
- rule.evaluate(change.getChange(), SubmitRuleOptions.defaults());
+ evaluate(change.getChange(), SubmitRuleOptions.defaults());
assertThat(submitRecords).hasSize(1);
SubmitRecord result = submitRecords.iterator().next();
@@ -90,19 +86,18 @@
@Test
public void doesNothingByDefault() throws Exception {
- CommentInput comment = newFileComment();
+ ReviewInput.CommentInput comment = newFileComment();
comment.unresolved = true;
PushOneCommit.Result r = createChangeWithComment(comment);
enableRule(false);
- Collection<SubmitRecord> submitRecords =
- rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
+ Collection<SubmitRecord> submitRecords = evaluate(r.getChange(), SubmitRuleOptions.defaults());
assertThat(submitRecords).isEmpty();
}
- private PushOneCommit.Result createChangeWithComment(CommentInput comment) throws Exception {
+ private PushOneCommit.Result createChangeWithComment(ReviewInput.CommentInput comment) throws Exception {
PushOneCommit.Result r = createChange("My change", FILENAME, "new content");
ReviewInput reviewInput = new ReviewInput();
reviewInput.comments = ImmutableMap.of(comment.path, ImmutableList.of(comment));
@@ -121,8 +116,15 @@
newState));
}
- private static CommentInput newFileComment() {
- CommentInput c = new CommentInput();
+ private Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions options) {
+ NoUnresolvedCommentsRule rule =
+ plugin.getSysInjector().getInstance(NoUnresolvedCommentsRule.class);
+
+ return rule.evaluate(cd, options);
+ }
+
+ private static ReviewInput.CommentInput newFileComment() {
+ ReviewInput.CommentInput c = new ReviewInput.CommentInput();
c.path = FILENAME;
c.side = Side.REVISION;
c.message = "nit: double space.";
diff --git a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/RequireNonAuthorApprovalRuleTest.java b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/RequireNonAuthorApprovalRuleTest.java
index b1e0082..7324f55 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/RequireNonAuthorApprovalRuleTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/simplesubmitrules/rules/RequireNonAuthorApprovalRuleTest.java
@@ -24,7 +24,6 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.PatchSet.Id;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import java.time.Instant;
import java.util.ArrayList;
@@ -87,7 +86,8 @@
return new PatchSetApproval(key, (short) value, Date.from(Instant.now()));
}
- private static PatchSetApproval.Key makeKey(Id psId, Account.Id accountId, LabelId labelId) {
+ private static PatchSetApproval.Key makeKey(
+ PatchSet.Id psId, Account.Id accountId, LabelId labelId) {
return new PatchSetApproval.Key(psId, accountId, labelId);
}
