Add label score definition to the OWNERS file
That allows to configure a specific score value that is sufficient from
the OWNERS perspective to approve the file changes.
For instance the following OWNERS configuration:
inherited: true
label: Code-Review, 1
owners:
- group/Legal
matchers:
- exact: LICENSE
Means that in case when LICENSE file gets modified then someone from the
legal department has to give `Code-Review+1`. IOW `Code-Review+2`
doesn't have to granted to all project files.
Notes:
* label score definition is global for this file and applies to all
matching files (or files if there are not matchers specified)
* when the `inherited: true` and no label definition then it is
inherited from the parent projects (if specified)
* regular developers still have to give `Code-Review+2` for a
change to be submittable as default submit rule is still evaluated
* single digit (0..9) is allowed as the score value
Bug: Issue 15556
Change-Id: I76627192b8627139a04e1feb6eb13109d8f3717d
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
index 95b63d2..2c8c619 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
@@ -44,7 +44,10 @@
Boolean inherited =
Optional.ofNullable(jsonNode.get("inherited")).map(JsonNode::asBoolean).orElse(true);
ret.setInherited(inherited);
- ret.setLabel(Optional.ofNullable(jsonNode.get("label")).map(JsonNode::asText));
+ ret.setLabel(
+ Optional.ofNullable(jsonNode.get("label"))
+ .map(JsonNode::asText)
+ .flatMap(LabelDefinition::parse));
addClassicMatcher(jsonNode, ret);
addMatchers(jsonNode, ret);
return Optional.of(ret);
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/LabelDefinition.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/LabelDefinition.java
new file mode 100644
index 0000000..08664fe
--- /dev/null
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/LabelDefinition.java
@@ -0,0 +1,97 @@
+// Copyright (C) 2023 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.owners.common;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.LabelId;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Describes the label together with score (the latter is optional) that is configured in the OWNERS
+ * file. File owners have to give the score for change to be submittable.
+ */
+public class LabelDefinition {
+ public static final LabelDefinition CODE_REVIEW = new LabelDefinition(LabelId.CODE_REVIEW, null);
+
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final Pattern LABEL_PATTERN =
+ Pattern.compile("^([a-zA-Z0-9-]+)(?:(?:\\s*,\\s*)(\\d))?$");
+
+ private final String name;
+ private final Optional<Short> score;
+
+ LabelDefinition(String name, Short score) {
+ this.name = name;
+ this.score = Optional.ofNullable(score);
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public Optional<Short> getScore() {
+ return score;
+ }
+
+ @Override
+ public String toString() {
+ StringBuilder builder = new StringBuilder();
+ builder.append("LabelDefinition [name=");
+ builder.append(name);
+ builder.append(", score=");
+ builder.append(score);
+ builder.append("]");
+ return builder.toString();
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(name, score);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) {
+ return true;
+ }
+
+ if ((obj == null) || getClass() != obj.getClass()) {
+ return false;
+ }
+
+ LabelDefinition other = (LabelDefinition) obj;
+ return Objects.equals(name, other.name) && Objects.equals(score, other.score);
+ }
+
+ public static Optional<LabelDefinition> parse(String definition) {
+ return Optional.ofNullable(definition)
+ .filter(def -> !def.isBlank())
+ .map(
+ def -> {
+ Matcher labelDef = LABEL_PATTERN.matcher(def.trim());
+ if (!labelDef.matches()) {
+ logger.atSevere().log("Parsing label definition [%s] has failed.", def);
+ return null;
+ }
+
+ return new LabelDefinition(
+ labelDef.group(1),
+ Optional.ofNullable(labelDef.group(2)).map(Short::valueOf).orElse(null));
+ });
+ }
+}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
index dc44ae4..7e7e705 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
@@ -38,7 +38,7 @@
private Map<String, Matcher> matchers = Maps.newHashMap();
/** Label that is required for submit. CodeReview if nothing is specified. */
- private Optional<String> label = Optional.empty();
+ private Optional<LabelDefinition> label = Optional.empty();
@Override
public String toString() {
@@ -85,11 +85,11 @@
return this.matchers.put(matcher.path, matcher);
}
- public void setLabel(Optional<String> label) {
+ public void setLabel(Optional<LabelDefinition> label) {
this.label = label;
}
- public Optional<String> getLabel() {
+ public Optional<LabelDefinition> getLabel() {
return label;
}
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
index 7811bd6..760b8a6 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
@@ -31,7 +31,7 @@
private Map<String, Set<Account.Id>> fileOwners = Maps.newHashMap();
private Map<String, Set<Account.Id>> fileReviewers = Maps.newHashMap();
private Map<String, Set<String>> fileGroupOwners = Maps.newHashMap();
- private Optional<String> label = Optional.empty();
+ private Optional<LabelDefinition> label = Optional.empty();
@Override
public String toString() {
@@ -122,11 +122,11 @@
fileGroupOwners.computeIfAbsent(file, (f) -> Sets.newHashSet()).addAll(groupOwners);
}
- public Optional<String> getLabel() {
+ public Optional<LabelDefinition> getLabel() {
return label;
}
- public void setLabel(Optional<String> label) {
+ public void setLabel(Optional<LabelDefinition> label) {
this.label = label;
}
}
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
index 9a28b04..8269ad6 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -90,7 +90,7 @@
private final boolean expandGroups;
- private final Optional<String> label;
+ private final Optional<LabelDefinition> label;
public PathOwners(
Accounts accounts,
@@ -186,7 +186,7 @@
return expandGroups;
}
- public Optional<String> getLabel() {
+ public Optional<LabelDefinition> getLabel() {
return label;
}
@@ -350,7 +350,7 @@
} else {
String ownersPath = partial + "OWNERS";
Optional<OwnersConfig> conf = getOwnersConfig(repository, ownersPath, branch);
- Optional<String> label = currentEntry.getLabel();
+ Optional<LabelDefinition> label = currentEntry.getLabel();
final Set<Id> owners = currentEntry.getOwners();
final Set<Id> reviewers = currentEntry.getReviewers();
Collection<Matcher> inheritedMatchers = currentEntry.getMatchers().values();
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
index 03b4403..1fbcc80 100644
--- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
+++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
@@ -33,7 +33,7 @@
*/
class PathOwnersEntry {
private final boolean inherited;
- private Optional<String> label;
+ private Optional<LabelDefinition> label;
private Set<Account.Id> owners = Sets.newHashSet();
private Set<Account.Id> reviewers = Sets.newHashSet();
private String ownersPath;
@@ -49,7 +49,7 @@
String path,
OwnersConfig config,
Accounts accounts,
- Optional<String> inheritedLabel,
+ Optional<LabelDefinition> inheritedLabel,
Set<Account.Id> inheritedOwners,
Set<Account.Id> inheritedReviewers,
Collection<Matcher> inheritedMatchers,
@@ -141,11 +141,11 @@
return inherited;
}
- public Optional<String> getLabel() {
+ public Optional<LabelDefinition> getLabel() {
return label;
}
- public void setLabel(Optional<String> label) {
+ public void setLabel(Optional<LabelDefinition> label) {
this.label = label;
}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/LabelDefinitionTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/LabelDefinitionTest.java
new file mode 100644
index 0000000..126e239
--- /dev/null
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/LabelDefinitionTest.java
@@ -0,0 +1,59 @@
+// Copyright (C) 2023 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.owners.common;
+
+import static com.google.common.truth.Truth8.assertThat;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Optional;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class LabelDefinitionTest {
+ @Parameterized.Parameters
+ public static Collection<Object[]> labels() {
+ return Arrays.asList(
+ new Object[][] {
+ {null, Optional.empty()},
+ {"", Optional.empty()},
+ {"foo,", Optional.empty()},
+ {"foo", Optional.of(new LabelDefinition("foo", null))},
+ {"foo,1", Optional.of(new LabelDefinition("foo", (short) 1))},
+ {"foo, 1", Optional.of(new LabelDefinition("foo", (short) 1))},
+ {"foo , 1", Optional.of(new LabelDefinition("foo", (short) 1))},
+ {"foo ,1 ", Optional.of(new LabelDefinition("foo", (short) 1))}
+ });
+ }
+
+ private final String input;
+ private final Optional<LabelDefinition> expected;
+
+ public LabelDefinitionTest(String input, Optional<LabelDefinition> expected) {
+ this.input = input;
+ this.expected = expected;
+ }
+
+ @Test
+ public void shouldParseLabelDefinition() {
+ // when
+ Optional<LabelDefinition> result = LabelDefinition.parse(input);
+
+ // then
+ assertThat(result).isEqualTo(expected);
+ }
+}
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
index 3f403a4..710a2b7 100644
--- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
+++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -148,7 +148,7 @@
// expect that classic configuration takes precedence over `OWNERS` file for the label
// definition
- assertThat(owners2.getLabel()).hasValue(EXPECTED_LABEL);
+ assertThat(owners2.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
@@ -183,7 +183,7 @@
assertEquals(2, ownersSet.size());
assertTrue(ownersSet.contains(USER_A_ID));
assertTrue(ownersSet.contains(USER_B_ID));
- assertThat(owners.getLabel()).hasValue(EXPECTED_LABEL);
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
@@ -229,7 +229,7 @@
// expect that `master` configuration overwrites the label definition of both `refs/meta/config`
// and parent repo
- assertThat(owners.getLabel()).hasValue(EXPECTED_LABEL);
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
@@ -278,7 +278,7 @@
assertTrue(ownersSet2.contains(USER_B_ID));
// expect that closer parent (parentRepository1) overwrites the label definition
- assertThat(owners.getLabel()).hasValue(EXPECTED_LABEL);
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
private void mockParentRepository(Project.NameKey repositoryName, Repository repository)
@@ -315,19 +315,46 @@
assertTrue(ownersSet.contains(USER_C_ID));
// expect that more specific configuration overwrites the label definition
- assertThat(owners.getLabel()).hasValue(EXPECTED_LABEL);
+ assertThat(owners.getLabel().map(LabelDefinition::getName)).hasValue(EXPECTED_LABEL);
}
@Test
- public void testParsingYaml() {
+ public void testParsingYamlWithLabelWithScore() {
String yamlString =
- "inherited: true\nlabel: " + EXPECTED_LABEL + "\nowners:\n- " + USER_C_EMAIL_COM;
+ "inherited: true\nlabel: " + EXPECTED_LABEL + ",1\nowners:\n- " + USER_C_EMAIL_COM;
Optional<OwnersConfig> config = getOwnersConfig(yamlString);
+
assertTrue(config.isPresent());
+
OwnersConfig ownersConfig = config.get();
assertTrue(ownersConfig.isInherited());
assertThat(ownersConfig.getLabel()).isPresent();
- assertThat(ownersConfig.getLabel().get()).isEqualTo(EXPECTED_LABEL);
+
+ LabelDefinition label = ownersConfig.getLabel().get();
+ assertThat(label.getName()).isEqualTo(EXPECTED_LABEL);
+ assertThat(label.getScore()).hasValue(1);
+
+ Set<String> owners = ownersConfig.getOwners();
+ assertEquals(1, owners.size());
+ assertTrue(owners.contains(USER_C_EMAIL_COM));
+ }
+
+ @Test
+ public void testParsingYamlWithLabelWithoutScore() {
+ String yamlString =
+ "inherited: true\nlabel: " + EXPECTED_LABEL + "\nowners:\n- " + USER_C_EMAIL_COM;
+ Optional<OwnersConfig> config = getOwnersConfig(yamlString);
+
+ assertTrue(config.isPresent());
+
+ OwnersConfig ownersConfig = config.get();
+ assertTrue(ownersConfig.isInherited());
+ assertThat(ownersConfig.getLabel()).isPresent();
+
+ LabelDefinition label = ownersConfig.getLabel().get();
+ assertThat(label.getName()).isEqualTo(EXPECTED_LABEL);
+ assertThat(label.getScore()).isEmpty();
+
Set<String> owners = ownersConfig.getOwners();
assertEquals(1, owners.size());
assertTrue(owners.contains(USER_C_EMAIL_COM));
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
index c93c3a4..35201fa 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/OwnersSubmitRequirement.java
@@ -25,7 +25,6 @@
import com.google.gerrit.entities.Account.Id;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelFunction;
-import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
import com.google.gerrit.entities.LegacySubmitRequirement;
@@ -47,6 +46,7 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.owners.common.Accounts;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
import com.googlesource.gerrit.owners.common.PathOwners;
import com.googlesource.gerrit.owners.common.PluginSettings;
import java.io.IOException;
@@ -142,8 +142,8 @@
ChangeNotes notes = cd.notes();
requireNonNull(notes, "notes");
LabelTypes labelTypes = projectState.getLabelTypes(notes);
- String label = resolveLabel(labelTypes, pathOwners.getLabel());
- LabelTypes ownersLabelType = ownersLabel(labelTypes, label, project);
+ LabelDefinition label = resolveLabel(labelTypes, pathOwners.getLabel());
+ Optional<LabelAndScore> ownersLabel = ownersLabel(labelTypes, label, project);
Account.Id uploader = notes.getCurrentPatchSet().uploader();
Map<Account.Id, List<PatchSetApproval>> approvalsByAccount =
Streams.stream(approvalsUtil.byPatchSet(notes, cd.currentPatchSet().id()))
@@ -153,8 +153,12 @@
fileOwners.entrySet().stream()
.filter(
requiredApproval ->
- isApprovalMissing(
- requiredApproval, uploader, approvalsByAccount, ownersLabelType))
+ ownersLabel
+ .map(
+ ol ->
+ isApprovalMissing(
+ requiredApproval, uploader, approvalsByAccount, ol))
+ .orElse(true))
.map(Map.Entry::getKey)
.collect(toSet());
@@ -162,7 +166,7 @@
missingApprovals.isEmpty()
? ok()
: notReady(
- label,
+ label.getName(),
String.format(
"Missing approvals for path(s): [%s]",
Joiner.on(", ").join(missingApprovals))));
@@ -181,82 +185,112 @@
* is configured in the OWNERS file then `Code-Review` will be selected.
*
* @param labelTypes labels configured for project
- * @param label that is configured in the OWNERS file
+ * @param label and score definition that is configured in the OWNERS file
*/
- static String resolveLabel(LabelTypes labelTypes, Optional<String> label) {
- return label.orElse(LabelId.CODE_REVIEW);
+ static LabelDefinition resolveLabel(LabelTypes labelTypes, Optional<LabelDefinition> label) {
+ return label.orElse(LabelDefinition.CODE_REVIEW);
}
/**
- * Create single label LabelTypes if label can be found or empty otherwise.
+ * Create {@link LabelAndScore} definition with a label LabelType if label can be found or empty
+ * otherwise. Note that score definition is copied from the OWNERS.
*
* @param labelTypes labels configured for project
- * @param label that is resolved from the OWNERS file
+ * @param label and score definition (optional) that is resolved from the OWNERS file
* @param project that change is evaluated for
*/
- static LabelTypes ownersLabel(LabelTypes labelTypes, String label, Project.NameKey project) {
- return new LabelTypes(
- labelTypes
- .byLabel(label)
- .map(Collections::singletonList)
- .orElseGet(
- () -> {
- logger.atSevere().log(
- "OWNERS label '%s' is not configured for '%s' project. Change is not submittable.",
- label, project);
- return Collections.emptyList();
- }));
+ static Optional<LabelAndScore> ownersLabel(
+ LabelTypes labelTypes, LabelDefinition label, Project.NameKey project) {
+ return labelTypes
+ .byLabel(label.getName())
+ .map(type -> new LabelAndScore(type, label.getScore()))
+ .or(
+ () -> {
+ logger.atSevere().log(
+ "OWNERS label '%s' is not configured for '%s' project. Change is not submittable.",
+ label, project);
+ return Optional.empty();
+ });
}
static boolean isApprovalMissing(
Map.Entry<String, Set<Account.Id>> requiredApproval,
Account.Id uploader,
Map<Account.Id, List<PatchSetApproval>> approvalsByAccount,
- LabelTypes labelTypes) {
+ LabelAndScore ownersLabel) {
return requiredApproval.getValue().stream()
.noneMatch(
- fileOwner -> isApprovedByOwner(fileOwner, uploader, approvalsByAccount, labelTypes));
+ fileOwner -> isApprovedByOwner(fileOwner, uploader, approvalsByAccount, ownersLabel));
}
static boolean isApprovedByOwner(
Account.Id fileOwner,
Account.Id uploader,
Map<Account.Id, List<PatchSetApproval>> approvalsByAccount,
- LabelTypes labelTypes) {
+ LabelAndScore ownersLabel) {
return Optional.ofNullable(approvalsByAccount.get(fileOwner))
.map(
approvals ->
approvals.stream()
.anyMatch(
approval ->
- hasSufficientApproval(approval, labelTypes, fileOwner, uploader)))
+ hasSufficientApproval(approval, ownersLabel, fileOwner, uploader)))
.orElse(false);
}
static boolean hasSufficientApproval(
- PatchSetApproval approval, LabelTypes labelTypes, Account.Id fileOwner, Account.Id uploader) {
- return labelTypes
- .byLabel(approval.labelId())
- .map(label -> isLabelApproved(label, fileOwner, uploader, approval))
- .orElse(false);
+ PatchSetApproval approval,
+ LabelAndScore ownersLabel,
+ Account.Id fileOwner,
+ Account.Id uploader) {
+ return ownersLabel.getLabelType().getLabelId().equals(approval.labelId())
+ && isLabelApproved(
+ ownersLabel.getLabelType(), ownersLabel.getScore(), fileOwner, uploader, approval);
}
static boolean isLabelApproved(
- LabelType label, Account.Id fileOwner, Account.Id uploader, PatchSetApproval approval) {
+ LabelType label,
+ Optional<Short> score,
+ Account.Id fileOwner,
+ Account.Id uploader,
+ PatchSetApproval approval) {
if (label.isIgnoreSelfApproval() && fileOwner.equals(uploader)) {
return false;
}
- LabelFunction function = label.getFunction();
- if (function.isMaxValueRequired()) {
- return label.isMaxPositive(approval);
+ return score
+ .map(value -> approval.value() >= value)
+ .orElseGet(
+ () -> {
+ LabelFunction function = label.getFunction();
+ if (function.isMaxValueRequired()) {
+ return label.isMaxPositive(approval);
+ }
+
+ if (function.isBlock() && label.isMaxNegative(approval)) {
+ return false;
+ }
+
+ return approval.value() > label.getDefaultValue();
+ });
+ }
+
+ static class LabelAndScore {
+ private final LabelType labelType;
+ private final Optional<Short> score;
+
+ LabelAndScore(LabelType labelType, Optional<Short> score) {
+ this.labelType = labelType;
+ this.score = score;
}
- if (function.isBlock() && label.isMaxNegative(approval)) {
- return false;
+ LabelType getLabelType() {
+ return labelType;
}
- return approval.value() > label.getDefaultValue();
+ Optional<Short> getScore() {
+ return score;
+ }
}
private Map<String, FileDiffOutput> getDiff(Project.NameKey project, ObjectId revision)
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md
index c71b761..62ac73c 100644
--- a/owners/src/main/resources/Documentation/config.md
+++ b/owners/src/main/resources/Documentation/config.md
@@ -45,7 +45,7 @@
```yaml
inherited: true
-label: Code-Review
+label: Code-Review, 1
owners:
- some.email@example.com
- User Name
@@ -94,6 +94,9 @@
> consideration only when `owners.enableSubmitRequirement = true`.
> Owners scores are matched against the label specified in the property in
> question.
+> The required label's score can be provided (by default label's scores
+> configuration is used) so that owners don't have to be granted with the
+> maximum label's score. Note that only single digit (0..9) is allowed.
For example, imagine the following tree:
@@ -197,7 +200,40 @@
### When `owners.enableSubmitRequirement = true`
-This case is not yet covered by the owners submit requirement implementation.
+This case is supported with the `Code-Review` label and `OWNERS` file
+modifications.
+
+The `OWNERS` file requires the label configuration to be added (here is the
+updated version):
+
+```yaml
+inherited: true
+label: Code-Review, 1
+owners:
+- John Doe
+- Doug Smith
+```
+
+But additionally one needs to modify the label on the particular project level
+to the following version:
+
+```
+[label "Code-Review"]
+ function = NoOp
+ defaultValue = 0
+ copyMinScore = true
+ copyAllScoresOnTrivialRebase = true
+ value = -2 This shall not be merged
+ value = -1 I would prefer this is not merged as is
+ value = 0 No score
+ value = +1 Looks good to me, but someone else must approve
+ value = +2 Looks good to me, approved
+```
+
+Note that `function` is set to `NoOp`.
+
+As a result Gerrit would make the change Submittable only if 'John Doe' or
+'Doug Smith' have provided at least a `Code-Review +1`.
### When `owners.enableSubmitRequirement = false` (default)
@@ -285,7 +321,30 @@
A change cannot be submitted until John Doe or Doug Smith add a label
"Owner-Approved", independently from being able to provide any Code-Review.
-## Example 4 - Owners based on matchers
+## Example 4 - OWNERS file without matchers, default Gerrit submit rules and owners Code-Review +1 required
+
+This is a variant of `example 3` when no additional label is created but owners
+shouldn't be granted with `Code-Review +2` for all project files as it might be
+outside of their comptenence/comfort zone.
+
+### When `owners.enableSubmitRequirement = true`
+
+Given an OWNERS configuration of:
+
+```yaml
+inherited: true
+label: Code-Review, 1
+owners:
+- John Doe
+- Doug Smith
+```
+
+A change cannot be submitted until 'John Doe' or 'Doug Smith' add
+`Code-Review+1` score. Note that regular developers still need to cast the
+`Code-Review+2` for a change to be submittable as default submit rule is
+still evaluated.
+
+## Example 5 - Owners based on matchers
Often the ownership comes from the developer's skills and competencies and
cannot be purely defined by the project's directory structure.
@@ -335,7 +394,7 @@
(Mister Dba) and the .css files (either John Creative or Matt Designer) have
provided their Code-Review +2 feedback.
-## Example 5 - Owners details on a per-file basis
+## Example 6 - Owners details on a per-file basis
### When `owners.enableSubmitRequirement = true`
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
index 73d4e3a..c393ebc 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementIT.java
@@ -43,12 +43,11 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.project.ProjectConfig;
+import com.google.gerrit.server.project.testing.TestLabels;
import com.google.inject.Inject;
-import java.io.IOException;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
import java.util.Collection;
import java.util.stream.Stream;
-import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.junit.Test;
@TestPlugin(name = "owners", sysModule = "com.googlesource.gerrit.owners.OwnersModule")
@@ -230,7 +229,7 @@
public void shouldRequireConfiguredLabelByCodeOwner() throws Exception {
TestAccount admin2 = accountCreator.admin2();
String labelId = "Foo";
- addOwnerFileToRoot(true, labelId, admin2);
+ addOwnerFileToRoot(true, LabelDefinition.parse(labelId).get(), admin2);
installLabel(labelId);
@@ -261,7 +260,7 @@
throws Exception {
TestAccount admin2 = accountCreator.admin2();
String notExistinglabelId = "Foo";
- addOwnerFileToRoot(true, notExistinglabelId, admin2);
+ addOwnerFileToRoot(true, LabelDefinition.parse(notExistinglabelId).get(), admin2);
PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
ChangeApi changeApi = forChange(r);
@@ -274,6 +273,94 @@
assertThat(changeStillNotReady.requirements).containsExactly(NOT_READY);
}
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldRequireConfiguredLabelScoreByCodeOwner() throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ changeApi.current().review(ReviewInput.approve());
+ ChangeInfo changeNotReadyAfterSelfApproval = changeApi.get();
+ assertThat(changeNotReadyAfterSelfApproval.submittable).isFalse();
+ assertThat(changeNotReadyAfterSelfApproval.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo changeReadyWithOwnerScore = forChange(r).get();
+ assertThat(changeReadyWithOwnerScore.submittable).isTrue();
+ assertThat(changeReadyWithOwnerScore.requirements).containsExactly(READY);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldConfiguredLabelScoreByCodeOwnerBeNotSufficientIfLabelRequiresMaxValue()
+ throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo ownersVoteNotSufficient = changeApi.get();
+ assertThat(ownersVoteNotSufficient.submittable).isFalse();
+ assertThat(ownersVoteNotSufficient.requirements).containsExactly(READY);
+ verifyHasSubmitRecord(
+ ownersVoteNotSufficient.submitRecords,
+ LabelId.CODE_REVIEW,
+ SubmitRecordInfo.Label.Status.NEED);
+
+ requestScopeOperations.setApiUser(admin.id());
+ forChange(r).current().review(ReviewInput.approve());
+ ChangeInfo changeReadyWithMaxScore = forChange(r).get();
+ assertThat(changeReadyWithMaxScore.submittable).isTrue();
+ assertThat(changeReadyWithMaxScore.requirements).containsExactly(READY);
+ verifyHasSubmitRecord(
+ changeReadyWithMaxScore.submitRecords,
+ LabelId.CODE_REVIEW,
+ SubmitRecordInfo.Label.Status.OK);
+ }
+
+ @Test
+ @GlobalPluginConfig(
+ pluginName = "owners",
+ name = "owners.enableSubmitRequirement",
+ value = "true")
+ public void shouldConfiguredLabelScoreByCodeOwnersOverwriteSubmitRequirement() throws Exception {
+ installLabel(TestLabels.codeReview().toBuilder().setFunction(LabelFunction.NO_OP).build());
+
+ TestAccount admin2 = accountCreator.admin2();
+ addOwnerFileToRoot(true, LabelDefinition.parse("Code-Review,1").get(), admin2);
+
+ PushOneCommit.Result r = createChange("Add a file", "foo", "bar");
+ ChangeApi changeApi = forChange(r);
+ ChangeInfo changeNotReady = changeApi.get();
+ assertThat(changeNotReady.submittable).isFalse();
+ assertThat(changeNotReady.requirements).containsExactly(NOT_READY);
+
+ requestScopeOperations.setApiUser(admin2.id());
+ forChange(r).current().review(ReviewInput.recommend());
+ ChangeInfo ownersVoteSufficient = forChange(r).get();
+ assertThat(ownersVoteSufficient.submittable).isTrue();
+ assertThat(ownersVoteSufficient.requirements).containsExactly(READY);
+ }
+
private void verifyHasSubmitRecord(
Collection<SubmitRecordInfo> records, String label, SubmitRecordInfo.Label.Status status) {
assertThat(
@@ -284,24 +371,17 @@
.isPresent();
}
- private void installVerifiedLabel()
- throws RepositoryNotFoundException, IOException, ConfigInvalidException {
+ private void installVerifiedLabel() throws Exception {
installLabel(LabelId.VERIFIED);
}
- private void installLabel(String labelId)
- throws IOException, ConfigInvalidException, RepositoryNotFoundException {
+ private void installLabel(String labelId) throws Exception {
LabelType verified =
labelBuilder(labelId, value(1, "Verified"), value(0, "No score"), value(-1, "Fails"))
.setFunction(LabelFunction.MAX_WITH_BLOCK)
.build();
- try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
- ProjectConfig cfg = projectConfigFactory.create(project);
- cfg.load(md);
- cfg.upsertLabelType(verified);
- cfg.commit(md);
- }
- projectCache.evictAndReindex(project);
+
+ installLabel(verified);
String heads = RefNames.REFS_HEADS + "*";
projectOperations
@@ -311,6 +391,16 @@
.update();
}
+ private void installLabel(LabelType label) throws Exception {
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig cfg = projectConfigFactory.create(project);
+ cfg.load(md);
+ cfg.upsertLabelType(label);
+ cfg.commit(md);
+ }
+ projectCache.evictAndReindex(project);
+ }
+
private ChangeApi forChange(PushOneCommit.Result r) throws RestApiException {
return gApi.changes().id(r.getChangeId());
}
@@ -358,11 +448,12 @@
""));
}
- private void addOwnerFileToRoot(boolean inherit, String label, TestAccount u) throws Exception {
+ private void addOwnerFileToRoot(boolean inherit, LabelDefinition label, TestAccount u)
+ throws Exception {
// Add OWNERS file to root:
//
// inherited: true
- // label: label
+ // label: label,score # score is optional
// owners:
// - u.email()
merge(
@@ -371,7 +462,14 @@
"master",
"Add OWNER file",
"OWNERS",
- String.format("inherited: %s\nlabel: %s\nowners:\n- %s\n", inherit, label, u.email()),
+ String.format(
+ "inherited: %s\nlabel: %s\nowners:\n- %s\n",
+ inherit,
+ String.format(
+ "%s%s",
+ label.getName(),
+ label.getScore().map(value -> String.format(",%d", value)).orElse("")),
+ u.email()),
""));
}
}
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java
index f8b8870..97113c0 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/OwnersSubmitRequirementTest.java
@@ -24,6 +24,7 @@
import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.isApprovalMissing;
import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.isApprovedByOwner;
import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.isLabelApproved;
+import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.ownersLabel;
import static com.googlesource.gerrit.owners.OwnersSubmitRequirement.resolveLabel;
import static org.mockito.Mockito.mock;
@@ -35,6 +36,8 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
+import com.googlesource.gerrit.owners.OwnersSubmitRequirement.LabelAndScore;
+import com.googlesource.gerrit.owners.common.LabelDefinition;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.Arrays;
@@ -47,53 +50,54 @@
public class OwnersSubmitRequirementTest {
private static String LABEL_ID = "foo";
+ private static LabelDefinition OWNERS_LABEL = LabelDefinition.parse(LABEL_ID).get();
+ private static LabelDefinition OWNERS_LABEL_WITH_SCORE =
+ LabelDefinition.parse(String.format("%s,1", LABEL_ID)).get();
private static int MAX_LABEL_VALUE = 1;
private static Project.NameKey PROJECT = Project.nameKey("project");
@Test
public void shouldResolveLabelToConfiguredOne() {
// when
- String label = resolveLabel(null, Optional.of(LABEL_ID));
+ LabelDefinition label = resolveLabel(null, Optional.of(OWNERS_LABEL));
// then
- assertThat(label).isEqualTo(LABEL_ID);
+ assertThat(label).isEqualTo(OWNERS_LABEL);
}
@Test
- public void shouldResolveLabelToCodeReviewIfProjectHasCodeReviewLabelConfigured() {
+ public void shouldResolveLabelToCodeReview() {
// given
LabelTypes types =
- new LabelTypes(
- List.of(verified(), label().setFunction(LabelFunction.NO_BLOCK).build(), codeReview()));
+ new LabelTypes(List.of(verified(), label().setFunction(LabelFunction.NO_BLOCK).build()));
// when
- String label = resolveLabel(types, Optional.empty());
+ LabelDefinition label = resolveLabel(types, Optional.empty());
// then
- assertThat(label).isEqualTo(LabelId.CODE_REVIEW);
+ assertThat(label).isEqualTo(LabelDefinition.CODE_REVIEW);
}
@Test
- public void shouldOwnersLabelContainOnlyConfiguredLabel() {
+ public void shouldOwnersLabelContainOnlyConfiguredLabelAndItsScore() {
// when
- LabelTypes result =
- OwnersSubmitRequirement.ownersLabel(
- new LabelTypes(List.of(label().build())), LABEL_ID, PROJECT);
+ Optional<LabelAndScore> result =
+ ownersLabel(new LabelTypes(List.of(label().build())), OWNERS_LABEL_WITH_SCORE, PROJECT);
// then
- assertThat(result.getLabelTypes()).hasSize(1);
- assertThat(result.byLabel(LABEL_ID)).isPresent();
+ assertThat(result.map(label -> label.getLabelType().getName())).hasValue(LABEL_ID);
+ assertThat(result.flatMap(LabelAndScore::getScore))
+ .isEqualTo(OWNERS_LABEL_WITH_SCORE.getScore());
}
@Test
public void shouldOwnersLabelBeEmptyIfNonExistingLabelIsConfigured() {
// when
- LabelTypes result =
- OwnersSubmitRequirement.ownersLabel(
- new LabelTypes(List.of(codeReview())), LABEL_ID, PROJECT);
+ Optional<LabelAndScore> result =
+ ownersLabel(new LabelTypes(List.of(codeReview())), OWNERS_LABEL, PROJECT);
// then
- assertThat(result.getLabelTypes()).isEmpty();
+ assertThat(result).isEmpty();
}
@Test
@@ -101,14 +105,14 @@
// given
Account.Id fileOwner = mock(Account.Id.class);
Account.Id uploader = mock(Account.Id.class);
- LabelTypes labelTypes = maxNoBlockLabelFooTypes();
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
Map<Account.Id, List<PatchSetApproval>> uploaderApproval =
Map.of(uploader, List.of(approvedBy(uploader, LABEL_ID, MAX_LABEL_VALUE)));
// when
boolean isApprovalMissing =
isApprovalMissing(
- Map.entry("path", Set.of(fileOwner)), uploader, uploaderApproval, labelTypes);
+ Map.entry("path", Set.of(fileOwner)), uploader, uploaderApproval, ownersLabel);
// then
assertThat(isApprovalMissing).isTrue();
@@ -119,14 +123,14 @@
// given
Account.Id fileOwner = mock(Account.Id.class);
Account.Id uploader = mock(Account.Id.class);
- LabelTypes labelTypes = maxNoBlockLabelFooTypes();
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
Map<Account.Id, List<PatchSetApproval>> fileOwnerApproval =
Map.of(fileOwner, List.of(approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE)));
// when
boolean isApprovalMissing =
isApprovalMissing(
- Map.entry("path", Set.of(fileOwner)), uploader, fileOwnerApproval, labelTypes);
+ Map.entry("path", Set.of(fileOwner)), uploader, fileOwnerApproval, ownersLabel);
// then
assertThat(isApprovalMissing).isFalse();
@@ -138,7 +142,7 @@
Account.Id fileOwnerA = mock(Account.Id.class);
Account.Id fileOwnerB = mock(Account.Id.class);
Account.Id uploader = mock(Account.Id.class);
- LabelTypes labelTypes = maxNoBlockLabelFooTypes();
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
Map<Account.Id, List<PatchSetApproval>> fileOwnerApproval =
Map.of(fileOwnerA, List.of(approvedBy(fileOwnerA, LABEL_ID, MAX_LABEL_VALUE)));
@@ -148,7 +152,7 @@
Map.entry("path", Set.of(fileOwnerA, fileOwnerB)),
uploader,
fileOwnerApproval,
- labelTypes);
+ ownersLabel);
// then
assertThat(isApprovalMissing).isFalse();
@@ -159,12 +163,13 @@
// given
Account.Id fileOwner = mock(Account.Id.class);
Account.Id uploader = mock(Account.Id.class);
- LabelTypes labelTypes = maxNoBlockLabelFooTypes();
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
Map<Account.Id, List<PatchSetApproval>> uploaderApproval =
Map.of(uploader, List.of(approvedBy(uploader, LABEL_ID, MAX_LABEL_VALUE)));
// when
- boolean approvedByOwner = isApprovedByOwner(fileOwner, fileOwner, uploaderApproval, labelTypes);
+ boolean approvedByOwner =
+ isApprovedByOwner(fileOwner, fileOwner, uploaderApproval, ownersLabel);
// then
assertThat(approvedByOwner).isFalse();
@@ -174,15 +179,16 @@
public void shouldNotBeApprovedWhenApprovalGivenForDifferentLabel() {
// given
Account.Id fileOwner = mock(Account.Id.class);
- LabelTypes labelTypes =
- new LabelTypes(
- List.of(label().setName("bar").setFunction(LabelFunction.MAX_NO_BLOCK).build()));
+ LabelAndScore ownersLabel =
+ new LabelAndScore(
+ label().setName("bar").setFunction(LabelFunction.MAX_NO_BLOCK).build(),
+ Optional.empty());
Map<Account.Id, List<PatchSetApproval>> fileOwnerForDifferentLabelApproval =
Map.of(fileOwner, List.of(approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE)));
// when
boolean approvedByOwner =
- isApprovedByOwner(fileOwner, fileOwner, fileOwnerForDifferentLabelApproval, labelTypes);
+ isApprovedByOwner(fileOwner, fileOwner, fileOwnerForDifferentLabelApproval, ownersLabel);
// then
assertThat(approvedByOwner).isFalse();
@@ -192,13 +198,13 @@
public void shouldBeApprovedByOwner() {
// given
Account.Id fileOwner = mock(Account.Id.class);
- LabelTypes labelTypes = maxNoBlockLabelFooTypes();
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
Map<Account.Id, List<PatchSetApproval>> fileOwnerApproval =
Map.of(fileOwner, List.of(approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE)));
// when
boolean approvedByOwner =
- isApprovedByOwner(fileOwner, fileOwner, fileOwnerApproval, labelTypes);
+ isApprovedByOwner(fileOwner, fileOwner, fileOwnerApproval, ownersLabel);
// then
assertThat(approvedByOwner).isTrue();
@@ -207,13 +213,13 @@
@Test
public void shouldHaveNotSufficientApprovalWhenLabelIsNotApproved() {
// given
- LabelType maxValueRequired = label().setFunction(LabelFunction.MAX_NO_BLOCK).build();
Account.Id fileOwner = mock(Account.Id.class);
- LabelTypes labelTypes = new LabelTypes(List.of(maxValueRequired));
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
// when
boolean hasSufficientApproval =
- hasSufficientApproval(approvedBy(fileOwner, LABEL_ID, 0), labelTypes, fileOwner, fileOwner);
+ hasSufficientApproval(
+ approvedBy(fileOwner, LABEL_ID, 0), ownersLabel, fileOwner, fileOwner);
// then
assertThat(hasSufficientApproval).isFalse();
@@ -223,11 +229,15 @@
public void shouldHaveNotSufficientApprovalWhenLabelDoesntMatch() {
// given
Account.Id fileOwner = mock(Account.Id.class);
- LabelTypes labelTypes = new LabelTypes(Collections.emptyList());
// when
boolean hasSufficientApproval =
- hasSufficientApproval(approvedBy(fileOwner, LABEL_ID, 0), labelTypes, fileOwner, fileOwner);
+ hasSufficientApproval(
+ approvedBy(fileOwner, LABEL_ID, 0),
+ new LabelAndScore(
+ LabelType.builder("foo", Collections.emptyList()).build(), Optional.empty()),
+ fileOwner,
+ fileOwner);
// then
assertThat(hasSufficientApproval).isFalse();
@@ -236,14 +246,13 @@
@Test
public void shouldHaveSufficientApprovalWhenLabelIsApproved() {
// given
- LabelType maxValueRequired = label().setFunction(LabelFunction.MAX_NO_BLOCK).build();
Account.Id fileOwner = mock(Account.Id.class);
- LabelTypes labelTypes = new LabelTypes(List.of(maxValueRequired));
+ LabelAndScore ownersLabel = maxNoBlockLabelFooOwnersLabel();
// when
boolean hasSufficientApproval =
hasSufficientApproval(
- approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE), labelTypes, fileOwner, fileOwner);
+ approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE), ownersLabel, fileOwner, fileOwner);
// then
assertThat(hasSufficientApproval).isTrue();
@@ -259,6 +268,7 @@
boolean approved =
isLabelApproved(
ignoreSelfApproval,
+ Optional.empty(),
fileOwner,
fileOwner,
approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE));
@@ -275,7 +285,12 @@
// when
boolean approved =
- isLabelApproved(maxValueRequired, fileOwner, fileOwner, approvedBy(fileOwner, LABEL_ID, 0));
+ isLabelApproved(
+ maxValueRequired,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, 0));
// then
assertThat(approved).isFalse();
@@ -291,6 +306,7 @@
boolean approved =
isLabelApproved(
maxValueRequired,
+ Optional.empty(),
fileOwner,
fileOwner,
approvedBy(fileOwner, LABEL_ID, MAX_LABEL_VALUE));
@@ -300,6 +316,59 @@
}
@Test
+ public void labelShouldBeApprovedWhenMaxValueIsRequiredButLowerScoreIsConfiguredForOwner() {
+ // given
+ LabelType maxValueRequired = codeReview();
+ Short ownersScore = 1;
+ Optional<Short> requiredOwnerScore = Optional.of(ownersScore);
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ maxValueRequired,
+ requiredOwnerScore,
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LabelId.CODE_REVIEW, ownersScore));
+
+ // then
+ assertThat(approved).isTrue();
+ }
+
+ @Test
+ public void
+ labelShouldNotBeApprovedWhenMaxValueIsRequiredLowerScoreIsConfiguredForOwnerAndTooLowScoreIsCast() {
+ // given
+ LabelType anyWithBlock =
+ label()
+ .setValues(
+ Arrays.asList(
+ value(3, "Great"),
+ value(2, "Approved"),
+ value(1, "Maybe"),
+ value(0, "No score"),
+ value(-1, "Blocked")))
+ .setFunction(LabelFunction.ANY_WITH_BLOCK)
+ .build();
+ Optional<Short> requiredOwnerScore = Optional.of((short) 2);
+ Short ownersScore = 1;
+ Account.Id fileOwner = mock(Account.Id.class);
+
+ // when
+ boolean approved =
+ isLabelApproved(
+ anyWithBlock,
+ requiredOwnerScore,
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LabelId.CODE_REVIEW, ownersScore));
+
+ // then
+ assertThat(approved).isFalse();
+ }
+
+ @Test
public void labelShouldNotBeApprovedWhenAnyValueWithBlockIsConfiguredAndMaxNegativeIsProvided() {
// given
LabelType anyWithBlock = label().setFunction(LabelFunction.ANY_WITH_BLOCK).build();
@@ -307,7 +376,12 @@
// when
boolean approved =
- isLabelApproved(anyWithBlock, fileOwner, fileOwner, approvedBy(fileOwner, LABEL_ID, -1));
+ isLabelApproved(
+ anyWithBlock,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, -1));
// then
assertThat(approved).isFalse();
@@ -330,7 +404,12 @@
// when
boolean approved =
- isLabelApproved(anyWithBlock, fileOwner, fileOwner, approvedBy(fileOwner, LABEL_ID, 1));
+ isLabelApproved(
+ anyWithBlock,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, 1));
// then
assertThat(approved).isTrue();
@@ -353,15 +432,20 @@
// when
boolean approved =
- isLabelApproved(anyWithBlock, fileOwner, fileOwner, approvedBy(fileOwner, LABEL_ID, 0));
+ isLabelApproved(
+ anyWithBlock,
+ Optional.empty(),
+ fileOwner,
+ fileOwner,
+ approvedBy(fileOwner, LABEL_ID, 0));
// then
assertThat(approved).isFalse();
}
- private static final LabelTypes maxNoBlockLabelFooTypes() {
+ private static final LabelAndScore maxNoBlockLabelFooOwnersLabel() {
LabelType maxValueRequired = label().setFunction(LabelFunction.MAX_NO_BLOCK).build();
- return new LabelTypes(List.of(maxValueRequired));
+ return new LabelAndScore(maxValueRequired, Optional.empty());
}
private static final LabelType.Builder label() {
