Merge "Move most of shortcut mixin features into service"
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 9a94d93..328c5de 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -99,6 +99,8 @@
import com.google.gerrit.server.cancellation.RequestCancelledException;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.TrackingFooters;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
@@ -253,6 +255,7 @@
TrackingFooters trackingFooters,
Metrics metrics,
RevisionJson.Factory revisionJsonFactory,
+ ExperimentFeatures experimentFeatures,
@GerritServerConfig Config cfg,
@Assisted Iterable<ListChangesOption> options,
@Assisted Optional<PluginDefinedInfosFactory> pluginDefinedInfosFactory) {
@@ -271,7 +274,9 @@
this.revisionJson = revisionJsonFactory.create(options);
this.options = Sets.immutableEnumSet(options);
this.includeMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInApi();
- this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD);
+ this.lazyLoad =
+ containsAnyOf(this.options, REQUIRE_LAZY_LOAD)
+ || lazyloadSubmitRequirements(this.options, experimentFeatures);
this.pluginDefinedInfosFactory = pluginDefinedInfosFactory;
logger.atFine().log("options = %s", options);
@@ -938,4 +943,20 @@
}
return ImmutableListMultimap.of();
}
+
+ private static boolean lazyloadSubmitRequirements(
+ Set<ListChangesOption> changeOptions, ExperimentFeatures experimentFeatures) {
+ // TODO(ghareeb,hiesel): Remove this method.
+ // We are testing the new submit requirements with users in lieu of upgrading the change index
+ // to a version that supports the new requirements.
+ // Upgrading now, before the feature is finalized would be counter productive, because the index
+ // format might change while we iterate over the feature.
+ // Allowing changes to lazyload parameters will slow down dashboards for users who have this
+ // feature enabled, but will backfill submit requirements that weren't loaded from the index by
+ // simply computing them.
+ return changeOptions.contains(SUBMIT_REQUIREMENTS)
+ && experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS_BACKFILLING_ON_DASHBOARD);
+ }
}
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index b060d3e..1486559 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -25,13 +25,25 @@
public static String GERRIT_BACKEND_REQUEST_FEATURE_REMOVE_REVISION_ETAG =
"GerritBackendRequestFeature__remove_revision_etag";
+ /** Enable storing submit requirements in NoteDb when the change is merged. */
+ public static final String GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE =
+ "GerritBackendRequestFeature__store_submit_requirements_on_merge";
+
/**
* Allow legacy {@link com.google.gerrit.entities.SubmitRecord}s to be converted and returned as
* submit requirements by the {@link
* com.google.gerrit.server.project.SubmitRequirementsEvaluator}.
*/
- public static final String GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS =
- "GerritBackendRequestFeature__enable_legacy_submit_requirements";
+ public static final String GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS =
+ "GerritBackendRequestFeature__enable_submit_requirements";
+
+ /**
+ * Allow SubmitRequirements to be computed freshly on dashboards irrespective of the value we
+ * retrieved from the change index.
+ */
+ public static final String
+ GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS_BACKFILLING_ON_DASHBOARD =
+ "GerritBackendRequestFeature__enable_submit_requirements_backfilling_on_dashboard";
/** Features, enabled by default in the current release. */
public static final ImmutableSet<String> DEFAULT_ENABLED_FEATURES =
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index eabee65..338b984 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -53,6 +53,7 @@
import com.google.inject.Singleton;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
+import java.io.Serializable;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.Arrays;
@@ -108,7 +109,7 @@
@Singleton
public class CommitRewriter {
/** Options to run {@link #backfillProject}. */
- public static class RunOptions {
+ public static class RunOptions implements Serializable {
/** Whether to rewrite the commit history or only find refs that need to be fixed. */
public boolean dryRun = true;
/**
diff --git a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
index 1a7d5af..d128633 100644
--- a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
+++ b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.notedb;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.project.SubmitRequirementsEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -24,6 +26,7 @@
public class StoreSubmitRequirementsOp implements BatchUpdateOp {
private final ChangeData.Factory changeDataFactory;
private final SubmitRequirementsEvaluator evaluator;
+ private final boolean storeRequirementsInNoteDb;
public interface Factory {
StoreSubmitRequirementsOp create();
@@ -31,13 +34,23 @@
@Inject
public StoreSubmitRequirementsOp(
- ChangeData.Factory changeDataFactory, SubmitRequirementsEvaluator evaluator) {
+ ChangeData.Factory changeDataFactory,
+ ExperimentFeatures experimentFeatures,
+ SubmitRequirementsEvaluator evaluator) {
this.changeDataFactory = changeDataFactory;
this.evaluator = evaluator;
+ this.storeRequirementsInNoteDb =
+ experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE);
}
@Override
public boolean updateChange(ChangeContext ctx) throws Exception {
+ if (!storeRequirementsInNoteDb) {
+ // Temporarily stop storing submit requirements in NoteDb when the change is merged.
+ return false;
+ }
// Create ChangeData using the project/change IDs instead of ctx.getChange(). We do that because
// for changes requiring a rebase before submission (e.g. if submit type = RebaseAlways), the
// RebaseOp inserts a new patchset that is visible here (via Change#getCurrentPatchset). If we
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 975ad52..8f0b535 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -943,8 +943,7 @@
if (lowerNames.containsKey(lower)) {
error(
String.format(
- "Submit requirement \"%s\" conflicts with \"%s\". Skipping the former.",
- name, lowerNames.get(lower)));
+ "Submit requirement '%s' conflicts with '%s'.", name, lowerNames.get(lower)));
continue;
}
lowerNames.put(lower, name);
@@ -958,9 +957,7 @@
if (blockExpr == null) {
error(
String.format(
- "Submit requirement \"%s\" does not define a submittability expression."
- + " Skipping this requirement.",
- name));
+ "Submit requirement '%s' does not define a submittability expression.", name));
continue;
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index 9555bf3..cc2c805 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -24,8 +24,6 @@
import com.google.gerrit.entities.SubmitRequirementResult;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
-import com.google.gerrit.server.experiments.ExperimentFeatures;
-import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.SubmitRequirementChangeQueryBuilder;
import com.google.inject.AbstractModule;
@@ -43,7 +41,6 @@
private final Provider<SubmitRequirementChangeQueryBuilder> queryBuilder;
private final ProjectCache projectCache;
private final SubmitRuleEvaluator.Factory legacyEvaluator;
- private final ExperimentFeatures experimentFeatures;
public static Module module() {
return new AbstractModule() {
@@ -60,12 +57,10 @@
private SubmitRequirementsEvaluatorImpl(
Provider<SubmitRequirementChangeQueryBuilder> queryBuilder,
ProjectCache projectCache,
- SubmitRuleEvaluator.Factory legacyEvaluator,
- ExperimentFeatures experimentFeatures) {
+ SubmitRuleEvaluator.Factory legacyEvaluator) {
this.queryBuilder = queryBuilder;
this.projectCache = projectCache;
this.legacyEvaluator = legacyEvaluator;
- this.experimentFeatures = experimentFeatures;
}
@Override
@@ -79,10 +74,7 @@
ChangeData cd, boolean includeLegacy) {
Map<SubmitRequirement, SubmitRequirementResult> projectConfigRequirements = getRequirements(cd);
Map<SubmitRequirement, SubmitRequirementResult> result = projectConfigRequirements;
- if (includeLegacy
- && experimentFeatures.isFeatureEnabled(
- ExperimentFeaturesConstants
- .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
+ if (includeLegacy) {
Map<SubmitRequirement, SubmitRequirementResult> legacyReqs =
SubmitRequirementsAdapter.getLegacyRequirements(legacyEvaluator, cd);
result =
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java b/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java
index 2e43eac..102d3f2 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java
@@ -50,10 +50,10 @@
result.putAll(projectConfigRequirements);
Map<String, SubmitRequirementResult> requirementsByName =
projectConfigRequirements.entrySet().stream()
- .collect(Collectors.toMap(sr -> sr.getKey().name(), sr -> sr.getValue()));
+ .collect(Collectors.toMap(sr -> sr.getKey().name().toLowerCase(), sr -> sr.getValue()));
for (Map.Entry<SubmitRequirement, SubmitRequirementResult> legacy :
legacyRequirements.entrySet()) {
- String name = legacy.getKey().name();
+ String name = legacy.getKey().name().toLowerCase();
SubmitRequirementResult projectConfigResult = requirementsByName.get(name);
SubmitRequirementResult legacyResult = legacy.getValue();
if (projectConfigResult != null && matchByStatus(projectConfigResult, legacyResult)) {
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 9ea6475..9961519 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -948,6 +948,10 @@
* com.google.gerrit.server.index.change.ChangeField#STORED_SUBMIT_REQUIREMENTS}.
*/
public Map<SubmitRequirement, SubmitRequirementResult> submitRequirements() {
+ if (!experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)) {
+ return Collections.emptyMap();
+ }
if (submitRequirements == null) {
if (!lazyload()) {
return Collections.emptyMap();
@@ -964,12 +968,6 @@
notes().getSubmitRequirementsResult().stream()
.filter(r -> !r.isLegacy())
.collect(Collectors.toMap(r -> r.submitRequirement(), Function.identity()));
- if (!experimentFeatures.isFeatureEnabled(
- ExperimentFeaturesConstants
- .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
- submitRequirements = projectConfigRequirements;
- return submitRequirements;
- }
Map<SubmitRequirement, SubmitRequirementResult> legacyRequirements =
SubmitRequirementsAdapter.getLegacyRequirements(submitRuleEvaluatorFactory, this);
submitRequirements =
@@ -981,7 +979,15 @@
public void setSubmitRequirements(
Map<SubmitRequirement, SubmitRequirementResult> submitRequirements) {
- this.submitRequirements = submitRequirements;
+ if (!experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS_BACKFILLING_ON_DASHBOARD)) {
+ // Only set back values from the index if the experiment is not active. While the experiment
+ // is active, we want
+ // to compute SRs from scratch to ensure fresh results.
+ // TODO(ghareeb, hiesel): Remove this.
+ this.submitRequirements = submitRequirements;
+ }
}
public List<SubmitRecord> submitRecords(SubmitRuleOptions options) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index e657c89..0d7210f 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4159,6 +4159,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_withLabelEqualsMax() throws Exception {
configSubmitRequirement(
project,
@@ -4185,6 +4188,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_withLabelEqualsMax_fromNonUploader() throws Exception {
configLabel("my-label", LabelFunction.NO_OP); // label function has no effect
projectOperations
@@ -4204,14 +4210,15 @@
PushOneCommit.Result r = createChange();
String changeId = r.getChangeId();
ChangeInfo change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ // The second requirement is coming from the legacy code-review label function
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "my-label", Status.UNSATISFIED, /* isLegacy= */ false);
// Voting with a max vote as the uploader will not satisfy the submit requirement.
voteLabel(changeId, "my-label", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "my-label", Status.UNSATISFIED, /* isLegacy= */ false);
@@ -4219,12 +4226,15 @@
requestScopeOperations.setApiUser(user.id());
voteLabel(changeId, "my-label", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "my-label", Status.SATISFIED, /* isLegacy= */ false);
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_withLabelEqualsMinBlockingSubmission() throws Exception {
configSubmitRequirement(
project,
@@ -4239,17 +4249,23 @@
String changeId = r.getChangeId();
ChangeInfo change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
// Requirement is satisfied because there are no votes
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
+ // Legacy requirement (coming from the label function definition) is not satisfied. We return
+ // both legacy and non-legacy requirements in this case since their statuses are not identical.
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
voteLabel(changeId, "code-review", -1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
// Requirement is still satisfied because -1 is not the max negative value
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
voteLabel(changeId, "code-review", -2);
change = gApi.changes().id(changeId).get();
@@ -4260,6 +4276,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_withMaxWithBlock_ignoringSelfApproval() throws Exception {
configLabel("my-label", LabelFunction.MAX_WITH_BLOCK);
projectOperations
@@ -4286,7 +4305,8 @@
// Admin (a.k.a uploader) adds a -1 min vote. This is going to block submission.
voteLabel(changeId, "my-label", -1);
ChangeInfo change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ // The other requirement is coming from the code-review label function
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "my-label", Status.UNSATISFIED, /* isLegacy= */ false);
@@ -4294,7 +4314,7 @@
requestScopeOperations.setApiUser(user.id());
voteLabel(changeId, "my-label", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "my-label", Status.UNSATISFIED, /* isLegacy= */ false);
@@ -4302,12 +4322,15 @@
requestScopeOperations.setApiUser(admin.id());
voteLabel(changeId, "my-label", 0);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "my-label", Status.SATISFIED, /* isLegacy= */ false);
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_withLabelEqualsAny() throws Exception {
configSubmitRequirement(
project,
@@ -4328,12 +4351,18 @@
voteLabel(changeId, "code-review", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
+ // Legacy and non-legacy requirements have mismatching status. Both are returned from the API.
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirementIsSatisfied_whenSubmittabilityExpressionIsFulfilled()
throws Exception {
configSubmitRequirement(
@@ -4372,6 +4401,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirementIsNotApplicable_whenApplicabilityExpressionIsNotFulfilled()
throws Exception {
configSubmitRequirement(
@@ -4387,14 +4419,19 @@
String changeId = r.getChangeId();
ChangeInfo change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.NOT_APPLICABLE, /* isLegacy= */ false);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirementIsOverridden_whenOverrideExpressionIsFulfilled() throws Exception {
- configLabel("build-cop-override", LabelFunction.MAX_WITH_BLOCK);
+ configLabel("build-cop-override", LabelFunction.NO_BLOCK);
projectOperations
.project(project)
.forUpdate()
@@ -4424,9 +4461,11 @@
voteLabel(changeId, "build-cop-override", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.OVERRIDDEN, /* isLegacy= */ false);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
}
@Test
@@ -4472,6 +4511,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_inheritedFromParentProject() throws Exception {
configSubmitRequirement(
allProjects,
@@ -4491,12 +4533,18 @@
voteLabel(changeId, "code-review", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
+ // Legacy requirement is coming from the label MaxWithBlock function. Still unsatisfied.
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_ignoredInChildProject_ifParentDoesNotAllowOverride()
throws Exception {
configSubmitRequirement(
@@ -4528,13 +4576,23 @@
voteLabel(changeId, "code-review", 1);
change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
+ assertThat(change.submitRequirements).hasSize(2);
// +1 was enough to fulfill the requirement: override in child project was ignored
assertSubmitRequirementStatus(
change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
+ // Legacy requirement is coming from the label MaxWithBlock function. Still unsatisfied.
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ true);
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS,
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
+ })
public void submitRequirement_storedForClosedChanges() throws Exception {
for (SubmitType submitType : SubmitType.values()) {
Project.NameKey project = createProjectForPush(submitType);
@@ -4576,6 +4634,13 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS,
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
+ })
public void submitRequirement_retrievedFromNoteDbForClosedChanges() throws Exception {
configSubmitRequirement(
project,
@@ -4621,9 +4686,11 @@
@Test
@GerritConfig(
name = "experiments.enabled",
- value =
- ExperimentFeaturesConstants
- .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
+ values = {
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS,
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
+ })
public void
submitRequirements_returnOneEntryForMatchingLegacyAndNonLegacyResultsWithTheSameName_ifLegacySubmitRecordsAreEnabled()
throws Exception {
@@ -4686,9 +4753,7 @@
@Test
@GerritConfig(
name = "experiments.enabled",
- value =
- ExperimentFeaturesConstants
- .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void
submitRequirements_returnTwoEntriesForMismatchingLegacyAndNonLegacyResultsWithTheSameName_ifLegacySubmitRecordsAreEnabled()
throws Exception {
@@ -4744,9 +4809,7 @@
@Test
@GerritConfig(
name = "experiments.enabled",
- value =
- ExperimentFeaturesConstants
- .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirements_returnForLegacySubmitRecords_ifEnabled() throws Exception {
configLabel("build-cop-override", LabelFunction.MAX_WITH_BLOCK);
projectOperations
@@ -4799,6 +4862,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS)
public void submitRequirement_backFilledFromIndexForActiveChanges() throws Exception {
configSubmitRequirement(
project,
@@ -4829,6 +4895,13 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS,
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE
+ })
public void submitRequirement_backFilledFromIndexForClosedChanges() throws Exception {
configSubmitRequirement(
project,
@@ -4860,6 +4933,35 @@
}
@Test
+ public void submitRequirements_notServedIfExperimentNotEnabled() throws Exception {
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("code-review")
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("label:code-review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).isEmpty();
+
+ voteLabel(changeId, "code-review", -1);
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).isEmpty();
+
+ voteLabel(changeId, "code-review", 2);
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).isEmpty();
+
+ gApi.changes().id(changeId).current().submit();
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).isEmpty();
+ }
+
+ @Test
public void fourByteEmoji() throws Exception {
// U+1F601 GRINNING FACE WITH SMILING EYES
String smile = new String(Character.toChars(0x1f601));
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java
index c6b57da..96db71a 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.entities.LegacySubmitRequirement;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.annotations.Exports;
@@ -28,6 +29,7 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Inject;
@@ -201,6 +203,37 @@
assertThat(rule.numberOfEvaluations.get()).isEqualTo(0);
}
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS_BACKFILLING_ON_DASHBOARD,
+ ExperimentFeaturesConstants.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_SUBMIT_REQUIREMENTS
+ })
+ public void submitRuleIsInvokedWhenQueryingChangeWithExperiment() throws Exception {
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ rule.numberOfEvaluations.set(0);
+ gApi.changes().query(changeId).withOptions(ListChangesOption.SUBMIT_REQUIREMENTS).get();
+
+ // Submit rules are invoked
+ assertThat(rule.numberOfEvaluations.get()).isEqualTo(1);
+ }
+
+ @Test
+ public void submitRuleIsNotInvokedWhenQueryingChangeWithoutExperiment() throws Exception {
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ rule.numberOfEvaluations.set(0);
+ gApi.changes().query(changeId).withOptions(ListChangesOption.SUBMIT_REQUIREMENTS).get();
+
+ // Submit rules are not invoked
+ assertThat(rule.numberOfEvaluations.get()).isEqualTo(0);
+ }
+
private List<ChangeInfo> queryIsSubmittable() throws Exception {
return gApi.changes().query("is:submittable project:" + project.get()).get();
}
diff --git a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsValidationIT.java b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsValidationIT.java
new file mode 100644
index 0000000..4675bc0
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsValidationIT.java
@@ -0,0 +1,252 @@
+// Copyright (C) 2021 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.acceptance.server.project;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.gerrit.acceptance.GitUtil.fetch;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.project.ProjectConfig;
+import java.util.Locale;
+import java.util.function.Consumer;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
+import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.util.RawParseUtils;
+import org.junit.Test;
+
+/**
+ * Tests validating submit requirements on upload of {@code project.config} to {@code
+ * refs/meta/config}.
+ */
+public class SubmitRequirementsValidationIT extends AbstractDaemonTest {
+ @Test
+ public void validSubmitRequirementIsAccepted_optionalParametersNotSet() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\""));
+
+ PushResult r = pushRefsMetaConfig();
+ assertOkStatus(r);
+ }
+
+ @Test
+ public void validSubmitRequirementIsAccepted_allParametersSet() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig -> {
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_DESCRIPTION,
+ /* value= */ "foo bar description");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_APPLICABILITY_EXPRESSION,
+ /* value= */ "branch:refs/heads/master");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\"");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_OVERRIDE_EXPRESSION,
+ /* value= */ "label:\"override=+1\"");
+ projectConfig.setBoolean(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_OVERRIDE_IN_CHILD_PROJECTS,
+ /* value= */ false);
+ });
+
+ PushResult r = pushRefsMetaConfig();
+ assertOkStatus(r);
+ }
+
+ @Test
+ public void conflictingSubmitRequirementsAreRejected() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig -> {
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\"");
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName.toLowerCase(Locale.US),
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\"");
+ });
+
+ PushResult r = pushRefsMetaConfig();
+ assertErrorStatus(
+ r,
+ "Invalid project configuration",
+ String.format(
+ "project.config: Submit requirement '%s' conflicts with '%s'.",
+ submitRequirementName.toLowerCase(Locale.US), submitRequirementName));
+ }
+
+ @Test
+ public void conflictingSubmitRequirementIsRejected() throws Exception {
+ fetchRefsMetaConfig();
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\""));
+ PushResult r = pushRefsMetaConfig();
+ assertOkStatus(r);
+
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName.toLowerCase(Locale.US),
+ /* name= */ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ /* value= */ "label:\"code-review=+2\""));
+ r = pushRefsMetaConfig();
+ assertErrorStatus(
+ r,
+ "Invalid project configuration",
+ String.format(
+ "project.config: Submit requirement '%s' conflicts with '%s'.",
+ submitRequirementName.toLowerCase(Locale.US), submitRequirementName));
+ }
+
+ @Test
+ public void submitRequirementWithoutSubmittabilityExpressionIsRejected() throws Exception {
+ fetchRefsMetaConfig();
+
+ String submitRequirementName = "Code-Review";
+ updateProjectConfig(
+ projectConfig ->
+ projectConfig.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ /* subsection= */ submitRequirementName,
+ /* name= */ ProjectConfig.KEY_SR_DESCRIPTION,
+ /* value= */ "foo bar description"));
+
+ PushResult r = pushRefsMetaConfig();
+ assertErrorStatus(
+ r,
+ "Invalid project configuration",
+ String.format(
+ "project.config: Submit requirement '%s' does not define a submittability expression.",
+ submitRequirementName));
+ }
+
+ private void fetchRefsMetaConfig() throws Exception {
+ fetch(testRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
+ testRepo.reset(RefNames.REFS_CONFIG);
+ }
+
+ private PushResult pushRefsMetaConfig() throws Exception {
+ return pushHead(testRepo, RefNames.REFS_CONFIG);
+ }
+
+ private void updateProjectConfig(Consumer<Config> configUpdater) throws Exception {
+ RevCommit head = getHead(testRepo.getRepository(), RefNames.REFS_CONFIG);
+ Config projectConfig = readProjectConfig(head);
+ configUpdater.accept(projectConfig);
+ RevCommit commit =
+ testRepo.update(
+ RefNames.REFS_CONFIG,
+ testRepo
+ .commit()
+ .parent(head)
+ .message("Update project config")
+ .author(admin.newIdent())
+ .committer(admin.newIdent())
+ .add(ProjectConfig.PROJECT_CONFIG, projectConfig.toText()));
+
+ testRepo.reset(commit);
+ }
+
+ private Config readProjectConfig(RevCommit commit) throws Exception {
+ try (TreeWalk tw =
+ TreeWalk.forPath(
+ testRepo.getRevWalk().getObjectReader(),
+ ProjectConfig.PROJECT_CONFIG,
+ commit.getTree())) {
+ if (tw == null) {
+ throw new IllegalStateException(
+ String.format("%s does not exist", ProjectConfig.PROJECT_CONFIG));
+ }
+ }
+ RevObject blob = testRepo.get(commit.getTree(), ProjectConfig.PROJECT_CONFIG);
+ byte[] data = testRepo.getRepository().open(blob).getCachedBytes(Integer.MAX_VALUE);
+ String content = RawParseUtils.decode(data);
+
+ Config projectConfig = new Config();
+ projectConfig.fromText(content);
+ return projectConfig;
+ }
+
+ public void assertOkStatus(PushResult result) {
+ RemoteRefUpdate refUpdate = result.getRemoteUpdate(RefNames.REFS_CONFIG);
+ assertThat(refUpdate).isNotNull();
+ assertWithMessage(getMessage(result, refUpdate))
+ .that(refUpdate.getStatus())
+ .isEqualTo(Status.OK);
+ }
+
+ public void assertErrorStatus(PushResult result, String... expectedMessages) {
+ RemoteRefUpdate refUpdate = result.getRemoteUpdate(RefNames.REFS_CONFIG);
+ assertThat(refUpdate).isNotNull();
+ assertWithMessage(getMessage(result, refUpdate))
+ .that(refUpdate.getStatus())
+ .isEqualTo(Status.REJECTED_OTHER_REASON);
+ for (String expectedMessage : expectedMessages) {
+ assertThat(result.getMessages()).contains(expectedMessage);
+ }
+ }
+
+ private String getMessage(PushResult result, RemoteRefUpdate refUpdate) {
+ StringBuilder b = new StringBuilder();
+ if (refUpdate.getMessage() != null) {
+ b.append(refUpdate.getMessage());
+ b.append("\n");
+ }
+ b.append(result.getMessages());
+ return b.toString();
+ }
+}
diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
index 7f0b685..9df59c2 100644
--- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
+++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java
@@ -310,9 +310,7 @@
assertThat(cfg.getValidationErrors()).hasSize(1);
assertThat(Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage())
.isEqualTo(
- "project.config: "
- + "Submit requirement \"Code-Review\" conflicts with \"code-review\". "
- + "Skipping the former.");
+ "project.config: Submit requirement 'Code-Review' conflicts with 'code-review'.");
}
@Test
@@ -332,8 +330,8 @@
assertThat(cfg.getValidationErrors()).hasSize(1);
assertThat(Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage())
.isEqualTo(
- "project.config: Submit requirement \"code-review\" does not define a submittability"
- + " expression. Skipping this requirement.");
+ "project.config: Submit requirement 'code-review' does not define a submittability"
+ + " expression.");
}
@Test
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 891482d..b10c17e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -95,9 +95,9 @@
} from '../../shared/gr-js-api-interface/gr-change-actions-js-api';
import {fireAlert, fireEvent, fireReload} from '../../../utils/event-util';
import {
- CODE_REVIEW,
getApprovalInfo,
getVotingRange,
+ StandardLabels,
} from '../../../utils/label-util';
import {CommentThread} from '../../../utils/comment-util';
import {ShowAlertEventDetail} from '../../../types/events';
@@ -975,8 +975,9 @@
// Allow the user to use quick approve to vote the max score on code review
// even if it is already granted by someone else. Does not apply if the
// user owns the change or has already granted the max score themselves.
- const codeReviewLabel = this.change.labels[CODE_REVIEW];
- const codeReviewPermittedValues = this.change.permitted_labels[CODE_REVIEW];
+ const codeReviewLabel = this.change.labels[StandardLabels.CODE_REVIEW];
+ const codeReviewPermittedValues =
+ this.change.permitted_labels[StandardLabels.CODE_REVIEW];
if (
!result &&
codeReviewLabel &&
@@ -988,7 +989,7 @@
getApprovalInfo(codeReviewLabel, this.account)?.value !==
getVotingRange(codeReviewLabel)?.max
) {
- result = CODE_REVIEW;
+ result = StandardLabels.CODE_REVIEW;
}
if (result) {
diff --git a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_test.js b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_test.js
index b3214c5..51d76b2 100644
--- a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_test.js
+++ b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_test.js
@@ -101,14 +101,12 @@
const labelsChangedHandler = sinon.stub();
element.addEventListener('labels-changed', labelsChangedHandler);
assert.ok(element.$.labelSelector);
- MockInteractions.tap(element.shadowRoot
- .querySelector('gr-button[data-value="-1"]'));
+ MockInteractions.tap(
+ element.shadowRoot.querySelector('gr-button[data-value="-1"]'));
await flush();
assert.strictEqual(element.selectedValue, '-1');
- assert.strictEqual(element.selectedItem
- .textContent.trim(), '-1');
- assert.strictEqual(
- element.$.selectedValueLabel.textContent.trim(), 'bad');
+ assert.strictEqual(element.selectedItem.textContent.trim(), '-1');
+ assert.strictEqual(element.$.selectedValueLabel.textContent.trim(), 'bad');
const detail = labelsChangedHandler.args[0][0].detail;
assert.equal(detail.name, 'Verified');
assert.equal(detail.value, '-1');
@@ -120,49 +118,48 @@
let index = 0;
const totalItems = 5;
// positive and first position
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'positive');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'positive');
// negative and first position
value = -1;
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'min');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'min');
// negative but not first position
index = 1;
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'negative');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'negative');
// neutral
value = 0;
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'neutral');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'neutral');
// positive but not last position
value = 1;
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'positive');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'positive');
// positive and last position
index = 4;
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'max');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'max');
// negative and last position
value = -1;
- assert.equal(element._computeVoteAttribute(value, index,
- totalItems), 'negative');
+ assert.equal(
+ element._computeVoteAttribute(value, index, totalItems), 'negative');
});
test('correct item is selected', () => {
// 1 should be the value of the selected item
assert.strictEqual(element.$.labelSelector.selected, '+1');
assert.strictEqual(
- element.$.labelSelector.selectedItem
- .textContent.trim(), '+1');
- assert.strictEqual(
- element.$.selectedValueLabel.textContent.trim(), 'good');
+ element.$.labelSelector.selectedItem.textContent.trim(), '+1');
+ assert.strictEqual(element.$.selectedValueLabel.textContent.trim(), 'good');
checkAriaCheckedValid();
});
test('_computeLabelValue', () => {
- assert.strictEqual(element._computeLabelValue(element.labels,
- element.permittedLabels,
- element.label), '+1');
+ assert.strictEqual(
+ element._computeLabelValue(
+ element.labels, element.permittedLabels, element.label),
+ '+1');
});
test('_computeBlankItems', () => {
@@ -174,18 +171,21 @@
'2': 4,
};
- assert.strictEqual(element._computeBlankItems(element.permittedLabels,
- 'Code-Review').length, 0);
+ assert.strictEqual(
+ element._computeBlankItems(element.permittedLabels, 'Code-Review')
+ .length,
+ 0);
- assert.strictEqual(element._computeBlankItems(element.permittedLabels,
- 'Verified').length, 1);
+ assert.strictEqual(
+ element._computeBlankItems(element.permittedLabels, 'Verified').length,
+ 1);
});
test('labelValues returns no keys', () => {
element.labelValues = {};
- assert.deepEqual(element._computeBlankItems(element.permittedLabels,
- 'Code-Review'), []);
+ assert.deepEqual(
+ element._computeBlankItems(element.permittedLabels, 'Code-Review'), []);
});
test('changes in label score are reflected in the DOM', async () => {
@@ -259,11 +259,8 @@
],
};
await flush();
- assert.strictEqual(element.$.labelSelector
- .items.length, 2);
- assert.strictEqual(
- element.root.querySelectorAll('.placeholder').length,
- 3);
+ assert.strictEqual(element.$.labelSelector.items.length, 2);
+ assert.strictEqual(element.root.querySelectorAll('.placeholder').length, 3);
element.permittedLabels = {
'Code-Review': [
@@ -279,11 +276,8 @@
],
};
await flush();
- assert.strictEqual(element.$.labelSelector
- .items.length, 5);
- assert.strictEqual(
- element.root.querySelectorAll('.placeholder').length,
- 0);
+ assert.strictEqual(element.$.labelSelector.items.length, 5);
+ assert.strictEqual(element.root.querySelectorAll('.placeholder').length, 0);
});
test('default_value', () => {
@@ -343,4 +337,3 @@
assert.isNull(element.selectedValue);
});
});
-
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js
index 14ea4f59..486f37a 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.js
@@ -16,10 +16,11 @@
*/
import '../../../test/common-test-setup-karma.js';
-import {queryAndAssert, resetPlugins, stubRestApi} from '../../../test/test-utils.js';
import './gr-reply-dialog.js';
-import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader.js';
+
+import {queryAndAssert, resetPlugins, stubRestApi} from '../../../test/test-utils.js';
import {_testOnly_initGerritPluginApi} from '../../shared/gr-js-api-interface/gr-gerrit.js';
+import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader.js';
const basicFixture = fixtureFromElement('gr-reply-dialog');
const pluginApi = _testOnly_initGerritPluginApi();
@@ -89,14 +90,12 @@
const sendStub = sinon.stub(element, 'send').returns(Promise.resolve());
element.$.ccs.$.entry.setText('test');
- MockInteractions.tap(element.shadowRoot
- .querySelector('gr-button.send'));
+ MockInteractions.tap(element.shadowRoot.querySelector('gr-button.send'));
assert.isFalse(sendStub.called);
flush();
element.$.ccs.$.entry.setText('test@test.test');
- MockInteractions.tap(element.shadowRoot
- .querySelector('gr-button.send'));
+ MockInteractions.tap(element.shadowRoot.querySelector('gr-button.send'));
assert.isTrue(sendStub.called);
});
@@ -107,9 +106,7 @@
replyApi.addReplyTextChangedCallback(text => {
const label = 'Code-Review';
const labelValue = replyApi.getLabelValue(label);
- if (labelValue &&
- labelValue === ' 0' &&
- text.indexOf('LGTM') === 0) {
+ if (labelValue && labelValue === ' 0' && text.indexOf('LGTM') === 0) {
replyApi.setLabelValue(label, '+1');
}
});
@@ -121,13 +118,13 @@
await flush();
const textarea = queryAndAssert(element, 'gr-textarea').getNativeTextarea();
textarea.value = 'LGTM';
- textarea.dispatchEvent(new CustomEvent(
- 'input', {bubbles: true, composed: true}));
+ textarea.dispatchEvent(
+ new CustomEvent('input', {bubbles: true, composed: true}));
await flush();
- const labelScoreRows = element.getLabelScores().shadowRoot
- .querySelector('gr-label-score-row[name="Code-Review"]');
- const selectedBtn = labelScoreRows.shadowRoot
- .querySelector('gr-button[data-value="+1"]');
+ const labelScoreRows = element.getLabelScores().shadowRoot.querySelector(
+ 'gr-label-score-row[name="Code-Review"]');
+ const selectedBtn =
+ labelScoreRows.shadowRoot.querySelector('gr-button[data-value="+1"]');
assert.isOk(selectedBtn);
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index be57ef6..b8932e5 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -98,9 +98,9 @@
import {GrAccountChip} from '../../shared/gr-account-chip/gr-account-chip';
import {GrOverlay} from '../../shared/gr-overlay/gr-overlay';
import {
- CODE_REVIEW,
getApprovalInfo,
getMaxAccounts,
+ StandardLabels,
} from '../../../utils/label-util';
import {pluralize} from '../../../utils/string-util';
import {
@@ -984,7 +984,7 @@
}
_computeCommentAccounts(threads: CommentThread[]) {
- const crLabel = this.change?.labels?.[CODE_REVIEW];
+ const crLabel = this.change?.labels?.[StandardLabels.CODE_REVIEW];
const maxCrVoteAccountIds = getMaxAccounts(crLabel).map(a => a._account_id);
const accountIds = new Set<AccountId>();
threads.forEach(thread => {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 91b8603..5a11f47 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -32,7 +32,7 @@
import {addListenerForTest} from '../../../test/test-utils';
import {stubRestApi} from '../../../test/test-utils';
import {JSON_PREFIX} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
-import {CODE_REVIEW} from '../../../utils/label-util';
+import {StandardLabels} from '../../../utils/label-util';
import {
createAccountWithId,
createChange,
@@ -2116,9 +2116,9 @@
test('_computeSendButtonDisabled_existingVote', async () => {
const account = createAccountWithId();
- (element.change!.labels![CODE_REVIEW]! as DetailedLabelInfo).all = [
- account,
- ];
+ (
+ element.change!.labels![StandardLabels.CODE_REVIEW]! as DetailedLabelInfo
+ ).all = [account];
await flush();
// User has already voted.
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
index e302b43..c493be4 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
@@ -36,6 +36,7 @@
hasNeutralStatus,
hasVotes,
iconForStatus,
+ orderSubmitRequirements,
} from '../../../utils/label-util';
import {fontStyles} from '../../../styles/gr-font-styles';
import {charsOnly, pluralize} from '../../../utils/string-util';
@@ -77,7 +78,8 @@
width: var(--line-height-normal, 20px);
height: var(--line-height-normal, 20px);
}
- iron-icon.check {
+ iron-icon.check,
+ iron-icon.overridden {
color: var(--success-foreground);
}
iron-icon.close {
@@ -130,9 +132,10 @@
}
override render() {
- const submit_requirements = (this.change?.submit_requirements ?? []).filter(
- req => req.status !== SubmitRequirementStatus.NOT_APPLICABLE
- );
+ const submit_requirements = orderSubmitRequirements(
+ this.change?.submit_requirements ?? []
+ ).filter(req => req.status !== SubmitRequirementStatus.NOT_APPLICABLE);
+
return html` <h3
class="metadata-title heading-3"
id="submit-requirements-caption"
diff --git a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
index da1a782..1a6239f 100644
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
@@ -160,6 +160,8 @@
<g id="feedback"><path d="M0 0h24v24H0z" fill="none"/><path d="M20 2H4c-1.1 0-1.99.9-1.99 2L2 22l4-4h14c1.1 0 2-.9 2-2V4c0-1.1-.9-2-2-2zm-7 12h-2v-2h2v2zm0-4h-2V6h2v4z"/></g>
<!-- This SVG is a copy from material.io https://fonts.google.com/icons?selected=Material+Icons&icon.query=description -->
<g id="description"><path xmlns="http://www.w3.org/2000/svg" d="M0 0h24v24H0V0z" fill="none"/><path xmlns="http://www.w3.org/2000/svg" d="M8 16h8v2H8zm0-4h8v2H8zm6-10H6c-1.1 0-2 .9-2 2v16c0 1.1.89 2 1.99 2H18c1.1 0 2-.9 2-2V8l-6-6zm4 18H6V4h7v5h5v11z"/></g>
+ <!-- This SVG is a copy from material.io https://fonts.google.com/icons?selected=Material+Icons&icon.query=settings_backup_restore and 0.65 scale and 4 translate https://fonts.google.com/icons?selected=Material+Icons&icon.query=done-->
+ <g id="overridden"><path xmlns="http://www.w3.org/2000/svg" d="M0 0h24v24H0V0z" fill="none"/><path xmlns="http://www.w3.org/2000/svg" d="M12 15 zM2 4v6h6V8H5.09C6.47 5.61 9.04 4 12 4c4.42 0 8 3.58 8 8s-3.58 8-8 8-8-3.58-8-8H2c0 5.52 4.48 10 10.01 10C17.53 22 22 17.52 22 12S17.53 2 12.01 2C8.73 2 5.83 3.58 4 6.01V4H2z"/><path xmlns="http://www.w3.org/2000/svg" d="M9.85 14.53 7.12 11.8l-.91.91L9.85 16.35 17.65 8.55l-.91-.91L9.85 14.53z"/></g>
</defs>
</svg>
</iron-iconset-svg>`;
diff --git a/polygerrit-ui/app/utils/label-util.ts b/polygerrit-ui/app/utils/label-util.ts
index 918d2ab..c50bbe2 100644
--- a/polygerrit-ui/app/utils/label-util.ts
+++ b/polygerrit-ui/app/utils/label-util.ts
@@ -31,7 +31,11 @@
import {assertNever, unique} from './common-util';
// Name of the standard Code-Review label.
-export const CODE_REVIEW = 'Code-Review';
+export enum StandardLabels {
+ CODE_REVIEW = 'Code-Review',
+ CODE_OWNERS = 'Code Owners',
+ PRESUBMIT_VERIFIED = 'Presubmit-Verified',
+}
export enum LabelStatus {
APPROVED = 'APPROVED',
@@ -178,9 +182,13 @@
}
export function labelCompare(labelName1: string, labelName2: string) {
- if (labelName1 === CODE_REVIEW && labelName2 === CODE_REVIEW) return 0;
- if (labelName1 === CODE_REVIEW) return -1;
- if (labelName2 === CODE_REVIEW) return 1;
+ if (
+ labelName1 === StandardLabels.CODE_REVIEW &&
+ labelName2 === StandardLabels.CODE_REVIEW
+ )
+ return 0;
+ if (labelName1 === StandardLabels.CODE_REVIEW) return -1;
+ if (labelName2 === StandardLabels.CODE_REVIEW) return 1;
return labelName1.localeCompare(labelName2);
}
@@ -189,7 +197,7 @@
labels: LabelNameToInfoMap
): LabelInfo | undefined {
for (const label of Object.keys(labels)) {
- if (label === CODE_REVIEW) {
+ if (label === StandardLabels.CODE_REVIEW) {
return labels[label];
}
}
@@ -219,10 +227,31 @@
case SubmitRequirementStatus.UNSATISFIED:
return 'close';
case SubmitRequirementStatus.OVERRIDDEN:
- return 'warning';
+ return 'overridden';
case SubmitRequirementStatus.NOT_APPLICABLE:
return 'info';
default:
assertNever(status, `Unsupported status: ${status}`);
}
}
+
+// TODO(milutin): This may be temporary for demo purposes
+const PRIORITY_REQUIREMENTS_ORDER: string[] = [
+ StandardLabels.CODE_REVIEW,
+ StandardLabels.CODE_OWNERS,
+ StandardLabels.PRESUBMIT_VERIFIED,
+];
+export function orderSubmitRequirements(
+ requirements: SubmitRequirementResultInfo[]
+) {
+ let priorityRequirementList: SubmitRequirementResultInfo[] = [];
+ for (const label of PRIORITY_REQUIREMENTS_ORDER) {
+ const priorityRequirement = requirements.filter(r => r.name === label);
+ priorityRequirementList =
+ priorityRequirementList.concat(priorityRequirement);
+ }
+ const nonPriorityRequirements = requirements.filter(
+ r => !PRIORITY_REQUIREMENTS_ORDER.includes(r.name)
+ );
+ return priorityRequirementList.concat(nonPriorityRequirements);
+}
