Merge "Support enabling WANT_LGTM=all via submit requirements"
diff --git a/Documentation/config-submit-requirements.txt b/Documentation/config-submit-requirements.txt
index 5ab1add..c258b96 100644
--- a/Documentation/config-submit-requirements.txt
+++ b/Documentation/config-submit-requirements.txt
@@ -473,6 +473,38 @@
+
`distinctvoters:[Code-Review,Trust,API-Review],count>2`
+[[operator_label_with_users_arg]]
+label:'<label><operator><value>,users=human_reviewers'::
++
+Extension of the link:user-search.html#labels[label] predicate that
+allows matching changes that have a matching vote from all human
+reviewers. Votes from service users (members of the
+link:access-control.html#service_users[Service Users] group) and the
+change owner are ignored.
++
+If link:config-project-config.html#reviewer.enableByEmail[reviewers by
+email] are present then "user=all_reviewers" doesn't match if the
+expected value is other than 0. Reviewers by email are reviewers that
+don't have a Gerrit account. Without Gerrit account they cannot vote
+on the change, which means changes that have any such reviewers never
+match when a vote from all reviewers is expected.
++
+If a change has no human reviewers, this operator doesn't match
+(because a human review is required but no human reviewer is present).
++
+Examples:
+`label:Code-Review=MAX,users=human_reviewers`
++
+`label:Code-Review>=1,users=human_reviewers`
++
+The 'users' arg cannot be combined with other arguments ('count',
+'user', 'group').
++
+'label:Code-Review=MAX,users=human_reviewers' can be used to
+implement "Want-Code-Review-From-All" functionaly, see
+link#require-code-review-approvals-from-all-human-reviewers-example[examples
+below].
+
[[operator_is_true]]
is:true::
+
@@ -557,7 +589,7 @@
== Examples
[[code-review-example]]
-=== Code-Review Example
+=== Require Code-Review approval from a non-uploader
To define a submit requirement for code-review that requires a maximum vote for
the “Code-Review” label from a non-uploader without a maximum negative vote:
@@ -571,7 +603,7 @@
----
[[exempt-branch-example]]
-=== Exempt a branch Example
+=== Exempt a branch
We could exempt a submit requirement from certain branches. For example,
project owners might want to skip the 'Code-Style' requirement from the
@@ -602,7 +634,7 @@
----
[[require-footer-example]]
-=== Require a footer Example
+=== Require a footer
It's possible to use a submit requirement to require a footer to be present in
the commit message.
@@ -614,6 +646,51 @@
submittableIf = hasfooter:\"Bug\"
----
+[[require-code-review-approvals-from-all-human-reviewers-example]]
+=== Require Code-Review approvals from all human reviewers
+
+The following submit requirement requires a 'Code-Review+MAX' approval
+from all human reviewers of the change. Votes from service users
+(members of the link:access-control.html#service_users[Service Users]
+group) and the change owner are ignored.
+
+The 'applicableIf' condition makes this submit requirement show up in
+the UI only if it is not satisfied.
+
+If a change has no human reviewers, this submit requirement is
+unsatisfied (because a human review is required but no human reviewer
+is present).
+
+----
+[submit-requirement "Want-Code-Review-From-All"]
+ description = A maximum 'Code-Review' vote is required from all \
+ human reviewers (service users that are reviewers \
+ are ignored).
+ applicableIf = -label:Code-Review=MAX,users=human_reviewers
+ submittableIf = label:Code-Review=MAX,users=human_reviewers
+----
+
+It is possible to configure the 'Want-Code-Review-From-All' submit
+requirement so that it only applies when a 'Want-LGTM: all' footer is
+present in the commit message. This way users can enable this submit
+requirement on demand by including this footer into their commit
+messages.
+
+Note, the footer key cannot contain underscores (e.g. using
+'Want_LGTM: all' as the footer does not work).
+
+----
+[submit-requirement "Want-Code-Review-From-All"]
+ description = A maximum 'Code-Review' vote is required from all \
+ human reviewers (service users that are reviewers \
+ are ignored).
+ applicableIf = footer:\"WANT-LGTM: all\" -label:Code-Review=MAX,users=human_reviewers
+ submittableIf = label:Code-Review=MAX,users=human_reviewers
+----
+
+For more information about the "users=human_reviewers" arg see
+link:#operator_label_with_users_arg[above].
+
GERRIT
------
Part of link:index.html[Gerrit Code Review]
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index f75984d..f30efd4 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -98,6 +98,7 @@
import com.google.gerrit.server.submitrequirement.predicate.DistinctVotersPredicate;
import com.google.gerrit.server.submitrequirement.predicate.FileEditsPredicate;
import com.google.gerrit.server.submitrequirement.predicate.HasSubmoduleUpdatePredicate;
+import com.google.gerrit.server.submitrequirement.predicate.SubmitRequirementLabelExtensionPredicate;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.inject.Injector;
import com.google.inject.Key;
@@ -201,6 +202,7 @@
factory(ChangeData.AssistedFactory.class);
factory(ChangeIsVisibleToPredicate.Factory.class);
factory(DistinctVotersPredicate.Factory.class);
+ factory(SubmitRequirementLabelExtensionPredicate.Factory.class);
factory(HasSubmoduleUpdatePredicate.Factory.class);
factory(ProjectState.Factory.class);
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 4ad8fa7..1f0bd6e 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -218,6 +218,7 @@
import com.google.gerrit.server.submitrequirement.predicate.DistinctVotersPredicate;
import com.google.gerrit.server.submitrequirement.predicate.FileEditsPredicate;
import com.google.gerrit.server.submitrequirement.predicate.HasSubmoduleUpdatePredicate;
+import com.google.gerrit.server.submitrequirement.predicate.SubmitRequirementLabelExtensionPredicate;
import com.google.gerrit.server.tools.ToolsCatalog;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.util.IdGenerator;
@@ -301,6 +302,7 @@
factory(ChangeJson.AssistedFactory.class);
factory(ChangeIsVisibleToPredicate.Factory.class);
factory(DistinctVotersPredicate.Factory.class);
+ factory(SubmitRequirementLabelExtensionPredicate.Factory.class);
factory(HasSubmoduleUpdatePredicate.Factory.class);
factory(DeadlineChecker.Factory.class);
factory(EmailNewPatchSet.Factory.class);
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index d598739..9b85582 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -236,6 +236,7 @@
public static final String ARG_ID_NON_UPLOADER = "non_uploader";
public static final String ARG_ID_NON_CONTRIBUTOR = "non_contributor";
public static final String ARG_COUNT = "count";
+ public static final String ARG_USERS = "users";
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
@@ -1108,6 +1109,9 @@
"count=%d is not allowed. Maximum allowed value for count is %d.",
count, LabelPredicate.MAX_COUNT));
}
+ } else if (key.equalsIgnoreCase(ARG_USERS)) {
+ throw new QueryParseException(
+ String.format("Cannot use the '%s' argument in search", ARG_USERS));
} else {
throw new QueryParseException("Invalid argument identifier '" + pair.getKey() + "'");
}
diff --git a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
index cb92ddd..55d3505 100644
--- a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
@@ -28,13 +28,16 @@
import com.google.gerrit.server.submitrequirement.predicate.RegexAuthorEmailPredicate;
import com.google.gerrit.server.submitrequirement.predicate.RegexCommitterEmailPredicate;
import com.google.gerrit.server.submitrequirement.predicate.RegexUploaderEmailPredicateFactory;
+import com.google.gerrit.server.submitrequirement.predicate.SubmitRequirementLabelExtensionPredicate;
import com.google.inject.Inject;
+import java.io.IOException;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/**
* A query builder for submit requirement expressions that includes all {@link ChangeQueryBuilder}
@@ -48,6 +51,8 @@
new QueryBuilder.Definition<>(SubmitRequirementChangeQueryBuilder.class);
private final DistinctVotersPredicate.Factory distinctVotersPredicateFactory;
+ private final SubmitRequirementLabelExtensionPredicate.Factory
+ submitRequirementLabelExtensionPredicateFactory;
private final HasSubmoduleUpdatePredicate.Factory hasSubmoduleUpdateFactory;
/**
@@ -70,11 +75,15 @@
SubmitRequirementChangeQueryBuilder(
Arguments args,
DistinctVotersPredicate.Factory distinctVotersPredicateFactory,
+ SubmitRequirementLabelExtensionPredicate.Factory
+ submitRequirementLabelExtensionPredicateFactory,
FileEditsPredicate.Factory fileEditsPredicateFactory,
HasSubmoduleUpdatePredicate.Factory hasSubmoduleUpdateFactory,
RegexUploaderEmailPredicateFactory regexUploaderEmailPredicateFactory) {
super(def, args);
this.distinctVotersPredicateFactory = distinctVotersPredicateFactory;
+ this.submitRequirementLabelExtensionPredicateFactory =
+ submitRequirementLabelExtensionPredicateFactory;
this.fileEditsPredicateFactory = fileEditsPredicateFactory;
this.hasSubmoduleUpdateFactory = hasSubmoduleUpdateFactory;
this.regexUploaderEmailPredicateFactory = regexUploaderEmailPredicateFactory;
@@ -150,6 +159,16 @@
return distinctVotersPredicateFactory.create(value);
}
+ @Override
+ public Predicate<ChangeData> label(String value)
+ throws QueryParseException, IOException, ConfigInvalidException {
+ if (SubmitRequirementLabelExtensionPredicate.matches(value)) {
+ return submitRequirementLabelExtensionPredicateFactory.create(value);
+ }
+ SubmitRequirementLabelExtensionPredicate.validateIfNoMatch(value);
+ return super.label(value);
+ }
+
/**
* A SR operator that can match with file path and content pattern. The value should be of the
* form:
diff --git a/java/com/google/gerrit/server/submitrequirement/predicate/SubmitRequirementLabelExtensionPredicate.java b/java/com/google/gerrit/server/submitrequirement/predicate/SubmitRequirementLabelExtensionPredicate.java
new file mode 100644
index 0000000..3a59394
--- /dev/null
+++ b/java/com/google/gerrit/server/submitrequirement/predicate/SubmitRequirementLabelExtensionPredicate.java
@@ -0,0 +1,211 @@
+// Copyright (C) 2024 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.google.gerrit.server.submitrequirement.predicate;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.common.base.Enums;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.common.primitives.Ints;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments;
+import com.google.gerrit.server.query.change.LabelPredicate;
+import com.google.gerrit.server.query.change.SubmitRequirementPredicate;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.util.Locale;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Extensions of the {@link LabelPredicate} that are only available for submit requirement
+ * expressions, but not for search.
+ *
+ * <p>Supported extensions:
+ *
+ * <ul>
+ * <li>"users=human_reviewers" arg, e.g. "label:Code-Review=MAX,users=human_reviewers" matches
+ * changes where all human reviewers have approved the change with Code-Review=MAX
+ * </ul>
+ */
+public class SubmitRequirementLabelExtensionPredicate extends SubmitRequirementPredicate {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ public interface Factory {
+ SubmitRequirementLabelExtensionPredicate create(String value) throws QueryParseException;
+ }
+
+ private static final Pattern PATTERN = Pattern.compile("(?<label>[^,]*),users=human_reviewers$");
+ private static final Pattern PATTERN_LABEL =
+ Pattern.compile("(?<label>[^,<>=]*)(?<op>=|<=|>=|<|>)(?<value>[^,]*)");
+
+ public static boolean matches(String value) {
+ return PATTERN.matcher(value).matches();
+ }
+
+ public static void validateIfNoMatch(String value) throws QueryParseException {
+ if (value.contains(",users=")) {
+ throw new QueryParseException(
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+ }
+ }
+
+ private final Arguments args;
+ private final ServiceUserClassifier serviceUserClassifier;
+ private final String label;
+
+ @Inject
+ public SubmitRequirementLabelExtensionPredicate(
+ Arguments args, ServiceUserClassifier serviceUserClassifier, @Assisted String value)
+ throws QueryParseException {
+ super("label", value);
+ this.args = args;
+ this.serviceUserClassifier = serviceUserClassifier;
+
+ Matcher m = PATTERN.matcher(value);
+ if (!m.matches()) {
+ throw new QueryParseException(
+ String.format("invalid value for '%s': %s", getOperator(), value));
+ }
+ this.label = validateLabel(m.group("label"));
+ }
+
+ private String validateLabel(String label) throws QueryParseException {
+ int eq = label.indexOf('=');
+
+ if (eq <= 0) {
+ return label;
+ }
+
+ String statusName = label.substring(eq + 1).toUpperCase(Locale.US);
+ SubmitRecord.Label.Status status =
+ Enums.getIfPresent(SubmitRecord.Label.Status.class, statusName).orNull();
+ if (status != null) {
+ // We would need to use SubmitRecordPredicate but can't because it doesn't implement
+ // Matchable.
+ throw new QueryParseException(
+ "Cannot use the 'users=human_reviewers' argument in conjunction with a submit record"
+ + " label status");
+ }
+ return label;
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ if (!cd.reviewersByEmail().byState(ReviewerStateInternal.REVIEWER).isEmpty()
+ && !matchZeroVotes(label)) {
+ // Reviewers by email are reviewers that don't have a Gerrit account. Without Gerrit
+ // account they cannot vote on the change, which means changes that have any such
+ // reviewers never match when we expect a vote != 0 from all reviewers.
+ logger.atFine().log(
+ "change %s doesn't match since there are reviewers by email"
+ + " (that don't have a matching approval): %s",
+ cd.change().getChangeId(), cd.reviewersByEmail().byState(ReviewerStateInternal.REVIEWER));
+ return false;
+ }
+
+ ImmutableSet<Account.Id> humanReviewers =
+ cd.reviewers().byState(ReviewerStateInternal.REVIEWER).stream()
+ // Ignore the change owner (if the change owner voted on their own change they are
+ // technically a reviewer).
+ .filter(accountId -> !accountId.equals(cd.change().getOwner()))
+ // Ignore reviewers that are service users.
+ .filter(accountId -> !serviceUserClassifier.isServiceUser(accountId))
+ .collect(toImmutableSet());
+
+ if (humanReviewers.isEmpty()) {
+ // a review from human reviewers is required, but no human reviewers are present
+ return false;
+ }
+
+ for (Account.Id reviewer : humanReviewers) {
+ if (!new LabelPredicate(
+ args,
+ label,
+ ImmutableSet.of(reviewer),
+ /* group= */ null,
+ /* count= */ null,
+ /* countOp= */ null)
+ .match(cd)) {
+ logger.atFine().log(
+ "change %s doesn't match because it misses matching approvals from: %s",
+ cd.change().getChangeId(), reviewer);
+ return false;
+ }
+ }
+
+ logger.atFine().log(
+ "change %s matches because it has matching approvals from all human reviewers: %s",
+ cd.change().getChangeId(), humanReviewers);
+ return true;
+ }
+
+ private boolean matchZeroVotes(String label) {
+ Matcher m = PATTERN_LABEL.matcher(label);
+ if (!m.matches()) {
+ return false;
+ }
+
+ String op = m.group("op");
+ String value = m.group("value");
+
+ Optional<Integer> intValue = Optional.ofNullable(Ints.tryParse(value));
+
+ if (op.equals("=") && (intValue.isPresent() && intValue.get() == 0)) {
+ return true;
+ } else if (op.equals("<=")) {
+ if (intValue.isPresent() && intValue.get() >= 0) {
+ return true;
+ } else if (value.equals("MAX")) {
+ return true;
+ }
+ return false;
+ } else if (op.equals("<")) {
+ if (intValue.isPresent() && intValue.get() > 0) {
+ return true;
+ } else if (value.equals("MAX")) {
+ return true;
+ }
+ } else if (op.equals(">=")) {
+ if (intValue.isPresent() && intValue.get() <= 0) {
+ return true;
+ } else if (value.equals("MIN")) {
+ return true;
+ }
+ return false;
+ } else if (op.equals(">")) {
+ if (intValue.isPresent() && intValue.get() < 0) {
+ return true;
+ } else if (value.equals("MIN")) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
index a5d86ce..9d40056 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
@@ -637,6 +637,205 @@
}
@Test
+ public void submitRequirement_wantCodeReviewMaxHumanReviewers() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
+
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Want-Code-Review-From-All")
+ .setApplicabilityExpression(
+ Optional.of(
+ SubmitRequirementExpression.create(
+ "-label:Code-Review=MAX,users=human_reviewers")))
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX,users=human_reviewers"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Code-Review is unsatisfied because there is no Code-Review+2 approval.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is unsatisfied (since a review from reviewers is required but no
+ // reviewer is present on the change).
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Add some reviewers
+ TestAccount reviewer1 = accountCreator.create("reviewer1");
+ gApi.changes().id(changeId).addReviewer("reviewer1");
+ TestAccount reviewer2 = accountCreator.create("reviewer2");
+ gApi.changes().id(changeId).addReviewer("reviewer2");
+
+ // Code-Review is unsatisfied because there is no Code-Review+2 approval.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is unsatisfied since there are reviewers on the change that
+ // didn't approve it yet.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ requestScopeOperations.setApiUser(reviewer1.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Code-Review is satisfied because there is Code-Review+2 approval from reviewer1.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is unsatisfied since there is no approval from reviewer2.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ requestScopeOperations.setApiUser(reviewer2.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Code-Review is satisfied because there are Code-Review+2 approvals from reviewer1 and
+ // reviewer2.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is not applicable since there are approval from all reviewers
+ // (reviewer1 and reviewer2) which makes "label:Code-Review=MAX,users=human_reviewers"
+ // satisfied.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+ }
+
+ @Test
+ public void submitRequirement_wantCodeReviewMaxHumanReviewers_enabledByFooter() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
+
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Want-Code-Review-From-All")
+ .setApplicabilityExpression(
+ Optional.of(
+ SubmitRequirementExpression.create(
+ "footer:\"WANT-LGTM: all\" -label:Code-Review=MAX,users=human_reviewers")))
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX,users=human_reviewers"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Add some reviewers
+ TestAccount reviewer1 = accountCreator.create("reviewer1");
+ gApi.changes().id(changeId).addReviewer("reviewer1");
+ TestAccount reviewer2 = accountCreator.create("reviewer2");
+ gApi.changes().id(changeId).addReviewer("reviewer2");
+
+ // Approve by one reviewer
+ requestScopeOperations.setApiUser(reviewer1.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Code-Review is satisfied because there is Code-Review+2 approval from reviewer1.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is not applicable since the commit message doesn't contain a
+ // "WANT-LGTM: all" footer.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+
+ // Amend the change to add a "WANT-LGTM: all" footer.
+ amendChange(
+ changeId,
+ PushOneCommit.SUBJECT
+ + "\n\nSome Description\n\nChange-Id: "
+ + changeId
+ + "\nWANT-LGTM: all\n",
+ PushOneCommit.FILE_NAME,
+ "content");
+
+ // Re-Approve by reviewer1.
+ requestScopeOperations.setApiUser(reviewer1.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Want-Code-Review-From-All is applicable since there is a "WANT-LGTM: all" footer and it is
+ // unsatisfied since there is no approval from reviewer2.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Approve by reviewer2.
+ requestScopeOperations.setApiUser(reviewer2.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Want-Code-Review-From-All is not applicable since there are approval from all reviewers
+ // (reviewer1 and reviewer2) which makes "label:Code-Review=MAX,users=human_reviewers"
+ // satisfied.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+ }
+
+ @Test
public void submitRequirementIsSatisfied_whenSubmittabilityExpressionIsFulfilled()
throws Exception {
configSubmitRequirement(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
index 8643489..61a06a3 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -39,13 +40,19 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.SubmitRequirementsEvaluatorImpl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
@@ -467,6 +474,373 @@
assertMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
}
+ @Test
+ public void label_requireVoteFromHumanReviewers() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
+
+ Account.Id owner = accountCreator.create("owner").id();
+ Account.Id reviewer1 = accountCreator.create("reviewer1").id();
+ Account.Id reviewer2 = accountCreator.create("reviewer2").id();
+ Account.Id reviewer3 = accountCreator.create("reviewer3").id();
+
+ Account.Id serviceUser = accountCreator.create("serviceUser").id();
+ gApi.groups().id(ServiceUserClassifier.SERVICE_USERS).addMembers(serviceUser.toString());
+
+ Change.Id changeApprovedByAllReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeApprovedByAllReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(
+ project,
+ changeApprovedByAllReviewers,
+ ReviewInput.approve(),
+ reviewer1,
+ reviewer2,
+ reviewer3);
+
+ Change.Id changeApprovedBySomeReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeApprovedBySomeReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(project, changeApprovedBySomeReviewers, ReviewInput.approve(), reviewer1, reviewer2);
+
+ Change.Id changeRecommendedByAllReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeRecommendedByAllReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(
+ project,
+ changeRecommendedByAllReviewers,
+ ReviewInput.recommend(),
+ reviewer1,
+ reviewer2,
+ reviewer3);
+
+ Change.Id changeRecommendedBySomeReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeRecommendedBySomeReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(
+ project, changeRecommendedBySomeReviewers, ReviewInput.recommend(), reviewer1, reviewer2);
+
+ Change.Id changeNoVotesByReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeNoVotesByReviewers, reviewer1, reviewer2, reviewer3);
+
+ Change.Id changeWithoutReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // change without reviewers doesn't match
+ assertNotMatching("label:Code-Review=MAX,users=human_reviewers", changeWithoutReviewers);
+
+ // match changes where all reviewers have the same vote
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review=2,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review=1,users=human_reviewers",
+ ImmutableList.of(changeRecommendedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeApprovedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // match changes where no reviewer voted (same as "label:Code-Review=0")
+ assertRequirement(
+ "label:Code-Review=0,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewers),
+ ImmutableList.of(
+ changeApprovedByAllReviewers,
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers));
+
+ // match changes where all reviewers have a vote <=, >=, < or >
+ assertRequirement(
+ "label:Code-Review<=2,users=human_reviewers",
+ ImmutableList.of(
+ changeApprovedByAllReviewers,
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review<=1,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers),
+ ImmutableList.of(changeApprovedByAllReviewers));
+ assertRequirement(
+ "label:Code-Review>=1,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers, changeRecommendedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review<1,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewers),
+ ImmutableList.of(
+ changeApprovedByAllReviewers,
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers));
+ assertRequirement(
+ "label:Code-Review>1,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // match changes where all reviewers have any (non-zero) vote
+ assertRequirement(
+ "label:Code-Review=ANY,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers, changeRecommendedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // votes of the change owners are ignored (as the change owner is not considered as a reviewer)
+ addReviews(project, changeApprovedByAllReviewers, ReviewInput.dislike(), owner);
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // missing votes from service users are fine
+ addReviewers(project, changeApprovedByAllReviewers, serviceUser);
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // votes from service users are ignored
+ addReviews(project, changeApprovedByAllReviewers, ReviewInput.dislike(), serviceUser);
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // when reviewers by email are present changes do not match, unless the expected value is 0
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig cfg = projectConfigFactory.create(project);
+ cfg.load(md);
+ cfg.updateProject(
+ update ->
+ update.setBooleanConfig(
+ BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL, InheritableBoolean.TRUE));
+ cfg.commit(md);
+ }
+ projectCache.evictAndReindex(project);
+ Change.Id changeRecommendedByAllReviewersWithReviewersByEmail =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(
+ project,
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ reviewer1,
+ reviewer2,
+ reviewer3);
+ addReviews(
+ project,
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ ReviewInput.recommend(),
+ reviewer1,
+ reviewer2,
+ reviewer3);
+ addReviewer(
+ project,
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ "email-without-account@example.com");
+ Change.Id changeNoVotesByReviewersWithReviewersByEmail =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(
+ project, changeNoVotesByReviewersWithReviewersByEmail, reviewer1, reviewer2, reviewer3);
+ addReviewer(
+ project, changeNoVotesByReviewersWithReviewersByEmail, "email-without-account@example.com");
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review=2,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review=ANY,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review=0,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of(changeRecommendedByAllReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<=2,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review<=0,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of(changeRecommendedByAllReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<=-1,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<2,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review<1,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of(changeRecommendedByAllReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<0,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review>=0,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review>=1,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review>-1,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review>0,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+
+ // cannot combine users=human_reviewers" with submit record status
+ assertError(
+ "label:Code-Review=ok,users=human_reviewers",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users=human_reviewers' argument in conjunction with a submit record label"
+ + " status");
+
+ // cannot combine "users" arg with a "user" arg
+ assertError(
+ "label:Code-Review=MAX,users=human_reviewers,user=reviewer1",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+
+ // cannot combine "users" arg with a "group" arg
+ assertError(
+ "label:Code-Review=MAX,users=human_reviewers,group=foo",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+
+ // cannot combine "users" arg with a positional arg
+ assertError(
+ "label:Code-Review=MAX,users=human_reviewers,reviewer1",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+ assertError(
+ "label:Code-Review=MAX,reviewer1,users=human_reviewers",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+
+ // label without "users=human_reviewers" still works
+ assertRequirement(
+ "label:Code-Review=MAX,user=reviewer1",
+ ImmutableList.of(changeApprovedByAllReviewers, changeApprovedBySomeReviewers),
+ ImmutableList.of(
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review=MAX,reviewer1",
+ ImmutableList.of(changeApprovedByAllReviewers, changeApprovedBySomeReviewers),
+ ImmutableList.of(
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ }
+
+ private void addReviewers(Project.NameKey project, Change.Id changeId, Account.Id... reviewers)
+ throws Exception {
+ for (Account.Id reviewer : reviewers) {
+ addReviewer(project, changeId, reviewer.toString());
+ }
+ }
+
+ private void addReviewer(Project.NameKey project, Change.Id changeId, String reviewer)
+ throws Exception {
+ gApi.changes().id(project.get(), changeId.get()).addReviewer(reviewer);
+ }
+
+ private void addReviews(
+ Project.NameKey project, Change.Id changeId, ReviewInput reviewInput, Account.Id... reviewers)
+ throws Exception {
+ for (Account.Id reviewer : reviewers) {
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(project.get(), changeId.get()).current().review(reviewInput);
+ }
+ }
+
private void approveAsUser(String changeId, Account.Id userId) throws Exception {
requestScopeOperations.setApiUser(userId);
approve(changeId);
@@ -540,13 +914,28 @@
return threeWayMerger.getResultTreeId();
}
+ private void assertRequirement(
+ String requirement,
+ ImmutableList<Change.Id> matchingChanges,
+ ImmutableList<Change.Id> nonMatchingChanges) {
+ for (Change.Id matchingChange : matchingChanges) {
+ assertMatching(requirement, matchingChange);
+ }
+
+ for (Change.Id nonMatchingChange : nonMatchingChanges) {
+ assertNotMatching(requirement, nonMatchingChange);
+ }
+ }
+
private void assertMatching(String requirement, Change.Id change) {
- assertThat(evaluate(requirement, change).status())
+ assertWithMessage("requirement \"%s\" doesn't match change %s", requirement, change)
+ .that(evaluate(requirement, change).status())
.isEqualTo(SubmitRequirementExpressionResult.Status.PASS);
}
private void assertNotMatching(String requirement, Change.Id change) {
- assertThat(evaluate(requirement, change).status())
+ assertWithMessage("requirement \"%s\" matches change %s", requirement, change)
+ .that(evaluate(requirement, change).status())
.isEqualTo(SubmitRequirementExpressionResult.Status.FAIL);
}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index a0aad96..1a546fa 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1552,6 +1552,12 @@
}
@Test
+ public void cannotUseUsersArgWithLabel() throws Exception {
+ assertFailingQuery(
+ "label:Code-Review=MAX,users=human_reviewers", "Cannot use the 'users' argument in search");
+ }
+
+ @Test
public void byLabelMulti() throws Exception {
Project.NameKey project = Project.nameKey("repo");
repo = createAndOpenProject(project);
