Merge "Submit Requirements - overridden icon"
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index 3977278..cc8d813 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -136,6 +136,19 @@
+
Changes originally submitted by a user in 'GROUP'.
+[[uploader]]
+uploader:'USER'::
++
+Changes where the latest patch set was uploaded by 'USER'.
+The special case of `uploader:self` will find changes uploaded
+by the caller.
+
+[[uploaderin]]
+uploaderin:'GROUP'::
++
+Changes where the latest patch set was uploaded by a user in
+'GROUP'.
+
[[query]]
query:'[name=]NAME[,user=USER]'::
+
@@ -466,6 +479,12 @@
True on any change where the current user is the change owner.
Same as `owner:self`.
+is:uploader::
++
+True on any change where the current user is the uploader of
+the latest patch set.
+Same as `uploader:self`.
+
is:reviewer::
+
True on any change where the current user is a reviewer.
diff --git a/java/com/google/gerrit/entities/SubmitRequirementResult.java b/java/com/google/gerrit/entities/SubmitRequirementResult.java
index b7fa398..13625c1 100644
--- a/java/com/google/gerrit/entities/SubmitRequirementResult.java
+++ b/java/com/google/gerrit/entities/SubmitRequirementResult.java
@@ -44,8 +44,14 @@
/**
* Whether this result was created from a legacy {@link SubmitRecord}, or by evaluating a {@link
* SubmitRequirement}.
+ *
+ * <p>If equals {@link Optional#empty()}, we treat the result as non-legacy (false).
*/
- public abstract boolean legacy();
+ public abstract Optional<Boolean> legacy();
+
+ public boolean isLegacy() {
+ return legacy().orElse(false);
+ }
@Memoized
public Status status() {
@@ -122,7 +128,7 @@
public abstract Builder patchSetCommitId(ObjectId value);
- public abstract Builder legacy(boolean value);
+ public abstract Builder legacy(Optional<Boolean> value);
public abstract SubmitRequirementResult build();
}
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/change/SubmitRequirementsJson.java b/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
index ebb0790..8eeec62 100644
--- a/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
+++ b/java/com/google/gerrit/server/change/SubmitRequirementsJson.java
@@ -47,7 +47,7 @@
submitRequirementExpressionToInfo(
req.submittabilityExpression(), result.submittabilityExpressionResult());
info.status = SubmitRequirementResultInfo.Status.valueOf(result.status().toString());
- info.isLegacy = result.legacy();
+ info.isLegacy = result.isLegacy();
return info;
}
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/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index bfe1ee1..b9569e4 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -294,6 +294,10 @@
public static final FieldDef<ChangeData, Integer> OWNER =
integer(ChangeQueryBuilder.FIELD_OWNER).build(changeGetter(c -> c.getOwner().get()));
+ /** Uploader of the latest patch set. */
+ public static final FieldDef<ChangeData, Integer> UPLOADER =
+ integer(ChangeQueryBuilder.FIELD_UPLOADER).build(cd -> cd.currentPatchSet().uploader().get());
+
/** References the source change number that this change was cherry-picked from. */
public static final FieldDef<ChangeData, Integer> CHERRY_PICK_OF_CHANGE =
integer(ChangeQueryBuilder.FIELD_CHERRY_PICK_OF_CHANGE)
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 30ab6e6a..9339d62 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -178,9 +178,14 @@
new Schema.Builder<ChangeData>().add(V68).add(ChangeField.CHERRY_PICK).build();
/** Added new field {@link ChangeField#ATTENTION_SET_USERS_COUNT}. */
+ @Deprecated
static final Schema<ChangeData> V70 =
new Schema.Builder<ChangeData>().add(V69).add(ChangeField.ATTENTION_SET_USERS_COUNT).build();
+ /** Added new field {@link ChangeField#UPLOADER}. */
+ static final Schema<ChangeData> V71 =
+ new Schema.Builder<ChangeData>().add(V70).add(ChangeField.UPLOADER).build();
+
/**
* Name of the change index to be used when contacting index backends or loading configurations.
*/
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/notedb/SubmitRequirementProtoConverter.java b/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java
index 9bf56d8..3caa4d4 100644
--- a/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java
+++ b/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java
@@ -34,14 +34,18 @@
SubmitRequirementResultProto.getDescriptor().findFieldByNumber(2);
private static final FieldDescriptor SR_OVERRIDE_EXPR_RESULT_FIELD =
SubmitRequirementResultProto.getDescriptor().findFieldByNumber(4);
+ private static final FieldDescriptor SR_LEGACY_FIELD =
+ SubmitRequirementResultProto.getDescriptor().findFieldByNumber(6);
@Override
public SubmitRequirementResultProto toProto(SubmitRequirementResult r) {
SubmitRequirementResultProto.Builder builder = SubmitRequirementResultProto.newBuilder();
builder
.setSubmitRequirement(SubmitRequirementSerializer.serialize(r.submitRequirement()))
- .setLegacy(r.legacy())
.setCommit(ObjectIdConverter.create().toByteString(r.patchSetCommitId()));
+ if (r.legacy().isPresent()) {
+ builder.setLegacy(r.legacy().get());
+ }
if (r.applicabilityExpressionResult().isPresent()) {
builder.setApplicabilityExpressionResult(
SubmitRequirementExpressionResultSerializer.serialize(
@@ -61,10 +65,12 @@
public SubmitRequirementResult fromProto(SubmitRequirementResultProto proto) {
SubmitRequirementResult.Builder builder =
SubmitRequirementResult.builder()
- .legacy(proto.getLegacy())
.patchSetCommitId(ObjectIdConverter.create().fromByteString(proto.getCommit()))
.submitRequirement(
SubmitRequirementSerializer.deserialize(proto.getSubmitRequirement()));
+ if (proto.hasField(SR_LEGACY_FIELD)) {
+ builder.legacy(Optional.of(proto.getLegacy()));
+ }
if (proto.hasField(SR_APPLICABILITY_EXPR_RESULT_FIELD)) {
builder.applicabilityExpressionResult(
Optional.of(
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 513aeed..975ad52 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -125,7 +125,6 @@
public static final String KEY_BRANCH = "branch";
public static final String SUBMIT_REQUIREMENT = "submit-requirement";
- public static final String KEY_SR_NAME = "name";
public static final String KEY_SR_DESCRIPTION = "description";
public static final String KEY_SR_APPLICABILITY_EXPRESSION = "applicableIf";
public static final String KEY_SR_SUBMITTABILITY_EXPRESSION = "submittableIf";
@@ -643,7 +642,7 @@
if (rc.getStringList(ACCESS, null, KEY_INHERIT_FROM).length > 1) {
// The config must not contain more than one parent to inherit from
// as there is no guarantee which of the parents would be used then.
- error(ValidationError.create(PROJECT_CONFIG, "Cannot inherit from multiple projects"));
+ error("Cannot inherit from multiple projects");
}
p.setParent(rc.getString(ACCESS, null, KEY_INHERIT_FROM));
@@ -696,10 +695,8 @@
String lower = name.toLowerCase();
if (lowerNames.containsKey(lower)) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Extension Panels \"%s\" conflicts with \"%s\"", name, lowerNames.get(lower))));
+ String.format(
+ "Extension Panels \"%s\" conflicts with \"%s\"", name, lowerNames.get(lower)));
}
lowerNames.put(lower, name);
extensionPanelSections.put(
@@ -725,26 +722,14 @@
ca.setAutoVerify(null);
} else if (rules.size() > 1) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- "Invalid rule in "
- + CONTRIBUTOR_AGREEMENT
- + "."
- + name
- + "."
- + KEY_AUTO_VERIFY
- + ": at most one group may be set"));
+ String.format(
+ "Invalid rule in %s.%s.%s: at most one group may be set",
+ CONTRIBUTOR_AGREEMENT, name, KEY_AUTO_VERIFY));
} else if (rules.get(0).getAction() != Action.ALLOW) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- "Invalid rule in "
- + CONTRIBUTOR_AGREEMENT
- + "."
- + name
- + "."
- + KEY_AUTO_VERIFY
- + ": the group must be allowed"));
+ String.format(
+ "Invalid rule in %s.%s.%s: the group must be allowed",
+ CONTRIBUTOR_AGREEMENT, name, KEY_AUTO_VERIFY));
} else {
ca.setAutoVerify(rules.get(0).getGroup());
}
@@ -792,21 +777,16 @@
if (ref.getUUID() != null) {
n.addGroup(ref);
} else {
- error(
- ValidationError.create(
- PROJECT_CONFIG,
- "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME));
+ error(String.format("group \"%s\" not in %s", ref.getName(), GroupList.FILE_NAME));
}
} else if (dst.startsWith("user ")) {
- error(ValidationError.create(PROJECT_CONFIG, dst + " not supported"));
+ error(String.format("%s not supported", dst));
} else {
try {
n.addAddress(Address.parse(dst));
} catch (IllegalArgumentException err) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- "notify section \"" + sectionName + "\" has invalid email \"" + dst + "\""));
+ String.format("notify section \"%s\" has invalid email \"%s\"", sectionName, dst));
}
}
}
@@ -867,7 +847,7 @@
try {
RefPattern.validateRegExp(refPattern);
} catch (InvalidNameException e) {
- error(ValidationError.create(PROJECT_CONFIG, "Invalid ref name: " + e.getMessage()));
+ error(String.format("Invalid ref name: %s", e.getMessage()));
return false;
}
return true;
@@ -895,9 +875,7 @@
// to fail fast if any of the patterns are invalid.
patterns.add(Pattern.compile(patternString).pattern());
} catch (PatternSyntaxException e) {
- error(
- ValidationError.create(
- PROJECT_CONFIG, "Invalid regular expression: " + e.getMessage()));
+ error(String.format("Invalid regular expression: %s", e.getMessage()));
continue;
}
}
@@ -924,15 +902,11 @@
rule = PermissionRule.fromString(ruleString, useRange);
} catch (IllegalArgumentException notRule) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- "Invalid rule in "
- + section
- + (subsection != null ? "." + subsection : "")
- + "."
- + varName
- + ": "
- + notRule.getMessage()));
+ String.format(
+ "Invalid rule in %s.%s: %s",
+ section + (subsection != null ? "." + subsection : ""),
+ varName,
+ notRule.getMessage()));
continue;
}
@@ -943,9 +917,7 @@
// all rules in the same file share the same GroupReference.
//
ref = groupList.resolve(rule.getGroup());
- error(
- ValidationError.create(
- PROJECT_CONFIG, "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME));
+ error(String.format("group \"%s\" not in %s", ref.getName(), GroupList.FILE_NAME));
}
perm.add(rule.toBuilder().setGroup(ref));
@@ -970,11 +942,9 @@
String lower = name.toLowerCase();
if (lowerNames.containsKey(lower)) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Submit requirement \"%s\" conflicts with \"%s\". Skipping the former.",
- name, lowerNames.get(lower))));
+ String.format(
+ "Submit requirement \"%s\" conflicts with \"%s\". Skipping the former.",
+ name, lowerNames.get(lower)));
continue;
}
lowerNames.put(lower, name);
@@ -987,12 +957,10 @@
if (blockExpr == null) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- (String.format(
- "Submit requirement \"%s\" does not define a submittability expression."
- + " Skipping this requirement.",
- name))));
+ String.format(
+ "Submit requirement \"%s\" does not define a submittability expression."
+ + " Skipping this requirement.",
+ name));
continue;
}
@@ -1019,10 +987,7 @@
for (String name : rc.getSubsections(LABEL)) {
String lower = name.toLowerCase();
if (lowerNames.containsKey(lower)) {
- error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format("Label \"%s\" conflicts with \"%s\"", name, lowerNames.get(lower))));
+ error(String.format("Label \"%s\" conflicts with \"%s\"", name, lowerNames.get(lower)));
}
lowerNames.put(lower, name);
@@ -1034,18 +999,13 @@
if (allValues.add(labelValue.getValue())) {
values.add(labelValue);
} else {
- error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format("Duplicate %s \"%s\" for label \"%s\"", KEY_VALUE, value, name)));
+ error(String.format("Duplicate %s \"%s\" for label \"%s\"", KEY_VALUE, value, name));
}
} catch (IllegalArgumentException notValue) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Invalid %s \"%s\" for label \"%s\": %s",
- KEY_VALUE, value, name, notValue.getMessage())));
+ String.format(
+ "Invalid %s \"%s\" for label \"%s\": %s",
+ KEY_VALUE, value, name, notValue.getMessage()));
}
}
@@ -1053,7 +1013,7 @@
try {
label = LabelType.builder(name, values);
} catch (IllegalArgumentException badName) {
- error(ValidationError.create(PROJECT_CONFIG, String.format("Invalid label \"%s\"", name)));
+ error(String.format("Invalid label \"%s\"", name));
continue;
}
@@ -1064,11 +1024,9 @@
: Optional.of(LabelFunction.MAX_WITH_BLOCK);
if (!function.isPresent()) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Invalid %s for label \"%s\". Valid names are: %s",
- KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet()))));
+ String.format(
+ "Invalid %s for label \"%s\". Valid names are: %s",
+ KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet())));
}
label.setFunction(function.orElse(null));
label.setCopyCondition(rc.getString(LABEL, name, KEY_COPY_CONDITION));
@@ -1078,11 +1036,7 @@
if (isInRange(dv, values)) {
label.setDefaultValue(dv);
} else {
- error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Invalid %s \"%s\" for label \"%s\"", KEY_DEFAULT_VALUE, dv, name)));
+ error(String.format("Invalid %s \"%s\" for label \"%s\"", KEY_DEFAULT_VALUE, dv, name));
}
}
label.setAllowPostSubmit(
@@ -1131,18 +1085,13 @@
short copyValue = Shorts.checkedCast(PermissionRule.parseInt(value));
if (!copyValues.add(copyValue)) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Duplicate %s \"%s\" for label \"%s\"", KEY_COPY_VALUE, value, name)));
+ String.format("Duplicate %s \"%s\" for label \"%s\"", KEY_COPY_VALUE, value, name));
}
} catch (IllegalArgumentException notValue) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Invalid %s \"%s\" for label \"%s\": %s",
- KEY_COPY_VALUE, value, name, notValue.getMessage())));
+ String.format(
+ "Invalid %s \"%s\" for label \"%s\": %s",
+ KEY_COPY_VALUE, value, name, notValue.getMessage()));
}
}
label.setCopyValues(copyValues);
@@ -1177,18 +1126,14 @@
commentLinkSections.put(name, buildCommentLink(rc, name, false));
} catch (PatternSyntaxException e) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Invalid pattern \"%s\" in commentlink.%s.match: %s",
- rc.getString(COMMENTLINK, name, KEY_MATCH), name, e.getMessage())));
+ String.format(
+ "Invalid pattern \"%s\" in commentlink.%s.match: %s",
+ rc.getString(COMMENTLINK, name, KEY_MATCH), name, e.getMessage()));
} catch (IllegalArgumentException e) {
error(
- ValidationError.create(
- PROJECT_CONFIG,
- String.format(
- "Error in pattern \"%s\" in commentlink.%s.match: %s",
- rc.getString(COMMENTLINK, name, KEY_MATCH), name, e.getMessage())));
+ String.format(
+ "Error in pattern \"%s\" in commentlink.%s.match: %s",
+ rc.getString(COMMENTLINK, name, KEY_MATCH), name, e.getMessage()));
}
}
}
@@ -1230,9 +1175,7 @@
if (groupName != null) {
GroupReference ref = groupList.byName(groupName);
if (ref == null) {
- error(
- ValidationError.create(
- PROJECT_CONFIG, "group \"" + groupName + "\" not in " + GroupList.FILE_NAME));
+ error(String.format("group \"%s\" not in %s", groupName, GroupList.FILE_NAME));
}
rc.setString(PLUGIN, plugin, name, value);
}
@@ -1782,11 +1725,15 @@
try {
return rc.getEnum(section, subsection, name, defaultValue);
} catch (IllegalArgumentException err) {
- error(ValidationError.create(PROJECT_CONFIG, err.getMessage()));
+ error(err.getMessage());
return defaultValue;
}
}
+ private void error(String errorMessage) {
+ error(ValidationError.create(PROJECT_CONFIG, errorMessage));
+ }
+
@Override
public void error(ValidationError error) {
if (validationErrors == null) {
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
index 7252fe9..539edc1 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
@@ -29,6 +29,7 @@
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
@@ -88,7 +89,7 @@
.setAllowOverrideInChildProjects(labelType.isCanOverride());
result.add(
SubmitRequirementResult.builder()
- .legacy(true)
+ .legacy(Optional.of(true))
.submitRequirement(req.build())
.submittabilityExpressionResult(
createExpressionResult(toExpression(atoms), mapStatus(label), atoms))
@@ -111,7 +112,7 @@
.build();
return ImmutableList.of(
SubmitRequirementResult.builder()
- .legacy(true)
+ .legacy(Optional.of(true))
.submitRequirement(sr)
.submittabilityExpressionResult(
createExpressionResult(
@@ -130,7 +131,7 @@
.build();
result.add(
SubmitRequirementResult.builder()
- .legacy(true)
+ .legacy(Optional.of(true))
.submitRequirement(sr)
.submittabilityExpressionResult(
createExpressionResult(
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index 2caa9dd..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 =
@@ -108,7 +100,7 @@
: Optional.empty();
return SubmitRequirementResult.builder()
- .legacy(false)
+ .legacy(Optional.of(false))
.submitRequirement(sr)
.patchSetCommitId(cd.currentPatchSet().commitId())
.submittabilityExpressionResult(blockingResult)
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 b8c8c07..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();
@@ -962,14 +966,8 @@
// Closed changes: Load submit requirement results from NoteDb.
Map<SubmitRequirement, SubmitRequirementResult> projectConfigRequirements =
notes().getSubmitRequirementsResult().stream()
- .filter(r -> !r.legacy())
+ .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/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 4e638df..043b37d 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -125,6 +125,14 @@
}
/**
+ * Returns a predicate that matches changes where the latest patch set was uploaded by the
+ * provided {@link com.google.gerrit.entities.Account.Id}.
+ */
+ public static Predicate<ChangeData> uploader(Account.Id id) {
+ return new ChangeIndexPredicate(ChangeField.UPLOADER, id.toString());
+ }
+
+ /**
* Returns a predicate that matches changes that are a cherry pick of the provided {@link
* com.google.gerrit.entities.Change.Id}.
*/
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index f1fe520..da36633 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -195,6 +195,8 @@
public static final String FIELD_SUBMISSIONID = "submissionid";
public static final String FIELD_TR = "tr";
public static final String FIELD_UNRESOLVED_COMMENT_COUNT = "unresolved";
+ public static final String FIELD_UPLOADER = "uploader";
+ public static final String FIELD_UPLOADERIN = "uploaderin";
public static final String FIELD_VISIBLETO = "visibleto";
public static final String FIELD_WATCHEDBY = "watchedby";
public static final String FIELD_WIP = "wip";
@@ -661,6 +663,14 @@
return ChangePredicates.owner(self());
}
+ if ("uploader".equalsIgnoreCase(value)) {
+ if (!args.getSchema().hasField(ChangeField.UPLOADER)) {
+ throw new QueryParseException(
+ "'is:uploader' operator is not supported by change index version");
+ }
+ return ChangePredicates.uploader(self());
+ }
+
if ("reviewer".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.WIP)) {
return Predicate.and(
@@ -1163,6 +1173,23 @@
}
@Operator
+ public Predicate<ChangeData> uploader(String who)
+ throws QueryParseException, IOException, ConfigInvalidException {
+ if (!args.getSchema().hasField(ChangeField.UPLOADER)) {
+ throw new QueryParseException("'uploader' operator is not supported by change index version");
+ }
+ return uploader(parseAccount(who, (AccountState s) -> true));
+ }
+
+ private Predicate<ChangeData> uploader(Set<Account.Id> who) {
+ List<Predicate<ChangeData>> p = Lists.newArrayListWithCapacity(who.size());
+ for (Account.Id id : who) {
+ p.add(ChangePredicates.uploader(id));
+ }
+ return Predicate.or(p);
+ }
+
+ @Operator
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
if (!args.index.getSchema().hasField(ChangeField.ATTENTION_SET_USERS)) {
@@ -1212,6 +1239,31 @@
}
@Operator
+ public Predicate<ChangeData> uploaderin(String group) throws QueryParseException, IOException {
+ if (!args.getSchema().hasField(ChangeField.UPLOADER)) {
+ throw new QueryParseException("'uploader' operator is not supported by change index version");
+ }
+
+ GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
+ if (g == null) {
+ throw error("Group " + group + " not found");
+ }
+
+ AccountGroup.UUID groupId = g.getUUID();
+ GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
+ if (!(groupDescription instanceof GroupDescription.Internal)) {
+ return new UploaderinPredicate(args.userFactory, groupId);
+ }
+
+ Set<Account.Id> accounts = getMembers(groupId);
+ List<Predicate<ChangeData>> p = Lists.newArrayListWithCapacity(accounts.size());
+ for (Account.Id id : accounts) {
+ p.add(ChangePredicates.uploader(id));
+ }
+ return Predicate.or(p);
+ }
+
+ @Operator
public Predicate<ChangeData> r(String who)
throws QueryParseException, IOException, ConfigInvalidException {
return reviewer(who);
diff --git a/java/com/google/gerrit/server/query/change/UploaderinPredicate.java b/java/com/google/gerrit/server/query/change/UploaderinPredicate.java
new file mode 100644
index 0000000..f44c0b5
--- /dev/null
+++ b/java/com/google/gerrit/server/query/change/UploaderinPredicate.java
@@ -0,0 +1,50 @@
+// 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.server.query.change;
+
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.index.query.PostFilterPredicate;
+import com.google.gerrit.server.IdentifiedUser;
+
+/**
+ * Predicate that matches changes where the latest patch set was uploaded by a user in the provided
+ * group.
+ */
+public class UploaderinPredicate extends PostFilterPredicate<ChangeData> {
+ protected final IdentifiedUser.GenericFactory userFactory;
+ protected final AccountGroup.UUID uuid;
+
+ public UploaderinPredicate(IdentifiedUser.GenericFactory userFactory, AccountGroup.UUID uuid) {
+ super(ChangeQueryBuilder.FIELD_UPLOADERIN, uuid.get());
+ this.userFactory = userFactory;
+ this.uuid = uuid;
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ PatchSet latestPatchSet = cd.currentPatchSet();
+ if (latestPatchSet == null) {
+ return false;
+ }
+ IdentifiedUser uploader = userFactory.create(latestPatchSet.uploader());
+ return uploader.getEffectiveGroups().contains(uuid);
+ }
+
+ @Override
+ public int getCost() {
+ return 2;
+ }
+}
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/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index 52eaf11..61002f9 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -687,7 +687,7 @@
.submitRequirementsResult(
ImmutableList.of(
SubmitRequirementResult.builder()
- .legacy(true)
+ .legacy(Optional.of(true))
.patchSetCommitId(
ObjectId.fromString("26e50c7d315a33a13e5cc00902781fa876bc36cd"))
.submitRequirement(
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 2c6dd66..332548a 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -601,6 +601,29 @@
}
@Test
+ public void byUploader() throws Exception {
+ assume().that(getSchema().hasField(ChangeField.UPLOADER)).isTrue();
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+ CurrentUser user2CurrentUser = userFactory.create(user2);
+
+ TestRepository<Repo> repo = createProject("repo");
+ Change change1 = insert(repo, newChange(repo), userId);
+ assertQuery("is:uploader", change1);
+ assertQuery("uploader:" + userId.get(), change1);
+ change1 = newPatchSet(repo, change1, user2CurrentUser);
+ // Uploader has changed
+ assertQuery("uploader:" + userId.get());
+ assertQuery("uploader:" + user2.get(), change1);
+
+ requestContext.setContext(newRequestContext(user2));
+ assertQuery("is:uploader", change1); // self (user2)
+
+ String nameEmail = user2CurrentUser.asIdentifiedUser().getNameEmail();
+ assertQuery("uploader: \"" + nameEmail + "\"", change1);
+ }
+
+ @Test
public void byAuthorExact() throws Exception {
assume().that(getSchema().hasField(ChangeField.EXACT_AUTHOR)).isTrue();
byAuthorOrCommitterExact("author:");
@@ -716,6 +739,20 @@
}
@Test
+ public void byUploaderIn() throws Exception {
+ assume().that(getSchema().hasField(ChangeField.UPLOADER)).isTrue();
+ TestRepository<Repo> repo = createProject("repo");
+ Change change1 = insert(repo, newChange(repo), userId);
+ assertQuery("uploaderin:Administrators", change1);
+
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+ CurrentUser user2CurrentUser = userFactory.create(user2);
+ newPatchSet(repo, change1, user2CurrentUser);
+ assertQuery("uploaderin:Administrators");
+ }
+
+ @Test
public void byProject() throws Exception {
TestRepository<Repo> repo1 = createProject("repo1");
TestRepository<Repo> repo2 = createProject("repo2");
@@ -2585,7 +2622,7 @@
gApi.changes().id(change2.getId().get()).current().review(new ReviewInput().message("comment"));
PatchSet.Id ps3_1 = change3.currentPatchSetId();
- change3 = newPatchSet(repo, change3);
+ change3 = newPatchSet(repo, change3, user);
assertThat(change3.currentPatchSetId()).isNotEqualTo(ps3_1);
// Response to previous patch set still counts as reviewing.
gApi.changes()
@@ -3891,7 +3928,8 @@
}
}
- protected Change newPatchSet(TestRepository<Repo> repo, Change c) throws Exception {
+ protected Change newPatchSet(TestRepository<Repo> repo, Change c, CurrentUser user)
+ throws Exception {
// Add a new file so the patch set is not a trivial rebase, to avoid default
// Code-Review label copying.
int n = c.currentPatchSetId().get() + 1;
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_html.ts b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.ts
index e21584e..e57a216 100644
--- a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row_html.ts
@@ -53,29 +53,31 @@
);
padding: 0 var(--spacing-m);
}
- gr-tooltip-content.iron-selected > gr-button[vote='max'] {
+ gr-button[vote='max'].iron-selected {
--button-background-color: var(--vote-color-approved);
}
- gr-tooltip-content.iron-selected > gr-button[vote='positive'] {
+ gr-button[vote='positive'].iron-selected {
--button-background-color: var(--vote-color-recommended);
}
- gr-tooltip-content.iron-selected > gr-button[vote='min'] {
+ gr-button[vote='min'].iron-selected {
--button-background-color: var(--vote-color-rejected);
}
- gr-tooltip-content.iron-selected > gr-button[vote='negative'] {
+ gr-button[vote='negative'].iron-selected {
--button-background-color: var(--vote-color-disliked);
}
- gr-tooltip-content.iron-selected > gr-button[vote='neutral'] {
+ gr-button[vote='neutral'].iron-selected {
--button-background-color: var(--vote-color-neutral);
}
- gr-tooltip-content.iron-selected
- > gr-button[vote='positive']::part(paper-button) {
+ gr-button[vote='positive'].iron-selected::part(paper-button) {
border-color: var(--vote-outline-recommended);
}
- gr-tooltip-content.iron-selected
- > gr-button[vote='negative']::part(paper-button) {
+ gr-button[vote='negative'].iron-selected::part(paper-button) {
border-color: var(--vote-outline-disliked);
}
+ gr-button > gr-tooltip-content {
+ margin: 0px -10px;
+ padding: 0px 10px;
+ }
.placeholder {
display: inline-block;
width: 42px;
@@ -118,20 +120,22 @@
aria-labelledby="labelName"
>
<template is="dom-repeat" items="[[_items]]" as="value">
- <gr-tooltip-content
- has-tooltip
+ <gr-button
+ role="radio"
+ vote$="[[_computeVoteAttribute(value, index, _items.length)]]"
title$="[[_computeLabelValueTitle(labels, label.name, value)]]"
data-name$="[[label.name]]"
data-value$="[[value]]"
+ aria-label$="[[value]]"
+ voteChip
>
- <gr-button
- role="radio"
- vote$="[[_computeVoteAttribute(value, index, _items.length)]]"
- voteChip
+ <gr-tooltip-content
+ has-tooltip
+ title$="[[_computeLabelValueTitle(labels, label.name, value)]]"
>
[[value]]
- </gr-button>
- </gr-tooltip-content>
+ </gr-tooltip-content>
+ </gr-button>
</template>
</iron-selector>
<template
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 34e959b..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,15 +101,12 @@
const labelsChangedHandler = sinon.stub();
element.addEventListener('labels-changed', labelsChangedHandler);
assert.ok(element.$.labelSelector);
- MockInteractions.tap(element.shadowRoot
- .querySelector(
- 'gr-tooltip-content[data-value="-1"] > gr-button'));
+ 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');
@@ -121,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', () => {
@@ -175,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 () => {
@@ -260,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': [
@@ -280,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', () => {
@@ -344,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 b8c9319..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-tooltip-content[data-value="+1"] > gr-button');
+ 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 17036e6..10e04f7 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
@@ -99,9 +99,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 {
@@ -986,7 +986,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 e57ffc7..c4bff57 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 78de838..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';
@@ -131,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-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 0d4ad8c..53b3bb9 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -682,7 +682,7 @@
* comment triggered a recomputation of comments and the text written by
* the user was lost.
*/
- if (!this._messageText) this._messageText = message || '';
+ if (!this._messageText || !this.editing) this._messageText = message || '';
}
_messageTextChanged(_: string, oldValue: string) {
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
index b963d4b..53e62ff 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
@@ -240,6 +240,29 @@
assert.equal(element._messageText, 'hello world');
});
+ test('comment message sets messageText when not edited', () => {
+ element.changeNum = 1 as NumericChangeId;
+ element.patchNum = 1 as PatchSetNum;
+ element._messageText = 'Some text';
+ element.comment = {
+ author: {
+ name: 'Mr. Peanutbutter',
+ email: 'tenn1sballchaser@aol.com' as EmailAddress,
+ },
+ line: 5,
+ path: 'test',
+ __editing: false,
+ __draft: true,
+ message: 'hello world',
+ };
+ // messageText was empty so overwrite the message now
+ assert.equal(element._messageText, 'hello world');
+
+ element.comment!.message = 'new message';
+ // messageText was already set so do not overwrite it
+ assert.equal(element._messageText, 'hello world');
+ });
+
test('_getPatchNum', () => {
element.side = 'PARENT';
element.patchNum = 1 as PatchSetNum;
diff --git a/polygerrit-ui/app/utils/label-util.ts b/polygerrit-ui/app/utils/label-util.ts
index 0e89500..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];
}
}
@@ -226,3 +234,24 @@
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);
+}
