Add option to plugin config to disallow copy score rules
There are settings where it is desirable that only a certain set of copy
score rules can be set. Similar to preventing the configuration of labels
with certain functions, we add an option to prevent the configuration of
copy score rules.
Change-Id: I878fa21f057fa75bc6e4e8b8cb8fcd5d027a8705
diff --git a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslator.java b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslator.java
index 8e61857..f7ff9b0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/simplesubmitrules/config/ConfigTranslator.java
@@ -15,6 +15,8 @@
package com.googlesource.gerrit.plugins.simplesubmitrules.config;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
@@ -30,10 +32,9 @@
import com.googlesource.gerrit.plugins.simplesubmitrules.api.CommentsRules;
import com.googlesource.gerrit.plugins.simplesubmitrules.api.LabelDefinition;
import com.googlesource.gerrit.plugins.simplesubmitrules.api.SubmitConfig;
-import java.util.Collection;
import java.util.HashSet;
-import java.util.List;
import java.util.Map;
+import java.util.Set;
/** Codec class used to convert {@link SubmitConfig} from/to a Gerrit config */
@Singleton
@@ -73,9 +74,12 @@
}
static void applyCopyScoreRulesTo(
- @Nullable Collection<String> copyScoreRules, LabelType labelType) {
- if (copyScoreRules == null) {
- return;
+ Set<String> copyScoreRules, Set<String> disallowedCopyScoreRules, LabelType labelType)
+ throws BadRequestException {
+ Set<String> disallowed =
+ Sets.intersection(ImmutableSet.copyOf(copyScoreRules), disallowedCopyScoreRules);
+ if (!disallowed.isEmpty()) {
+ throw new BadRequestException("copy score rules " + disallowed + " are forbidden");
}
labelType.setCopyMinScore(copyScoreRules.contains(ProjectConfig.KEY_COPY_MIN_SCORE));
@@ -153,8 +157,8 @@
}
if (definition.getFunction().isPresent()) {
- List<String> disallowedLabelFunctions =
- ImmutableList.copyOf(
+ Set<String> disallowedLabelFunctions =
+ ImmutableSet.copyOf(
hostPluginConfig.getStringList("disallowedLabelFunctions-" + label));
LabelFunction function = definition.getFunction().get();
if (disallowedLabelFunctions.contains(function.getFunctionName())) {
@@ -162,10 +166,16 @@
}
labelType.setFunction(function);
}
+
// TODO(hiesel): Remove fallback to copyScores
- applyCopyScoreRulesTo(
- definition.copyScoreRules != null ? definition.copyScoreRules : definition.copyScores,
- labelType);
+ Set<String> copyScoreRules =
+ definition.copyScoreRules != null ? definition.copyScoreRules : definition.copyScores;
+ if (copyScoreRules != null) {
+ Set<String> disallowedCopyScoreRules =
+ ImmutableSet.copyOf(
+ hostPluginConfig.getStringList("disallowedCopyScoreRules-" + label));
+ applyCopyScoreRulesTo(copyScoreRules, disallowedCopyScoreRules, labelType);
+ }
}
}
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md
index 8ad2fa1..3da2fae 100644
--- a/src/main/resources/Documentation/about.md
+++ b/src/main/resources/Documentation/about.md
@@ -5,7 +5,7 @@
- Ability to prevent submission if there are unresolved comments.
- Ability to require approval, and to consider approval from the uploader of the
latest patch set or not.
-- (soon!) a simple PolyGerrit UI to configure the labels and how they work.
+- A simple PolyGerrit UI to configure the labels and how they work.
### Inheritance
This plugin supports configuration inheritance, following a worst case scenario when this is
@@ -124,3 +124,15 @@
[plugin "simple-submit"]
disallowedLabelFunctions-Code-Review = MaxNoBlock
```
+
+#### disallowedCopyScoreRules-<label-name>
+
+This config will prevent users from changing adding referenced copy score rules
+in the label configuration that is referenced in the name to a matching value.
+However, the config does not effect existing labels that already have
+the forbidden value.
+
+```
+[plugin "simple-submit"]
+ disallowedCopyScoreRules-Code-Review = copyMaxScore
+```
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 4b3d61c..07799ad 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
@@ -16,17 +16,21 @@
import static com.google.common.truth.Truth.assertThat;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.googlesource.gerrit.plugins.simplesubmitrules.api.LabelDefinition;
import java.util.function.BiConsumer;
import java.util.function.Predicate;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.ExpectedException;
public class ConfigTranslatorTest {
- /** Test the translations for each config key */
+ @Rule public ExpectedException exception = ExpectedException.none();
+
@Test
- public void checkLabelTranslation() {
+ public void checkLabelTranslation() throws Exception {
checkTranslation("copyMinScore", LabelType::isCopyMinScore, LabelType::setCopyMinScore);
checkTranslation("copyMaxScore", LabelType::isCopyMaxScore, LabelType::setCopyMaxScore);
@@ -51,11 +55,22 @@
LabelType::setCopyAllScoresOnTrivialRebase);
}
+ @Test
+ public void checkDisallowedCopyScoreThrowsBadRequest() throws Exception {
+ exception.expect(BadRequestException.class);
+ exception.expectMessage("copy score rules [copyAllScoresIfNoChange] are forbidden");
+ ConfigTranslator.applyCopyScoreRulesTo(
+ ImmutableSet.of("copyAllScoresIfNoChange", "copyAllScoresOnMergeFirstParentUpdate"),
+ ImmutableSet.of("copyAllScoresIfNoChange"),
+ LabelType.withDefaultValues("Verified"));
+ }
+
/** Helper method to check that conversion from/to Gerrit works for both true and false values */
private static void checkTranslation(
String copyScoreName,
Predicate<LabelType> functionToCheck,
- BiConsumer<LabelType, Boolean> functionToSet) {
+ BiConsumer<LabelType, Boolean> functionToSet)
+ throws Exception {
checkLabelToGerritPresent(copyScoreName, functionToCheck);
checkLabelToGerritAbsent(copyScoreName, functionToCheck);
@@ -92,18 +107,19 @@
}
private static void checkLabelToGerritPresent(
- String copyScoreName, Predicate<LabelType> functionToCheck) {
+ String copyScoreName, Predicate<LabelType> functionToCheck) throws Exception {
LabelType label = LabelType.withDefaultValues("Verified");
- ConfigTranslator.applyCopyScoreRulesTo(ImmutableList.of(copyScoreName), label);
+ ConfigTranslator.applyCopyScoreRulesTo(
+ ImmutableSet.of(copyScoreName), ImmutableSet.of(), label);
assertThat(functionToCheck.test(label)).named("[case %s:true]", copyScoreName).isTrue();
}
private static void checkLabelToGerritAbsent(
- String copyScoreName, Predicate<LabelType> functionToCheck) {
+ String copyScoreName, Predicate<LabelType> functionToCheck) throws Exception {
LabelType label = LabelType.withDefaultValues("Verified");
- ConfigTranslator.applyCopyScoreRulesTo(ImmutableList.of(), label);
+ ConfigTranslator.applyCopyScoreRulesTo(ImmutableSet.of(), ImmutableSet.of(), label);
assertThat(functionToCheck.test(label)).named("[case %s:false]", copyScoreName).isFalse();
}
}
