Merge "Enable -Xlint:unchecked"
diff --git a/Documentation/index.txt b/Documentation/index.txt
index 9afd6e3..c3d79b1 100644
--- a/Documentation/index.txt
+++ b/Documentation/index.txt
@@ -15,6 +15,7 @@
== Contributor Guides
. link:dev-community.html[Gerrit Community]
. link:dev-community.html#how-to-contribute[How to Contribute]
+.. link:dev-readme.html[Developer Setup]
== User Guides
. link:intro-user.html[User Guide]
diff --git a/WORKSPACE b/WORKSPACE
index da15966..779237b 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -136,7 +136,7 @@
load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories", "yarn_install")
node_repositories(
- node_version = "16.16.0",
+ node_version = "17.9.1",
yarn_version = "1.22.19",
)
diff --git a/java/com/google/gerrit/index/query/IntegerRangePredicate.java b/java/com/google/gerrit/index/query/IntegerRangePredicate.java
index 850c4a5..278d2af 100644
--- a/java/com/google/gerrit/index/query/IntegerRangePredicate.java
+++ b/java/com/google/gerrit/index/query/IntegerRangePredicate.java
@@ -14,13 +14,13 @@
package com.google.gerrit.index.query;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.RangeUtil.Range;
public abstract class IntegerRangePredicate<T> extends IndexPredicate<T> {
private final Range range;
- protected IntegerRangePredicate(FieldDef<T, Integer> type, String value)
+ protected IntegerRangePredicate(SchemaField<T, Integer> type, String value)
throws QueryParseException {
super(type, value);
range = RangeUtil.getRange(value, Integer.MIN_VALUE, Integer.MAX_VALUE);
diff --git a/java/com/google/gerrit/index/query/InternalQuery.java b/java/com/google/gerrit/index/query/InternalQuery.java
index ef69b4c..b6418a9 100644
--- a/java/com/google/gerrit/index/query/InternalQuery.java
+++ b/java/com/google/gerrit/index/query/InternalQuery.java
@@ -22,11 +22,11 @@
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.Index;
import com.google.gerrit.index.IndexCollection;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;
@@ -77,10 +77,10 @@
}
@SafeVarargs
- public final Q setRequestedFields(FieldDef<T, ?>... fields) {
+ public final Q setRequestedFields(SchemaField<T, ?>... fields) {
checkArgument(fields.length > 0, "requested field list is empty");
queryProcessor.setRequestedFields(
- Arrays.stream(fields).map(FieldDef::getName).collect(toSet()));
+ Arrays.stream(fields).map(SchemaField::getName).collect(toSet()));
return self();
}
diff --git a/java/com/google/gerrit/index/query/TimestampRangePredicate.java b/java/com/google/gerrit/index/query/TimestampRangePredicate.java
index 29d6f22..1fd81a6 100644
--- a/java/com/google/gerrit/index/query/TimestampRangePredicate.java
+++ b/java/com/google/gerrit/index/query/TimestampRangePredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.index.query;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.json.JavaSqlTimestampHelper;
import java.sql.Timestamp;
import java.time.Instant;
@@ -30,7 +30,7 @@
}
}
- protected TimestampRangePredicate(FieldDef<I, Timestamp> def, String name, String value) {
+ protected TimestampRangePredicate(SchemaField<I, Timestamp> def, String name, String value) {
super(def, name, value);
}
diff --git a/java/com/google/gerrit/lucene/ChangeSubIndex.java b/java/com/google/gerrit/lucene/ChangeSubIndex.java
index ce50473..f7b1f2c 100644
--- a/java/com/google/gerrit/lucene/ChangeSubIndex.java
+++ b/java/com/google/gerrit/lucene/ChangeSubIndex.java
@@ -125,7 +125,7 @@
} else if (f == ChangeField.UPDATED) {
long t = ((Timestamp) getOnlyElement(values.getValues())).getTime();
doc.add(new NumericDocValuesField(UPDATED_SORT_FIELD, t));
- } else if (f == ChangeField.MERGED_ON) {
+ } else if (f == ChangeField.MERGED_ON_SPEC) {
long t = ((Timestamp) getOnlyElement(values.getValues())).getTime();
doc.add(new NumericDocValuesField(MERGED_ON_SORT_FIELD, t));
}
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 079380e..b85f40e 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -103,7 +103,7 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
static final String UPDATED_SORT_FIELD = sortFieldName(ChangeField.UPDATED);
- static final String MERGED_ON_SORT_FIELD = sortFieldName(ChangeField.MERGED_ON);
+ static final String MERGED_ON_SORT_FIELD = sortFieldName(ChangeField.MERGED_ON_SPEC);
static final String ID_STR_SORT_FIELD = sortFieldName(ChangeField.LEGACY_ID_STR);
private static final String CHANGES = "changes";
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 68d3a3e..6f7f452 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -203,18 +203,26 @@
/** When this change was merged, time since January 1, 1970. */
// TODO(issue-15518): Migrate type for timestamp index fields from Timestamp to Instant
- public static final FieldDef<ChangeData, Timestamp> MERGED_ON =
- timestamp(ChangeQueryBuilder.FIELD_MERGED_ON)
+ public static final IndexedField<ChangeData, Timestamp> MERGED_ON_FIELD =
+ IndexedField.<ChangeData>timestampBuilder("MergedOn")
.stored()
.build(
cd -> cd.getMergedOn().map(Timestamp::from).orElse(null),
(cd, field) -> cd.setMergedOn(field != null ? field.toInstant() : null));
+ public static final IndexedField<ChangeData, Timestamp>.SearchSpec MERGED_ON_SPEC =
+ MERGED_ON_FIELD.timestamp(ChangeQueryBuilder.FIELD_MERGED_ON);
+
/** List of full file paths modified in the current patch set. */
- public static final FieldDef<ChangeData, Iterable<String>> PATH =
+ public static final IndexedField<ChangeData, Iterable<String>> PATH_FIELD =
// Named for backwards compatibility.
- exact(ChangeQueryBuilder.FIELD_FILE)
- .buildRepeatable(cd -> firstNonNull(cd.currentFilePaths(), ImmutableList.of()));
+ IndexedField.<ChangeData>iterableStringBuilder("File")
+ .build(cd -> firstNonNull(cd.currentFilePaths(), ImmutableList.of()));
+
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec PATH_SPEC =
+ PATH_FIELD
+ // Named for backwards compatibility.
+ .exact(ChangeQueryBuilder.FIELD_FILE);
public static Set<String> getFileParts(ChangeData cd) {
List<String> paths = cd.currentFilePaths();
@@ -230,24 +238,27 @@
}
/** Hashtags tied to a change */
- public static final FieldDef<ChangeData, Iterable<String>> HASHTAG =
- exact(ChangeQueryBuilder.FIELD_HASHTAG)
- .buildRepeatable(cd -> cd.hashtags().stream().map(String::toLowerCase).collect(toSet()));
+ public static final IndexedField<ChangeData, Iterable<String>> HASHTAG_FIELD =
+ IndexedField.<ChangeData>iterableStringBuilder("Hashtag")
+ .size(200)
+ .build(cd -> cd.hashtags().stream().map(String::toLowerCase).collect(toSet()));
+
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec HASHTAG_SPEC =
+ HASHTAG_FIELD.exact(ChangeQueryBuilder.FIELD_HASHTAG);
/** Hashtags as fulltext field for in-string search. */
- public static final FieldDef<ChangeData, Iterable<String>> FUZZY_HASHTAG =
- fullText("hashtag2")
- .buildRepeatable(cd -> cd.hashtags().stream().map(String::toLowerCase).collect(toSet()));
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec FUZZY_HASHTAG =
+ HASHTAG_FIELD.fullText("hashtag2");
/** Hashtags as prefix field for in-string search. */
- public static final FieldDef<ChangeData, Iterable<String>> PREFIX_HASHTAG =
- prefix("hashtag3")
- .buildRepeatable(cd -> cd.hashtags().stream().map(String::toLowerCase).collect(toSet()));
+ public static final IndexedField<ChangeData, Iterable<String>>.SearchSpec PREFIX_HASHTAG =
+ HASHTAG_FIELD.prefix("hashtag3");
/** Hashtags with original case. */
- public static final FieldDef<ChangeData, Iterable<byte[]>> HASHTAG_CASE_AWARE =
- storedOnly("_hashtag")
- .buildRepeatable(
+ public static final IndexedField<ChangeData, Iterable<byte[]>> HASHTAG_CASE_AWARE_FIELD =
+ IndexedField.<ChangeData>iterableByteArrayBuilder("HashtagCaseAware")
+ .stored()
+ .build(
cd -> cd.hashtags().stream().map(t -> t.getBytes(UTF_8)).collect(toSet()),
(cd, field) ->
cd.setHashtags(
@@ -255,6 +266,9 @@
.map(f -> new String(f, UTF_8))
.collect(toImmutableSet())));
+ public static final IndexedField<ChangeData, Iterable<byte[]>>.SearchSpec
+ HASHTAG_CASE_AWARE_SPEC = HASHTAG_CASE_AWARE_FIELD.storedOnly("_hashtag");
+
/** Components of each file path modified in the current patch set. */
public static final FieldDef<ChangeData, Iterable<String>> FILE_PART =
exact(ChangeQueryBuilder.FIELD_FILEPART).buildRepeatable(ChangeField::getFileParts);
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 82f9b87..0878ffa5 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -17,7 +17,6 @@
import static com.google.gerrit.index.SchemaUtil.schema;
import com.google.common.collect.ImmutableList;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.IndexedField;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.SchemaDefinitions;
@@ -35,7 +34,7 @@
static final Schema<ChangeData> V74 =
schema(
/* version= */ 74,
- ImmutableList.<FieldDef<ChangeData, ?>>of(
+ ImmutableList.of(
ChangeField.ADDED,
ChangeField.APPROVAL,
ChangeField.ASSIGNEE,
@@ -63,10 +62,7 @@
ChangeField.EXTENSION,
ChangeField.FILE_PART,
ChangeField.FOOTER,
- ChangeField.FUZZY_HASHTAG,
ChangeField.GROUP,
- ChangeField.HASHTAG,
- ChangeField.HASHTAG_CASE_AWARE,
ChangeField.ID,
ChangeField.IS_PURE_REVERT,
ChangeField.IS_SUBMITTABLE,
@@ -74,11 +70,9 @@
ChangeField.LEGACY_ID_STR,
ChangeField.MERGE,
ChangeField.MERGEABLE,
- ChangeField.MERGED_ON,
ChangeField.ONLY_EXTENSIONS,
ChangeField.OWNER,
ChangeField.PATCH_SET,
- ChangeField.PATH,
ChangeField.PENDING_REVIEWER,
ChangeField.PENDING_REVIEWER_BY_EMAIL,
ChangeField.PRIVATE,
@@ -103,19 +97,28 @@
ChangeField.UPLOADER,
ChangeField.WIP),
ImmutableList.<IndexedField<ChangeData, ?>>of(
- ChangeField.SUBMISSIONID_FIELD,
- ChangeField.STATUS_FIELD,
+ ChangeField.HASHTAG_CASE_AWARE_FIELD,
+ ChangeField.HASHTAG_FIELD,
+ ChangeField.MERGED_ON_FIELD,
+ ChangeField.PATH_FIELD,
ChangeField.PROJECT_FIELD,
ChangeField.REF_FIELD,
+ ChangeField.STATUS_FIELD,
+ ChangeField.SUBMISSIONID_FIELD,
ChangeField.TOPIC_FIELD),
ImmutableList.<IndexedField<ChangeData, ?>.SearchSpec>of(
ChangeField.EXACT_TOPIC,
+ ChangeField.FUZZY_HASHTAG,
ChangeField.FUZZY_TOPIC,
- ChangeField.SUBMISSIONID_SPEC,
- ChangeField.STATUS_SPEC,
- ChangeField.PROJECT_SPEC,
+ ChangeField.HASHTAG_CASE_AWARE_SPEC,
+ ChangeField.HASHTAG_SPEC,
+ ChangeField.MERGED_ON_SPEC,
+ ChangeField.PATH_SPEC,
ChangeField.PROJECTS_SPEC,
- ChangeField.REF_SPEC));
+ ChangeField.PROJECT_SPEC,
+ ChangeField.REF_SPEC,
+ ChangeField.STATUS_SPEC,
+ ChangeField.SUBMISSIONID_SPEC));
/**
* Added new field {@link ChangeField#PREFIX_HASHTAG} and {@link ChangeField#PREFIX_TOPIC} to
@@ -125,7 +128,7 @@
static final Schema<ChangeData> V75 =
new Schema.Builder<ChangeData>()
.add(V74)
- .add(ChangeField.PREFIX_HASHTAG)
+ .addSearchSpecs(ChangeField.PREFIX_HASHTAG)
.addSearchSpecs(ChangeField.PREFIX_TOPIC)
.build();
diff --git a/java/com/google/gerrit/server/query/change/AfterPredicate.java b/java/com/google/gerrit/server/query/change/AfterPredicate.java
index 2514989..d3e3477 100644
--- a/java/com/google/gerrit/server/query/change/AfterPredicate.java
+++ b/java/com/google/gerrit/server/query/change/AfterPredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.QueryParseException;
import java.sql.Timestamp;
import java.time.Instant;
@@ -26,7 +26,7 @@
public class AfterPredicate extends TimestampRangeChangePredicate {
protected final Instant cut;
- public AfterPredicate(FieldDef<ChangeData, Timestamp> def, String name, String value)
+ public AfterPredicate(SchemaField<ChangeData, Timestamp> def, String name, String value)
throws QueryParseException {
super(def, name, value);
cut = parse(value);
diff --git a/java/com/google/gerrit/server/query/change/BeforePredicate.java b/java/com/google/gerrit/server/query/change/BeforePredicate.java
index 5d682fb..e9ddbff 100644
--- a/java/com/google/gerrit/server/query/change/BeforePredicate.java
+++ b/java/com/google/gerrit/server/query/change/BeforePredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.QueryParseException;
import java.sql.Timestamp;
import java.time.Instant;
@@ -26,7 +26,7 @@
public class BeforePredicate extends TimestampRangeChangePredicate {
protected final Instant cut;
- public BeforePredicate(FieldDef<ChangeData, Timestamp> def, String name, String value)
+ public BeforePredicate(SchemaField<ChangeData, Timestamp> def, String name, String value)
throws QueryParseException {
super(def, name, value);
cut = parse(value);
diff --git a/java/com/google/gerrit/server/query/change/BooleanPredicate.java b/java/com/google/gerrit/server/query/change/BooleanPredicate.java
index 6ca3acc..d6df7e0 100644
--- a/java/com/google/gerrit/server/query/change/BooleanPredicate.java
+++ b/java/com/google/gerrit/server/query/change/BooleanPredicate.java
@@ -14,10 +14,10 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
public class BooleanPredicate extends ChangeIndexPredicate {
- public BooleanPredicate(FieldDef<ChangeData, String> field) {
+ public BooleanPredicate(SchemaField<ChangeData, String> field) {
super(field, "1");
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java
index d86d366..c69f021 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java
@@ -14,18 +14,19 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
/**
* Predicate that is mapped to a field in the change index, with additional filtering done in the
* {@code match} method.
*/
public abstract class ChangeIndexPostFilterPredicate extends ChangeIndexPredicate {
- protected ChangeIndexPostFilterPredicate(FieldDef<ChangeData, ?> def, String value) {
+ protected ChangeIndexPostFilterPredicate(SchemaField<ChangeData, ?> def, String value) {
super(def, value);
}
- protected ChangeIndexPostFilterPredicate(FieldDef<ChangeData, ?> def, String name, String value) {
+ protected ChangeIndexPostFilterPredicate(
+ SchemaField<ChangeData, ?> def, String name, String value) {
super(def, name, value);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 7dad123..74d7a5c 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -200,14 +200,14 @@
/** Returns a predicate that matches changes that modified the provided {@code path}. */
public static Predicate<ChangeData> path(String path) {
- return new ChangeIndexPredicate(ChangeField.PATH, path);
+ return new ChangeIndexPredicate(ChangeField.PATH_SPEC, path);
}
/** Returns a predicate that matches changes tagged with the provided {@code hashtag}. */
public static Predicate<ChangeData> hashtag(String hashtag) {
// Use toLowerCase without locale to match behavior in ChangeField.
return new ChangeIndexPredicate(
- ChangeField.HASHTAG, HashtagsUtil.cleanupHashtag(hashtag).toLowerCase());
+ ChangeField.HASHTAG_SPEC, HashtagsUtil.cleanupHashtag(hashtag).toLowerCase());
}
/** Returns a predicate that matches changes tagged with the provided {@code hashtag}. */
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 3c69fbf..88b235e 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -536,16 +536,16 @@
@Operator
public Predicate<ChangeData> mergedBefore(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON, OPERATOR_MERGED_BEFORE);
+ checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
return new BeforePredicate(
- ChangeField.MERGED_ON, ChangeQueryBuilder.OPERATOR_MERGED_BEFORE, value);
+ ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_BEFORE, value);
}
@Operator
public Predicate<ChangeData> mergedAfter(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON, OPERATOR_MERGED_AFTER);
+ checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
return new AfterPredicate(
- ChangeField.MERGED_ON, ChangeQueryBuilder.OPERATOR_MERGED_AFTER, value);
+ ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_AFTER, value);
}
@Operator
diff --git a/java/com/google/gerrit/server/query/change/IntegerRangeChangePredicate.java b/java/com/google/gerrit/server/query/change/IntegerRangeChangePredicate.java
index 312c04e..b6059f7 100644
--- a/java/com/google/gerrit/server/query/change/IntegerRangeChangePredicate.java
+++ b/java/com/google/gerrit/server/query/change/IntegerRangeChangePredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.IntegerRangePredicate;
import com.google.gerrit.index.query.Matchable;
import com.google.gerrit.index.query.QueryParseException;
@@ -22,7 +22,7 @@
public abstract class IntegerRangeChangePredicate extends IntegerRangePredicate<ChangeData>
implements Matchable<ChangeData> {
- protected IntegerRangeChangePredicate(FieldDef<ChangeData, Integer> type, String value)
+ protected IntegerRangeChangePredicate(SchemaField<ChangeData, Integer> type, String value)
throws QueryParseException {
super(type, value);
}
diff --git a/java/com/google/gerrit/server/query/change/RegexHashtagPredicate.java b/java/com/google/gerrit/server/query/change/RegexHashtagPredicate.java
index 24efa6a..f62780a 100644
--- a/java/com/google/gerrit/server/query/change/RegexHashtagPredicate.java
+++ b/java/com/google/gerrit/server/query/change/RegexHashtagPredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import static com.google.gerrit.server.index.change.ChangeField.HASHTAG;
+import static com.google.gerrit.server.index.change.ChangeField.HASHTAG_SPEC;
import dk.brics.automaton.RegExp;
import dk.brics.automaton.RunAutomaton;
@@ -23,7 +23,7 @@
protected final RunAutomaton pattern;
public RegexHashtagPredicate(String re) {
- super(HASHTAG, re);
+ super(HASHTAG_SPEC, re);
if (re.startsWith("^")) {
re = re.substring(1);
diff --git a/java/com/google/gerrit/server/query/change/RegexPathPredicate.java b/java/com/google/gerrit/server/query/change/RegexPathPredicate.java
index 4c3c04c..9368047 100644
--- a/java/com/google/gerrit/server/query/change/RegexPathPredicate.java
+++ b/java/com/google/gerrit/server/query/change/RegexPathPredicate.java
@@ -19,7 +19,7 @@
public class RegexPathPredicate extends ChangeRegexPredicate {
public RegexPathPredicate(String re) {
- super(ChangeField.PATH, re);
+ super(ChangeField.PATH_SPEC, re);
}
@Override
diff --git a/java/com/google/gerrit/server/query/change/TimestampRangeChangePredicate.java b/java/com/google/gerrit/server/query/change/TimestampRangeChangePredicate.java
index abbd0c9..0b2d32d 100644
--- a/java/com/google/gerrit/server/query/change/TimestampRangeChangePredicate.java
+++ b/java/com/google/gerrit/server/query/change/TimestampRangeChangePredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.Matchable;
import com.google.gerrit.index.query.TimestampRangePredicate;
import java.sql.Timestamp;
@@ -22,7 +22,7 @@
public abstract class TimestampRangeChangePredicate extends TimestampRangePredicate<ChangeData>
implements Matchable<ChangeData> {
protected TimestampRangeChangePredicate(
- FieldDef<ChangeData, Timestamp> def, String name, String value) {
+ SchemaField<ChangeData, Timestamp> def, String name, String value) {
super(def, name, value);
}
diff --git a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
new file mode 100644
index 0000000..ff26ff3
--- /dev/null
+++ b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
@@ -0,0 +1,411 @@
+// Copyright (C) 2022 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.schema;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.entities.LabelFunction;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementExpression;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.project.ProjectConfig;
+import com.google.gerrit.server.project.ProjectLevelConfig;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Optional;
+import javax.inject.Inject;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
+
+/**
+ * A class with logic for migrating existing label functions to submit requirements and resetting
+ * the label functions to {@link LabelFunction#NO_BLOCK}.
+ *
+ * <p>Important note: Callers should do this migration only if this gerrit installation has no
+ * Prolog submit rules (i.e. no rules.pl file in refs/meta/config). Otherwise, the newly created
+ * submit requirements might not behave as intended.
+ *
+ * <p>The conversion is done as follows:
+ *
+ * <ul>
+ * <li>MaxWithBlock is translated to submittableIf = label:$lbl=MAX AND -label:$lbl=MIN
+ * <li>MaxNoBlock is translated to submittableIf = label:$lbl=MAX
+ * <li>AnyWithBlock is translated to submittableIf = -label:$lbl=MIN
+ * <li>NoBlock/NoOp are translated to applicableIf = is:false (not applicable)
+ * <li>PatchSetLock labels are left as is
+ * </ul>
+ *
+ * If the label has {@link LabelType#isIgnoreSelfApproval()}, the max vote is appended with the
+ * 'user=non_uploader' argument.
+ *
+ * <p>If there is an existing label and there exists a "submit requirement" with the same name, then
+ * the logic will leave the "submit requirement" as is and will not replace it. But in this case,
+ * the label will be reset to NO_BLOCK anyway.
+ */
+public class MigrateLabelFunctionsToSubmitRequirement {
+ private static final String COMMIT_MSG = "Migrate label functions to submit requirements";
+ private final GitRepositoryManager repoManager;
+ private final PersonIdent serverUser;
+
+ public enum Status {
+ /**
+ * The migrator updated the project config and created new submit requirements and/or did reset
+ * label functions.
+ */
+ MIGRATED,
+
+ /** The project had prolog rules, and the migration was skipped. */
+ HAS_PROLOG,
+
+ /**
+ * The project was migrated with a previous run of this class. The migration for this run was
+ * skipped.
+ */
+ PREVIOUSLY_MIGRATED,
+
+ /**
+ * Migration was run for the project but did not update the project.config because it was
+ * up-to-date.
+ */
+ NO_CHANGE
+ }
+
+ @Inject
+ public MigrateLabelFunctionsToSubmitRequirement(
+ GitRepositoryManager repoManager, @GerritPersonIdent PersonIdent serverUser) {
+ this.repoManager = repoManager;
+ this.serverUser = serverUser;
+ }
+
+ /**
+ * For each label function, create a corresponding submit-requirement and set the label function
+ * to NO_BLOCK. Blocking label functions are substituted with blocking submit-requirements.
+ * Non-blocking label functions are substituted with non-applicable submit requirements, allowing
+ * the label vote to be surfaced as a trigger vote (optional label).
+ *
+ * @return {@link Status} reflecting the status of the migration.
+ */
+ public Status executeMigration(Project.NameKey project, UpdateUI ui)
+ throws IOException, ConfigInvalidException {
+ if (hasPrologRules(project)) {
+ ui.message(String.format("Skipping project %s because it has prolog rules", project));
+ return Status.HAS_PROLOG;
+ }
+ ProjectLevelConfig.Bare projectConfig =
+ new ProjectLevelConfig.Bare(ProjectConfig.PROJECT_CONFIG);
+ boolean migrationPerformed = false;
+ try (Repository repo = repoManager.openRepository(project);
+ MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, project, repo)) {
+ if (hasMigrationAlreadyRun(repo)) {
+ ui.message(
+ String.format(
+ "Skipping migrating label functions to submit requirements for project '%s'"
+ + " because it has been previously migrated",
+ project));
+ return Status.PREVIOUSLY_MIGRATED;
+ }
+ projectConfig.load(project, repo);
+ Config cfg = projectConfig.getConfig();
+ Map<String, LabelAttributes> labelTypes = getLabelTypes(cfg);
+ Map<String, SubmitRequirement> existingSubmitRequirements = loadSubmitRequirements(cfg);
+ boolean updated = false;
+ for (Map.Entry<String, LabelAttributes> lt : labelTypes.entrySet()) {
+ String labelName = lt.getKey();
+ LabelAttributes attributes = lt.getValue();
+ if (attributes.function().equals("PatchSetLock")) {
+ // PATCH_SET_LOCK functions should be left as is
+ continue;
+ }
+ // If the function is other than "NoBlock" we want to reset the label function regardless
+ // of whether there exists a "submit requirement".
+ if (!attributes.function().equals("NoBlock")) {
+ updated = true;
+ writeLabelFunction(cfg, labelName, "NoBlock");
+ }
+ SubmitRequirement sr = createSrFromLabelDef(labelName, attributes);
+ // Make the operation idempotent by skipping creating the submit-requirement if one was
+ // already created or previously existed.
+ if (existingSubmitRequirements.containsKey(labelName.toLowerCase(Locale.ROOT))) {
+ if (!sr.equals(existingSubmitRequirements.get(labelName.toLowerCase(Locale.ROOT)))) {
+ ui.message(
+ String.format(
+ "Warning: Skipping creating a submit requirement for label '%s'. An existing "
+ + "submit requirement is already present but its definition is not "
+ + "identical to the existing label definition.",
+ labelName));
+ }
+ continue;
+ }
+ updated = true;
+ ui.message(
+ String.format(
+ "Project %s: Creating a submit requirement for label %s", project, labelName));
+ writeSubmitRequirement(cfg, sr);
+ }
+ if (updated) {
+ commit(projectConfig, md);
+ migrationPerformed = true;
+ }
+ }
+ return migrationPerformed ? Status.MIGRATED : Status.NO_CHANGE;
+ }
+
+ /**
+ * Returns a Map containing label names as string in keys along with some of its attributes (that
+ * we need in the migration) like canOverride, ignoreSelfApproval and function in the value.
+ */
+ private Map<String, LabelAttributes> getLabelTypes(Config cfg) {
+ Map<String, LabelAttributes> labelTypes = new HashMap<>();
+ for (String labelName : cfg.getSubsections(ProjectConfig.LABEL)) {
+ String function = cfg.getString(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_FUNCTION);
+ boolean canOverride =
+ cfg.getBoolean(
+ ProjectConfig.LABEL,
+ labelName,
+ ProjectConfig.KEY_CAN_OVERRIDE,
+ /* defaultValue= */ true);
+ boolean ignoreSelfApproval =
+ cfg.getBoolean(
+ ProjectConfig.LABEL,
+ labelName,
+ ProjectConfig.KEY_IGNORE_SELF_APPROVAL,
+ /* defaultValue= */ false);
+ LabelAttributes attributes =
+ LabelAttributes.create(
+ function == null ? "MaxWithBlock" : function, canOverride, ignoreSelfApproval);
+ labelTypes.put(labelName, attributes);
+ }
+ return labelTypes;
+ }
+
+ private void writeSubmitRequirement(Config cfg, SubmitRequirement sr) {
+ if (sr.description().isPresent()) {
+ cfg.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ sr.name(),
+ ProjectConfig.KEY_SR_DESCRIPTION,
+ sr.description().get());
+ }
+ if (sr.applicabilityExpression().isPresent()) {
+ cfg.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ sr.name(),
+ ProjectConfig.KEY_SR_APPLICABILITY_EXPRESSION,
+ sr.applicabilityExpression().get().expressionString());
+ }
+ cfg.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ sr.name(),
+ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION,
+ sr.submittabilityExpression().expressionString());
+ if (sr.overrideExpression().isPresent()) {
+ cfg.setString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ sr.name(),
+ ProjectConfig.KEY_SR_OVERRIDE_EXPRESSION,
+ sr.overrideExpression().get().expressionString());
+ }
+ cfg.setBoolean(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ sr.name(),
+ ProjectConfig.KEY_SR_OVERRIDE_IN_CHILD_PROJECTS,
+ sr.allowOverrideInChildProjects());
+ }
+
+ private void writeLabelFunction(Config cfg, String labelName, String function) {
+ cfg.setString(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_FUNCTION, function);
+ }
+
+ private void commit(ProjectLevelConfig.Bare projectConfig, MetaDataUpdate md) throws IOException {
+ md.getCommitBuilder().setAuthor(serverUser);
+ md.getCommitBuilder().setCommitter(serverUser);
+ md.setMessage(COMMIT_MSG);
+ projectConfig.commit(md);
+ }
+
+ private static SubmitRequirement createSrFromLabelDef(
+ String labelName, LabelAttributes attributes) {
+ if (!isBlockingOrRequiredLabel(attributes.function())) {
+ return createNonApplicableSr(labelName, attributes.canOverride());
+ }
+ return createBlockingOrRequiredSr(labelName, attributes);
+ }
+
+ private static SubmitRequirement createNonApplicableSr(String labelName, boolean canOverride) {
+ return SubmitRequirement.builder()
+ .setName(labelName)
+ .setApplicabilityExpression(SubmitRequirementExpression.of("is:false"))
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("is:true"))
+ .setAllowOverrideInChildProjects(canOverride)
+ .build();
+ }
+
+ /**
+ * Create a "submit requirement" that is only satisfied if the label is voted with the max votes
+ * and/or not voted by the min vote, according to the label attributes.
+ */
+ private static SubmitRequirement createBlockingOrRequiredSr(
+ String labelName, LabelAttributes attributes) {
+ SubmitRequirement.Builder builder =
+ SubmitRequirement.builder()
+ .setName(labelName)
+ .setAllowOverrideInChildProjects(attributes.canOverride());
+ String maxPart =
+ String.format("label:%s=MAX", labelName)
+ + (attributes.ignoreSelfApproval() ? ",user=non_uploader" : "");
+ switch (attributes.function()) {
+ case "MaxWithBlock":
+ builder.setSubmittabilityExpression(
+ SubmitRequirementExpression.create(
+ String.format("%s AND -label:%s=MIN", maxPart, labelName)));
+ break;
+ case "AnyWithBlock":
+ builder.setSubmittabilityExpression(
+ SubmitRequirementExpression.create(String.format("-label:%s=MIN", labelName)));
+ break;
+ case "MaxNoBlock":
+ builder.setSubmittabilityExpression(SubmitRequirementExpression.create(maxPart));
+ break;
+ default:
+ break;
+ }
+ return builder.build();
+ }
+
+ private static boolean isBlockingOrRequiredLabel(String function) {
+ return function.equals("AnyWithBlock")
+ || function.equals("MaxWithBlock")
+ || function.equals("MaxNoBlock");
+ }
+
+ public boolean anyProjectHasProlog(Collection<Project.NameKey> allProjects) throws IOException {
+ for (Project.NameKey p : allProjects) {
+ if (hasPrologRules(p)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean hasPrologRules(Project.NameKey project) throws IOException {
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk rw = new RevWalk(repo);
+ ObjectReader reader = rw.getObjectReader()) {
+ Ref refsConfig = repo.exactRef(RefNames.REFS_CONFIG);
+ if (refsConfig == null) {
+ // Project does not have a refs/meta/config and no rules.pl consequently.
+ return false;
+ }
+ RevCommit commit = repo.parseCommit(refsConfig.getObjectId());
+ try (TreeWalk tw = TreeWalk.forPath(reader, "rules.pl", commit.getTree())) {
+ if (tw != null) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+ }
+
+ /**
+ * Returns a map containing submit requirement names in lower name as keys, with {@link
+ * com.google.gerrit.entities.SubmitRequirement} as value.
+ */
+ private Map<String, SubmitRequirement> loadSubmitRequirements(Config rc) {
+ Map<String, SubmitRequirement> allRequirements = new LinkedHashMap<>();
+ for (String name : rc.getSubsections(ProjectConfig.SUBMIT_REQUIREMENT)) {
+ String description =
+ rc.getString(ProjectConfig.SUBMIT_REQUIREMENT, name, ProjectConfig.KEY_SR_DESCRIPTION);
+ String applicabilityExpr =
+ rc.getString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ name,
+ ProjectConfig.KEY_SR_APPLICABILITY_EXPRESSION);
+ String submittabilityExpr =
+ rc.getString(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ name,
+ ProjectConfig.KEY_SR_SUBMITTABILITY_EXPRESSION);
+ String overrideExpr =
+ rc.getString(
+ ProjectConfig.SUBMIT_REQUIREMENT, name, ProjectConfig.KEY_SR_OVERRIDE_EXPRESSION);
+ boolean canInherit =
+ rc.getBoolean(
+ ProjectConfig.SUBMIT_REQUIREMENT,
+ name,
+ ProjectConfig.KEY_SR_OVERRIDE_IN_CHILD_PROJECTS,
+ false);
+ SubmitRequirement submitRequirement =
+ SubmitRequirement.builder()
+ .setName(name)
+ .setDescription(Optional.ofNullable(description))
+ .setApplicabilityExpression(SubmitRequirementExpression.of(applicabilityExpr))
+ .setSubmittabilityExpression(SubmitRequirementExpression.create(submittabilityExpr))
+ .setOverrideExpression(SubmitRequirementExpression.of(overrideExpr))
+ .setAllowOverrideInChildProjects(canInherit)
+ .build();
+ allRequirements.put(name.toLowerCase(Locale.ROOT), submitRequirement);
+ }
+ return allRequirements;
+ }
+
+ private static boolean hasMigrationAlreadyRun(Repository repo) throws IOException {
+ try (RevWalk revWalk = new RevWalk(repo)) {
+ Ref refsMetaConfig = repo.exactRef(RefNames.REFS_CONFIG);
+ if (refsMetaConfig == null) {
+ return false;
+ }
+ revWalk.markStart(revWalk.parseCommit(refsMetaConfig.getObjectId()));
+ RevCommit commit;
+ while ((commit = revWalk.next()) != null) {
+ if (COMMIT_MSG.equals(commit.getShortMessage())) {
+ return true;
+ }
+ }
+ return false;
+ }
+ }
+
+ @AutoValue
+ abstract static class LabelAttributes {
+ abstract String function();
+
+ abstract boolean canOverride();
+
+ abstract boolean ignoreSelfApproval();
+
+ static LabelAttributes create(
+ String function, boolean canOverride, boolean ignoreSelfApproval) {
+ return new AutoValue_MigrateLabelFunctionsToSubmitRequirement_LabelAttributes(
+ function, canOverride, ignoreSelfApproval);
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
new file mode 100644
index 0000000..7aadc08
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
@@ -0,0 +1,383 @@
+// Copyright (C) 2022 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.pgm;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.projects.SubmitRequirementApi;
+import com.google.gerrit.extensions.common.LabelDefinitionInfo;
+import com.google.gerrit.extensions.common.LabelDefinitionInput;
+import com.google.gerrit.extensions.common.SubmitRequirementInfo;
+import com.google.gerrit.extensions.common.SubmitRequirementInput;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.server.schema.MigrateLabelFunctionsToSubmitRequirement;
+import com.google.gerrit.server.schema.MigrateLabelFunctionsToSubmitRequirement.Status;
+import com.google.gerrit.server.schema.UpdateUI;
+import com.google.inject.Inject;
+import java.util.Set;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Test;
+
+/** Test for {@link com.google.gerrit.server.schema.MigrateLabelFunctionsToSubmitRequirement}. */
+@Sandboxed
+public class MigrateLabelFunctionsToSubmitRequirementIT extends AbstractDaemonTest {
+
+ @Inject private ProjectOperations projectOperations;
+
+ @Test
+ public void migrateBlockingLabel_maxWithBlock() throws Exception {
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_maxNoBlock() throws Exception {
+ createLabel("Foo", "MaxNoBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_anyWithBlock() throws Exception {
+ createLabel("Foo", "AnyWithBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "-label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_maxWithBlock_withIgnoreSelfApproval() throws Exception {
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ true);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX,user=non_uploader AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_maxNoBlock_withIgnoreSelfApproval() throws Exception {
+ createLabel("Foo", "MaxNoBlock", /* ignoreSelfApproval= */ true);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX,user=non_uploader",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateNonBlockingLabel_NoBlock() throws Exception {
+ createLabel("Foo", "NoBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "is:false",
+ /* submittabilityExpression= */ "is:true",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateNonBlockingLabel_NoOp() throws Exception {
+ createLabel("Foo", "NoBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "is:false",
+ /* submittabilityExpression= */ "is:true",
+ /* canOverride= */ true);
+
+ // The NoOp function is converted to NoBlock. Both are same.
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateNonBlockingLabel_PatchSetLock_doesNothing() throws Exception {
+ createLabel("Foo", "PatchSetLock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.NO_CHANGE);
+ // No submit requirement created for the patchset lock label function
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(0);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertNonExistentSr(/* srName = */ "Foo");
+ assertLabelFunction("Foo", "PatchSetLock");
+ }
+
+ @Test
+ public void migrationIsCommittedWithServerIdent() throws Exception {
+ RevCommit oldMetaCommit = projectOperations.project(project).getHead(RefNames.REFS_CONFIG);
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+
+ RevCommit newMetaCommit = projectOperations.project(project).getHead(RefNames.REFS_CONFIG);
+ assertThat(newMetaCommit).isNotEqualTo(oldMetaCommit);
+ assertThat(newMetaCommit.getCommitterIdent().getEmailAddress())
+ .isEqualTo(serverIdent.get().getEmailAddress());
+ }
+
+ @Test
+ public void migrationIsIdempotent() throws Exception {
+ String oldRefsConfigId;
+ try (Repository repo = repoManager.openRepository(project)) {
+ oldRefsConfigId = repo.exactRef(RefNames.REFS_CONFIG).getObjectId().toString();
+ }
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ false);
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ // Running the migration causes REFS_CONFIG to change.
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+ try (Repository repo = repoManager.openRepository(project)) {
+ assertThat(oldRefsConfigId)
+ .isNotEqualTo(repo.exactRef(RefNames.REFS_CONFIG).getObjectId().toString());
+ oldRefsConfigId = repo.exactRef(RefNames.REFS_CONFIG).getObjectId().toString();
+ }
+
+ // No new SRs will be created. No conflicting submit requirements either since the migration
+ // detects that a previous run was made and skips the migration.
+ updateUI = runMigration(/* expectedResult= */ Status.PREVIOUSLY_MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(0);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+ // Running the migration a second time won't update REFS_CONFIG.
+ try (Repository repo = repoManager.openRepository(project)) {
+ assertThat(oldRefsConfigId)
+ .isEqualTo(repo.exactRef(RefNames.REFS_CONFIG).getObjectId().toString());
+ }
+ }
+
+ @Test
+ public void migrationDoesNotCreateANewSubmitRequirement_ifSRAlreadyExists_matchingWithMigration()
+ throws Exception {
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ false);
+ createSubmitRequirement("Foo", "label:Foo=MAX AND -label:Foo=MIN", /* canOverride= */ true);
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ // No new submit requirements are created.
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(0);
+ // No conflicting submit requirements from migration vs. what was previously configured.
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ // The existing SR was left as is.
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ }
+
+ @Test
+ public void
+ migrationDoesNotCreateANewSubmitRequirement_ifSRAlreadyExists_mismatchingWithMigration()
+ throws Exception {
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ false);
+ createSubmitRequirement("Foo", "project:" + project, /* canOverride= */ true);
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "project:" + project,
+ /* canOverride= */ true);
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ // One conflicting submit requirement between migration vs. what was previously configured.
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(1);
+
+ // The existing SR was left as is.
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ null,
+ /* submittabilityExpression= */ "project:" + project,
+ /* canOverride= */ true);
+ }
+
+ @Test
+ public void migrationResetsBlockingLabel_ifSRAlreadyExists() throws Exception {
+ createLabel("Foo", "MaxWithBlock", /* ignoreSelfApproval= */ false);
+ createSubmitRequirement("Foo", "owner:" + admin.email(), /* canOverride= */ true);
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(0);
+
+ // The label function was reset
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ private TestUpdateUI runMigration(Status expectedResult) throws Exception {
+ TestUpdateUI updateUi = new TestUpdateUI();
+ MigrateLabelFunctionsToSubmitRequirement executor =
+ new MigrateLabelFunctionsToSubmitRequirement(repoManager, serverIdent.get());
+ Status status = executor.executeMigration(project, updateUi);
+ assertThat(status).isEqualTo(expectedResult);
+ projectCache.evictAndReindex(project);
+ return updateUi;
+ }
+
+ private void createLabel(String labelName, String function, boolean ignoreSelfApproval)
+ throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.name = labelName;
+ input.function = function;
+ input.ignoreSelfApproval = ignoreSelfApproval;
+ input.values = ImmutableMap.of("+1", "Looks Good", " 0", "Don't Know", "-1", "Looks Bad");
+ gApi.projects().name(project.get()).label(labelName).create(input);
+ }
+
+ @CanIgnoreReturnValue
+ private SubmitRequirementApi createSubmitRequirement(
+ String name, String submitExpression, boolean canOverride) throws Exception {
+ SubmitRequirementInput input = new SubmitRequirementInput();
+ input.name = name;
+ input.submittabilityExpression = submitExpression;
+ input.allowOverrideInChildProjects = canOverride;
+ return gApi.projects().name(project.get()).submitRequirement(name).create(input);
+ }
+
+ private void assertLabelFunction(String labelName, String function) throws Exception {
+ LabelDefinitionInfo info = gApi.projects().name(project.get()).label(labelName).get();
+ assertThat(info.function).isEqualTo(function);
+ }
+
+ private void assertNonExistentSr(String srName) {
+ ResourceNotFoundException foo =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.projects().name(project.get()).submitRequirement("Foo").get());
+ assertThat(foo.getMessage()).isEqualTo("Submit requirement '" + srName + "' does not exist");
+ }
+
+ private void assertExistentSr(
+ String srName,
+ String applicabilityExpression,
+ String submittabilityExpression,
+ boolean canOverride)
+ throws Exception {
+ SubmitRequirementInfo sr = gApi.projects().name(project.get()).submitRequirement(srName).get();
+ assertThat(sr.applicabilityExpression).isEqualTo(applicabilityExpression);
+ assertThat(sr.submittabilityExpression).isEqualTo(submittabilityExpression);
+ assertThat(sr.allowOverrideInChildProjects).isEqualTo(canOverride);
+ }
+
+ private static class TestUpdateUI implements UpdateUI {
+ int existingSrsMismatchingWithMigration = 0;
+ int newlyCreatedSrs = 0;
+
+ @Override
+ public void message(String message) {
+ if (message.startsWith("Warning")) {
+ existingSrsMismatchingWithMigration += 1;
+ } else if (message.startsWith("Project")) {
+ newlyCreatedSrs += 1;
+ }
+ }
+
+ @Override
+ public boolean yesno(boolean defaultValue, String message) {
+ return false;
+ }
+
+ @Override
+ public void waitForUser() {}
+
+ @Override
+ public String readString(String defaultValue, Set<String> allowedValues, String message) {
+ return null;
+ }
+
+ @Override
+ public boolean isBatch() {
+ return false;
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
index 71695f3..04f9827 100644
--- a/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
+++ b/javatests/com/google/gerrit/httpd/ProjectBasicAuthFilterTest.java
@@ -45,6 +45,7 @@
import java.nio.charset.StandardCharsets;
import java.time.Instant;
import java.util.Base64;
+import java.util.Collection;
import java.util.Optional;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
@@ -72,8 +73,6 @@
@Mock private AccountCache accountCache;
- @Mock private AccountState accountState;
-
@Mock private AccountManager accountManager;
@Mock private AuthConfig authConfig;
@@ -105,12 +104,8 @@
authRequestFactory = new AuthRequest.Factory(extIdKeyFactory);
pwdVerifier = new PasswordVerifier(extIdKeyFactory, authConfig);
- Account account = Account.builder(Account.id(1000000), Instant.now()).build();
authSuccessful =
new AuthResult(AUTH_ACCOUNT_ID, extIdKeyFactory.create("username", AUTH_USER), false);
- doReturn(Optional.of(accountState)).when(accountCache).getByUsername(AUTH_USER);
- doReturn(Optional.of(accountState)).when(accountCache).get(AUTH_ACCOUNT_ID);
- doReturn(account).when(accountState).account();
doReturn(authSuccessful).when(accountManager).authenticate(any());
doReturn(new WebSessionManager.Key(AUTH_COOKIE_VALUE)).when(webSessionManager).createKey(any());
@@ -123,6 +118,7 @@
@Test
public void shouldAllowAnonymousRequest() throws Exception {
+ initAccount();
initMockedWebSession();
res.setStatus(HttpServletResponse.SC_OK);
@@ -143,6 +139,7 @@
@Test
public void shouldRequestAuthenticationForBasicAuthRequest() throws Exception {
+ initAccount();
initMockedWebSession();
req.addHeader("Authorization", "Basic " + AUTH_USER_B64);
res.setStatus(HttpServletResponse.SC_OK);
@@ -165,6 +162,7 @@
@Test
public void shouldAuthenticateSucessfullyAgainstRealmAndReturnCookie() throws Exception {
+ initAccount();
initWebSessionWithoutCookie();
requestBasicAuth(req);
res.setStatus(HttpServletResponse.SC_OK);
@@ -191,9 +189,10 @@
@Test
public void shouldValidateUserPasswordAndNotReturnCookie() throws Exception {
+ ExternalId extId = createUsernamePasswordExternalId();
+ initAccount(ImmutableSet.of(extId));
initWebSessionWithoutCookie();
requestBasicAuth(req);
- initMockedUsernamePasswordExternalId();
doReturn(GitBasicAuthPolicy.HTTP).when(authConfig).getGitBasicAuthPolicy();
res.setStatus(HttpServletResponse.SC_OK);
@@ -217,6 +216,7 @@
@Test
public void shouldNotReauthenticateForGitPostRequest() throws Exception {
+ initAccount();
req.setPathInfo("/a/project.git/git-upload-pack");
req.setMethod("POST");
req.addHeader("Content-Type", "application/x-git-upload-pack-request");
@@ -229,6 +229,7 @@
@Test
public void shouldReauthenticateForRegularRequestEvenIfAlreadySignedIn() throws Exception {
+ initAccount();
doReturn(GitBasicAuthPolicy.LDAP).when(authConfig).getGitBasicAuthPolicy();
doFilterForRequestWhenAlreadySignedIn();
@@ -239,6 +240,7 @@
@Test
public void shouldReauthenticateEvenIfHasExistingCookie() throws Exception {
+ initAccount();
initWebSessionWithCookie("GerritAccount=" + AUTH_COOKIE_VALUE);
res.setStatus(HttpServletResponse.SC_OK);
requestBasicAuth(req);
@@ -262,6 +264,7 @@
@Test
public void shouldFailedAuthenticationAgainstRealm() throws Exception {
+ initAccount();
initMockedWebSession();
requestBasicAuth(req);
@@ -285,6 +288,17 @@
assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
}
+ private void initAccount() throws Exception {
+ initAccount(ImmutableSet.of());
+ }
+
+ private void initAccount(Collection<ExternalId> extIds) throws Exception {
+ Account account = Account.builder(Account.id(1000000), Instant.now()).build();
+ AccountState accountState = AccountState.forAccount(account, extIds);
+ doReturn(Optional.of(accountState)).when(accountCache).getByUsername(AUTH_USER);
+ doReturn(Optional.of(accountState)).when(accountCache).get(AUTH_ACCOUNT_ID);
+ }
+
private void doFilterForRequestWhenAlreadySignedIn()
throws IOException, ServletException, AccountException {
initMockedWebSession();
@@ -322,14 +336,12 @@
doReturn(webSession).when(webSessionItem).get();
}
- private void initMockedUsernamePasswordExternalId() {
- ExternalId extId =
- extIdFactory.createWithPassword(
- extIdKeyFactory.create(ExternalId.SCHEME_USERNAME, AUTH_USER),
- AUTH_ACCOUNT_ID,
- null,
- AUTH_PASSWORD);
- doReturn(ImmutableSet.builder().add(extId).build()).when(accountState).externalIds();
+ private ExternalId createUsernamePasswordExternalId() {
+ return extIdFactory.createWithPassword(
+ extIdKeyFactory.create(ExternalId.SCHEME_USERNAME, AUTH_USER),
+ AUTH_ACCOUNT_ID,
+ null,
+ AUTH_PASSWORD);
}
private void requestBasicAuth(FakeHttpServletRequest fakeReq) {
diff --git a/javatests/com/google/gerrit/server/extensions/events/GitReferenceUpdatedTest.java b/javatests/com/google/gerrit/server/extensions/events/GitReferenceUpdatedTest.java
index 96919be..7bdb23c 100644
--- a/javatests/com/google/gerrit/server/extensions/events/GitReferenceUpdatedTest.java
+++ b/javatests/com/google/gerrit/server/extensions/events/GitReferenceUpdatedTest.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.extensions.events;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.events.GitBatchRefUpdateListener;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
@@ -22,6 +23,7 @@
import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import java.io.IOException;
+import java.time.Instant;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.lib.BatchRefUpdate;
@@ -41,10 +43,12 @@
private DynamicSet<GitReferenceUpdatedListener> refUpdatedListeners;
private DynamicSet<GitBatchRefUpdateListener> batchRefUpdateListeners;
+ private final AccountState updater =
+ AccountState.forAccount(Account.builder(Account.id(1), Instant.now()).build());
+
@Mock GitReferenceUpdatedListener refUpdatedListener;
@Mock GitBatchRefUpdateListener batchRefUpdateListener;
@Mock EventUtil util;
- @Mock AccountState updater;
@Before
public void setup() {
diff --git a/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java b/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java
index 537867a..2aa9ca4 100644
--- a/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java
+++ b/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java
@@ -42,9 +42,11 @@
static final Schema<ChangeData> V2 =
schema(
2,
- ImmutableList.<FieldDef<ChangeData, ?>>of(ChangeField.PATH, ChangeField.UPDATED),
- ImmutableList.<IndexedField<ChangeData, ?>>of(ChangeField.STATUS_FIELD),
- ImmutableList.<IndexedField<ChangeData, ?>.SearchSpec>of(ChangeField.STATUS_SPEC));
+ ImmutableList.<FieldDef<ChangeData, ?>>of(ChangeField.UPDATED),
+ ImmutableList.<IndexedField<ChangeData, ?>>of(
+ ChangeField.PATH_FIELD, ChangeField.STATUS_FIELD),
+ ImmutableList.<IndexedField<ChangeData, ?>.SearchSpec>of(
+ ChangeField.PATH_SPEC, ChangeField.STATUS_SPEC));
private static class Source implements ChangeDataSource {
private final Predicate<ChangeData> p;
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 9be7772..d727ba1 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -2015,7 +2015,7 @@
@Test
public void byMergedBefore() throws Exception {
- assume().that(getSchema().hasField(ChangeField.MERGED_ON)).isTrue();
+ assume().that(getSchema().hasField(ChangeField.MERGED_ON_SPEC)).isTrue();
long thirtyHoursInMs = MILLISECONDS.convert(30, HOURS);
// Stop the clock, will set time to specific test values.
@@ -2075,7 +2075,7 @@
@Test
public void byMergedAfter() throws Exception {
- assume().that(getSchema().hasField(ChangeField.MERGED_ON)).isTrue();
+ assume().that(getSchema().hasField(ChangeField.MERGED_ON_SPEC)).isTrue();
long thirtyHoursInMs = MILLISECONDS.convert(30, HOURS);
// Stop the clock, will set time to specific test values.
@@ -2145,7 +2145,7 @@
@Test
public void updatedThenMergedOrder() throws Exception {
- assume().that(getSchema().hasField(ChangeField.MERGED_ON)).isTrue();
+ assume().that(getSchema().hasField(ChangeField.MERGED_ON_SPEC)).isTrue();
long thirtyHoursInMs = MILLISECONDS.convert(30, HOURS);
// Stop the clock, will set time to specific test values.
diff --git a/plugins/replication b/plugins/replication
index ced31c0..8f465cd 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit ced31c0c7d56cbb3e10a8da35a8ddd5db1bba550
+Subproject commit 8f465cd3f1f3039a937bd3d863c192e33d8347a2
diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md
index ac8712b..510ce54 100644
--- a/polygerrit-ui/README.md
+++ b/polygerrit-ui/README.md
@@ -24,11 +24,13 @@
Follow the instructions
[here](https://gerrit-review.googlesource.com/Documentation/dev-bazel.html#_installation)
-to get and install Bazel.
+to get and install Bazel. The `npm install -g @bazel/bazelisk` method is
+probably easiest since you will have npm as part of Nodejs.
## Installing [Node.js](https://nodejs.org/en/download/) and npm packages
-The minimum nodejs version supported is 10.x+.
+The minimum nodejs version supported is 10.x+. We recommend at least the latest
+LTS (v16 as of October 2022).
```sh
# Debian experimental
@@ -80,11 +82,12 @@
## Setup typescript support in the IDE
-Modern IDE should automatically handle typescript settings from the
-`polygerrit-ui/app/tsconfig.json` files. IDE places compiled files in the
-`.ts-out/pg` directory at the root of gerrit workspace and you can configure IDE
-to exclude the whole .ts-out directory. To do it in the IntelliJ IDEA click on
-this directory and select "Mark Directory As > Excluded" in the context menu.
+Modern IDEs should automatically handle typescript settings from the
+`polygerrit-ui/app/tsconfig.json` files. The `tsc` compiler places compiled
+files in the `.ts-out/pg` directory at the root of gerrit workspace and you can
+configure the IDE to exclude the whole .ts-out directory. To do it in the
+IntelliJ IDEA click on this directory and select "Mark Directory As > Excluded"
+in the context menu.
However, if you receive some errors from IDE, you can try to configure IDE
manually. For example, if IntelliJ IDEA shows
@@ -92,22 +95,27 @@
options `--project polygerrit-ui/app/tsconfig.json` in the IDE settings.
-## Serving files locally
+## Developing locally
-#### Web Dev Server
+The preferred method for development is to serve the web files locally using the
+Web Dev Server and then view a running gerrit instance (local or otherwise) to
+replace its web client with the local one using the Gerrit FE Dev Helper
+extension.
-To test the local frontend against production data or a local test site execute:
+### Web Dev Server
+
+The [Web Dev Server](https://modern-web.dev/docs/dev-server/overview/) serves
+the compiled web files and dependencies unbundled over localhost. Start it using
+this command:
```sh
yarn start
```
-This command starts the [Web Dev Server](https://modern-web.dev/docs/dev-server/overview/).
To inject plugins or other files, we use the [Gerrit FE Dev Helper](https://chrome.google.com/webstore/detail/gerrit-fe-dev-helper/jimgomcnodkialnpmienbomamgomglkd) Chrome extension.
If any issues occured, please refer to the Troubleshooting section at the bottom or contact the team!
-## Running locally against production data
### Chrome extension: Gerrit FE Dev Helper
@@ -120,7 +128,7 @@
To use this extension, just follow its [readme here](https://gerrit.googlesource.com/gerrit-fe-dev-helper/+/master/README.md).
-## Running locally against a Gerrit test site
+### Running locally against a Gerrit test site
Set up a local test site once:
@@ -144,26 +152,38 @@
--dev-cdn http://localhost:8081
```
+The Web Dev Server is currently not serving fonts or other static assets. Follow
+[Issue 16341](https://bugs.chromium.org/p/gerrit/issues/detail?id=16341) for
+fixing this issue.
+
*NOTE* You can use any other cdn here, for example: https://cdn.googlesource.com/polygerrit_ui/678.0
## Running Tests
For daily development you typically only want to run and debug individual tests.
-There are several ways to run tests.
+Our tests run using the
+[Web Test Runner](https://modern-web.dev/docs/test-runner/overview/). There are
+several ways to trigger tests:
-* Run all tests:
+* Run all tests once:
```sh
yarn test
```
-* Run all tests under bazel:
+* Run all tests and then watches for changes. Change a file will trigger all
+tests affected by the changes.
+```sh
+yarn test:watch
+```
+
+* Run all tests once under bazel:
```sh
./polygerrit-ui/app/run_test.sh
```
-* Run a single test file:
+* Run a single test file and rerun on any changes affecting it:
```
-yarn test:single "**/async-foreach-behavior_test.js"
+yarn test:single "**/gr-comment_test.ts"
```
Compiling code:
@@ -172,34 +192,9 @@
yarn compile:local
# Watch mode:
-## Terminal 1:
yarn compile:watch
-## Terminal 2, test & watch a file for example:
-yarn test:single "**/async-foreach-behavior_test.js"
```
-### Generated file overview
-
-A generated file starts with imports followed by a static content with
-different type definitions. You can skip this part - it doesn't contains
-anything usefule.
-
-After the static content there is a class definition. Example:
-```typescript
-export class GrCreateGroupDialogCheck extends GrCreateGroupDialog {
- templateCheck() {
- // Converted template
- // Each HTML element from the template is wrapped into own block.
- }
-}
-```
-
-The converted template usually quite straightforward, but in some cases
-additional functions are added. For example, `<element x=[[y.a]]>` converts into
-`el.x = y!.a` if y is a simple type. However, if y has a union type, like - `y:A|B`,
-then the generated code looks like `el.x=__f(y)!.a` (`y!.a` may result in a TS error
-if `a` is defined only in one type of a union).
-
## Style guide
We follow the [Google JavaScript Style Guide](https://google.github.io/styleguide/javascriptguide.xml)
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-hashtag-flow/gr-change-list-hashtag-flow.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-hashtag-flow/gr-change-list-hashtag-flow.ts
index 2b3c3df4..ad1f718 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-hashtag-flow/gr-change-list-hashtag-flow.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-hashtag-flow/gr-change-list-hashtag-flow.ts
@@ -15,7 +15,7 @@
import '@polymer/iron-dropdown/iron-dropdown';
import {IronDropdownElement} from '@polymer/iron-dropdown/iron-dropdown';
import {getAppContext} from '../../../services/app-context';
-import {notUndefined} from '../../../types/types';
+import {isDefined} from '../../../types/types';
import {unique} from '../../../utils/common-util';
import {AutocompleteSuggestion} from '../../shared/gr-autocomplete/gr-autocomplete';
import {when} from 'lit/directives/when.js';
@@ -215,7 +215,7 @@
private renderExistingHashtags() {
const hashtags = this.selectedChanges
.flatMap(change => change.hashtags ?? [])
- .filter(notUndefined)
+ .filter(isDefined)
.filter(unique);
return html`
<div class="chips">
@@ -302,7 +302,7 @@
);
this.existingHashtagSuggestions = (suggestions ?? [])
.flatMap(change => change.hashtags ?? [])
- .filter(notUndefined)
+ .filter(isDefined)
.filter(unique);
return this.existingHashtagSuggestions.map(hashtag => {
return {name: hashtag, value: hashtag};
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-topic-flow/gr-change-list-topic-flow.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-topic-flow/gr-change-list-topic-flow.ts
index ac1ba23..363b1ae 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-topic-flow/gr-change-list-topic-flow.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-topic-flow/gr-change-list-topic-flow.ts
@@ -15,7 +15,7 @@
import '@polymer/iron-dropdown/iron-dropdown';
import {IronDropdownElement} from '@polymer/iron-dropdown/iron-dropdown';
import {getAppContext} from '../../../services/app-context';
-import {notUndefined} from '../../../types/types';
+import {isDefined} from '../../../types/types';
import {unique} from '../../../utils/common-util';
import {AutocompleteSuggestion} from '../../shared/gr-autocomplete/gr-autocomplete';
import {when} from 'lit/directives/when.js';
@@ -190,7 +190,7 @@
private renderExistingTopicsMode() {
const topics = this.selectedChanges
.map(change => change.topic)
- .filter(notUndefined)
+ .filter(isDefined)
.filter(unique);
const removeDisabled =
this.selectedExistingTopics.size === 0 ||
@@ -347,7 +347,7 @@
);
this.existingTopicSuggestions = (suggestions ?? [])
.map(change => change.topic)
- .filter(notUndefined)
+ .filter(isDefined)
.filter(unique);
return this.existingTopicSuggestions.map(topic => {
return {name: topic, value: topic};
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index 228e7ce..81245cc 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -61,7 +61,7 @@
import {fireAlert, fireEvent, fireReload} from '../../../utils/event-util';
import {
EditRevisionInfo,
- notUndefined,
+ isDefined,
ParsedChangeInfo,
} from '../../../types/types';
import {
@@ -1124,7 +1124,7 @@
.then(response =>
(response ?? [])
.map(change => change.topic)
- .filter(notUndefined)
+ .filter(isDefined)
.filter(unique)
.map(topic => {
return {name: topic, value: topic};
@@ -1140,7 +1140,7 @@
.then(response =>
(response ?? [])
.flatMap(change => change.hashtags ?? [])
- .filter(notUndefined)
+ .filter(isDefined)
.filter(unique)
.map(hashtag => {
return {name: hashtag, value: hashtag};
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
index 73653bb..ed1f46d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
@@ -40,7 +40,7 @@
} from '../../../utils/comment-util';
import {pluralize} from '../../../utils/string-util';
import {AccountInfo} from '../../../types/common';
-import {notUndefined} from '../../../types/types';
+import {isDefined} from '../../../types/types';
import {Tab} from '../../../constants/constants';
import {ChecksTabState, CommentTabState} from '../../../types/events';
import {spinnerStyles} from '../../../styles/gr-spinner-styles';
@@ -670,7 +670,7 @@
return commentThreads
.map(getFirstComment)
.map(comment => comment?.author ?? this.selfAccount)
- .filter(notUndefined);
+ .filter(isDefined);
}
}
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 cd42a9f..26b9a63 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
@@ -33,7 +33,7 @@
import {TargetElement} from '../../../api/plugin';
import {
FixIronA11yAnnouncer,
- notUndefined,
+ isDefined,
ParsedChangeInfo,
} from '../../../types/types';
import {
@@ -129,7 +129,7 @@
GrComment,
} from '../../shared/gr-comment/gr-comment';
import {ShortcutController} from '../../lit/shortcut-controller';
-import {Key, Modifier} from '../../../utils/dom-util';
+import {Key, Modifier, whenVisible} from '../../../utils/dom-util';
import {GrThreadList} from '../gr-thread-list/gr-thread-list';
export enum FocusTarget {
@@ -241,6 +241,8 @@
@property({type: Object})
projectConfig?: ConfigInfo;
+ @query('#patchsetLevelComment') patchsetLevelGrComment?: GrComment;
+
@query('#reviewers') reviewersList?: GrAccountList;
@query('#ccs') ccsList?: GrAccountList;
@@ -1411,13 +1413,13 @@
)
.filter(user => !this.currentAttentionSet.has(user))
.map(user => allAccounts.find(a => getUserId(a) === user))
- .filter(notUndefined);
+ .filter(isDefined);
const newAttentionSetUsers = (
await Promise.all(
newAttentionSetAdditions.map(a => this.accountsModel.fillDetails(a))
)
- ).filter(notUndefined);
+ ).filter(isDefined);
for (const user of newAttentionSetUsers) {
let reason;
@@ -1446,11 +1448,7 @@
reviewInput.remove_from_attention_set
);
- const patchsetLevelComment = queryAndAssert<GrComment>(
- this,
- '#patchsetLevelComment'
- );
- await patchsetLevelComment.save();
+ await this.patchsetLevelGrComment?.save();
assertIsDefined(this.change, 'change');
reviewInput.reviewers = this.computeReviewers();
@@ -1494,13 +1492,17 @@
if (!section || section === FocusTarget.ANY) {
section = this.chooseFocusTarget();
}
- if (section === FocusTarget.REVIEWERS) {
- const reviewerEntry = this.reviewersList?.focusStart;
- setTimeout(() => reviewerEntry?.focus());
- } else if (section === FocusTarget.CCS) {
- const ccEntry = this.ccsList?.focusStart;
- setTimeout(() => ccEntry?.focus());
- }
+ whenVisible(this, () => {
+ if (section === FocusTarget.REVIEWERS) {
+ const reviewerEntry = this.reviewersList?.focusStart;
+ reviewerEntry?.focus();
+ } else if (section === FocusTarget.CCS) {
+ const ccEntry = this.ccsList?.focusStart;
+ ccEntry?.focus();
+ } else {
+ this.patchsetLevelGrComment?.focus();
+ }
+ });
}
chooseFocusTarget() {
@@ -1860,11 +1862,7 @@
bubbles: false,
})
);
- const patchsetLevelComment = queryAndAssert<GrComment>(
- this,
- '#patchsetLevelComment'
- );
- await patchsetLevelComment.save();
+ await this.patchsetLevelGrComment?.save();
this.rebuildReviewerArrays();
}
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 acd1755..8185139 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
@@ -15,6 +15,7 @@
queryAndAssert,
stubFlags,
stubRestApi,
+ waitUntilVisible,
} from '../../../test/test-utils';
import {ChangeStatus, ReviewerState} from '../../../constants/constants';
import {JSON_PREFIX} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
@@ -1444,14 +1445,10 @@
test('focusOn', async () => {
await element.updateComplete;
- const clock = sinon.useFakeTimers();
const chooseFocusTargetSpy = sinon.spy(element, 'chooseFocusTarget');
element.focusOn();
- // element.focus() is called after a setTimeout(). The focusOn() method
- // does not trigger any changes in the element hence element.updateComplete
- // resolves immediately and cannot be used here, hence tick the clock here
- // explicitly instead
- clock.tick(1);
+ await waitUntilVisible(element); // let whenVisible resolve
+
assert.equal(chooseFocusTargetSpy.callCount, 1);
assert.equal(element?.shadowRoot?.activeElement?.tagName, 'GR-COMMENT');
assert.equal(
@@ -1460,7 +1457,8 @@
);
element.focusOn(element.FocusTarget.ANY);
- clock.tick(1);
+ await waitUntilVisible(element); // let whenVisible resolve
+
assert.equal(chooseFocusTargetSpy.callCount, 2);
assert.equal(element?.shadowRoot?.activeElement?.tagName, 'GR-COMMENT');
assert.equal(
@@ -1469,7 +1467,8 @@
);
element.focusOn(element.FocusTarget.BODY);
- clock.tick(1);
+ await waitUntilVisible(element); // let whenVisible resolve
+
assert.equal(chooseFocusTargetSpy.callCount, 2);
assert.equal(element?.shadowRoot?.activeElement?.tagName, 'GR-COMMENT');
assert.equal(
@@ -1478,23 +1477,21 @@
);
element.focusOn(element.FocusTarget.REVIEWERS);
- clock.tick(1);
+ await waitUntilVisible(element); // let whenVisible resolve
+
assert.equal(chooseFocusTargetSpy.callCount, 2);
- assert.equal(
- element?.shadowRoot?.activeElement?.tagName,
- 'GR-ACCOUNT-LIST'
+ await waitUntil(
+ () => element?.shadowRoot?.activeElement?.tagName === 'GR-ACCOUNT-LIST'
);
assert.equal(element?.shadowRoot?.activeElement?.id, 'reviewers');
element.focusOn(element.FocusTarget.CCS);
- clock.tick(1);
assert.equal(chooseFocusTargetSpy.callCount, 2);
assert.equal(
element?.shadowRoot?.activeElement?.tagName,
'GR-ACCOUNT-LIST'
);
- assert.equal(element?.shadowRoot?.activeElement?.id, 'ccs');
- clock.restore();
+ await waitUntil(() => element?.shadowRoot?.activeElement?.id === 'ccs');
});
test('chooseFocusTarget', () => {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 33fd2ee..2bda156 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -317,7 +317,15 @@
if (this.view !== GerritView.CHANGE) return;
const browserUrl = new URL(window.location.toString());
const stateUrl = new URL(createChangeUrl(state), browserUrl);
+
+ // Keeping the hash and certain parameters are stop-gap solution. We
+ // should find better ways of maintaining an overall consistent URL
+ // state.
stateUrl.hash = browserUrl.hash;
+ for (const p of browserUrl.searchParams.entries()) {
+ if (p[0] === 'experiment') stateUrl.searchParams.append(p[0], p[1]);
+ }
+
if (browserUrl.toString() !== stateUrl.toString()) {
page.replace(
stateUrl.toString(),
@@ -1352,31 +1360,28 @@
}
handleQueryRoute(ctx: PageContext) {
- const state: SearchViewState = {
+ const state: Partial<SearchViewState> = {
view: GerritView.SEARCH,
query: ctx.params[0],
offset: ctx.params[2],
- changes: [],
- loading: false,
};
// Note that router model view must be updated before view models.
- this.setState(state);
- this.searchViewModel.setState(state);
+ this.setState(state as AppElementParams);
+ this.searchViewModel.updateState(state);
}
handleChangeIdQueryRoute(ctx: PageContext) {
// TODO(pcc): This will need to indicate that this was a change ID query if
// standard queries gain the ability to search places like commit messages
// for change IDs.
- const state: SearchViewState = {
+ const state: Partial<SearchViewState> = {
view: GerritView.SEARCH,
query: ctx.params[0],
- changes: [],
- loading: false,
+ offset: undefined,
};
// Note that router model view must be updated before view models.
- this.setState(state);
- this.searchViewModel.setState(state);
+ this.setState(state as AppElementParams);
+ this.searchViewModel.updateState(state);
}
handleQueryLegacySuffixRoute(ctx: PageContext) {
@@ -1537,8 +1542,9 @@
patchNum: convertToPatchSetNum(ctx.params[3]) as RevisionPatchSetNum,
view: GerritView.CHANGE,
edit: true,
- tab: queryMap.get('tab') ?? '',
};
+ const tab = queryMap.get('tab');
+ if (tab) state.tab = tab;
if (queryMap.has('forceReload')) {
state.forceReload = true;
history.replaceState(
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index b550ea6..d1d0c86 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -403,9 +403,7 @@
view: GerritView.SEARCH,
query: 'project:foo/bar/baz',
offset: undefined,
- changes: [],
- loading: false,
- });
+ } as AppElementParams);
ctx.params[1] = '123';
ctx.params[2] = '123';
@@ -413,9 +411,7 @@
view: GerritView.SEARCH,
query: 'project:foo/bar/baz',
offset: '123',
- changes: [],
- loading: false,
- });
+ } as AppElementParams);
});
test('handleQueryLegacySuffixRoute', () => {
@@ -433,9 +429,8 @@
assertctxToParams(ctx, 'handleChangeIdQueryRoute', {
view: GerritView.SEARCH,
query: 'I0123456789abcdef0123456789abcdef01234567',
- changes: [],
- loading: false,
- });
+ offset: undefined,
+ } as AppElementParams);
});
suite('handleRegisterRoute', () => {
@@ -1322,7 +1317,6 @@
view: GerritView.CHANGE,
patchNum: 3 as RevisionPatchSetNum,
edit: true,
- tab: '',
};
router.handleChangeEditRoute(ctx);
diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts
index c9603f4..99b47da 100644
--- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts
+++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info.ts
@@ -8,6 +8,7 @@
import '../../shared/gr-date-formatter/gr-date-formatter';
import '../../../styles/gr-form-styles';
import '../../../styles/shared-styles';
+import '../../shared/gr-account-chip/gr-account-chip';
import {AccountDetailInfo, ServerInfo} from '../../../types/common';
import {EditableAccountField} from '../../../constants/constants';
import {getAppContext} from '../../../services/app-context';
@@ -209,6 +210,12 @@
</iron-input>
</span>
</section>
+ <section>
+ <span class="title">Account chip preview</span>
+ <span class="value">
+ <gr-account-chip .account=${this.account}></gr-account-chip>
+ </span>
+ </section>
</div>`;
}
diff --git a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts
index 518828a..bcd3964 100644
--- a/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts
+++ b/polygerrit-ui/app/elements/settings/gr-account-info/gr-account-info_test.ts
@@ -109,6 +109,10 @@
</iron-input>
</span>
</section>
+ <section>
+ <span class="title">Account chip preview</span>
+ <span class="value"><gr-account-chip></gr-account-chip></span>
+ </section>
</div>
`
);
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
index e459c05..3fd1b82 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
@@ -7,12 +7,13 @@
import '../../../styles/shared-styles';
import {GrCursorManager} from '../gr-cursor-manager/gr-cursor-manager';
import {fireEvent} from '../../../utils/event-util';
-import {addShortcut, Key} from '../../../utils/dom-util';
+import {Key} from '../../../utils/dom-util';
import {FitController} from '../../lit/fit-controller';
import {css, html, LitElement, PropertyValues} from 'lit';
import {customElement, property, query} from 'lit/decorators.js';
import {repeat} from 'lit/directives/repeat.js';
import {sharedStyles} from '../../../styles/shared-styles';
+import {ShortcutController} from '../../lit/shortcut-controller';
declare global {
interface HTMLElementTagNameMap {
@@ -64,8 +65,7 @@
@query('#suggestions') suggestionsDiv?: HTMLDivElement;
- /** Called in disconnectedCallback. */
- private cleanups: (() => void)[] = [];
+ private readonly shortcuts = new ShortcutController(this);
// visible for testing
cursor = new GrCursorManager();
@@ -132,29 +132,19 @@
super();
this.cursor.cursorTargetClass = 'selected';
this.cursor.focusOnMove = true;
+ this.shortcuts.addLocal({key: Key.UP}, () => this.handleUp());
+ this.shortcuts.addLocal({key: Key.DOWN}, () => this.handleDown());
+ this.shortcuts.addLocal({key: Key.ENTER}, () => this.handleEnter());
+ this.shortcuts.addLocal({key: Key.ESC}, () => this.handleEscape());
+ this.shortcuts.addLocal({key: Key.TAB}, () => this.handleTab());
}
override connectedCallback() {
super.connectedCallback();
- this.cleanups.push(addShortcut(this, {key: Key.UP}, () => this.handleUp()));
- this.cleanups.push(
- addShortcut(this, {key: Key.DOWN}, () => this.handleDown())
- );
- this.cleanups.push(
- addShortcut(this, {key: Key.ENTER}, () => this.handleEnter())
- );
- this.cleanups.push(
- addShortcut(this, {key: Key.ESC}, () => this.handleEscape())
- );
- this.cleanups.push(
- addShortcut(this, {key: Key.TAB}, () => this.handleTab())
- );
}
override disconnectedCallback() {
this.cursor.unsetCursor();
- for (const cleanup of this.cleanups) cleanup();
- this.cleanups = [];
super.disconnectedCallback();
}
@@ -165,8 +155,13 @@
}
override updated(changedProperties: PropertyValues) {
- if (changedProperties.has('suggestions')) {
- this.onSuggestionsChanged();
+ if (
+ changedProperties.has('suggestions') ||
+ changedProperties.has('isHidden')
+ ) {
+ if (!this.isHidden) {
+ this.computeCursorStopsAndRefit();
+ }
}
}
@@ -209,7 +204,6 @@
open() {
this.isHidden = false;
- this.onSuggestionsChanged();
}
getCurrentText() {
@@ -299,14 +293,12 @@
return this.cursor.target;
}
- onSuggestionsChanged() {
+ computeCursorStopsAndRefit() {
if (this.suggestions.length > 0) {
- if (!this.isHidden) {
- this.cursor.stops = Array.from(
- this.suggestionsDiv?.querySelectorAll('li') ?? []
- );
- this.resetCursorIndex();
- }
+ this.cursor.stops = Array.from(
+ this.suggestionsDiv?.querySelectorAll('li') ?? []
+ );
+ this.resetCursorIndex();
} else {
this.cursor.stops = [];
}
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
index 9a0ed5e..641dd2d 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
@@ -170,7 +170,7 @@
});
test('updated suggestions resets cursor stops', async () => {
- const resetStopsSpy = sinon.spy(element, 'onSuggestionsChanged');
+ const resetStopsSpy = sinon.spy(element, 'computeCursorStopsAndRefit');
element.suggestions = [];
await waitUntil(() => resetStopsSpy.called);
});
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 721349f..056dfe5 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -49,7 +49,7 @@
} from '../../../types/events';
import {fire, fireEvent} from '../../../utils/event-util';
import {assertIsDefined, assert} from '../../../utils/common-util';
-import {Key, Modifier} from '../../../utils/dom-util';
+import {Key, Modifier, whenVisible} from '../../../utils/dom-util';
import {commentsModelToken} from '../../../models/comments/comments-model';
import {sharedStyles} from '../../../styles/shared-styles';
import {subscribe} from '../../lit/subscription-controller';
@@ -811,7 +811,8 @@
// fixed. Currently diff line doesn't match commit message line, because
// of metadata in diff, which aren't in content api request.
if (this.comment.path === SpecialFilePath.COMMIT_MESSAGE) return nothing;
- if (this.isOwner) return nothing;
+ // TODO(milutin): disable user suggestions for owners, after user study.
+ // if (this.isOwner) return nothing;
return html`<gr-button
link
class="action suggestEdit"
@@ -974,6 +975,14 @@
}
}
+ override updated(changed: PropertyValues) {
+ if (changed.has('editing')) {
+ if (this.editing && !this.permanentEditingMode) {
+ whenVisible(this, () => this.textarea?.putCursorAtEnd());
+ }
+ }
+ }
+
override willUpdate(changed: PropertyValues) {
this.firstWillUpdate();
if (changed.has('editing')) {
@@ -1062,7 +1071,6 @@
this.unresolved = this.comment?.unresolved ?? true;
this.originalMessage = this.messageText;
this.originalUnresolved = this.unresolved;
- setTimeout(() => this.textarea?.putCursorAtEnd(), 1);
}
// Parent components such as the reply dialog might be interested in whether
@@ -1081,6 +1089,10 @@
return !this.messageText?.trimEnd();
}
+ override focus() {
+ this.textarea?.focus();
+ }
+
private handleEsc() {
// vim users don't like ESC to cancel/discard, so only do this when the
// comment text is empty.
@@ -1178,6 +1190,9 @@
async save() {
if (!isDraftOrUnsaved(this.comment)) throw new Error('not a draft');
+ // If it's an unsaved comment then it does not have a draftID yet which
+ // means sending another save() request will create a new draft
+ if (isUnsaved(this.comment) && this.saving) return;
try {
this.saving = true;
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-endpoints.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-endpoints.ts
index 9ced917..76b3c15 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-endpoints.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-endpoints.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {PluginApi} from '../../../api/plugin';
-import {notUndefined} from '../../../types/types';
+import {isDefined} from '../../../types/types';
import {HookApi, PluginElement} from '../../../api/hook';
// eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -161,7 +161,7 @@
return [];
}
return Array.from(new Set(modulesData.map(m => m.pluginUrl))).filter(
- notUndefined
+ isDefined
);
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index 9a114e6..b38d71b 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -302,14 +302,16 @@
return this.textarea!.textarea;
}
+ override focus() {
+ this.textarea?.textarea.focus();
+ }
+
putCursorAtEnd() {
const textarea = this.getNativeTextarea();
// Put the cursor at the end always.
textarea.selectionStart = textarea.value.length;
textarea.selectionEnd = textarea.selectionStart;
- setTimeout(() => {
- textarea.focus();
- });
+ textarea.focus();
}
private getVisibleDropdown() {
@@ -343,7 +345,7 @@
e.preventDefault();
e.stopPropagation();
this.getVisibleDropdown().cursorUp();
- this.textarea!.textarea.focus();
+ this.focus();
}
private handleDownKey(e: KeyboardEvent) {
@@ -353,7 +355,7 @@
e.preventDefault();
e.stopPropagation();
this.getVisibleDropdown().cursorDown();
- this.textarea!.textarea.focus();
+ this.focus();
}
private handleTabKey(e: KeyboardEvent) {
@@ -568,7 +570,7 @@
async handleTextChanged() {
await this.computeSuggestions();
this.openOrResetDropdown();
- this.textarea!.textarea.focus();
+ this.focus();
}
private openEmojiDropdown() {
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts
index 6b35056..965d6b1 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -753,15 +753,14 @@
patchset === ChecksPatchset.LATEST
? this.changeModel.latestPatchNum$
: this.checksSelectedPatchsetNumber$,
- this.reloadSubjects[pluginName].pipe(
- throttleTime(1000, undefined, {trailing: true, leading: true})
- ),
+ this.reloadSubjects[pluginName],
pollIntervalMs === 0 ? from([0]) : timer(0, pollIntervalMs),
this.documentVisibilityChange$,
])
.pipe(
takeWhile(_ => !!this.providers[pluginName]),
filter(_ => document.visibilityState !== 'hidden'),
+ throttleTime(500, undefined, {leading: true, trailing: true}),
switchMap(([change, patchNum]): Observable<FetchResponse> => {
if (!change || !patchNum) return of(this.empty());
if (typeof patchNum !== 'number') return of(this.empty());
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 3489c5a..a19db6b 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -108,7 +108,7 @@
assert.equal(model.changeNum, testChange._number);
});
- test('reload throttle', async () => {
+ test('fetch throttle', async () => {
const clock = sinon.useFakeTimers();
let change: ParsedChangeInfo | undefined = undefined;
model.changeModel.change$.subscribe(c => (change = c));
@@ -125,16 +125,31 @@
const testChange = createParsedChange();
model.changeModel.updateStateChange(testChange);
await waitUntil(() => change === testChange);
- clock.tick(1);
+
+ model.reload('test-plugin');
+ model.reload('test-plugin');
+ model.reload('test-plugin');
+
+ // Does not emit at 'leading' of throttle interval,
+ // because fetch() is not called when change is undefined.
+ assert.equal(fetchSpy.callCount, 0);
+
+ // 600 ms is greater than the 500 ms throttle time.
+ clock.tick(600);
+ // emits at 'trailing' of throttle interval
assert.equal(fetchSpy.callCount, 1);
- // The second reload call will be processed, but only after a 1s throttle.
model.reload('test-plugin');
- clock.tick(100);
- assert.equal(fetchSpy.callCount, 1);
- // 2000 ms is greater than the 1000 ms throttle time.
- clock.tick(2000);
+ model.reload('test-plugin');
+ model.reload('test-plugin');
+ model.reload('test-plugin');
+ // emits at 'leading' of throttle interval
assert.equal(fetchSpy.callCount, 2);
+
+ // 600 ms is greater than the 500 ms throttle time.
+ clock.tick(600);
+ // emits at 'trailing' of throttle interval
+ assert.equal(fetchSpy.callCount, 3);
});
test('triggerAction', async () => {
@@ -282,8 +297,8 @@
const testChange = createParsedChange();
model.changeModel.updateStateChange(testChange);
await waitUntil(() => change === testChange);
+ clock.tick(600); // need to wait for 500ms throttle
await waitUntilCalled(fetchSpy, 'fetch');
- clock.tick(1);
const pollCount = fetchSpy.callCount;
// polling should continue while we wait
@@ -309,6 +324,7 @@
const testChange = createParsedChange();
model.changeModel.updateStateChange(testChange);
await waitUntil(() => change === testChange);
+ clock.tick(600); // need to wait for 500ms throttle
await waitUntilCalled(fetchSpy, 'fetch');
clock.tick(1);
const pollCount = fetchSpy.callCount;
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index 7ccdf91..6a5933c 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -17,7 +17,7 @@
import {PatchSetNumber} from '../../api/rest-api';
import {FixSuggestionInfo, FixReplacementInfo} from '../../types/common';
import {OpenFixPreviewEventDetail} from '../../types/events';
-import {notUndefined} from '../../types/types';
+import {isDefined} from '../../types/types';
import {PROVIDED_FIX_ID} from '../../utils/comment-util';
import {assert, assertNever} from '../../utils/common-util';
import {fire} from '../../utils/event-util';
@@ -94,7 +94,7 @@
if (!result?.fixes) return;
const fixSuggestions = result.fixes
.map(f => rectifyFix(f, result?.checkName))
- .filter(notUndefined);
+ .filter(isDefined);
if (fixSuggestions.length === 0) return;
const eventDetail: OpenFixPreviewEventDetail = {
patchNum: result.patchset as PatchSetNumber,
@@ -116,7 +116,7 @@
if (!fix?.replacements) return undefined;
const replacements = fix.replacements
.map(rectifyReplacement)
- .filter(notUndefined);
+ .filter(isDefined);
if (replacements.length === 0) return undefined;
return {
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index 8ed4b69..da49ddb 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -30,7 +30,7 @@
import {RouterModel} from '../../services/router/router-model';
import {Finalizable} from '../../services/registry';
import {define} from '../dependency';
-import {combineLatest, forkJoin, from, Observable} from 'rxjs';
+import {combineLatest, forkJoin, from, Observable, of} from 'rxjs';
import {fire, fireAlert, fireEvent} from '../../utils/event-util';
import {CURRENT} from '../../utils/patch-set-util';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
@@ -52,7 +52,7 @@
shareReplay,
switchMap,
} from 'rxjs/operators';
-import {notUndefined} from '../../types/types';
+import {isDefined} from '../../types/types';
export interface CommentState {
/** undefined means 'still loading' */
@@ -291,11 +291,15 @@
(user, index) =>
index === users.findIndex(u => getUserId(u) === getUserId(user))
);
+ // forkJoin only emits value when the array is non-empty
+ if (uniqueUsers.length === 0) {
+ return of(uniqueUsers);
+ }
const filledUsers$: Observable<AccountInfo | undefined>[] =
uniqueUsers.map(user => from(this.accountsModel.fillDetails(user)));
return forkJoin(filledUsers$);
}),
- map(users => users.filter(notUndefined)),
+ map(users => users.filter(isDefined)),
distinctUntilChanged(deepEqual),
shareReplay(1)
);
@@ -314,11 +318,15 @@
(user, index) =>
index === users.findIndex(u => getUserId(u) === getUserId(user))
);
+ // forkJoin only emits value when the array is non-empty
+ if (uniqueUsers.length === 0) {
+ return of(uniqueUsers);
+ }
const filledUsers$: Observable<AccountInfo | undefined>[] =
uniqueUsers.map(user => from(this.accountsModel.fillDetails(user)));
return forkJoin(filledUsers$);
}),
- map(users => users.filter(notUndefined)),
+ map(users => users.filter(isDefined)),
distinctUntilChanged(deepEqual),
shareReplay(1)
);
diff --git a/polygerrit-ui/app/models/comments/comments-model_test.ts b/polygerrit-ui/app/models/comments/comments-model_test.ts
index 6cbdc63..32ea1bc 100644
--- a/polygerrit-ui/app/models/comments/comments-model_test.ts
+++ b/polygerrit-ui/app/models/comments/comments-model_test.ts
@@ -150,4 +150,40 @@
assert.deepEqual(mentionedUsers, [account]);
});
+
+ test('empty mentions are emitted', async () => {
+ const account = {
+ ...createAccountWithEmail('abcd@def.com' as EmailAddress),
+ registered_on: '2015-03-12 18:32:08.000000000' as Timestamp,
+ };
+ stubRestApi('getAccountDetails').returns(Promise.resolve(account));
+ const model = new CommentsModel(
+ getAppContext().routerModel,
+ testResolver(changeModelToken),
+ getAppContext().accountsModel,
+ getAppContext().restApiService,
+ getAppContext().reportingService
+ );
+ let mentionedUsers: AccountInfo[] = [];
+ const draft = {...createDraft(), message: 'hey @abc@def.com'};
+ model.mentionedUsersInDrafts$.subscribe(x => (mentionedUsers = x));
+ model.setState({
+ drafts: {
+ 'abc.txt': [draft],
+ },
+ discardedDrafts: [],
+ });
+
+ await waitUntil(() => mentionedUsers.length > 0);
+
+ assert.deepEqual(mentionedUsers, [account]);
+
+ model.setState({
+ drafts: {
+ 'abc.txt': [],
+ },
+ discardedDrafts: [],
+ });
+ await waitUntil(() => mentionedUsers.length === 0);
+ });
});
diff --git a/polygerrit-ui/app/models/user/user-model.ts b/polygerrit-ui/app/models/user/user-model.ts
index 2f2fe7d..428ddbb 100644
--- a/polygerrit-ui/app/models/user/user-model.ts
+++ b/polygerrit-ui/app/models/user/user-model.ts
@@ -26,7 +26,7 @@
import {Finalizable} from '../../services/registry';
import {select} from '../../utils/observable-util';
import {Model} from '../model';
-import {notUndefined} from '../../types/types';
+import {isDefined} from '../../types/types';
export interface UserState {
/**
@@ -99,17 +99,17 @@
readonly preferences$: Observable<PreferencesInfo> = select(
this.state$,
userState => userState.preferences
- ).pipe(filter(notUndefined));
+ ).pipe(filter(isDefined));
readonly diffPreferences$: Observable<DiffPreferencesInfo> = select(
this.state$,
userState => userState.diffPreferences
- ).pipe(filter(notUndefined));
+ ).pipe(filter(isDefined));
readonly editPreferences$: Observable<EditPreferencesInfo> = select(
this.state$,
userState => userState.editPreferences
- ).pipe(filter(notUndefined));
+ ).pipe(filter(isDefined));
readonly preferenceDiffViewMode$: Observable<DiffViewMode> = select(
this.preferences$,
diff --git a/polygerrit-ui/app/scripts/gr-reviewer-suggestions-provider/gr-reviewer-suggestions-provider.ts b/polygerrit-ui/app/scripts/gr-reviewer-suggestions-provider/gr-reviewer-suggestions-provider.ts
index 0bb02d8..e90c9be 100644
--- a/polygerrit-ui/app/scripts/gr-reviewer-suggestions-provider/gr-reviewer-suggestions-provider.ts
+++ b/polygerrit-ui/app/scripts/gr-reviewer-suggestions-provider/gr-reviewer-suggestions-provider.ts
@@ -20,7 +20,7 @@
import {assertNever} from '../../utils/common-util';
import {AutocompleteSuggestion} from '../../elements/shared/gr-autocomplete/gr-autocomplete';
import {allSettled, isFulfilled} from '../../utils/async-util';
-import {notUndefined, ParsedChangeInfo} from '../../types/types';
+import {isDefined, ParsedChangeInfo} from '../../types/types';
import {accountKey} from '../../utils/account-util';
import {
AccountId,
@@ -63,7 +63,7 @@
const suggestionsByChangeIndex = resultsByChangeIndex
.filter(isFulfilled)
.map(result => result.value)
- .filter(notUndefined);
+ .filter(isDefined);
if (suggestionsByChangeIndex.length !== resultsByChangeIndex.length) {
// one of the requests failed, so don't allow any suggestions.
return [];
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index d6ad434..6d45ad4 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -15,7 +15,7 @@
import {UserModel} from '../models/user/user-model';
import {queryAndAssert, query} from '../utils/common-util';
import {FlagsService} from '../services/flags/flags';
-import {Key, Modifier} from '../utils/dom-util';
+import {Key, Modifier, whenVisible} from '../utils/dom-util';
import {Observable} from 'rxjs';
import {filter, take, timeout} from 'rxjs/operators';
import {HighlightService} from '../services/highlight/highlight-service';
@@ -220,6 +220,12 @@
});
}
+export async function waitUntilVisible(element: Element): Promise<void> {
+ return new Promise(resolve => {
+ whenVisible(element, () => resolve());
+ });
+}
+
export function waitUntilCalled(stub: SinonStub | SinonSpy, name: string) {
return waitUntil(() => stub.called, `${name} was not called`);
}
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index 04052e2..8d55e36 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -20,7 +20,7 @@
} from './common';
import {AuthRequestInit} from '../services/gr-auth/gr-auth';
-export function notUndefined<T>(x: T): x is NonNullable<T> {
+export function isDefined<T>(x: T): x is NonNullable<T> {
return x !== undefined && x !== null;
}
diff --git a/tools/BUILD b/tools/BUILD
index 68f013b..e25dcc5 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -126,7 +126,7 @@
"-Xep:DoNotCallSuggester:ERROR",
"-Xep:DoNotClaimAnnotations:ERROR",
"-Xep:DoNotMock:ERROR",
- "-Xep:DoNotMockAutoValue:WARN",
+ "-Xep:DoNotMockAutoValue:ERROR",
"-Xep:DoubleBraceInitialization:ERROR",
"-Xep:DoubleCheckedLocking:ERROR",
"-Xep:DuplicateMapKeys:ERROR",
@@ -147,7 +147,7 @@
"-Xep:EqualsUsingHashCode:ERROR",
"-Xep:EqualsWrongThing:ERROR",
"-Xep:ErroneousThreadPoolConstructorChecker:ERROR",
- "-Xep:EscapedEntity:WARN",
+ "-Xep:EscapedEntity:ERROR",
"-Xep:ExpectedExceptionChecker:ERROR",
"-Xep:ExtendingJUnitAssert:ERROR",
"-Xep:ExtendsAutoValue:ERROR",
@@ -242,7 +242,7 @@
"-Xep:JavaLocalTimeGetNano:ERROR",
"-Xep:JavaPeriodGetDays:ERROR",
"-Xep:JavaTimeDefaultTimeZone:ERROR",
- "-Xep:JavaUtilDate:WARN",
+ "-Xep:JavaUtilDate:ERROR",
"-Xep:JdkObsolete:ERROR",
"-Xep:JodaConstructors:ERROR",
"-Xep:JodaDateTimeConstants:ERROR",
