Merge changes I2b80c1b1,I06fee550
* changes:
Remove Assignee from configs.
Remove assignee from Change value classes.
diff --git a/Documentation/config-submit-requirements.txt b/Documentation/config-submit-requirements.txt
index d61dbc43..ba20cea 100644
--- a/Documentation/config-submit-requirements.txt
+++ b/Documentation/config-submit-requirements.txt
@@ -160,6 +160,22 @@
link:http://www.brics.dk/automaton/[dk.brics.automaton library,role=external,window=_blank]
is used for the evaluation of such patterns.
+[[operator_committeremail]]
+committeremail:'EMAIL_PATTERN'::
++
+An operator that returns true if the change committer's email address matches a
+specific regular expression pattern. The
+link:http://www.brics.dk/automaton/[dk.brics.automaton library,role=external,window=_blank]
+is used for the evaluation of such patterns.
+
+[[operator_uploaderemail]]
+uploaderemail:'EMAIL_PATTERN'::
++
+An operator that returns true if the change uploader's primary email address
+matches a specific regular expression pattern. The
+link:http://www.brics.dk/automaton/[dk.brics.automaton library,role=external,window=_blank]
+is used for the evaluation of such patterns.
+
[[operator_distinctvoters]]
distinctvoters:'[Label1,Label2,...,LabelN],value=MAX,count>1'::
+
diff --git a/Documentation/js_licenses.txt b/Documentation/js_licenses.txt
index e2afbf5..f685af9 100644
--- a/Documentation/js_licenses.txt
+++ b/Documentation/js_licenses.txt
@@ -603,39 +603,6 @@
----
-[[ba-linkify]]
-ba-linkify
-
-* ba-linkify
-
-[[ba-linkify_license]]
-----
-Copyright (c) 2009 "Cowboy" Ben Alman
-
-Permission is hereby granted, free of charge, to any person
-obtaining a copy of this software and associated documentation
-files (the "Software"), to deal in the Software without
-restriction, including without limitation the rights to use,
-copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the
-Software is furnished to do so, subject to the following
-conditions:
-
-The above copyright notice and this permission notice shall be
-included in all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
-EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
-OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
-NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
-HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
-WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
-FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
-OTHER DEALINGS IN THE SOFTWARE.
-
-----
-
-
[[codemirror-minified]]
codemirror-minified
@@ -1274,38 +1241,6 @@
----
-[[isarray]]
-isarray
-
-* isarray
-
-[[isarray_license]]
-----
-(MIT)
-
-Copyright (c) 2013 Julian Gruber <julian@juliangruber.com>;
-
-Permission is hereby granted, free of charge, to any person obtaining a copy of
-this software and associated documentation files (the "Software"), to deal in
-the Software without restriction, including without limitation the rights to
-use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
-of the Software, and to permit persons to whom the Software is furnished to do
-so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in all
-copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-SOFTWARE.
-
-----
-
-
[[marked]]
marked
@@ -1363,7 +1298,7 @@
[[page]]
page
-* page
+* polygerrit-gr-page
[[page_license]]
----
@@ -1393,38 +1328,6 @@
----
-[[path-to-regexp]]
-path-to-regexp
-
-* path-to-regexp
-
-[[path-to-regexp_license]]
-----
-The MIT License (MIT)
-
-Copyright (c) 2014 Blake Embrey (hello@blakeembrey.com)
-
-Permission is hereby granted, free of charge, to any person obtaining a copy
-of this software and associated documentation files (the "Software"), to deal
-in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the Software is
-furnished to do so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in
-all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-THE SOFTWARE.
-
-----
-
-
[[resemblejs]]
resemblejs
diff --git a/Documentation/licenses.txt b/Documentation/licenses.txt
index 8ccbcab..be41d0c 100644
--- a/Documentation/licenses.txt
+++ b/Documentation/licenses.txt
@@ -3507,39 +3507,6 @@
----
-[[ba-linkify]]
-ba-linkify
-
-* ba-linkify
-
-[[ba-linkify_license]]
-----
-Copyright (c) 2009 "Cowboy" Ben Alman
-
-Permission is hereby granted, free of charge, to any person
-obtaining a copy of this software and associated documentation
-files (the "Software"), to deal in the Software without
-restriction, including without limitation the rights to use,
-copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the
-Software is furnished to do so, subject to the following
-conditions:
-
-The above copyright notice and this permission notice shall be
-included in all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
-EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
-OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
-NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
-HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
-WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
-FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
-OTHER DEALINGS IN THE SOFTWARE.
-
-----
-
-
[[codemirror-minified]]
codemirror-minified
@@ -4178,38 +4145,6 @@
----
-[[isarray]]
-isarray
-
-* isarray
-
-[[isarray_license]]
-----
-(MIT)
-
-Copyright (c) 2013 Julian Gruber <julian@juliangruber.com>;
-
-Permission is hereby granted, free of charge, to any person obtaining a copy of
-this software and associated documentation files (the "Software"), to deal in
-the Software without restriction, including without limitation the rights to
-use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
-of the Software, and to permit persons to whom the Software is furnished to do
-so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in all
-copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-SOFTWARE.
-
-----
-
-
[[marked]]
marked
@@ -4267,7 +4202,7 @@
[[page]]
page
-* page
+* polygerrit-gr-page
[[page_license]]
----
@@ -4297,38 +4232,6 @@
----
-[[path-to-regexp]]
-path-to-regexp
-
-* path-to-regexp
-
-[[path-to-regexp_license]]
-----
-The MIT License (MIT)
-
-Copyright (c) 2014 Blake Embrey (hello@blakeembrey.com)
-
-Permission is hereby granted, free of charge, to any person obtaining a copy
-of this software and associated documentation files (the "Software"), to deal
-in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the Software is
-furnished to do so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in
-all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-THE SOFTWARE.
-
-----
-
-
[[resemblejs]]
resemblejs
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index 9d237af..3c7ec2b 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -63,6 +63,7 @@
"//java/com/google/gerrit/pgm/util",
"//java/com/google/gerrit/truth",
"//java/com/google/gerrit/acceptance/config",
+ "//java/com/google/gerrit/acceptance/testsuite/group",
"//java/com/google/gerrit/acceptance/testsuite/project",
"//java/com/google/gerrit/server/fixes/testing",
"//java/com/google/gerrit/server/data",
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/BUILD b/java/com/google/gerrit/acceptance/testsuite/group/BUILD
new file mode 100644
index 0000000..d4f1175
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/BUILD
@@ -0,0 +1,25 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+package(default_testonly = 1)
+
+java_library(
+ name = "group",
+ srcs = glob(["*.java"]),
+ visibility = ["//visibility:public"],
+ deps = [
+ "//java/com/google/gerrit/acceptance:function",
+ "//java/com/google/gerrit/common:annotations",
+ "//java/com/google/gerrit/common:server",
+ "//java/com/google/gerrit/entities",
+ "//java/com/google/gerrit/exceptions",
+ "//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/server",
+ "//lib:guava",
+ "//lib:jgit",
+ "//lib:jgit-junit",
+ "//lib/auto:auto-value",
+ "//lib/auto:auto-value-annotations",
+ "//lib/commons:lang3",
+ "//lib/guice",
+ ],
+)
diff --git a/java/com/google/gerrit/entities/KeyUtil.java b/java/com/google/gerrit/entities/KeyUtil.java
index 0f14cd9..4aec7ac 100644
--- a/java/com/google/gerrit/entities/KeyUtil.java
+++ b/java/com/google/gerrit/entities/KeyUtil.java
@@ -66,7 +66,13 @@
return r.toString();
}
- public static String decode(final String key) {
+ public static String decode(String key) {
+ // URLs use percentage encoding which replaces unsafe ASCII characters with a '%' followed by
+ // two hexadecimal digits. If there is '%' that is not followed by two hexadecimal digits
+ // the code below fails with an IllegalArgumentException. To prevent this replace any '%'
+ // that is not followed by two hexadecimal digits by "%25", which is the URL encoding for '%'.
+ key = key.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
+
if (key.indexOf('%') < 0) {
return key.replace('+', ' ');
}
diff --git a/java/com/google/gerrit/extensions/restapi/IdString.java b/java/com/google/gerrit/extensions/restapi/IdString.java
index b2538fa..a69919f 100644
--- a/java/com/google/gerrit/extensions/restapi/IdString.java
+++ b/java/com/google/gerrit/extensions/restapi/IdString.java
@@ -38,7 +38,16 @@
/** Returns the decoded value of the string. */
public String get() {
- return Url.decode(urlEncoded);
+ String data = urlEncoded;
+
+ // URLs use percentage encoding which replaces unsafe ASCII characters with a '%' followed by
+ // two hexadecimal digits. If there is '%' that is not followed by two hexadecimal digits
+ // Url.decode(String) fails with an IllegalArgumentException. To prevent this replace any '%'
+ // that is not followed by two hexadecimal digits by "%25", which is the URL encoding for '%',
+ // before calling Url.decode(String).
+ data = data.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
+
+ return Url.decode(data);
}
/** Returns true if the string is the empty string. */
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 903a4c0..ad0dd8b 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -34,12 +34,14 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.edit.tree.ChangeFileContentModification;
import com.google.gerrit.server.edit.tree.DeleteFileModification;
import com.google.gerrit.server.edit.tree.RenameFileModification;
import com.google.gerrit.server.edit.tree.RestoreFileModification;
import com.google.gerrit.server.edit.tree.TreeCreator;
import com.google.gerrit.server.edit.tree.TreeModification;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
@@ -108,15 +110,15 @@
PermissionBackend permissionBackend,
ChangeEditUtil changeEditUtil,
PatchSetUtil patchSetUtil,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ GitReferenceUpdated gitReferenceUpdated) {
this.currentUser = currentUser;
this.permissionBackend = permissionBackend;
this.zoneId = gerritIdent.getZoneId();
this.changeEditUtil = changeEditUtil;
this.patchSetUtil = patchSetUtil;
this.projectCache = projectCache;
-
- noteDbEdits = new NoteDbEdits(zoneId, indexer, currentUser);
+ noteDbEdits = new NoteDbEdits(gitReferenceUpdated, zoneId, indexer, currentUser);
}
/**
@@ -173,10 +175,14 @@
notes.getChangeId(), currentPatchSet.id()));
}
- rebase(repository, changeEdit, currentPatchSet);
+ rebase(notes.getProjectName(), repository, changeEdit, currentPatchSet);
}
- private void rebase(Repository repository, ChangeEdit changeEdit, PatchSet currentPatchSet)
+ private void rebase(
+ Project.NameKey project,
+ Repository repository,
+ ChangeEdit changeEdit,
+ PatchSet currentPatchSet)
throws IOException, MergeConflictException, InvalidChangeOperationException {
RevCommit currentEditCommit = changeEdit.getEditCommit();
if (currentEditCommit.getParentCount() == 0) {
@@ -194,7 +200,13 @@
createCommit(repository, basePatchSetCommit, newTreeId, commitMessage, nowTimestamp);
noteDbEdits.baseEditOnDifferentPatchset(
- repository, changeEdit, currentPatchSet, currentEditCommit, newEditCommitId, nowTimestamp);
+ project,
+ repository,
+ changeEdit,
+ currentPatchSet,
+ currentEditCommit,
+ newEditCommitId,
+ nowTimestamp);
}
/**
@@ -719,11 +731,17 @@
private final ZoneId zoneId;
private final ChangeIndexer indexer;
private final Provider<CurrentUser> currentUser;
+ private final GitReferenceUpdated gitReferenceUpdated;
- NoteDbEdits(ZoneId zoneId, ChangeIndexer indexer, Provider<CurrentUser> currentUser) {
+ NoteDbEdits(
+ GitReferenceUpdated gitReferenceUpdated,
+ ZoneId zoneId,
+ ChangeIndexer indexer,
+ Provider<CurrentUser> currentUser) {
this.zoneId = zoneId;
this.indexer = indexer;
this.currentUser = currentUser;
+ this.gitReferenceUpdated = gitReferenceUpdated;
}
ChangeEdit createEdit(
@@ -753,6 +771,10 @@
return RefNames.refsEdit(me.getAccountId(), change.getId(), basePatchset.id());
}
+ private AccountState getUpdater() {
+ return currentUser.get().asIdentifiedUser().state();
+ }
+
ChangeEdit updateEdit(
Project.NameKey projectName,
Repository repository,
@@ -795,9 +817,11 @@
throw new IOException(message);
}
}
+ gitReferenceUpdated.fire(projectName, ru, getUpdater());
}
void baseEditOnDifferentPatchset(
+ Project.NameKey project,
Repository repository,
ChangeEdit changeEdit,
PatchSet currentPatchSet,
@@ -807,6 +831,7 @@
throws IOException {
String newEditRefName = getEditRefName(changeEdit.getChange(), currentPatchSet);
updateReferenceWithNameChange(
+ project,
repository,
changeEdit.getRefName(),
currentEditCommit,
@@ -817,6 +842,7 @@
}
private void updateReferenceWithNameChange(
+ Project.NameKey projectName,
Repository repository,
String currentRefName,
ObjectId currentObjectId,
@@ -838,6 +864,7 @@
throw new IOException("failed: " + cmd);
}
}
+ gitReferenceUpdated.fire(projectName, batchRefUpdate, getUpdater());
}
static RevCommit lookupCommit(Repository repository, ObjectId commitId) throws IOException {
diff --git a/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/java/com/google/gerrit/server/edit/ChangeEditUtil.java
index 74834ab..3474590 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditUtil.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditUtil.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.PatchSetInserter;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -70,6 +71,8 @@
private final ChangeKindCache changeKindCache;
private final PatchSetUtil psUtil;
+ private final GitReferenceUpdated gitReferenceUpdated;
+
@Inject
ChangeEditUtil(
GitRepositoryManager gitManager,
@@ -77,13 +80,15 @@
ChangeIndexer indexer,
Provider<CurrentUser> userProvider,
ChangeKindCache changeKindCache,
- PatchSetUtil psUtil) {
+ PatchSetUtil psUtil,
+ GitReferenceUpdated gitReferenceUpdated) {
this.gitManager = gitManager;
this.patchSetInserterFactory = patchSetInserterFactory;
this.indexer = indexer;
this.userProvider = userProvider;
this.changeKindCache = changeKindCache;
this.psUtil = psUtil;
+ this.gitReferenceUpdated = gitReferenceUpdated;
}
/**
@@ -237,7 +242,7 @@
return writeSquashedCommit(rw, inserter, parent, edit);
}
- private static void deleteRef(Repository repo, ChangeEdit edit) throws IOException {
+ private void deleteRef(Repository repo, ChangeEdit edit) throws IOException {
String refName = edit.getRefName();
RefUpdate ru = repo.updateRef(refName, true);
ru.setExpectedOldObjectId(edit.getEditCommit());
@@ -261,6 +266,10 @@
default:
throw new IOException(String.format("Failed to delete ref %s: %s", refName, result));
}
+ gitReferenceUpdated.fire(
+ edit.getChange().getProject(),
+ ru,
+ /* updater= */ userProvider.get().asIdentifiedUser().state());
}
private static RevCommit writeSquashedCommit(
diff --git a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
index b0e270c..2f063f6 100644
--- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
+++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
@@ -103,6 +103,13 @@
memberships.put(user, membership);
}
+ /**
+ * Remove a the memberships of the given user. No-op if the user does not have any memberships.
+ */
+ public void removeMembershipsOf(Account.Id user) {
+ memberships.remove(user);
+ }
+
@Override
public boolean handles(AccountGroup.UUID uuid) {
if (uuid != null) {
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index f60e9e5..870fc3e 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -155,6 +155,8 @@
PredicateResult predicateResult = evaluatePredicateTree(predicate, changeData);
return SubmitRequirementExpressionResult.create(expression, predicateResult);
} catch (QueryParseException | SubmitRequirementEvaluationException e) {
+ logger.atWarning().withCause(e).log(
+ "Failed to evaluate submit requirement expression: %s", expression.expressionString());
return SubmitRequirementExpressionResult.error(expression, e.getMessage());
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 4cd15df..738eab3 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1269,7 +1269,8 @@
AccountGroup.UUID groupId = g.getUUID();
GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
- if (!(groupDescription instanceof GroupDescription.Internal)) {
+ if (!(groupDescription instanceof GroupDescription.Internal)
+ || containsExernalSubGroups((GroupDescription.Internal) groupDescription)) {
return new OwnerinPredicate(args.userFactory, groupId);
}
@@ -1292,7 +1293,8 @@
AccountGroup.UUID groupId = g.getUUID();
GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
- if (!(groupDescription instanceof GroupDescription.Internal)) {
+ if (!(groupDescription instanceof GroupDescription.Internal)
+ || containsExernalSubGroups((GroupDescription.Internal) groupDescription)) {
return new UploaderinPredicate(args.userFactory, groupId);
}
@@ -1665,6 +1667,22 @@
return Predicate.and(predicates);
}
+ private boolean containsExernalSubGroups(GroupDescription.Internal internalGroup)
+ throws IOException {
+ for (AccountGroup.UUID subGroupUuid : internalGroup.getSubgroups()) {
+ GroupDescription.Basic subGroupDescription = args.groupBackend.get(subGroupUuid);
+ if (!(subGroupDescription instanceof GroupDescription.Internal)) {
+ return true;
+ }
+ boolean containsExernalSubGroups =
+ containsExernalSubGroups((GroupDescription.Internal) subGroupDescription);
+ if (containsExernalSubGroups) {
+ return true;
+ }
+ }
+ return false;
+ }
+
private Set<Account.Id> getMembers(AccountGroup.UUID g) throws IOException {
Set<Account.Id> accounts;
Set<Account.Id> allMembers =
@@ -1743,7 +1761,7 @@
return value;
}
- /** Returns {@link Account.Id} of the identified calling user. */
+ /** Returns {@link com.google.gerrit.entities.Account.Id} of the identified calling user. */
public Account.Id self() throws QueryParseException {
return args.getIdentifiedUser().getAccountId();
}
diff --git a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
index 698628e..5632c14 100644
--- a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
@@ -25,6 +25,8 @@
import com.google.gerrit.server.submitrequirement.predicate.FileEditsPredicate.FileEditsArgs;
import com.google.gerrit.server.submitrequirement.predicate.HasSubmoduleUpdatePredicate;
import com.google.gerrit.server.submitrequirement.predicate.RegexAuthorEmailPredicate;
+import com.google.gerrit.server.submitrequirement.predicate.RegexCommitterEmailPredicate;
+import com.google.gerrit.server.submitrequirement.predicate.RegexUploaderEmailPredicateFactory;
import com.google.inject.Inject;
import java.util.List;
import java.util.Locale;
@@ -60,17 +62,20 @@
private static final Splitter SUBMODULE_UPDATE_SPLITTER = Splitter.on(",");
private final FileEditsPredicate.Factory fileEditsPredicateFactory;
+ private final RegexUploaderEmailPredicateFactory regexUploaderEmailPredicateFactory;
@Inject
SubmitRequirementChangeQueryBuilder(
Arguments args,
DistinctVotersPredicate.Factory distinctVotersPredicateFactory,
FileEditsPredicate.Factory fileEditsPredicateFactory,
- HasSubmoduleUpdatePredicate.Factory hasSubmoduleUpdateFactory) {
+ HasSubmoduleUpdatePredicate.Factory hasSubmoduleUpdateFactory,
+ RegexUploaderEmailPredicateFactory regexUploaderEmailPredicateFactory) {
super(def, args);
this.distinctVotersPredicateFactory = distinctVotersPredicateFactory;
this.fileEditsPredicateFactory = fileEditsPredicateFactory;
this.hasSubmoduleUpdateFactory = hasSubmoduleUpdateFactory;
+ this.regexUploaderEmailPredicateFactory = regexUploaderEmailPredicateFactory;
}
@Override
@@ -129,6 +134,16 @@
}
@Operator
+ public Predicate<ChangeData> committerEmail(String who) throws QueryParseException {
+ return new RegexCommitterEmailPredicate(who);
+ }
+
+ @Operator
+ public Predicate<ChangeData> uploaderEmail(String who) throws QueryParseException {
+ return regexUploaderEmailPredicateFactory.create(who);
+ }
+
+ @Operator
public Predicate<ChangeData> distinctvoters(String value) throws QueryParseException {
return distinctVotersPredicateFactory.create(value);
}
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index 3d9d588..b262b46 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -259,10 +259,13 @@
/**
* Bots don't process automatic rules, the only attention set change they do is this rule: Add
- * owner and uploader when a bot votes negatively.
+ * owner and uploader when a bot votes negatively, but only if the change is open.
*/
private void botsWithNegativeLabelsAddOwnerAndUploader(
BatchUpdate bu, ChangeNotes changeNotes, ReviewInput input) {
+ if (changeNotes.getChange().isClosed()) {
+ return;
+ }
if (input.labels != null && input.labels.values().stream().anyMatch(vote -> vote < 0)) {
Account.Id uploader = changeNotes.getCurrentPatchSet().uploader();
Account.Id owner = changeNotes.getChange().getOwner();
diff --git a/java/com/google/gerrit/server/submitrequirement/predicate/RegexCommitterEmailPredicate.java b/java/com/google/gerrit/server/submitrequirement/predicate/RegexCommitterEmailPredicate.java
new file mode 100644
index 0000000..f991d31
--- /dev/null
+++ b/java/com/google/gerrit/server/submitrequirement/predicate/RegexCommitterEmailPredicate.java
@@ -0,0 +1,57 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.submitrequirement.predicate;
+
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.SubmitRequirementPredicate;
+import dk.brics.automaton.RegExp;
+import dk.brics.automaton.RunAutomaton;
+
+/**
+ * A submit requirement predicate that matches with changes having the committer email's address
+ * matching a specific regular expression pattern.
+ */
+public class RegexCommitterEmailPredicate extends SubmitRequirementPredicate {
+ protected final RunAutomaton committerEmailPattern;
+
+ public RegexCommitterEmailPredicate(String pattern) throws QueryParseException {
+ super("committeremail", pattern);
+
+ if (pattern.startsWith("^")) {
+ pattern = pattern.substring(1);
+ }
+
+ if (pattern.endsWith("$") && !pattern.endsWith("\\$")) {
+ pattern = pattern.substring(0, pattern.length() - 1);
+ }
+
+ try {
+ this.committerEmailPattern = new RunAutomaton(new RegExp(pattern).toAutomaton());
+ } catch (IllegalArgumentException e) {
+ throw new QueryParseException(String.format("invalid regular expression: %s", pattern), e);
+ }
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ return committerEmailPattern.run(cd.getCommitter().getEmailAddress());
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+}
diff --git a/java/com/google/gerrit/server/submitrequirement/predicate/RegexUploaderEmailPredicate.java b/java/com/google/gerrit/server/submitrequirement/predicate/RegexUploaderEmailPredicate.java
new file mode 100644
index 0000000..9566546
--- /dev/null
+++ b/java/com/google/gerrit/server/submitrequirement/predicate/RegexUploaderEmailPredicate.java
@@ -0,0 +1,71 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.submitrequirement.predicate;
+
+import com.google.auto.factory.AutoFactory;
+import com.google.auto.factory.Provided;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.SubmitRequirementPredicate;
+import dk.brics.automaton.RegExp;
+import dk.brics.automaton.RunAutomaton;
+import java.util.Optional;
+
+/**
+ * A submit requirement predicate that matches with changes having the uploader's email address
+ * matching a specific regular expression pattern.
+ */
+@AutoFactory
+public class RegexUploaderEmailPredicate extends SubmitRequirementPredicate {
+ protected final RunAutomaton uploaderEmailPattern;
+ private final AccountCache accountCache;
+
+ public RegexUploaderEmailPredicate(@Provided AccountCache accountCache, String pattern)
+ throws QueryParseException {
+ super("uploaderemail", pattern);
+ this.accountCache = accountCache;
+
+ if (pattern.startsWith("^")) {
+ pattern = pattern.substring(1);
+ }
+
+ if (pattern.endsWith("$") && !pattern.endsWith("\\$")) {
+ pattern = pattern.substring(0, pattern.length() - 1);
+ }
+
+ try {
+ this.uploaderEmailPattern = new RunAutomaton(new RegExp(pattern).toAutomaton());
+ } catch (IllegalArgumentException e) {
+ throw new QueryParseException(String.format("invalid regular expression: %s", pattern), e);
+ }
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ Optional<AccountState> accountState = accountCache.get(cd.currentPatchSet().uploader());
+ if (!accountState.isPresent()) {
+ return false;
+ }
+ String email = accountState.get().account().preferredEmail();
+ return email == null ? false : uploaderEmailPattern.run(email);
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+}
diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD
index 861fa00..e5234fe 100644
--- a/java/com/google/gerrit/testing/BUILD
+++ b/java/com/google/gerrit/testing/BUILD
@@ -15,6 +15,7 @@
runtime_deps = ["//java/com/google/gerrit/index/testing"],
deps = [
"//java/com/google/gerrit/acceptance/config",
+ "//java/com/google/gerrit/acceptance/testsuite/group",
"//java/com/google/gerrit/acceptance/testsuite/project",
"//java/com/google/gerrit/auth",
"//java/com/google/gerrit/common:annotations",
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 936b448..e86fd09 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -21,6 +21,8 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperationsImpl;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperationsImpl;
import com.google.gerrit.auth.AuthModule;
@@ -279,6 +281,7 @@
install(new ConfigExperimentFeaturesModule());
bind(ProjectOperations.class).to(ProjectOperationsImpl.class);
+ bind(GroupOperations.class).to(GroupOperationsImpl.class);
bind(TestGroupBackend.class).in(SINGLETON);
DynamicSet.bind(binder(), GroupBackend.class).to(TestGroupBackend.class);
}
diff --git a/javatests/com/google/gerrit/acceptance/BUILD b/javatests/com/google/gerrit/acceptance/BUILD
index 75c90f2..fe451c4 100644
--- a/javatests/com/google/gerrit/acceptance/BUILD
+++ b/javatests/com/google/gerrit/acceptance/BUILD
@@ -5,6 +5,7 @@
srcs = glob(["**/*.java"]),
deps = [
"//java/com/google/gerrit/acceptance:lib",
+ "//java/com/google/gerrit/acceptance/testsuite/group",
"//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//java/com/google/gerrit/truth",
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 443e064..1d2ddc2 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -1488,6 +1488,64 @@
}
@Test
+ public void robotReviewWithNegativeLabelDoesntAddOwnerIfChangeIsMerged() throws Exception {
+ TestAccount robot =
+ accountCreator.create(
+ "robot2", "robot2@example.com", "Ro Bot", "Ro", ServiceUserClassifier.SERVICE_USERS);
+
+ PushOneCommit.Result r = createChange();
+
+ // The robot votes with Code-Review-1 on patch set 1.
+ // Without this vote the robot cannot (re-)apply a negative vote on the change after it was
+ // merged change later.
+ requestScopeOperations.setApiUser(robot.id());
+ change(r).revision(1).review(ReviewInput.dislike());
+
+ // Amend the change so that patch set 2 gets created.
+ requestScopeOperations.setApiUser(admin.id());
+ amendChange(r.getChangeId()).assertOkStatus();
+
+ // Approve the change.
+ approve(r.getChangeId());
+
+ // User adds a comment so that the admin user is added to the attention set.
+ // This has to be a comment from a user, since comments from robots do not trigger attention set
+ // updates.
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message = "A comment";
+ change(r).current().review(reviewInput);
+
+ // Verify that the admin user was added to the attention set.
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Someone else replied on the change");
+
+ // Submit the change.
+ requestScopeOperations.setApiUser(admin.id());
+ change(r).current().submit();
+
+ // Verify that the attention set was cleared on submit.
+ attentionSet = Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Change was submitted");
+
+ // Re-apply the negative robot vote on patch set 1.
+ // Note it's possible to a apply a negative vote on merged changes if it wasn't already present
+ // since we disallow downgrading votes on merged changes (e.g. downgrade from not present aka 0
+ // to -1 is not allowed).
+ requestScopeOperations.setApiUser(robot.id());
+ change(r).revision(1).review(ReviewInput.dislike());
+
+ // Verify that re-applying the negative robot vote on patch set 1 didn't add the admin user
+ // back to the attention set.
+ attentionSet = Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+ assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Change was submitted");
+ }
+
+ @Test
public void robotCommentDoesNotAddOwnerOnClosedChanges() throws Exception {
TestAccount robot =
accountCreator.create(
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
index 06e24ab..779d8eb 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
@@ -19,6 +19,7 @@
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.restapi.IdString;
import org.junit.Test;
public class ChangeIdIT extends AbstractDaemonTest {
@@ -47,6 +48,13 @@
}
@Test
+ public void invalidProjectChangeNumberReturnsNotFound() throws Exception {
+ RestResponse res =
+ adminRestSession.get(changeDetail(IdString.fromDecoded("<%=FOO%>~1").encoded()));
+ res.assertNotFound();
+ }
+
+ @Test
public void changeNumberReturnsChange() throws Exception {
PushOneCommit.Result c = createChange();
RestResponse res = adminRestSession.get(changeDetail(getNumericChangeId(c.getChangeId())));
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
index b94ea37..ca7c3c5 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -109,4 +109,13 @@
.fromJson(r.getReader(), new TypeToken<Map<String, ProjectAccessInfo>>() {}.getType());
assertThat(infoByProject.keySet()).containsExactly(project.get());
}
+
+ @Test
+ public void listAccess_invalidProject() throws Exception {
+ String invalidProject = "<%=FOO%>";
+ RestResponse r =
+ adminRestSession.get("/access/?project=" + IdString.fromDecoded(invalidProject));
+ r.assertNotFound();
+ assertThat(r.getEntityContent()).isEqualTo(invalidProject);
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
index d3c4949..9e27e93 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/SubmitRequirementsEvaluatorIT.java
@@ -493,6 +493,74 @@
SubmitRequirementResult.Status.UNSATISFIED);
}
+ @Test
+ public void byCommitterEmail() throws Exception {
+ TestAccount user2 =
+ accountCreator.create("Foo", "user@example.com", "User", /* displayName = */ null);
+ requestScopeOperations.setApiUser(user2.id());
+ ChangeInfo info =
+ gApi.changes().create(new ChangeInput(project.get(), "master", "Test Change")).get();
+ ChangeData cd =
+ changeQueryProvider
+ .get()
+ .byLegacyChangeId(Change.Id.tryParse(Integer.toString(info._number)).get())
+ .get(0);
+
+ // Match by email works
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "committeremail:\"^.*@example\\.com\"",
+ SubmitRequirementResult.Status.SATISFIED);
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "committeremail:\"^user@.*\\.com\"",
+ SubmitRequirementResult.Status.SATISFIED);
+
+ // Match by name does not work
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "committeremail:\"^Foo$\"",
+ SubmitRequirementResult.Status.UNSATISFIED);
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "committeremail:\"^User$\"",
+ SubmitRequirementResult.Status.UNSATISFIED);
+ }
+
+ @Test
+ public void byUploaderEmail() throws Exception {
+ TestAccount user2 =
+ accountCreator.create("Foo", "user@example.com", "User", /* displayName = */ null);
+ requestScopeOperations.setApiUser(user2.id());
+ ChangeInfo info =
+ gApi.changes().create(new ChangeInput(project.get(), "master", "Test Change")).get();
+ ChangeData cd =
+ changeQueryProvider
+ .get()
+ .byLegacyChangeId(Change.Id.tryParse(Integer.toString(info._number)).get())
+ .get(0);
+
+ // Match by email works
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "uploaderemail:\"^.*@example\\.com\"",
+ SubmitRequirementResult.Status.SATISFIED);
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "uploaderemail:\"^user@.*\\.com\"",
+ SubmitRequirementResult.Status.SATISFIED);
+
+ // Match by name does not work
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "uploaderemail:\"^Foo$\"",
+ SubmitRequirementResult.Status.UNSATISFIED);
+ checkSubmitRequirementResult(
+ cd,
+ /* submittabilityExpr= */ "uploaderemail:\"^User$\"",
+ SubmitRequirementResult.Status.UNSATISFIED);
+ }
+
private void checkSubmitRequirementResult(
ChangeData cd, String submittabilityExpr, SubmitRequirementResult.Status expectedStatus) {
SubmitRequirement sr =
diff --git a/javatests/com/google/gerrit/extensions/restapi/IdStringTest.java b/javatests/com/google/gerrit/extensions/restapi/IdStringTest.java
new file mode 100644
index 0000000..3a92864
--- /dev/null
+++ b/javatests/com/google/gerrit/extensions/restapi/IdStringTest.java
@@ -0,0 +1,28 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.restapi;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import org.junit.Test;
+
+/** Unit tests for {@link IdString}. */
+public class IdStringTest {
+ @Test
+ public void decodeStringWithPercentageThatIsNotFollowedByTwoHexadecimalDigits() throws Exception {
+ String s = "<%=FOO%>";
+ assertThat(IdString.fromUrl(s).get()).isEqualTo(s);
+ }
+}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 0586542..96a8dea 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -51,6 +51,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.FakeSubmitRule;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.RawInputUtil;
@@ -58,6 +59,7 @@
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.GroupDescription;
import com.google.gerrit.entities.GroupReference;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
@@ -203,6 +205,7 @@
@Inject protected AuthRequest.Factory authRequestFactory;
@Inject protected ExternalIdFactory externalIdFactory;
@Inject protected ProjectOperations projectOperations;
+ @Inject protected GroupOperations groupOperations;
@Inject private ProjectConfig.Factory projectConfigFactory;
@@ -782,6 +785,41 @@
assertQuery("ownerin:\"Registered Users\"", change3, change2, change1);
assertQuery("ownerin:\"Registered Users\" project:repo", change3, change2, change1);
assertQuery("ownerin:\"Registered Users\" status:merged", change3);
+
+ GroupDescription.Basic externalGroup = testGroupBackend.create("External Group");
+ try {
+ testGroupBackend.setMembershipsOf(
+ user2, new ListGroupMembership(ImmutableList.of(externalGroup.getGroupUUID())));
+
+ assertQuery(
+ "ownerin:\"" + TestGroupBackend.PREFIX + externalGroup.getName() + "\"",
+ change3,
+ change2);
+
+ String nameOfGroupThatContainsExternalGroupAsSubgroup = "test-group-1";
+ AccountGroup.UUID uuidOfGroupThatContainsExternalGroupAsSubgroup =
+ groupOperations
+ .newGroup()
+ .name(nameOfGroupThatContainsExternalGroupAsSubgroup)
+ .addSubgroup(externalGroup.getGroupUUID())
+ .create();
+ assertQuery(
+ "ownerin:\"" + nameOfGroupThatContainsExternalGroupAsSubgroup + "\"", change3, change2);
+
+ String nameOfGroupThatContainsExternalGroupAsSubSubgroup = "test-group-2";
+ groupOperations
+ .newGroup()
+ .name(nameOfGroupThatContainsExternalGroupAsSubSubgroup)
+ .addSubgroup(uuidOfGroupThatContainsExternalGroupAsSubgroup)
+ .create();
+ assertQuery(
+ "ownerin:\"" + nameOfGroupThatContainsExternalGroupAsSubSubgroup + "\"",
+ change3,
+ change2);
+ } finally {
+ testGroupBackend.removeMembershipsOf(user2);
+ testGroupBackend.remove(externalGroup.getGroupUUID());
+ }
}
@Test
@@ -798,6 +836,37 @@
assertQuery("uploaderin:Administrators");
assertQuery("uploaderin:\"Registered Users\"", change1);
+
+ GroupDescription.Basic externalGroup = testGroupBackend.create("External Group");
+ try {
+ testGroupBackend.setMembershipsOf(
+ user2, new ListGroupMembership(ImmutableList.of(externalGroup.getGroupUUID())));
+
+ assertQuery(
+ "uploaderin:\"" + TestGroupBackend.PREFIX + externalGroup.getName() + "\"", change1);
+
+ String nameOfGroupThatContainsExternalGroupAsSubgroup = "test-group-1";
+ AccountGroup.UUID uuidOfGroupThatContainsExternalGroupAsSubgroup =
+ groupOperations
+ .newGroup()
+ .name(nameOfGroupThatContainsExternalGroupAsSubgroup)
+ .addSubgroup(externalGroup.getGroupUUID())
+ .create();
+ assertQuery("uploaderin:\"" + nameOfGroupThatContainsExternalGroupAsSubgroup + "\"", change1);
+
+ String nameOfGroupThatContainsExternalGroupAsSubSubgroup = "test-group-2";
+ groupOperations
+ .newGroup()
+ .name(nameOfGroupThatContainsExternalGroupAsSubSubgroup)
+ .addSubgroup(uuidOfGroupThatContainsExternalGroupAsSubgroup)
+ .create();
+ assertQuery(
+ "uploaderin:\"" + nameOfGroupThatContainsExternalGroupAsSubSubgroup + "\"", change1);
+
+ } finally {
+ testGroupBackend.removeMembershipsOf(user2);
+ testGroupBackend.remove(externalGroup.getGroupUUID());
+ }
}
@Test
@@ -3829,14 +3898,6 @@
}
@Test
- public void byOwnerInvalidQuery() throws Exception {
- repo = createAndOpenProject("repo");
- insert("repo", newChange(repo), userId);
- String nameEmail = user.asIdentifiedUser().getNameEmail();
- assertQuery("owner: \"" + nameEmail + "\"\\");
- }
-
- @Test
public void byDeletedChange() throws Exception {
repo = createAndOpenProject("repo");
Change change = insert("repo", newChange(repo));
diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD
index 32a646e..57a3c4b 100644
--- a/javatests/com/google/gerrit/server/query/change/BUILD
+++ b/javatests/com/google/gerrit/server/query/change/BUILD
@@ -19,6 +19,7 @@
deps = [
"//java/com/google/gerrit/acceptance:lib",
"//java/com/google/gerrit/acceptance/config",
+ "//java/com/google/gerrit/acceptance/testsuite/group",
"//java/com/google/gerrit/acceptance/testsuite/project",
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/common:server",
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 5cae012..7f383f9 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -59,16 +59,9 @@
}
@Test
- @Override
- public void byOwnerInvalidQuery() throws Exception {
- repo = createAndOpenProject("repo");
- Change change1 = insert("repo", newChange(repo), userId);
- String nameEmail = user.asIdentifiedUser().getNameEmail();
-
+ public void invalidQuery() throws Exception {
BadRequestException thrown =
- assertThrows(
- BadRequestException.class,
- () -> assertQuery("owner: \"" + nameEmail + "\"\\", change1));
+ assertThrows(BadRequestException.class, () -> newQuery("\\").get());
assertThat(thrown).hasMessageThat().contains("Cannot create full-text query with value: \\");
}
diff --git a/package.json b/package.json
index ae1bb2f..362b9dc 100644
--- a/package.json
+++ b/package.json
@@ -35,8 +35,8 @@
"scripts": {
"setup": "yarn && yarn --cwd=polygerrit-ui && yarn --cwd=polygerrit-ui/app",
"clean": "git clean -fdx && bazel clean --expunge",
- "compile:local": "tsc --project ./polygerrit-ui/app/tsconfig.json",
- "compile:watch": "npm run compile:local -- --preserveWatchOutput --watch",
+ "compile": "tsc --project ./polygerrit-ui/app/tsconfig.json",
+ "compile:watch": "npm run compile -- --preserveWatchOutput --watch",
"start": "run-p -rl compile:watch start:server",
"start:server": "web-dev-server",
"test": "yarn --cwd=polygerrit-ui test",
@@ -50,7 +50,8 @@
"safe_bazelisk": "if which bazelisk >/dev/null; then bazel_bin=bazelisk; else bazel_bin=bazel; fi && $bazel_bin",
"eslint": "npm run safe_bazelisk test polygerrit-ui/app:lint_test",
"eslintfix": "npm run safe_bazelisk run polygerrit-ui/app:lint_bin -- -- --fix $(pwd)/polygerrit-ui/app",
- "litlint": "npm run safe_bazelisk run polygerrit-ui/app:lit_analysis"
+ "litlint": "npm run safe_bazelisk run polygerrit-ui/app:lit_analysis",
+ "lint": "eslint -c polygerrit-ui/app/.eslintrc.js --ignore-path polygerrit-ui/app/.eslintignore polygerrit-ui/app"
},
"repository": {
"type": "git",
diff --git a/plugins/package.json b/plugins/package.json
index 331a417..b7e373a 100644
--- a/plugins/package.json
+++ b/plugins/package.json
@@ -3,7 +3,7 @@
"description": "Gerrit Code Review - frontend plugin dependencies, each plugin may depend on a subset of these",
"browser": true,
"dependencies": {
- "@gerritcodereview/typescript-api": "3.7.0",
+ "@gerritcodereview/typescript-api": "3.8.0",
"@polymer/decorators": "^3.0.0",
"@polymer/polymer": "^3.4.1",
"@open-wc/testing": "^3.1.6",
diff --git a/plugins/yarn.lock b/plugins/yarn.lock
index 368b3e0..010bf92 100644
--- a/plugins/yarn.lock
+++ b/plugins/yarn.lock
@@ -35,10 +35,10 @@
dependencies:
"@types/chai" "^4.2.12"
-"@gerritcodereview/typescript-api@3.7.0":
- version "3.7.0"
- resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.7.0.tgz#ae3886b5c4ddc6a02659a11d577e1df0b6158727"
- integrity sha512-8zeZClN1gur+Isrn02bRXJ0wUjYvK99jQxg36ZbDelrGDglXKddf8QQkZmaH9sYIRcCFDLlh5+ZlRUTcXTuDVA==
+"@gerritcodereview/typescript-api@3.8.0":
+ version "3.8.0"
+ resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.8.0.tgz#2e418b814d7451c40365b2dc4f88e9965ece0769"
+ integrity sha512-wUkIWUx99Rj1vxRYQISxyzN0nplqu7t5sRDyJ8R3yNNkvALQAMC6Whj63qzCsZsymVFzC5up3y+ZVxaeh7b+xA==
"@lit/reactive-element@^1.0.0", "@lit/reactive-element@^1.3.0", "@lit/reactive-element@^1.4.0":
version "1.4.1"
diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md
index 510ce54..a89c0b7 100644
--- a/polygerrit-ui/README.md
+++ b/polygerrit-ui/README.md
@@ -189,7 +189,7 @@
Compiling code:
```sh
# Compile frontend once to check for type errors:
-yarn compile:local
+yarn compile
# Watch mode:
yarn compile:watch
diff --git a/polygerrit-ui/app/.eslintrc.js b/polygerrit-ui/app/.eslintrc.js
index c519465..89259dc 100644
--- a/polygerrit-ui/app/.eslintrc.js
+++ b/polygerrit-ui/app/.eslintrc.js
@@ -399,19 +399,6 @@
},
},
{
- files: ['test/functional/**/*.js'],
- // Settings for functional tests. These scripts are node scripts.
- // Turn off "no-undef" to allow any global variable
- env: {
- browser: false,
- node: true,
- es6: false,
- },
- rules: {
- 'no-undef': 'off',
- },
- },
- {
files: ['*_html.js', 'gr-icons.js', '*-theme.js', '*-styles.js'],
rules: {
'max-len': 'off',
diff --git a/polygerrit-ui/app/api/package.json b/polygerrit-ui/app/api/package.json
index 79c8bb6..7df755b 100644
--- a/polygerrit-ui/app/api/package.json
+++ b/polygerrit-ui/app/api/package.json
@@ -1,9 +1,9 @@
{
"name": "@gerritcodereview/typescript-api",
- "version": "3.7.0",
+ "version": "3.8.0",
"description": "Gerrit Code Review - TypeScript API",
"homepage": "https://www.gerritcodereview.com/",
"browser": true,
"dependencies": {},
"license": "Apache-2.0"
-}
+}
\ No newline at end of file
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts
index 312f384..bef3166 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.ts
@@ -513,7 +513,6 @@
test('obsolete column in preferences not visible', () => {
assert.isTrue(element.isColumnEnabled('Subject'));
- assert.isFalse(element.isColumnEnabled('Assignee'));
});
test('showStar and showNumber', async () => {
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 b726292..77a51db 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
@@ -70,6 +70,8 @@
const DETAILS_QUOTA: Map<RunStatus | Category, number> = new Map();
DETAILS_QUOTA.set(Category.ERROR, 7);
DETAILS_QUOTA.set(Category.WARNING, 2);
+DETAILS_QUOTA.set(Category.INFO, 2);
+DETAILS_QUOTA.set(Category.SUCCESS, 2);
DETAILS_QUOTA.set(RunStatus.RUNNING, 2);
@customElement('gr-change-summary')
@@ -417,8 +419,27 @@
if (hasResultsOf(run, category)) return true;
return category === Category.SUCCESS && hasCompletedWithoutResults(run);
});
+ const hasRunning = this.runs.some(isRunningOrScheduled);
+ const hasWarning = this.runs.some(run =>
+ hasResultsOf(run, Category.WARNING)
+ );
+ const hasError = this.runs.some(run => hasResultsOf(run, Category.ERROR));
const count = (run: CheckRun) => getResultsOf(run, category);
- if (category === Category.SUCCESS || category === Category.INFO) {
+
+ // Sometimes INFO and SUCCESS results should not consume much UI space and
+ // not grab any attention, e.g. when there are errors. Then let's
+ // aggressively collapse them into one small chip. But if INFO and SUCCESS
+ // is all we have, then make use of the one line we have and show expanded
+ // chips.
+ if (
+ category === Category.SUCCESS &&
+ (hasRunning || hasError || hasWarning || runs.length > 3)
+ ) {
+ return this.renderChecksChipsCollapsed(runs, category, count);
+ } else if (
+ category === Category.INFO &&
+ (hasRunning || hasError || runs.length > 3)
+ ) {
return this.renderChecksChipsCollapsed(runs, category, count);
}
return this.renderChecksChipsExpanded(runs, category);
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts
index 05036ab..3c5371f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary_test.ts
@@ -6,13 +6,15 @@
import '../../../test/common-test-setup';
import {fixture, html, assert} from '@open-wc/testing';
import {GrChangeSummary} from './gr-change-summary';
-import {queryAndAssert} from '../../../utils/common-util';
+import {queryAll, queryAndAssert} from '../../../utils/common-util';
import {fakeRun0} from '../../../models/checks/checks-fakes';
import {
createAccountWithEmail,
+ createCheckResult,
createComment,
createCommentThread,
createDraft,
+ createRun,
} from '../../../test/test-data-generators';
import {stubFlags} from '../../../test/test-utils';
import {Timestamp} from '../../../api/rest-api';
@@ -22,6 +24,9 @@
CommentsModel,
commentsModelToken,
} from '../../../models/comments/comments-model';
+import {GrChecksChip} from './gr-checks-chip';
+import {CheckRun} from '../../../models/checks/checks-model';
+import {Category, RunStatus} from '../../../api/checks';
suite('gr-change-summary test', () => {
let element: GrChangeSummary;
@@ -117,6 +122,73 @@
);
});
+ suite('checks summary', () => {
+ const checkSummary = async (runs: CheckRun[], texts: string[]) => {
+ element.runs = runs;
+ element.showChecksSummary = true;
+ await element.updateComplete;
+ const chips = queryAll<GrChecksChip>(element, 'gr-checks-chip') ?? [];
+ assert.deepEqual(
+ [...chips].map(c => `${c.statusOrCategory} ${c.text}`),
+ texts
+ );
+ };
+
+ test('single success', async () => {
+ checkSummary([createRun()], ['SUCCESS test-name']);
+ });
+
+ test('single running', async () => {
+ checkSummary(
+ [createRun({status: RunStatus.RUNNING})],
+ ['RUNNING test-name']
+ );
+ });
+
+ test('single info', async () => {
+ checkSummary(
+ [
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.INFO})],
+ }),
+ ],
+ ['INFO test-name']
+ );
+ });
+
+ test('single of each collapses INFO and SUCCESS', async () => {
+ checkSummary(
+ [
+ createRun({status: RunStatus.RUNNING}),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.SUCCESS})],
+ }),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.INFO})],
+ }),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.WARNING})],
+ }),
+ createRun({
+ status: RunStatus.COMPLETED,
+ results: [createCheckResult({category: Category.ERROR})],
+ }),
+ ],
+ [
+ 'ERROR test-name',
+ 'WARNING test-name',
+ 'INFO 1',
+ 'SUCCESS 1',
+ 'RUNNING test-name',
+ ]
+ );
+ });
+ });
+
test('renders mentions summary', async () => {
stubFlags('isEnabled').returns(true);
// recreate element so that flag protected subscriptions are added
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index d4627e2..72a6a3a 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1259,7 +1259,7 @@
@click=${(e: MouseEvent) => {
// We don't want to handle clicks on the star or the <a> link.
// Calling `stopPropagation()` from the click handler of <a> is not an
- // option, because then the click does not reach the top-level page.js
+ // option, because then the click does not reach the top-level gr-page
// click handler and would result is a full page reload.
if ((e.target as HTMLElement)?.nodeName !== 'GR-BUTTON') return;
this.copyLinksDropdown?.toggleDropdown();
@@ -2080,13 +2080,7 @@
// Private but used in tests.
viewStateChanged() {
if (!this.viewState) return;
-
- if (this.isChangeObsolete()) {
- // Tell the app element that we are not going to handle the new change
- // number and that they have to create a new change view.
- fireEvent(this, EventType.RECREATE_CHANGE_VIEW);
- return;
- }
+ if (this.isChangeObsolete()) return;
if (this.viewState.basePatchNum === undefined)
this.viewState.basePatchNum = PARENT;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 9b78e64..ac4c1f9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -1557,21 +1557,6 @@
assert.isOk(element.patchRange?.patchNum);
});
- test('do not handle new change numbers', async () => {
- const recreateSpy = sinon.spy();
- element.addEventListener('recreate-change-view', recreateSpy);
-
- const value: ChangeViewState = createChangeViewState();
- element.viewState = value;
- await element.updateComplete;
- assert.isFalse(recreateSpy.calledOnce);
-
- value.changeNum = 555111333 as NumericChangeId;
- element.viewState = {...value};
- await element.updateComplete;
- assert.isTrue(recreateSpy.calledOnce);
- });
-
test('related changes are updated when loadData is called', async () => {
await element.updateComplete;
const relatedChanges = element.shadowRoot!.querySelector(
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 218594b..f3c23aa 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
@@ -588,7 +588,7 @@
border: 1px solid var(--border-color);
border-radius: var(--border-radius);
margin-top: var(--spacing-m);
- background-color: var(--assignee-highlight-color);
+ background-color: var(--line-item-highlight-color);
}
.attentionTip div gr-icon {
margin-right: var(--spacing-s);
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-page.ts b/polygerrit-ui/app/elements/core/gr-router/gr-page.ts
new file mode 100644
index 0000000..3af8207
--- /dev/null
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-page.ts
@@ -0,0 +1,365 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * This file was originally a copy of https://github.com/visionmedia/page.js.
+ * It was converted to TypeScript and stripped off lots of code that we don't
+ * need in Gerrit. Thus we reproduce the original LICENSE in js_licenses.txt.
+ */
+
+/**
+ * This is what registered routes have to provide, see `registerRoute()` and
+ * `registerExitRoute()`.
+ * `context` provides information about the matched parameters in the URL.
+ * Then you can decide to handle the route exclusively (not calling `next()`),
+ * or to pass it on to other registered routes. Normally you would not call
+ * `next()`, because your regex matching the URL was specific enough.
+ */
+export type PageCallback = (
+ context: PageContext,
+ next: PageNextCallback
+) => void;
+
+/** See comment on `PageCallback` above. */
+export type PageNextCallback = () => void;
+
+/** Options for starting the router. */
+export interface PageOptions {
+ /**
+ * Should the router inspect the current URL and dispatch it when the router
+ * is started? Default is `true`, but can be turned off for testing.
+ */
+ dispatch: boolean;
+
+ /**
+ * The base path of the application. For Gerrit this must be set to
+ * getBaseUrl().
+ */
+ base: string;
+}
+
+/**
+ * The browser `History` API allows `pushState()` to contain an arbitrary state
+ * object. Our router only sets `path` on the state and inspects it when
+ * handling `popstate` events. This interface is internal only.
+ */
+interface PageState {
+ path?: string;
+}
+
+const clickEvent = document.ontouchstart ? 'touchstart' : 'click';
+
+export class Page {
+ /**
+ * When a new URL is dispatched all these routes are called one after another.
+ * If a route decides that it wants to handle a URL, then it does not call
+ * next().
+ */
+ private entryRoutes: PageCallback[] = [];
+
+ /**
+ * Before a new URL is dispatched exit routes for the previous URL are called.
+ * They can clean up some state for example. But they could also prevent the
+ * user from navigating away (from within the app), if they don't call next().
+ */
+ private exitRoutes: PageCallback[] = [];
+
+ /**
+ * The path that is currently being dispatched. This is used, so that we can
+ * check whether a context is still valid, i.e. ctx.path === currentPath.
+ */
+ private currentPath = '';
+
+ /**
+ * The base path of the application. For Gerrit this must be set to
+ * getBaseUrl(). For example https://gerrit.wikimedia.org/ uses r/ as its
+ * base path.
+ */
+ private base = '';
+
+ /**
+ * Is set at the beginning of start() and stop(), so that you cannot start
+ * the routing twice.
+ */
+ private running = false;
+
+ /**
+ * Keeping around the previous context for being able to call exit routes
+ * after creating a new context.
+ */
+ private prevPageContext?: PageContext;
+
+ /**
+ * We don't want to handle popstate events before the document is loaded.
+ */
+ private documentLoaded = false;
+
+ start(options: PageOptions = {dispatch: true, base: ''}) {
+ if (this.running) return;
+ this.running = true;
+ this.base = options.base;
+
+ window.document.addEventListener(clickEvent, this.clickHandler);
+ window.addEventListener('load', this.loadHandler);
+ window.addEventListener('popstate', this.popStateHandler);
+ if (document.readyState === 'complete') this.documentLoaded = true;
+
+ if (options.dispatch) {
+ const loc = window.location;
+ this.replace(loc.pathname + loc.search + loc.hash);
+ }
+ }
+
+ stop() {
+ if (!this.running) return;
+ this.currentPath = '';
+ this.running = false;
+
+ window.document.removeEventListener(clickEvent, this.clickHandler);
+ window.removeEventListener('popstate', this.popStateHandler);
+ window.removeEventListener('load', this.loadHandler);
+ }
+
+ show(path: string, push = true) {
+ const ctx = new PageContext(path, {}, this.base);
+ const prev = this.prevPageContext;
+ this.prevPageContext = ctx;
+ this.currentPath = ctx.path;
+ this.dispatch(ctx, prev);
+ if (push && !ctx.preventPush) ctx.pushState();
+ }
+
+ redirect(to: string) {
+ setTimeout(() => this.replace(to), 0);
+ }
+
+ replace(path: string, state: PageState = {}, dispatch = true) {
+ const ctx = new PageContext(path, state, this.base);
+ const prev = this.prevPageContext;
+ this.prevPageContext = ctx;
+ this.currentPath = ctx.path;
+ ctx.replaceState(); // replace before dispatching, which may redirect
+ if (dispatch) this.dispatch(ctx, prev);
+ }
+
+ dispatch(ctx: PageContext, prev?: PageContext) {
+ let j = 0;
+ const nextExit = () => {
+ const fn = this.exitRoutes[j++];
+ // First call the exit routes of the previous context. Then proceed
+ // to the entry routes for the new context.
+ if (!fn) {
+ nextEnter();
+ return;
+ }
+ fn(prev!, nextExit);
+ };
+
+ let i = 0;
+ const nextEnter = () => {
+ const fn = this.entryRoutes[i++];
+
+ // Concurrency protection. The context is not valid anymore.
+ // Stop calling any further route handlers.
+ if (ctx.path !== this.currentPath) {
+ ctx.preventPush = true;
+ return;
+ }
+
+ // You must register a route that handles everything (.*) and does not
+ // call next().
+ if (!fn) throw new Error('No route has handled the URL.');
+
+ fn(ctx, nextEnter);
+ };
+
+ if (prev) {
+ nextExit();
+ } else {
+ nextEnter();
+ }
+ }
+
+ registerRoute(re: RegExp, fn: PageCallback) {
+ this.entryRoutes.push(createRoute(re, fn));
+ }
+
+ registerExitRoute(re: RegExp, fn: PageCallback) {
+ this.exitRoutes.push(createRoute(re, fn));
+ }
+
+ loadHandler = () => {
+ setTimeout(() => (this.documentLoaded = true), 0);
+ };
+
+ clickHandler = (e: MouseEvent | TouchEvent) => {
+ if ((e as MouseEvent).button !== 0) return;
+ if (e.metaKey || e.ctrlKey || e.shiftKey) return;
+ if (e.defaultPrevented) return;
+
+ let el = e.target as HTMLAnchorElement;
+ const eventPath = e.composedPath();
+ if (eventPath) {
+ for (let i = 0; i < eventPath.length; i++) {
+ const pathEl = eventPath[i] as HTMLAnchorElement;
+ if (!pathEl.nodeName) continue;
+ if (pathEl.nodeName.toUpperCase() !== 'A') continue;
+ if (!pathEl.href) continue;
+
+ el = pathEl;
+ break;
+ }
+ }
+
+ while (el && 'A' !== el.nodeName.toUpperCase())
+ el = el.parentNode as HTMLAnchorElement;
+ if (!el || 'A' !== el.nodeName.toUpperCase()) return;
+
+ if (el.hasAttribute('download') || el.getAttribute('rel') === 'external')
+ return;
+ const link = el.getAttribute('href');
+ if (samePath(el) && (el.hash || '#' === link)) return;
+ if (link && link.indexOf('mailto:') > -1) return;
+ if (el.target) return;
+ if (!sameOrigin(el.href)) return;
+
+ let path = el.pathname + el.search + (el.hash ?? '');
+ path = path[0] !== '/' ? '/' + path : path;
+
+ const orig = path;
+ if (path.indexOf(this.base) === 0) {
+ path = path.substr(this.base.length);
+ }
+ if (this.base && orig === path && window.location.protocol !== 'file:') {
+ return;
+ }
+ e.preventDefault();
+ this.show(orig);
+ };
+
+ popStateHandler = (e: PopStateEvent) => {
+ if (!this.documentLoaded) return;
+ if (e.state) {
+ const path = e.state.path;
+ this.replace(path, e.state);
+ } else {
+ const loc = window.location;
+ this.show(loc.pathname + loc.search + loc.hash, /* push */ false);
+ }
+ };
+}
+
+function sameOrigin(href: string) {
+ if (!href) return false;
+ const url = new URL(href, window.location.toString());
+ const loc = window.location;
+ return (
+ loc.protocol === url.protocol &&
+ loc.hostname === url.hostname &&
+ loc.port === url.port
+ );
+}
+
+function samePath(url: HTMLAnchorElement) {
+ const loc = window.location;
+ return url.pathname === loc.pathname && url.search === loc.search;
+}
+
+function escapeRegExp(s: string) {
+ return s.replace(/([.+*?=^!:${}()[\]|/\\])/g, '\\$1');
+}
+
+function decodeURIComponentString(val: string | undefined | null) {
+ if (!val) return '';
+ return decodeURIComponent(val.replace(/\+/g, ' '));
+}
+
+export class PageContext {
+ /**
+ * Includes everything: base, path, query and hash.
+ */
+ canonicalPath = '';
+
+ /**
+ * Does not include base path.
+ * Does not include hash.
+ * Includes query string.
+ */
+ path = '';
+
+ /** Does not include hash. */
+ querystring = '';
+
+ hash = '';
+
+ /**
+ * Regular expression matches of capturing groups. The first entry params[0]
+ * corresponds to the first capturing group. The entire matched string is not
+ * returned in this array.
+ */
+ params: string[] = [];
+
+ /**
+ * Prevents `show()` from eventually calling `pushState()`. For example if
+ * the current context is not "valid" anymore, i.e. the URL has changed in the
+ * meantime.
+ *
+ * This is router internal state. Do not use it from routes.
+ */
+ preventPush = false;
+
+ private title = '';
+
+ constructor(
+ path: string,
+ private readonly state: PageState = {},
+ pageBase = ''
+ ) {
+ this.title = window.document.title;
+
+ if ('/' === path[0] && 0 !== path.indexOf(pageBase)) path = pageBase + path;
+ this.canonicalPath = path;
+ const re = new RegExp('^' + escapeRegExp(pageBase));
+ this.path = path.replace(re, '') || '/';
+ this.state.path = path;
+
+ const i = path.indexOf('?');
+ this.querystring =
+ i !== -1 ? decodeURIComponentString(path.slice(i + 1)) : '';
+
+ // Does the path include a hash? If yes, then remove it from path and
+ // querystring.
+ if (this.path.indexOf('#') === -1) return;
+ const parts = this.path.split('#');
+ this.path = parts[0];
+ this.hash = decodeURIComponentString(parts[1]) || '';
+ this.querystring = this.querystring.split('#')[0];
+ }
+
+ pushState() {
+ window.history.pushState(this.state, this.title, this.canonicalPath);
+ }
+
+ replaceState() {
+ window.history.replaceState(this.state, this.title, this.canonicalPath);
+ }
+}
+
+function createRoute(re: RegExp, fn: Function) {
+ return (ctx: PageContext, next: Function) => {
+ const qsIndex = ctx.path.indexOf('?');
+ const pathname = qsIndex !== -1 ? ctx.path.slice(0, qsIndex) : ctx.path;
+ const matches = re.exec(decodeURIComponent(pathname));
+ if (matches) {
+ ctx.params = matches
+ .slice(1)
+ .map(match => decodeURIComponentString(match));
+ fn(ctx, next);
+ } else {
+ next();
+ }
+ };
+}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts
new file mode 100644
index 0000000..d194bf55
--- /dev/null
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-page_test.ts
@@ -0,0 +1,118 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import '../../../test/common-test-setup';
+import {html, assert, fixture, waitUntil} from '@open-wc/testing';
+import './gr-router';
+import {Page, PageContext} from './gr-page';
+
+suite('gr-page tests', () => {
+ let page: Page;
+
+ setup(() => {
+ page = new Page();
+ page.start({dispatch: false, base: ''});
+ });
+
+ teardown(() => {
+ page.stop();
+ });
+
+ test('click handler', async () => {
+ const spy = sinon.spy();
+ page.registerRoute(/\/settings/, spy);
+ const link = await fixture<HTMLAnchorElement>(
+ html`<a href="/settings"></a>`
+ );
+ link.click();
+ assert.isTrue(spy.calledOnce);
+ });
+
+ test('register route and exit', () => {
+ const handleA = sinon.spy();
+ const handleAExit = sinon.stub();
+ page.registerRoute(/\/A/, handleA);
+ page.registerExitRoute(/\/A/, handleAExit);
+
+ page.show('/A');
+ assert.equal(handleA.callCount, 1);
+ assert.equal(handleAExit.callCount, 0);
+
+ page.show('/B');
+ assert.equal(handleA.callCount, 1);
+ assert.equal(handleAExit.callCount, 1);
+ });
+
+ test('register, show, replace', () => {
+ const handleA = sinon.spy();
+ const handleB = sinon.spy();
+ page.registerRoute(/\/A/, handleA);
+ page.registerRoute(/\/B/, handleB);
+
+ page.show('/A');
+ assert.equal(handleA.callCount, 1);
+ assert.equal(handleB.callCount, 0);
+
+ page.show('/B');
+ assert.equal(handleA.callCount, 1);
+ assert.equal(handleB.callCount, 1);
+
+ page.replace('/A');
+ assert.equal(handleA.callCount, 2);
+ assert.equal(handleB.callCount, 1);
+
+ page.replace('/B');
+ assert.equal(handleA.callCount, 2);
+ assert.equal(handleB.callCount, 2);
+ });
+
+ test('popstate browser back', async () => {
+ const handleA = sinon.spy();
+ const handleB = sinon.spy();
+ page.registerRoute(/\/A/, handleA);
+ page.registerRoute(/\/B/, handleB);
+
+ page.show('/A');
+ assert.equal(handleA.callCount, 1);
+ assert.equal(handleB.callCount, 0);
+
+ page.show('/B');
+ assert.equal(handleA.callCount, 1);
+ assert.equal(handleB.callCount, 1);
+
+ window.history.back();
+ await waitUntil(() => window.location.href.includes('/A'));
+ assert.equal(handleA.callCount, 2);
+ assert.equal(handleB.callCount, 1);
+ });
+
+ test('register pattern, check context', async () => {
+ let context: PageContext;
+ const handler = (ctx: PageContext) => (context = ctx);
+ page.registerRoute(/\/asdf\/(.*)\/qwer\/(.*)\//, handler);
+ page.stop();
+ page.start({dispatch: false, base: '/base'});
+
+ page.show('/base/asdf/1234/qwer/abcd/');
+
+ await waitUntil(() => !!context);
+ assert.equal(context!.canonicalPath, '/base/asdf/1234/qwer/abcd/');
+ assert.equal(context!.path, '/asdf/1234/qwer/abcd/');
+ assert.equal(context!.querystring, '');
+ assert.equal(context!.hash, '');
+ assert.equal(context!.params[0], '1234');
+ assert.equal(context!.params[1], 'abcd');
+
+ page.show('/asdf//qwer////?a=b#go');
+
+ await waitUntil(() => !!context);
+ assert.equal(context!.canonicalPath, '/base/asdf//qwer////?a=b#go');
+ assert.equal(context!.path, '/asdf//qwer////?a=b');
+ assert.equal(context!.querystring, 'a=b');
+ assert.equal(context!.hash, 'go');
+ assert.equal(context!.params[0], '');
+ assert.equal(context!.params[1], '//');
+ });
+});
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 3255d75..997d9d5 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -3,12 +3,7 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {
- Options,
- page,
- PageContext,
- PageNextCallback,
-} from '../../../utils/page-wrapper-utils';
+import {Page, PageOptions, PageContext, PageNextCallback} from './gr-page';
import {NavigationService} from '../gr-navigation/gr-navigation';
import {getAppContext} from '../../../services/app-context';
import {
@@ -108,7 +103,7 @@
// TODO: Move all patterns to view model files and use the `Route` interface,
// which will enforce using `RegExp` in its `urlPattern` property.
const RoutePattern = {
- ROOT: '/',
+ ROOT: /^\/$/,
DASHBOARD: /^\/dashboard\/(.+)$/,
CUSTOM_DASHBOARD: /^\/dashboard\/?$/,
@@ -302,7 +297,7 @@
private view?: GerritView;
- readonly page = page.create();
+ readonly page = new Page();
constructor(
private readonly reporting: ReportingService,
@@ -340,12 +335,7 @@
}
if (browserUrl.toString() !== stateUrl.toString()) {
- this.page.replace(
- stateUrl.toString(),
- null,
- /* init: */ false,
- /* dispatch: */ false
- );
+ this.page.replace(stateUrl.toString(), {}, /* dispatch: */ false);
}
}),
this.routerModel.routerView$.subscribe(view => (this.view = view)),
@@ -429,13 +419,13 @@
*/
redirectToLogin(returnUrl: string) {
const basePath = getBaseUrl() || '';
- this.page(
+ this.setUrl(
'/login/' + encodeURIComponent(returnUrl.substring(basePath.length))
);
}
/**
- * Hashes parsed by page.js exclude "inner" hashes, so a URL like "/a#b#c"
+ * Hashes parsed by gr-page exclude "inner" hashes, so a URL like "/a#b#c"
* is parsed to have a hash of "b" rather than "b#c". Instead, this method
* parses hashes correctly. Will return an empty string if there is no hash.
*
@@ -464,18 +454,18 @@
* @return A promise yielding the original route ctx
* (if it resolves).
*/
- redirectIfNotLoggedIn(ctx: PageContext) {
+ redirectIfNotLoggedIn(path: string) {
return this.restApiService.getLoggedIn().then(loggedIn => {
if (loggedIn) {
return Promise.resolve();
} else {
- this.redirectToLogin(ctx.canonicalPath);
+ this.redirectToLogin(path);
return Promise.reject(new Error());
}
});
}
- /** Page.js middleware that warms the REST API's logged-in cache line. */
+ /** gr-page middleware that warms the REST API's logged-in cache line. */
private loadUserMiddleware(_: PageContext, next: PageNextCallback) {
this.restApiService.getLoggedIn().then(() => {
next();
@@ -485,11 +475,10 @@
/**
* Map a route to a method on the router.
*
- * @param pattern The page.js pattern for the route.
+ * @param pattern The regex pattern for the route.
* @param handlerName The method name for the handler. If the
* route is matched, the handler will be executed with `this` referring
- * to the component. Its return value will be discarded so that it does
- * not interfere with page.js.
+ * to the component. Its return value will be discarded.
* TODO: Get rid of this parameter. This is really not something that the
* router wants to be concerned with. The reporting service and the view
* models should figure that out between themselves.
@@ -499,24 +488,23 @@
* redirect specifies the matched URL to be used after successful auth.
*/
mapRoute(
- pattern: string | RegExp,
+ pattern: RegExp,
handlerName: string,
handler: (ctx: PageContext) => void,
authRedirect?: boolean
) {
- this.page(
- pattern,
- (ctx, next) => this.loadUserMiddleware(ctx, next),
- ctx => {
- this.reporting.locationChanged(handlerName);
- const promise = authRedirect
- ? this.redirectIfNotLoggedIn(ctx)
- : Promise.resolve();
- promise.then(() => {
- handler(ctx);
- });
- }
+ this.page.registerRoute(pattern, (ctx, next) =>
+ this.loadUserMiddleware(ctx, next)
);
+ this.page.registerRoute(pattern, ctx => {
+ this.reporting.locationChanged(handlerName);
+ const promise = authRedirect
+ ? this.redirectIfNotLoggedIn(ctx.canonicalPath)
+ : Promise.resolve();
+ promise.then(() => {
+ handler(ctx);
+ });
+ });
}
/**
@@ -583,16 +571,11 @@
}
_testOnly_startRouter() {
- this.startRouter({dispatch: false, popstate: false});
+ this.startRouter({dispatch: false, base: getBaseUrl()});
}
- startRouter(opts: Options = {}) {
- const base = getBaseUrl();
- if (base) {
- this.page.base(base);
- }
-
- this.page.exit('*', (_, next) => {
+ startRouter(opts: PageOptions = {dispatch: true, base: getBaseUrl()}) {
+ this.page.registerExitRoute(/(.*)/, (_, next) => {
if (!this._isRedirecting) {
this.reporting.beforeLocationChanged();
}
@@ -603,7 +586,7 @@
// Remove the tracking param 'usp' (User Source Parameter) from the URL,
// just to have users look at cleaner URLs.
- this.page((ctx, next) => {
+ this.page.registerRoute(/(.*)/, (ctx, next) => {
if (window.URLSearchParams) {
const pathname = toPathname(ctx.canonicalPath);
const searchParams = toSearchParams(ctx.canonicalPath);
@@ -619,7 +602,7 @@
});
// Middleware
- this.page((ctx, next) => {
+ this.page.registerRoute(/(.*)/, (ctx, next) => {
document.body.scrollTop = 0;
if (ctx.hash.match(RoutePattern.PLUGIN_SCREEN)) {
@@ -937,7 +920,7 @@
// For backward compatibility with GWT links.
if (hash) {
// In certain login flows the server may redirect to a hash without
- // a leading slash, which page.js doesn't handle correctly.
+ // a leading slash, which gr-page doesn't handle correctly.
if (hash[0] !== '/') {
hash = '/' + hash;
}
@@ -1087,7 +1070,7 @@
const state: AdminViewState = {
view: GerritView.ADMIN,
adminView: AdminChildView.GROUPS,
- offset: ctx.params[2] ?? '0',
+ offset: ctx.params[2] || '0',
filter: ctx.params[1] ?? null,
openCreateModal:
!ctx.params[1] && !ctx.params[2] && ctx.hash === 'create',
@@ -1184,7 +1167,7 @@
view: GerritView.REPO,
detail: RepoDetailView.BRANCHES,
repo: ctx.params[0] as RepoName,
- offset: ctx.params[2] ?? '0',
+ offset: ctx.params[2] || '0',
filter: ctx.params[1] ?? null,
};
// Note that router model view must be updated before view models.
@@ -1197,7 +1180,7 @@
view: GerritView.REPO,
detail: RepoDetailView.TAGS,
repo: ctx.params[0] as RepoName,
- offset: ctx.params[2] ?? '0',
+ offset: ctx.params[2] || '0',
filter: ctx.params[1] ?? null,
};
// Note that router model view must be updated before view models.
@@ -1209,7 +1192,7 @@
const state: AdminViewState = {
view: GerritView.ADMIN,
adminView: AdminChildView.REPOS,
- offset: ctx.params[2] ?? '0',
+ offset: ctx.params[2] || '0',
filter: ctx.params[1] ?? null,
openCreateModal:
!ctx.params[1] && !ctx.params[2] && ctx.hash === 'create',
@@ -1239,7 +1222,7 @@
const state: AdminViewState = {
view: GerritView.ADMIN,
adminView: AdminChildView.PLUGINS,
- offset: ctx.params[2] ?? '0',
+ offset: ctx.params[2] || '0',
filter: ctx.params[1] ?? null,
};
// Note that router model view must be updated before view models.
@@ -1251,7 +1234,7 @@
const state: SearchViewState = {
view: GerritView.SEARCH,
query: ctx.params[0],
- offset: ctx.params[2],
+ offset: ctx.params[2] || '0',
loading: false,
};
// Note that router model view must be updated before view models.
@@ -1266,7 +1249,7 @@
const state: SearchViewState = {
view: GerritView.SEARCH,
query: ctx.params[0],
- offset: undefined,
+ offset: '0',
loading: false,
};
// Note that router model view must be updated before view models.
@@ -1327,15 +1310,17 @@
const repo = ctx.params[0] as RepoName;
const commentId = ctx.params[2] as UrlEncodedCommentId;
- const [comments, robotComments, change] = await Promise.all([
+ const [comments, robotComments, drafts, change] = await Promise.all([
this.restApiService.getDiffComments(changeNum),
this.restApiService.getDiffRobotComments(changeNum),
+ this.restApiService.getDiffDrafts(changeNum),
this.restApiService.getChangeDetail(changeNum),
]);
const comment =
findComment(addPath(comments), commentId) ??
- findComment(addPath(robotComments), commentId);
+ findComment(addPath(robotComments), commentId) ??
+ findComment(addPath(drafts), commentId);
const path = comment?.path;
const patchsets = computeAllPatchSets(change);
const latestPatchNum = computeLatestPatchNum(patchsets);
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 87d100c..c4cb465 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
@@ -5,7 +5,7 @@
*/
import '../../../test/common-test-setup';
import './gr-router';
-import {Page, PageContext} from '../../../utils/page-wrapper-utils';
+import {Page, PageContext} from './gr-page';
import {
stubBaseUrl,
stubRestApi,
@@ -125,7 +125,6 @@
const requiresAuth: any = {};
const doesNotRequireAuth: any = {};
sinon.stub(page, 'start');
- sinon.stub(page, 'base');
sinon
.stub(router, 'mapRoute')
.callsFake((_pattern, methodName, _method, usesAuth) => {
@@ -212,20 +211,8 @@
test('redirectIfNotLoggedIn while logged in', () => {
stubRestApi('getLoggedIn').returns(Promise.resolve(true));
- const ctx = {
- save() {},
- handled: true,
- canonicalPath: '',
- path: '',
- querystring: '',
- pathname: '',
- state: '',
- title: '',
- hash: '',
- params: {test: 'test'},
- };
const redirectStub = sinon.stub(router, 'redirectToLogin');
- return router.redirectIfNotLoggedIn(ctx).then(() => {
+ return router.redirectIfNotLoggedIn('somepath').then(() => {
assert.isFalse(redirectStub.called);
});
});
@@ -233,21 +220,9 @@
test('redirectIfNotLoggedIn while logged out', () => {
stubRestApi('getLoggedIn').returns(Promise.resolve(false));
const redirectStub = sinon.stub(router, 'redirectToLogin');
- const ctx = {
- save() {},
- handled: true,
- canonicalPath: '',
- path: '',
- querystring: '',
- pathname: '',
- state: '',
- title: '',
- hash: '',
- params: {test: 'test'},
- };
return new Promise(resolve => {
router
- .redirectIfNotLoggedIn(ctx)
+ .redirectIfNotLoggedIn('somepath')
.then(() => {
assert.isTrue(false, 'Should never execute');
})
@@ -321,17 +296,6 @@
await waitUntilCalled(handlePassThroughRoute, 'handlePassThroughRoute');
}
- function createPageContext(): PageContext {
- return {
- canonicalPath: '',
- path: '',
- querystring: '',
- pathname: '',
- hash: '',
- params: {},
- };
- }
-
setup(() => {
stubRestApi('setInProjectLookup');
redirectStub = sinon.stub(router, 'redirect');
@@ -395,9 +359,8 @@
) => {
onExit = _onExit;
};
- sinon.stub(page, 'exit').callsFake(onRegisteringExit);
+ sinon.stub(page, 'registerExitRoute').callsFake(onRegisteringExit);
sinon.stub(page, 'start');
- sinon.stub(page, 'base');
router._testOnly_startRouter();
router.handleDefaultRoute();
@@ -474,7 +437,10 @@
suite('ROOT', () => {
test('closes for closeAfterLogin', () => {
- const ctx = {...createPageContext(), querystring: 'closeAfterLogin'};
+ const ctx = {
+ querystring: 'closeAfterLogin',
+ canonicalPath: '',
+ } as PageContext;
const closeStub = sinon.stub(window, 'close');
const result = router.handleRootRoute(ctx);
assert.isNotOk(result);
@@ -595,7 +561,7 @@
adminView: AdminChildView.GROUPS,
offset: '0',
openCreateModal: false,
- filter: null,
+ filter: '',
};
await checkUrlToState('/admin/groups', defaultState);
@@ -1029,7 +995,7 @@
view: GerritView.DOCUMENTATION_SEARCH,
filter: 'asdf',
});
- // Percent decoding works fine. page.js decodes twice, so the only problem
+ // Percent decoding works fine. gr-page decodes twice, so the only problem
// is having `%25` in the URL, because the first decoding pass will yield
// `%`, and then the second decoding pass will throw `URI malformed`.
await checkUrlToState('/Documentation/q/filter:as%20%2fdf', {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index c74993f..c7d6948 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -53,11 +53,7 @@
import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
import {GrApplyFixDialog} from '../gr-apply-fix-dialog/gr-apply-fix-dialog';
import {CommentMap} from '../../../utils/comment-util';
-import {
- EventType,
- OpenFixPreviewEvent,
- ValueChangedEvent,
-} from '../../../types/events';
+import {OpenFixPreviewEvent, ValueChangedEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {assertIsDefined, queryAndAssert} from '../../../utils/common-util';
import {Key, toggleClass, whenVisible} from '../../../utils/dom-util';
@@ -417,14 +413,12 @@
changeNum => {
if (!changeNum || this.changeNum === changeNum) return;
- // We are only setting the changeNum of the diff view once!
+ // We are only setting the changeNum of the diff view once.
// Everything in the diff view is tied to the change. It seems better to
// force the re-creation of the diff view when the change number changes.
- if (!this.changeNum) {
- this.changeNum = changeNum;
- } else {
- fireEvent(this, EventType.RECREATE_DIFF_VIEW);
- }
+ // The parent element will make sure that a new change view is created
+ // when the change number changes (using the `keyed` directive).
+ if (!this.changeNum) this.changeNum = changeNum;
}
);
subscribe(
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index eceb05e..b096f76 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -32,7 +32,7 @@
import {navigationToken} from './core/gr-navigation/gr-navigation';
import {getAppContext} from '../services/app-context';
import {routerToken} from './core/gr-router/gr-router';
-import {AccountDetailInfo} from '../types/common';
+import {AccountDetailInfo, NumericChangeId} from '../types/common';
import {
constructServerErrorMsg,
GrErrorManager,
@@ -62,6 +62,7 @@
import {customElement, property, query, state} from 'lit/decorators.js';
import {Shortcut, ShortcutController} from './lit/shortcut-controller';
import {cache} from 'lit/directives/cache.js';
+import {keyed} from 'lit/directives/keyed.js';
import {assertIsDefined} from '../utils/common-util';
import './gr-css-mixins';
import {isDarkTheme, prefersDarkColorScheme} from '../utils/theme-util';
@@ -123,6 +124,9 @@
// TODO: Introduce a wrapper element for CHANGE, DIFF, EDIT view.
@state() private childView?: ChangeChildView;
+ // Used as a key for caching the CHANGE, DIFF, EDIT view.
+ @state() private changeNum?: NumericChangeId;
+
@state() private lastError?: ErrorInfo;
// private but used in test
@@ -148,12 +152,6 @@
// (e.g. shortcut dialog) is open
@state() private mainAriaHidden = false;
- // Triggers dom-if unsetting/setting restamp behaviour in lit
- @state() private invalidateChangeViewCache = false;
-
- // Triggers dom-if unsetting/setting restamp behaviour in lit
- @state() private invalidateDiffViewCache = false;
-
@state() private theme = AppTheme.AUTO;
readonly getRouter = resolve(this, routerToken);
@@ -189,12 +187,6 @@
document.addEventListener(EventType.LOCATION_CHANGE, () =>
this.handleLocationChange()
);
- this.addEventListener(EventType.RECREATE_CHANGE_VIEW, () =>
- this.handleRecreateView()
- );
- this.addEventListener(EventType.RECREATE_DIFF_VIEW, () =>
- this.handleRecreateView()
- );
document.addEventListener(EventType.GR_RPC_LOG, e => this.handleRpcLog(e));
this.shortcuts.addAbstract(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, () =>
this.showKeyboardShortcuts()
@@ -246,8 +238,13 @@
subscribe(
this,
() => this.getChangeViewModel().childView$,
- childView => {
- this.childView = childView;
+ childView => (this.childView = childView)
+ );
+ subscribe(
+ this,
+ () => this.getChangeViewModel().changeNum$,
+ changeNum => {
+ this.changeNum = changeNum;
}
);
@@ -375,8 +372,19 @@
${this.renderHeader()}
<main ?aria-hidden=${this.mainAriaHidden}>
${this.renderMobileSearch()} ${this.renderChangeListView()}
- ${this.renderDashboardView()} ${this.renderChangeView()}
- ${this.renderEditorView()} ${this.renderDiffView()}
+ ${this.renderDashboardView()}
+ ${
+ // `keyed(this.changeNum, ...)` makes sure that these views are not
+ // re-used across changes, which is a precaution, because we have run
+ // into issue with that. That could be re-considered at some point.
+ keyed(
+ this.changeNum,
+ html`
+ ${this.renderChangeView()} ${this.renderEditorView()}
+ ${this.renderDiffView()}
+ `
+ )
+ }
${this.renderSettingsView()} ${this.renderAdminView()}
${this.renderPluginScreen()} ${this.renderCLAView()}
${this.renderDocumentationSearch()}
@@ -473,18 +481,15 @@
}
private renderChangeView() {
- if (this.invalidateChangeViewCache) {
- this.updateComplete.then(() => (this.invalidateChangeViewCache = false));
- return nothing;
- }
- return cache(this.isChangeView() ? this.changeViewTemplate() : nothing);
- }
-
- // Template as not to create duplicates, for renderChangeView() only.
- private changeViewTemplate() {
- return html`
- <gr-change-view .backPage=${this.lastSearchPage}></gr-change-view>
- `;
+ // The `cache()` is required for re-using the change view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isChangeView()
+ ? html`<gr-change-view
+ .backPage=${this.lastSearchPage}
+ ></gr-change-view>`
+ : nothing
+ );
}
private isChangeView() {
@@ -494,9 +499,11 @@
);
}
- private isDiffView() {
- return (
- this.view === GerritView.CHANGE && this.childView === ChangeChildView.DIFF
+ private renderEditorView() {
+ // The `cache()` is required for re-using the editor view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isEditorView() ? html`<gr-editor-view></gr-editor-view>` : nothing
);
}
@@ -506,21 +513,18 @@
);
}
- private renderEditorView() {
- if (!this.isEditorView()) return nothing;
- return html`<gr-editor-view></gr-editor-view>`;
- }
-
private renderDiffView() {
- if (this.invalidateDiffViewCache) {
- this.updateComplete.then(() => (this.invalidateDiffViewCache = false));
- return nothing;
- }
- return cache(this.isDiffView() ? this.diffViewTemplate() : nothing);
+ // The `cache()` is required for re-using the diff view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isDiffView() ? html`<gr-diff-view></gr-diff-view>` : nothing
+ );
}
- private diffViewTemplate() {
- return html`<gr-diff-view></gr-diff-view>`;
+ private isDiffView() {
+ return (
+ this.view === GerritView.CHANGE && this.childView === ChangeChildView.DIFF
+ );
}
private renderSettingsView() {
@@ -619,15 +623,6 @@
(this.account && this.account._account_id) || null;
}
- /**
- * Throws away the view and re-creates it. The view itself fires an event, if
- * it wants to be re-created.
- */
- private handleRecreateView() {
- this.invalidateChangeViewCache = true;
- this.invalidateDiffViewCache = true;
- }
-
private async viewChanged() {
if (
this.params &&
diff --git a/polygerrit-ui/app/elements/integration_test.ts b/polygerrit-ui/app/elements/integration_test.ts
index 2f9006f..a6e02be 100644
--- a/polygerrit-ui/app/elements/integration_test.ts
+++ b/polygerrit-ui/app/elements/integration_test.ts
@@ -67,7 +67,7 @@
router.setUrl(createSettingsUrl());
await assertView('gr-settings-view');
- router.setUrl(createSearchUrl({query: 'asdf'}));
+ window.history.back();
view = await assertView('gr-change-list-view');
assert.equal(assertItems(view).length, 3);
});
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index dd0fbca..062aa54 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -883,7 +883,7 @@
}
private handleCommentAck() {
- this.createReplyComment('Ack', false, false);
+ this.createReplyComment('Acknowledged', false, false);
}
private handleCommentDone() {
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 97eafc9..b46fdba 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -320,10 +320,10 @@
await element.updateComplete;
});
- test('handle Ack', async () => {
+ test('handle Acknowledge', async () => {
queryAndAssert<GrButton>(element, '#ackBtn').click();
waitUntilCalled(stub, 'saveDraft()');
- assert.equal(stub.lastCall.firstArg.message, 'Ack');
+ assert.equal(stub.lastCall.firstArg.message, 'Acknowledged');
assert.equal(stub.lastCall.firstArg.unresolved, false);
assert.isTrue(element.saving);
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
index 3187ada..cf279ba 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
@@ -69,19 +69,19 @@
});
test('does not apply rewrites within links', async () => {
- element.content = 'google.com/LinkRewriteMe';
+ element.content = 'http://google.com/LinkRewriteMe';
await element.updateComplete;
assert.shadowDom.equal(
element,
/* HTML */ `
<pre class="plaintext">
- <a
+ http://google.com/<a
href="http://google.com/LinkRewriteMe"
rel="noopener"
target="_blank"
>
- google.com/LinkRewriteMe
+ LinkRewriteMe
</a>
</pre>
`
@@ -149,20 +149,18 @@
});
test('renders text with links and rewrites', async () => {
- element.content = `text with plain link: google.com
- \ntext with config link: LinkRewriteMe
- \ntext with complex link: A Link 12`;
+ element.content = `
+ text with plain link: http://google.com
+ text with config link: LinkRewriteMe
+ text with complex link: A Link 12`;
await element.updateComplete;
assert.shadowDom.equal(
element,
/* HTML */ `
<pre class="plaintext">
- text with plain link:
- <a href="http://google.com" rel="noopener" target="_blank">
- google.com
- </a>
- text with config link:
+ text with plain link: http://google.com
+ text with config link:
<a
href="http://google.com/LinkRewriteMe"
rel="noopener"
@@ -212,7 +210,7 @@
});
test('renders text with links and rewrites', async () => {
element.content = `text
- \ntext with plain link: google.com
+ \ntext with plain link: http://google.com
\ntext with config link: LinkRewriteMe
\ntext without a link: NotA Link 15 cats
\ntext with complex link: A Link 12`;
@@ -227,7 +225,7 @@
<p>
text with plain link:
<a href="http://google.com" rel="noopener" target="_blank">
- google.com
+ http://google.com
</a>
</p>
<p>
@@ -259,7 +257,7 @@
test('does not render if too long', async () => {
element.content = `text
- text with plain link: google.com
+ text with plain link: http://google.com
text with config link: LinkRewriteMe
text without a link: NotA Link 15 cats
text with complex link: A Link 12`;
@@ -271,9 +269,8 @@
/* HTML */ `
<pre class="plaintext">
text
- text with plain link:
- <a href="http://google.com" rel="noopener" target="_blank">google.com</a>
- text with config link:
+ text with plain link: http://google.com
+ text with config link:
<a
href="http://google.com/LinkRewriteMe"
rel="noopener"
@@ -302,7 +299,7 @@
\n#### h4-heading
\n##### h5-heading
\n###### h6-heading
- \n# heading with plain link: google.com
+ \n# heading with plain link: http://google.com
\n# heading with config link: LinkRewriteMe`;
await element.updateComplete;
@@ -320,7 +317,7 @@
<h1>
heading with plain link:
<a href="http://google.com" rel="noopener" target="_blank">
- google.com
+ http://google.com
</a>
</h1>
<h1>
@@ -480,7 +477,7 @@
test('renders block quotes with links and rewrites', async () => {
element.content = `> block quote
- \n> block quote with plain link: google.com
+ \n> block quote with plain link: http://google.com
\n> block quote with config link: LinkRewriteMe`;
await element.updateComplete;
@@ -496,7 +493,7 @@
<p>
block quote with plain link:
<a href="http://google.com" rel="noopener" target="_blank">
- google.com
+ http://google.com
</a>
</p>
</blockquote>
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
index abc7f3c..2730fcf 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
@@ -160,6 +160,12 @@
.status .value {
white-space: pre-wrap;
}
+ /* Make sure that users cannot break the layout with super long
+ "About Me" texts. */
+ div.status {
+ max-height: 8em;
+ overflow-y: auto;
+ }
`,
];
}
@@ -273,7 +279,7 @@
return html`
<div class="status">
<span class="title">About me:</span>
- <span class="value">${this.account.status}</span>
+ <span class="value">${this.account.status.trim()}</span>
</div>
`;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts
index a1ac781..bd47ec8 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents_test.ts
@@ -38,7 +38,7 @@
email: 'kermit@gmail.com' as EmailAddress,
username: 'kermit',
name: 'Kermit The Frog',
- status: 'I am a frog',
+ status: ' I am a frog ',
_account_id: 31415926535 as AccountId,
};
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 e1e4ca5..20a6463 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -627,13 +627,7 @@
}
private fireChangedEvents() {
- // This is a bit redundant, because the `text` property has `notify:true`,
- // so whenever the `text` changes the component fires two identical events
- // `text-changed` and `value-changed`.
- fire(this, 'value-changed', {value: this.text});
fire(this, 'text-changed', {value: this.text});
- // Relay the event.
- fire(this, 'bind-value-changed', {value: this.text});
}
private indent(e: KeyboardEvent): void {
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
index 0400e85..711866a 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea_test.ts
@@ -50,7 +50,7 @@
// Needed for Safari tests. selectionStart is not updated when text is
// updated.
const listenerStub = sinon.stub();
- element.addEventListener('bind-value-changed', listenerStub);
+ element.addEventListener('text-changed', listenerStub);
stubRestApi('getSuggestedAccounts').returns(
Promise.resolve([
createAccountWithEmail('abc@google.com'),
@@ -166,7 +166,7 @@
test('emoji dropdown does not open if mention dropdown is open', async () => {
const listenerStub = sinon.stub();
- element.addEventListener('bind-value-changed', listenerStub);
+ element.addEventListener('text-changed', listenerStub);
const resetSpy = sinon.spy(element, 'resetDropdown');
stubRestApi('getSuggestedAccounts').returns(
Promise.resolve([
@@ -217,7 +217,7 @@
test('mention dropdown does not open if emoji dropdown is open', async () => {
const listenerStub = sinon.stub();
- element.addEventListener('bind-value-changed', listenerStub);
+ element.addEventListener('text-changed', listenerStub);
element.textarea!.focus();
await waitUntil(() => element.textarea!.focused === true);
@@ -311,7 +311,7 @@
// Needed for Safari tests. selectionStart is not updated when text is
// updated.
const listenerStub = sinon.stub();
- element.addEventListener('bind-value-changed', listenerStub);
+ element.addEventListener('text-changed', listenerStub);
element.textarea!.focus();
await waitUntil(() => element.textarea!.focused === true);
element.textarea!.selectionStart = 1;
diff --git a/polygerrit-ui/app/models/views/admin_test.ts b/polygerrit-ui/app/models/views/admin_test.ts
index c9b7801..b6089af 100644
--- a/polygerrit-ui/app/models/views/admin_test.ts
+++ b/polygerrit-ui/app/models/views/admin_test.ts
@@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {assert} from '@open-wc/testing';
+import {PageContext} from '../../elements/core/gr-router/gr-page';
import {GerritView} from '../../services/router/router-model';
import '../../test/common-test-setup';
import {AdminChildView, PLUGIN_LIST_ROUTE} from './admin';
@@ -20,7 +21,7 @@
assert.isFalse(pattern.test('//admin/plugins?'));
assert.isFalse(pattern.test('/admin/plugins//'));
- assert.deepEqual(createState({}), {
+ assert.deepEqual(createState(new PageContext('')), {
view: GerritView.ADMIN,
adminView: AdminChildView.PLUGINS,
});
diff --git a/polygerrit-ui/app/models/views/base.ts b/polygerrit-ui/app/models/views/base.ts
index 72bec33..5c388f4 100644
--- a/polygerrit-ui/app/models/views/base.ts
+++ b/polygerrit-ui/app/models/views/base.ts
@@ -3,6 +3,7 @@
* Copyright 2022 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
+import {PageContext} from '../../elements/core/gr-router/gr-page';
import {GerritView} from '../../services/router/router-model';
export interface ViewState {
@@ -10,22 +11,10 @@
}
/**
- * While we are using page.js this interface will normally be implemented by
- * PageContext, but it helps testing and independence to have our own type
- * here.
- */
-export interface UrlInfo {
- querystring?: string;
- hash?: string;
- /** What the regular expression matching returns. */
- params?: {[paramIndex: string]: string};
-}
-
-/**
* Based on `urlPattern` knows whether a URL matches and if so, then
* `createState()` can produce a `ViewState` from the matched URL.
*/
export interface Route<T extends ViewState> {
urlPattern: RegExp;
- createState: (info: UrlInfo) => T;
+ createState: (ctx: PageContext) => T;
}
diff --git a/polygerrit-ui/app/models/views/change_test.ts b/polygerrit-ui/app/models/views/change_test.ts
index 1e71bbd..837e362 100644
--- a/polygerrit-ui/app/models/views/change_test.ts
+++ b/polygerrit-ui/app/models/views/change_test.ts
@@ -74,7 +74,7 @@
...createChangeViewState(),
repo: 'x+/y+/z+/w' as RepoName,
};
- assert.equal(createChangeUrl(state), '/c/x%2B/y%2B/z%2B/w/+/42');
+ assert.equal(createChangeUrl(state), '/c/x%252B/y%252B/z%252B/w/+/42');
});
test('createDiffUrl', () => {
@@ -85,7 +85,7 @@
};
assert.equal(
createDiffUrl(params),
- '/c/test-project/+/42/12/x%2By/path.cpp'
+ '/c/test-project/+/42/12/x%252By/path.cpp'
);
window.CANONICAL_PATH = '/base';
@@ -93,10 +93,10 @@
window.CANONICAL_PATH = undefined;
params.repo = 'test' as RepoName;
- assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%2By/path.cpp');
+ assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%252By/path.cpp');
params.basePatchNum = 6 as BasePatchSetNum;
- assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%2By/path.cpp');
+ assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%252By/path.cpp');
params.diffView = {
path: 'foo bar/my+file.txt%',
@@ -105,7 +105,7 @@
delete params.basePatchNum;
assert.equal(
createDiffUrl(params),
- '/c/test/+/42/2/foo+bar/my%2Bfile.txt%2525'
+ '/c/test/+/42/2/foo+bar/my%252Bfile.txt%2525'
);
params.diffView = {
@@ -129,7 +129,7 @@
repo: 'x+/y' as RepoName,
diffView: {path: 'x+y/path.cpp'},
};
- assert.equal(createDiffUrl(params), '/c/x%2B/y/+/42/12/x%2By/path.cpp');
+ assert.equal(createDiffUrl(params), '/c/x%252B/y/+/42/12/x%252By/path.cpp');
});
test('createEditUrl', () => {
@@ -140,7 +140,7 @@
};
assert.equal(
createEditUrl(params),
- '/c/test-project/+/42/12/x%2By/path.cpp,edit#31'
+ '/c/test-project/+/42/12/x%252By/path.cpp,edit#31'
);
window.CANONICAL_PATH = '/base';
diff --git a/polygerrit-ui/app/node_modules_licenses/licenses.ts b/polygerrit-ui/app/node_modules_licenses/licenses.ts
index b5b313e..155dd6c 100644
--- a/polygerrit-ui/app/node_modules_licenses/licenses.ts
+++ b/polygerrit-ui/app/node_modules_licenses/licenses.ts
@@ -329,14 +329,6 @@
license: SharedLicenses.Polymer2018,
},
{
- name: 'ba-linkify',
- license: {
- name: 'ba-linkify',
- type: LicenseTypes.Mit,
- packageLicenseFile: 'LICENSE-MIT',
- },
- },
- {
name: 'codemirror-minified',
license: {
name: 'codemirror-minified',
@@ -377,6 +369,10 @@
license: SharedLicenses.Polymer2018,
},
{
+ name: 'polygerrit-gr-page',
+ license: SharedLicenses.Page,
+ },
+ {
name: 'web-vitals',
license: {
name: 'web-vitals',
diff --git a/polygerrit-ui/app/package.json b/polygerrit-ui/app/package.json
index b71ad77..41f4043 100644
--- a/polygerrit-ui/app/package.json
+++ b/polygerrit-ui/app/package.json
@@ -34,14 +34,12 @@
"@types/resize-observer-browser": "^0.1.5",
"@webcomponents/shadycss": "^1.10.2",
"@webcomponents/webcomponentsjs": "^1.3.3",
- "ba-linkify": "^1.0.1",
"codemirror-minified": "^5.65.0",
"highlight.js": "^11.5.0",
"highlightjs-closure-templates": "https://github.com/highlightjs/highlightjs-closure-templates",
"highlightjs-structured-text": "https://github.com/highlightjs/highlightjs-structured-text",
"immer": "^9.0.5",
"lit": "^2.2.3",
- "page": "^1.11.6",
"polymer-bridges": "file:../../polymer-bridges/",
"polymer-resin": "^2.0.1",
"resemblejs": "^4.0.0",
diff --git a/polygerrit-ui/app/rollup.config.js b/polygerrit-ui/app/rollup.config.js
index be60a63..bc9d7a8 100644
--- a/polygerrit-ui/app/rollup.config.js
+++ b/polygerrit-ui/app/rollup.config.js
@@ -73,14 +73,15 @@
customResolveOptions: {
// By default, it tries to use page.mjs file instead of page.js
// when importing 'page/page'.
+ // TODO: page.was removed. Is something obsolete here?
extensions: ['.js'],
moduleDirectory: 'external/ui_npm/node_modules',
},
}),
define({
- replacements: {
- 'process.env.NODE_ENV': JSON.stringify('production'),
- },
+ replacements: {
+ 'process.env.NODE_ENV': JSON.stringify('production'),
+ },
}),
importLocalFontMetaUrlResolver()],
};
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
index b81f6d5..06e8204 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
@@ -188,13 +188,22 @@
export function initWebVitals(reportingService: ReportingService) {
function reportWebVitalMetric(name: Timing, metric: Metric) {
+ let score = metric.value;
+ // CLS good score is 0.1 and poor score is 0.25. Logging system
+ // prefers integers, so we multiple by 100;
+ if (name === Timing.CLS) {
+ score *= 100;
+ }
reportingService.reporter(
TIMING.TYPE,
TIMING.CATEGORY.UI_LATENCY,
name,
- metric.value,
- JSON.stringify(metric),
- false
+ score,
+ {
+ navigationType: metric.navigationType,
+ rating: metric.rating,
+ entries: metric.entries,
+ }
);
}
diff --git a/polygerrit-ui/app/styles/gr-change-list-styles.ts b/polygerrit-ui/app/styles/gr-change-list-styles.ts
index 3763213..34cf872 100644
--- a/polygerrit-ui/app/styles/gr-change-list-styles.ts
+++ b/polygerrit-ui/app/styles/gr-change-list-styles.ts
@@ -14,11 +14,11 @@
background-color: var(--selection-background-color);
}
gr-change-list-item[highlight] {
- background-color: var(--assignee-highlight-color);
+ background-color: var(--line-item-highlight-color);
}
gr-change-list-item[highlight][selected],
gr-change-list-item[highlight]:focus {
- background-color: var(--assignee-highlight-selection-color);
+ background-color: var(--line-item-highlight-selection-color);
}
.groupTitle td,
.cell {
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts
index 6dbda3e..107ee16 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.ts
+++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -248,10 +248,13 @@
--table-subheader-background-color: var(--background-color-tertiary);
--view-background-color: var(--background-color-primary);
/* unique background colors */
+ /* TODO: Remove assignee colors once references are migrated */
--assignee-highlight-color: #fcfad6;
- /* TODO: Find a nicer way to combine the --assignee-highlight-color and the
- --selection-background-color than to just invent another unique color. */
--assignee-highlight-selection-color: #f6f4d0;
+ --line-item-highlight-color: #fcfad6;
+ /* TODO: Find a nicer way to combine the --line-item-highlight-color and the
+ --selection-background-color than to just invent another unique color. */
+ --line-item-highlight-selection-color: #f6f4d0;
--chip-selected-background-color: var(--blue-50);
--edit-mode-background-color: #ebf5fb;
--emphasis-color: #fff9c4;
@@ -415,8 +418,8 @@
--diff-trailing-whitespace-indicator: #ff9ad2;
--focused-line-outline-color: var(--blue-700);
--coverage-covered-line-num-color: var(--deemphasized-text-color);
- --coverage-covered: #e0f2f1;
- --coverage-not-covered: #ffd1a4;
+ --coverage-covered: var(--cyan-100);
+ --coverage-not-covered: var(--orange-100);
--ranged-comment-hint-text-color: var(--orange-900);
--token-highlighting-color: #fffd54;
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.ts b/polygerrit-ui/app/styles/themes/dark-theme.ts
index c6884a6..a183c86 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.ts
+++ b/polygerrit-ui/app/styles/themes/dark-theme.ts
@@ -123,8 +123,8 @@
/* directly derived from primary background colors */
/* empty, because inheriting from app-theme is just fine
/* unique background colors */
- --assignee-highlight-color: #3a361c;
- --assignee-highlight-selection-color: #423e24;
+ --line-item-highlight-color: #3a361c;
+ --line-item-highlight-selection-color: #423e24;
--chip-selected-background-color: #3c4455;
--edit-mode-background-color: #5c0a36;
--emphasis-color: #383f4a;
@@ -241,9 +241,9 @@
--diff-tab-indicator-color: var(--deemphasized-text-color);
--diff-trailing-whitespace-indicator: #ff9ad2;
--focused-line-outline-color: var(--blue-200);
- --coverage-covered: #37674a;
+ --coverage-covered: var(--cyan-tonal);
--coverage-covered-line-num-color: var(--gray-200);
- --coverage-not-covered: #6b3600;
+ --coverage-not-covered: var(--orange-tonal);
--ranged-comment-hint-text-color: var(--blue-50);
--token-highlighting-color: var(--yellow-tonal);
diff --git a/polygerrit-ui/app/test/functional/README.md b/polygerrit-ui/app/test/functional/README.md
deleted file mode 100644
index 82c6133..0000000
--- a/polygerrit-ui/app/test/functional/README.md
+++ /dev/null
@@ -1,54 +0,0 @@
-# Functional test suite
-
-## Installing Docker (OSX)
-
-Simplest way to install all of those is to use Homebrew:
-
-```
-brew cask install docker
-```
-
-This will install a Docker in Applications. To run if from the command-line:
-
-```
-open /Applications/Docker.app
-```
-
-It'll require privileged access and will require user password to be entered.
-
-To validate Docker is installed correctly, run hello-world image:
-
-```
-docker run hello-world
-```
-
-## Building a Docker image
-
-Should be done once only for development purposes, run from the Gerrit checkout
-path:
-
-```
-docker build -t gerrit/polygerrit-functional:v1 \
- polygerrit-ui/app/test/functional/infra
-```
-
-## Running a smoke test
-
-Running a smoke test from Gerrit checkout path:
-
-```
-./polygerrit-ui/app/test/functional/run_functional.sh
-```
-
-The successful output should be something similar to this:
-
-```
-Starting local server..
-Starting Webdriver..
-Started
-.
-
-
-1 spec, 0 failures
-Finished in 2.565 seconds
-```
diff --git a/polygerrit-ui/app/test/functional/infra/Dockerfile b/polygerrit-ui/app/test/functional/infra/Dockerfile
deleted file mode 100644
index e642176..0000000
--- a/polygerrit-ui/app/test/functional/infra/Dockerfile
+++ /dev/null
@@ -1,38 +0,0 @@
-FROM selenium/standalone-chrome-debug
-
-USER root
-
-# nvm environment variables
-ENV NVM_DIR /usr/local/nvm
-ENV NODE_VERSION 9.4.0
-
-# install nvm
-# https://github.com/creationix/nvm#install-script
-RUN wget -qO- https://raw.githubusercontent.com/creationix/nvm/v0.33.8/install.sh | bash
-
-# install node and npm
-RUN [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" \
- && nvm install $NODE_VERSION \
- && nvm alias default $NODE_VERSION \
- && nvm use default
-
-ENV NODE_PATH $NVM_DIR/v$NODE_VERSION/lib/node_modules
-ENV PATH $NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH
-
-RUN npm install -g jasmine
-RUN npm install -g http-server
-
-USER seluser
-
-RUN mkdir -p /tmp/app
-WORKDIR /tmp/app
-
-RUN npm init -y
-RUN npm install --save selenium-webdriver
-
-EXPOSE 8080
-
-COPY test-infra.js /tmp/app/node_modules
-COPY run.sh /tmp/app/
-
-ENTRYPOINT [ "/tmp/app/run.sh" ]
diff --git a/polygerrit-ui/app/test/functional/infra/run.sh b/polygerrit-ui/app/test/functional/infra/run.sh
deleted file mode 100755
index 4beb3dd..0000000
--- a/polygerrit-ui/app/test/functional/infra/run.sh
+++ /dev/null
@@ -1,14 +0,0 @@
-#!/bin/sh
-echo Starting local server..
-cp /app/polygerrit_ui.zip .
-unzip -q polygerrit_ui.zip
-nohup http-server polygerrit_ui > /tmp/http-server.log 2>&1 &
-
-echo Starting Webdriver..
-nohup /opt/bin/entry_point.sh > /tmp/webdriver.log 2>&1 &
-
-# Wait for servers to start
-sleep 5
-
-cp $@ .
-jasmine $(basename $@)
diff --git a/polygerrit-ui/app/test/functional/infra/test-infra.js b/polygerrit-ui/app/test/functional/infra/test-infra.js
deleted file mode 100644
index 2619694..0000000
--- a/polygerrit-ui/app/test/functional/infra/test-infra.js
+++ /dev/null
@@ -1,24 +0,0 @@
-'use strict';
-
-const {Builder} = require('selenium-webdriver');
-
-let driver;
-
-function setup() {
- return new Builder()
- .forBrowser('chrome')
- .usingServer('http://localhost:4444/wd/hub')
- .build()
- .then(d => {
- driver = d;
- return driver.get('http://localhost:8080');
- })
- .then(() => driver);
-}
-
-function cleanup() {
- return driver.quit();
-}
-
-exports.setup = setup;
-exports.cleanup = cleanup;
diff --git a/polygerrit-ui/app/test/functional/run_functional.sh b/polygerrit-ui/app/test/functional/run_functional.sh
deleted file mode 100755
index 7ce57b8..0000000
--- a/polygerrit-ui/app/test/functional/run_functional.sh
+++ /dev/null
@@ -1,10 +0,0 @@
-#!/usr/bin/env bash
-
-bazel build //polygerrit-ui/app:polygerrit_ui
-
-docker run --rm \
- -p 5900:5900 \
- -v `pwd`/polygerrit-ui/app/test/functional:/tests \
- -v `pwd`/bazel-genfiles/polygerrit-ui/app:/app \
- -it gerrit/polygerrit-functional:v1 \
- /tests/test.js
diff --git a/polygerrit-ui/app/test/functional/test.js b/polygerrit-ui/app/test/functional/test.js
deleted file mode 100644
index ae572af..0000000
--- a/polygerrit-ui/app/test/functional/test.js
+++ /dev/null
@@ -1,21 +0,0 @@
-/**
- * @fileoverview Minimal viable frontend functional test.
- */
-'use strict';
-
-const {until} = require('selenium-webdriver');
-const {setup, cleanup} = require('test-infra');
-
-jasmine.DEFAULT_TIMEOUT_INTERVAL = 20000;
-
-describe('example ', () => {
- let driver;
-
- beforeAll(() => setup().then(d => driver = d));
-
- afterAll(() => cleanup());
-
- it('should update title', () => driver.wait(
- until.titleIs('status:open · Gerrit Code Review'), 5000
- ));
-});
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index fe33302..05e4df7 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -110,7 +110,7 @@
SubmitRequirementResultInfo,
SubmitRequirementStatus,
} from '../api/rest-api';
-import {CheckResult, RunResult} from '../models/checks/checks-model';
+import {CheckResult, CheckRun, RunResult} from '../models/checks/checks-model';
import {Category, RunStatus} from '../api/checks';
import {DiffInfo} from '../api/diff';
import {SearchViewState} from '../models/views/search';
@@ -750,7 +750,7 @@
return {
view: GerritView.SEARCH,
query: '',
- offset: undefined,
+ offset: '0',
loading: false,
};
}
@@ -767,7 +767,7 @@
view: GerritView.ADMIN,
adminView: AdminChildView.REPOS,
offset: '0',
- filter: null,
+ filter: '',
openCreateModal: false,
};
}
@@ -777,7 +777,7 @@
view: GerritView.ADMIN,
adminView: AdminChildView.PLUGINS,
offset: '0',
- filter: null,
+ filter: '',
};
}
@@ -799,7 +799,7 @@
view: GerritView.REPO,
detail: RepoDetailView.BRANCHES,
offset: '0',
- filter: null,
+ filter: '',
};
}
@@ -808,7 +808,7 @@
view: GerritView.REPO,
detail: RepoDetailView.TAGS,
offset: '0',
- filter: null,
+ filter: '',
};
}
@@ -1103,6 +1103,19 @@
};
}
+export function createRun(partial: Partial<CheckRun> = {}): CheckRun {
+ return {
+ attemptDetails: [],
+ checkName: 'test-name',
+ internalRunId: 'test-internal-run-id',
+ isLatestAttempt: true,
+ isSingleAttempt: true,
+ pluginName: 'test-plugin-name',
+ status: RunStatus.COMPLETED,
+ ...partial,
+ };
+}
+
export function createRunResult(): RunResult {
return {
attemptDetails: [],
@@ -1119,11 +1132,14 @@
};
}
-export function createCheckResult(): CheckResult {
+export function createCheckResult(
+ partial: Partial<CheckResult> = {}
+): CheckResult {
return {
category: Category.ERROR,
summary: 'error',
internalResultId: 'test-internal-result-id',
+ ...partial,
};
}
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index f642af7..2cc6742 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -28,8 +28,6 @@
OPEN_FIX_PREVIEW = 'open-fix-preview',
CLOSE_FIX_PREVIEW = 'close-fix-preview',
PAGE_ERROR = 'page-error',
- RECREATE_CHANGE_VIEW = 'recreate-change-view',
- RECREATE_DIFF_VIEW = 'recreate-diff-view',
RELOAD = 'reload',
REPLY = 'reply',
SERVER_ERROR = 'server-error',
diff --git a/polygerrit-ui/app/utils/link-util.ts b/polygerrit-ui/app/utils/link-util.ts
index ec1e7e7..48e9c07 100644
--- a/polygerrit-ui/app/utils/link-util.ts
+++ b/polygerrit-ui/app/utils/link-util.ts
@@ -3,7 +3,6 @@
* Copyright 2022 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import 'ba-linkify/ba-linkify';
import {CommentLinkInfo, CommentLinks} from '../types/common';
import {getBaseUrl} from './url-util';
@@ -16,21 +15,8 @@
base: string,
repoCommentLinks: CommentLinks
): string {
- const parts: string[] = [];
- window.linkify(insertZeroWidthSpace(base), {
- callback: (text, href) => {
- if (href) {
- parts.push(removeZeroWidthSpace(createLinkTemplate(href, text)));
- } else {
- const rewriteResults = getRewriteResultsFromConfig(
- text,
- repoCommentLinks
- );
- parts.push(removeZeroWidthSpace(applyRewrites(text, rewriteResults)));
- }
- },
- });
- return parts.join('');
+ const rewriteResults = getRewriteResultsFromConfig(base, repoCommentLinks);
+ return applyRewrites(base, rewriteResults);
}
/**
@@ -141,22 +127,6 @@
);
}
-/**
- * Some tools are known to look for reviewers/CCs by finding lines such as
- * "R=foo@gmail.com, bar@gmail.com". However, "=" is technically a valid email
- * character, so ba-linkify interprets the entire string "R=foo@gmail.com" as an
- * email address. To fix this, we insert a zero width space character \u200B
- * before linking that prevents ba-linkify from associating the prefix with the
- * email. After linking we remove the zero width space.
- */
-function insertZeroWidthSpace(base: string) {
- return base.replace(/^(R=|CC=)/g, '$&\u200B');
-}
-
-function removeZeroWidthSpace(base: string) {
- return base.replace(/\u200B/g, '');
-}
-
function createLinkTemplate(
href: string,
displayText: string,
diff --git a/polygerrit-ui/app/utils/link-util_test.ts b/polygerrit-ui/app/utils/link-util_test.ts
index f5c13e8..e4e719b 100644
--- a/polygerrit-ui/app/utils/link-util_test.ts
+++ b/polygerrit-ui/app/utils/link-util_test.ts
@@ -63,18 +63,6 @@
`${link('foo', 'foo.gov')} ${link('foo', 'foo.gov')}`
);
});
-
- test('does not apply within normal links', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('google.com', {
- ogle: {
- match: 'ogle',
- link: 'gerritcodereview.com',
- },
- }),
- link('google.com', 'http://google.com')
- );
- });
});
test('for overlapping rewrites prefer the latest ending', () => {
@@ -165,45 +153,4 @@
)}`
);
});
-
- suite('normal links', () => {
- test('links urls', () => {
- const googleLink = link('google.com', 'http://google.com');
- const mapsLink = link('maps.google.com', 'http://maps.google.com');
-
- assert.equal(
- linkifyUrlsAndApplyRewrite('google.com, maps.google.com', {}),
- `${googleLink}, ${mapsLink}`
- );
- });
-
- test('links emails without including R= prefix', () => {
- const fooEmail = link('foo@gmail.com', 'mailto:foo@gmail.com');
- const barEmail = link('bar@gmail.com', 'mailto:bar@gmail.com');
- assert.equal(
- linkifyUrlsAndApplyRewrite('R=foo@gmail.com, bar@gmail.com', {}),
- `R=${fooEmail}, ${barEmail}`
- );
- });
-
- test('links emails without including CC= prefix', () => {
- const fooEmail = link('foo@gmail.com', 'mailto:foo@gmail.com');
- const barEmail = link('bar@gmail.com', 'mailto:bar@gmail.com');
- assert.equal(
- linkifyUrlsAndApplyRewrite('CC=foo@gmail.com, bar@gmail.com', {}),
- `CC=${fooEmail}, ${barEmail}`
- );
- });
-
- test('links emails maintains R= and CC= within addresses', () => {
- const fooBarBazEmail = link(
- 'fooR=barCC=baz@gmail.com',
- 'mailto:fooR=barCC=baz@gmail.com'
- );
- assert.equal(
- linkifyUrlsAndApplyRewrite('fooR=barCC=baz@gmail.com', {}),
- fooBarBazEmail
- );
- });
- });
});
diff --git a/polygerrit-ui/app/utils/page-wrapper-utils.ts b/polygerrit-ui/app/utils/page-wrapper-utils.ts
deleted file mode 100644
index 58bb024..0000000
--- a/polygerrit-ui/app/utils/page-wrapper-utils.ts
+++ /dev/null
@@ -1,49 +0,0 @@
-/**
- * @license
- * Copyright 2020 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-// @ts-ignore: Bazel is not yet configured to download the types
-import pagejs from 'page';
-
-// Reexport page.js. To make it work rollup patches page.js and replace "this"
-// to "window". Otherwise, it can't assign global property. We can't import
-// page.mjs because typescript doesn't support mjs extensions
-export interface Page {
- (pattern: string | RegExp, ...pageCallback: PageCallback[]): void;
- (pageCallback: PageCallback): void;
- show(url: string): void;
- redirect(url: string): void;
- replace(path: string, state: null, init: boolean, dispatch: boolean): void;
- base(url: string): void;
- start(opts: Options): void;
- stop(): void;
- exit(pattern: string | RegExp, ...pageCallback: PageCallback[]): void;
-}
-
-export interface Options {
- popstate?: boolean;
- dispatch?: boolean;
-}
-
-// See https://visionmedia.github.io/page.js/ for details
-export interface PageContext {
- canonicalPath: string;
- path: string;
- querystring: string;
- pathname: string;
- hash: string;
- params: {[paramIndex: string]: string};
-}
-
-export type PageNextCallback = () => void;
-
-export type PageCallback = (
- context: PageContext,
- next: PageNextCallback
-) => void;
-
-// Must only be used by gr-router!
-// TODO: Move this into gr-router. Note that there is a Google import rule
-// that would need to be modified.
-export const page = pagejs as unknown as {create(): Page};
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 355e54b..183671f 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -64,7 +64,7 @@
export function convertToPatchSetNum(
patchset: string | undefined
): PatchSetNum | undefined {
- if (patchset === undefined) return patchset;
+ if (!patchset) return undefined;
if (!isPatchSetNum(patchset)) {
console.error('string is not of type PatchSetNum');
}
diff --git a/polygerrit-ui/app/utils/url-util.ts b/polygerrit-ui/app/utils/url-util.ts
index 22a9721..af1e32e 100644
--- a/polygerrit-ui/app/utils/url-util.ts
+++ b/polygerrit-ui/app/utils/url-util.ts
@@ -86,12 +86,15 @@
* encodeURIComponent() with some tweaks.
*/
export function encodeURL(url: string): string {
- // page.js decodes the entire URL, and then decodes once more the
+ // gr-page decodes the entire URL, and then decodes once more the
// individual regex matching groups. It uses `decodeURIComponent()`, which
// will choke on singular `%` chars without two trailing digits. We prefer
// to not double encode *everything* (just for readaiblity and simplicity),
// but `%` *must* be double encoded.
let output = url.replaceAll('%', '%25');
+ // `+` also requires double encoding, because `%2B` would be decoded to `+`
+ // and then replaced by ` `.
+ output = output.replaceAll('+', '%2B');
// This escapes ALL characters EXCEPT:
// A–Z a–z 0–9 - _ . ! ~ * ' ( )
@@ -127,7 +130,7 @@
output = output.replace(/%40/g, '@');
output = output.replace(/%2F/g, '/');
- // page.js replaces `+` by ` ` in addition to calling `decodeURIComponent()`.
+ // gr-page replaces `+` by ` ` in addition to calling `decodeURIComponent()`.
// So we can use `+` to increase readability.
output = output.replace(/%20/g, '+');
@@ -138,6 +141,10 @@
* Single decode for URL components. Will decode plus signs ('+') to spaces.
* Note: because this function decodes once, it is not the inverse of
* encodeURL.
+ *
+ * This function must only be used for decoding data returned by the REST API.
+ * Don't use it for decoding browser URLs. The only place for decoding browser
+ * URLs must gr-page.ts.
*/
export function singleDecodeURL(url: string): string {
const withoutPlus = url.replace(/\+/g, '%20');
diff --git a/polygerrit-ui/app/utils/url-util_test.ts b/polygerrit-ui/app/utils/url-util_test.ts
index 16f85dd..7466a90 100644
--- a/polygerrit-ui/app/utils/url-util_test.ts
+++ b/polygerrit-ui/app/utils/url-util_test.ts
@@ -110,6 +110,10 @@
assert.equal(encodeURL('abc%def'), 'abc%2525def');
});
+ test('double encodes +', () => {
+ assert.equal(encodeURL('abc+def'), 'abc%252Bdef');
+ });
+
test('does not encode colon and slash', () => {
assert.equal(encodeURL(':/'), ':/');
});
diff --git a/polygerrit-ui/app/yarn.lock b/polygerrit-ui/app/yarn.lock
index b1549b3..7f8168e 100644
--- a/polygerrit-ui/app/yarn.lock
+++ b/polygerrit-ui/app/yarn.lock
@@ -488,11 +488,6 @@
delegates "^1.0.0"
readable-stream "^2.0.6"
-ba-linkify@^1.0.1:
- version "1.0.1"
- resolved "https://registry.yarnpkg.com/ba-linkify/-/ba-linkify-1.0.1.tgz#664cf5744947c6e8611f1fbbaf7d9f315f982f4c"
- integrity sha1-Zkz1dElHxuhhHx+7r32fMV+YL0w=
-
balanced-match@^1.0.0:
version "1.0.2"
resolved "https://registry.yarnpkg.com/balanced-match/-/balanced-match-1.0.2.tgz#e83e3a7e3f300b34cb9d87f615fa0cbf357690ee"
@@ -677,11 +672,6 @@
resolved "https://registry.yarnpkg.com/is-fullwidth-code-point/-/is-fullwidth-code-point-2.0.0.tgz#a3b30a5c4f199183167aaab93beefae3ddfb654f"
integrity sha1-o7MKXE8ZkYMWeqq5O+764937ZU8=
-isarray@0.0.1:
- version "0.0.1"
- resolved "https://registry.yarnpkg.com/isarray/-/isarray-0.0.1.tgz#8a18acfca9a8f4177e09abfc6038939b05d1eedf"
- integrity sha1-ihis/Kmo9Bd+Cav8YDiTmwXR7t8=
-
isarray@~1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/isarray/-/isarray-1.0.0.tgz#bb935d48582cba168c06834957a54a3e07124f11"
@@ -813,25 +803,11 @@
dependencies:
wrappy "1"
-page@^1.11.6:
- version "1.11.6"
- resolved "https://registry.yarnpkg.com/page/-/page-1.11.6.tgz#5ef4efc7073749b8085ccdaa0dcd7c9e0de12fe3"
- integrity sha512-P6e2JfzkBrPeFCIPplLP7vDDiU84RUUZMrWdsH4ZBGJ8OosnwFkcUkBHp1DTIjuipLliw9yQn/ZJsXZvarsO+g==
- dependencies:
- path-to-regexp "~1.2.1"
-
path-is-absolute@^1.0.0:
version "1.0.1"
resolved "https://registry.yarnpkg.com/path-is-absolute/-/path-is-absolute-1.0.1.tgz#174b9268735534ffbc7ace6bf53a5a9e1b5c5f5f"
integrity sha1-F0uSaHNVNP+8es5r9TpanhtcX18=
-path-to-regexp@~1.2.1:
- version "1.2.1"
- resolved "https://registry.yarnpkg.com/path-to-regexp/-/path-to-regexp-1.2.1.tgz#b33705c140234d873c8721c7b9fd8b541ed3aff9"
- integrity sha1-szcFwUAjTYc8hyHHuf2LVB7Tr/k=
- dependencies:
- isarray "0.0.1"
-
"polymer-bridges@file:../../polymer-bridges":
version "1.0.0"
diff --git a/resources/com/google/gerrit/server/commit-msg_test.sh b/resources/com/google/gerrit/server/commit-msg_test.sh
index 2c256ff..0cc3da0 100755
--- a/resources/com/google/gerrit/server/commit-msg_test.sh
+++ b/resources/com/google/gerrit/server/commit-msg_test.sh
@@ -43,6 +43,31 @@
fi
}
+function test_empty_with_cutoff {
+ rm -f input
+ cat << EOF > input
+# Please enter the commit message for your changes.
+# ------------------------ >8 ------------------------
+# Do not modify or remove the line above.
+# Everything below it will be ignored.
+diff --git a/file.txt b/file.txt
+index 625fd613d9..03aeba3b21 100755
+--- a/file.txt
++++ b/file.txt
+@@ -38,6 +38,7 @@
+ context
+ line
+
++hello, world
+
+ context
+ line
+EOF
+ if ${hook} input ; then
+ fail "must fail on empty message"
+ fi
+}
+
function test_keep_cutoff_line {
if ! prereq_modern_git ; then
echo "old version of Git detected; skipping scissors test."
diff --git a/resources/com/google/gerrit/server/tools/root/hooks/commit-msg b/resources/com/google/gerrit/server/tools/root/hooks/commit-msg
index d9fd1f1..0154d43 100755
--- a/resources/com/google/gerrit/server/tools/root/hooks/commit-msg
+++ b/resources/com/google/gerrit/server/tools/root/hooks/commit-msg
@@ -50,7 +50,7 @@
trap 'rm -f "$dest" "$dest-2"' EXIT
-if ! git stripspace --strip-comments < "$1" > "${dest}" ; then
+if ! cat "$1" | sed -e '/>8/q' | git stripspace --strip-comments > "${dest}" ; then
echo "cannot strip comments from $1"
exit 1
fi
diff --git a/tools/node_tools/node_modules_licenses/licenses-map.ts b/tools/node_tools/node_modules_licenses/licenses-map.ts
index 7dfb23e..642a749 100644
--- a/tools/node_tools/node_modules_licenses/licenses-map.ts
+++ b/tools/node_tools/node_modules_licenses/licenses-map.ts
@@ -97,6 +97,9 @@
*/
public generateMap(nodeModulesFiles: ReadonlyArray<string>): LicensesMap {
const installedPackages = this.getInstalledPackages(nodeModulesFiles);
+ // Static packages that are not inside `node_modules` directories.
+ // gr-page.ts was derived from page.js, so we reproduce the original LICENSE.
+ installedPackages.push({name: 'polygerrit-gr-page', version: 'current', rootPath: 'polygerrit-ui/app/elements/core/gr-router/', files: ['gr-page.ts']});
const licensedFilesGroupedByLicense = this.getLicensedFilesGroupedByLicense(installedPackages);
const result: LicensesMap = {};
