Merge "Reduce number of calls to ?q=change:"
diff --git a/Documentation/licenses.txt b/Documentation/licenses.txt
index 8b67da3..1193e57 100644
--- a/Documentation/licenses.txt
+++ b/Documentation/licenses.txt
@@ -48,7 +48,6 @@
* commons:codec
* commons:compress
* commons:dbcp
-* commons:lang
* commons:lang3
* commons:net
* commons:pool
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 66b3645..7c478ae 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7653,20 +7653,28 @@
[options="header",cols="1,^1,5"]
|===========================
-|Field Name ||Description
-|`base` |optional|
+|Field Name ||Description
+|`base` |optional|
The new parent revision. This can be a ref or a SHA-1 to a concrete patchset. +
Alternatively, a change number can be specified, in which case the current
patch set is inferred. +
Empty string is used for rebasing directly on top of the target branch,
which effectively breaks dependency towards a parent change.
-|`allow_conflicts`|optional, defaults to false|
+|`allow_conflicts` |optional, defaults to false|
If `true`, the rebase also succeeds if there are conflicts. +
If there are conflicts the file contents of the rebased patch set contain
git conflict markers to indicate the conflicts. +
Callers can find out whether there were conflicts by checking the
`contains_git_conflicts` field in the returned link:#change-info[ChangeInfo]. +
If there are conflicts the change is marked as work-in-progress.
+|`validation_options`|optional|
+Map with key-value pairs that are forwarded as options to the commit validation
+listeners (e.g. can be used to skip certain validations). Which validation
+options are supported depends on the installed commit validation listeners.
+Gerrit core doesn't support any validation options, but commit validation
+listeners that are implemented in plugins may. Please refer to the
+documentation of the installed plugins to learn whether they support validation
+options. Unknown validation options are silently ignored.
|===========================
[[related-change-and-commit-info]]
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index 37b4780..4298663 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -41,7 +41,7 @@
"//lib:jgit",
"//lib:jgit-ssh-apache",
"//lib/commons:compress",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/flogger:api",
"//lib/guice",
"//lib/guice:guice-assistedinject",
diff --git a/java/com/google/gerrit/acceptance/InProcessProtocol.java b/java/com/google/gerrit/acceptance/InProcessProtocol.java
index 15be85c..17ce595 100644
--- a/java/com/google/gerrit/acceptance/InProcessProtocol.java
+++ b/java/com/google/gerrit/acceptance/InProcessProtocol.java
@@ -235,9 +235,9 @@
PermissionBackend.ForProject perm = permissionBackend.currentUser().project(req.project);
try {
- perm.check(ProjectPermission.RUN_UPLOAD_PACK);
- } catch (AuthException e) {
- throw new ServiceNotAuthorizedException(e.getMessage(), e);
+ if (!perm.test(ProjectPermission.RUN_UPLOAD_PACK)) {
+ throw new ServiceNotAuthorizedException("upload pack not permitted");
+ }
} catch (PermissionBackendException e) {
throw new RuntimeException(e);
}
diff --git a/java/com/google/gerrit/acceptance/TestMetricMaker.java b/java/com/google/gerrit/acceptance/TestMetricMaker.java
index 2620f99..7baefb8 100644
--- a/java/com/google/gerrit/acceptance/TestMetricMaker.java
+++ b/java/com/google/gerrit/acceptance/TestMetricMaker.java
@@ -20,7 +20,7 @@
import com.google.inject.Singleton;
import java.util.HashMap;
import java.util.Map;
-import org.apache.commons.lang.mutable.MutableLong;
+import org.apache.commons.lang3.mutable.MutableLong;
/**
* {@link com.google.gerrit.metrics.MetricMaker} to be bound in tests.
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/BUILD b/java/com/google/gerrit/acceptance/testsuite/project/BUILD
index f9e2fb5..850a133 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/BUILD
+++ b/java/com/google/gerrit/acceptance/testsuite/project/BUILD
@@ -17,7 +17,7 @@
"//lib:jgit-junit",
"//lib/auto:auto-value",
"//lib/auto:auto-value-annotations",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/guice",
],
)
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
index 18da4b3..deeb843 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -43,7 +43,7 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
-import org.apache.commons.lang.RandomStringUtils;
+import org.apache.commons.lang3.RandomStringUtils;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
diff --git a/java/com/google/gerrit/extensions/api/changes/RebaseInput.java b/java/com/google/gerrit/extensions/api/changes/RebaseInput.java
index 10559a3..e9b05cc 100644
--- a/java/com/google/gerrit/extensions/api/changes/RebaseInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/RebaseInput.java
@@ -14,6 +14,8 @@
package com.google.gerrit.extensions.api.changes;
+import java.util.Map;
+
public class RebaseInput {
public String base;
@@ -24,4 +26,6 @@
* to indicate the conflicts.
*/
public boolean allowConflicts;
+
+ public Map<String, String> validationOptions;
}
diff --git a/java/com/google/gerrit/httpd/BUILD b/java/com/google/gerrit/httpd/BUILD
index ea7c609..1284829 100644
--- a/java/com/google/gerrit/httpd/BUILD
+++ b/java/com/google/gerrit/httpd/BUILD
@@ -37,7 +37,7 @@
"//lib:soy",
"//lib/auto:auto-value",
"//lib/auto:auto-value-annotations",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/flogger:api",
"//lib/guice",
"//lib/guice:guice-assistedinject",
diff --git a/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java
index e3a401a..dddc298 100644
--- a/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java
+++ b/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java
@@ -85,7 +85,7 @@
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.util.IO;
import org.eclipse.jgit.util.RawParseUtils;
diff --git a/java/com/google/gerrit/pgm/BUILD b/java/com/google/gerrit/pgm/BUILD
index d4c5a87..df64bc7 100644
--- a/java/com/google/gerrit/pgm/BUILD
+++ b/java/com/google/gerrit/pgm/BUILD
@@ -48,7 +48,7 @@
"//lib:servlet-api-without-neverlink",
"//lib/auto:auto-value",
"//lib/auto:auto-value-annotations",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/flogger:api",
"//lib/guice",
"//lib/guice:guice-assistedinject",
diff --git a/java/com/google/gerrit/pgm/LocalUsernamesToLowerCase.java b/java/com/google/gerrit/pgm/LocalUsernamesToLowerCase.java
index f651994..b7ff1f7 100644
--- a/java/com/google/gerrit/pgm/LocalUsernamesToLowerCase.java
+++ b/java/com/google/gerrit/pgm/LocalUsernamesToLowerCase.java
@@ -38,7 +38,7 @@
import java.util.Collection;
import java.util.Locale;
import java.util.Optional;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ProgressMonitor;
import org.eclipse.jgit.lib.Repository;
diff --git a/java/com/google/gerrit/server/BUILD b/java/com/google/gerrit/server/BUILD
index 9470931..95ad1ba 100644
--- a/java/com/google/gerrit/server/BUILD
+++ b/java/com/google/gerrit/server/BUILD
@@ -111,7 +111,6 @@
"//lib/commons:codec",
"//lib/commons:compress",
"//lib/commons:dbcp",
- "//lib/commons:lang",
"//lib/commons:lang3",
"//lib/commons:net",
"//lib/commons:validator",
diff --git a/java/com/google/gerrit/server/account/AccountControl.java b/java/com/google/gerrit/server/account/AccountControl.java
index 3f7f3f2..f5f9b3d 100644
--- a/java/com/google/gerrit/server/account/AccountControl.java
+++ b/java/com/google/gerrit/server/account/AccountControl.java
@@ -24,7 +24,6 @@
import com.google.gerrit.entities.PermissionRule;
import com.google.gerrit.exceptions.NoSuchGroupException;
import com.google.gerrit.extensions.common.AccountVisibility;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.group.SystemGroupBackend;
@@ -259,10 +258,7 @@
private boolean viewAll() {
if (viewAll == null) {
try {
- perm.check(GlobalPermission.VIEW_ALL_ACCOUNTS);
- viewAll = true;
- } catch (AuthException e) {
- viewAll = false;
+ viewAll = perm.test(GlobalPermission.VIEW_ALL_ACCOUNTS);
} catch (PermissionBackendException e) {
logger.atFine().withCause(e).log(
"Failed to check %s global capability for user %s",
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 1eb65ec..8824d56 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -27,7 +27,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.gerrit.entities.Account;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.index.Schema;
import com.google.gerrit.server.CurrentUser;
@@ -443,9 +442,10 @@
// more strict here.
boolean canSeeSecondaryEmails = false;
try {
- permissionBackend.user(self.get()).check(GlobalPermission.MODIFY_ACCOUNT);
- canSeeSecondaryEmails = true;
- } catch (AuthException | PermissionBackendException e) {
+ if (permissionBackend.user(self.get()).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ canSeeSecondaryEmails = true;
+ }
+ } catch (PermissionBackendException e) {
// remains false
}
return accountQueryProvider.get().enforceVisibility(true)
diff --git a/java/com/google/gerrit/server/account/GroupControl.java b/java/com/google/gerrit/server/account/GroupControl.java
index d42db60..fd18d3e 100644
--- a/java/com/google/gerrit/server/account/GroupControl.java
+++ b/java/com/google/gerrit/server/account/GroupControl.java
@@ -19,7 +19,6 @@
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.GroupDescription;
import com.google.gerrit.exceptions.NoSuchGroupException;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -189,10 +188,7 @@
private boolean canAdministrateServer() {
try {
- perm.check(GlobalPermission.ADMINISTRATE_SERVER);
- return true;
- } catch (AuthException e) {
- return false;
+ return perm.test(GlobalPermission.ADMINISTRATE_SERVER);
} catch (PermissionBackendException e) {
logger.atFine().log(
"Failed to check %s global capability for user %s",
diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index 8d227f7..227ff70 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -223,10 +223,7 @@
.statePermitsRead()) {
return false;
}
- permissionBackend.absentUser(accountId).change(notes).check(ChangePermission.READ);
- return true;
- } catch (AuthException e) {
- return false;
+ return permissionBackend.absentUser(accountId).change(notes).test(ChangePermission.READ);
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log(
"Failed to check if account %d can see change %d",
@@ -330,11 +327,9 @@
for (Map.Entry<String, Short> vote : approvals.entrySet()) {
String name = vote.getKey();
Short value = vote.getValue();
- try {
- forChange.check(new LabelPermission.WithValue(name, value));
- } catch (AuthException e) {
+ if (!forChange.test(new LabelPermission.WithValue(name, value))) {
throw new AuthException(
- String.format("applying label \"%s\": %d is restricted", name, value), e);
+ String.format("applying label \"%s\": %d is restricted", name, value));
}
}
}
diff --git a/java/com/google/gerrit/server/audit/BUILD b/java/com/google/gerrit/server/audit/BUILD
index 9ed5879..bde7404 100644
--- a/java/com/google/gerrit/server/audit/BUILD
+++ b/java/com/google/gerrit/server/audit/BUILD
@@ -58,7 +58,7 @@
"//lib/bouncycastle:bcprov-neverlink",
"//lib/commons:compress",
"//lib/commons:dbcp",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/commons:net",
"//lib/commons:validator",
"//lib/flogger:api",
diff --git a/java/com/google/gerrit/server/cancellation/BUILD b/java/com/google/gerrit/server/cancellation/BUILD
index 05530a5..00f27a9 100644
--- a/java/com/google/gerrit/server/cancellation/BUILD
+++ b/java/com/google/gerrit/server/cancellation/BUILD
@@ -9,6 +9,6 @@
deps = [
"//java/com/google/gerrit/common:annotations",
"//lib:guava",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
],
)
diff --git a/java/com/google/gerrit/server/cancellation/RequestCancelledException.java b/java/com/google/gerrit/server/cancellation/RequestCancelledException.java
index d89701f..7a456c0 100644
--- a/java/com/google/gerrit/server/cancellation/RequestCancelledException.java
+++ b/java/com/google/gerrit/server/cancellation/RequestCancelledException.java
@@ -17,7 +17,7 @@
import com.google.common.base.Throwables;
import com.google.gerrit.common.Nullable;
import java.util.Optional;
-import org.apache.commons.lang.WordUtils;
+import org.apache.commons.lang3.text.WordUtils;
/** Exception to signal that the current request is cancelled and should be aborted. */
public class RequestCancelledException extends RuntimeException {
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 57f94ff..a0fa8e9 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -17,8 +17,10 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
+import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -94,6 +96,7 @@
private boolean sendEmail = true;
private boolean storeCopiedVotes = true;
private boolean matchAuthorToCommitterDate = false;
+ private ImmutableListMultimap<String, String> validationOptions = ImmutableListMultimap.of();
private CodeReviewCommit rebasedCommit;
private PatchSet.Id rebasedPatchSetId;
@@ -191,6 +194,13 @@
return this;
}
+ public RebaseChangeOp setValidationOptions(
+ ImmutableListMultimap<String, String> validationOptions) {
+ requireNonNull(validationOptions, "validationOptions may not be null");
+ this.validationOptions = validationOptions;
+ return this;
+ }
+
@Override
public void updateRepo(RepoContext ctx)
throws MergeConflictException, InvalidChangeOperationException, RestApiException, IOException,
@@ -241,6 +251,8 @@
patchSetInserter.setWorkInProgress(true);
}
+ patchSetInserter.setValidationOptions(validationOptions);
+
if (postMessage) {
patchSetInserter.setMessage(
messageForRebasedChange(rebasedPatchSetId, originalPatchSet.id(), rebasedCommit));
diff --git a/java/com/google/gerrit/server/change/ReviewerJson.java b/java/com/google/gerrit/server/change/ReviewerJson.java
index 6189708..f0f3a8f 100644
--- a/java/com/google/gerrit/server/change/ReviewerJson.java
+++ b/java/com/google/gerrit/server/change/ReviewerJson.java
@@ -26,7 +26,6 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewerInfo;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.permissions.LabelPermission;
@@ -129,11 +128,8 @@
continue;
}
- try {
- perm.check(new LabelPermission(type.get()));
+ if (perm.test(new LabelPermission(type.get()))) {
out.approvals.put(name, formatValue((short) 0));
- } catch (AuthException e) {
- // Do nothing.
}
}
}
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index bfc7841..c98fcaa 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -48,7 +48,6 @@
import com.google.gerrit.extensions.api.changes.ReviewerResult;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.AccountInfo;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.AnonymousUser;
@@ -378,9 +377,10 @@
@Nullable
private ReviewerModification addByEmail(ReviewerInput input, ChangeNotes notes, CurrentUser user)
throws PermissionBackendException {
- try {
- permissionBackend.user(anonymousProvider.get()).change(notes).check(ChangePermission.READ);
- } catch (AuthException e) {
+ if (!permissionBackend
+ .user(anonymousProvider.get())
+ .change(notes)
+ .test(ChangePermission.READ)) {
return fail(
input,
FailureType.OTHER,
@@ -399,15 +399,10 @@
private boolean isValidReviewer(BranchNameKey branch, Account member)
throws PermissionBackendException {
- try {
- // Check ref permission instead of change permission, since change permissions take into
- // account the private bit, whereas adding a user as a reviewer is explicitly allowing them to
- // see private changes.
- permissionBackend.absentUser(member.id()).ref(branch).check(RefPermission.READ);
- return true;
- } catch (AuthException e) {
- return false;
- }
+ // Check ref permission instead of change permission, since change permissions take into
+ // account the private bit, whereas adding a user as a reviewer is explicitly allowing them to
+ // see private changes.
+ return permissionBackend.absentUser(member.id()).ref(branch).test(RefPermission.READ);
}
private ReviewerModification fail(ReviewerInput input, FailureType failureType, String error) {
diff --git a/java/com/google/gerrit/server/change/RevisionJson.java b/java/com/google/gerrit/server/change/RevisionJson.java
index 5a2a0eb..0321fcb 100644
--- a/java/com/google/gerrit/server/change/RevisionJson.java
+++ b/java/com/google/gerrit/server/change/RevisionJson.java
@@ -48,7 +48,6 @@
import com.google.gerrit.extensions.config.DownloadScheme;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.Extension;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
@@ -353,9 +352,7 @@
}
private boolean isWorldReadable(ChangeData cd) throws PermissionBackendException {
- try {
- permissionBackend.user(anonymous).change(cd).check(ChangePermission.READ);
- } catch (AuthException ae) {
+ if (!permissionBackend.user(anonymous).change(cd).test(ChangePermission.READ)) {
return false;
}
ProjectState projectState =
diff --git a/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java b/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java
index 4032e63..7fd075e 100644
--- a/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java
+++ b/java/com/google/gerrit/server/config/ConfigUpdatedEvent.java
@@ -20,7 +20,7 @@
import java.util.LinkedHashSet;
import java.util.Objects;
import java.util.Set;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.lib.Config;
/**
diff --git a/java/com/google/gerrit/server/documentation/MarkdownFormatter.java b/java/com/google/gerrit/server/documentation/MarkdownFormatter.java
index 307f3c5..6bc2744 100644
--- a/java/com/google/gerrit/server/documentation/MarkdownFormatter.java
+++ b/java/com/google/gerrit/server/documentation/MarkdownFormatter.java
@@ -36,7 +36,7 @@
import java.net.URL;
import java.nio.charset.Charset;
import java.util.concurrent.atomic.AtomicBoolean;
-import org.apache.commons.lang.StringEscapeUtils;
+import org.apache.commons.lang3.StringEscapeUtils;
import org.eclipse.jgit.util.RawParseUtils;
import org.eclipse.jgit.util.TemporaryBuffer;
@@ -78,7 +78,7 @@
}
public MarkdownFormatter setCss(String css) {
- this.css = StringEscapeUtils.escapeHtml(css);
+ this.css = StringEscapeUtils.escapeHtml4(css);
return this;
}
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 81f98e6..232aa6a 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -407,14 +407,15 @@
// Not allowed to edit if the current patch set is locked.
patchSetUtil.checkPatchSetNotLocked(notes);
- try {
- permissionBackend.currentUser().change(notes).check(ChangePermission.ADD_PATCH_SET);
- projectCache
- .get(notes.getProjectName())
- .orElseThrow(illegalState(notes.getProjectName()))
- .checkStatePermitsWrite();
- } catch (AuthException denied) {
- throw new AuthException("edit not permitted", denied);
+ boolean canEdit =
+ permissionBackend.currentUser().change(notes).test(ChangePermission.ADD_PATCH_SET);
+ canEdit &=
+ projectCache
+ .get(notes.getProjectName())
+ .orElseThrow(illegalState(notes.getProjectName()))
+ .statePermitsWrite();
+ if (!canEdit) {
+ throw new AuthException("edit not permitted");
}
}
diff --git a/java/com/google/gerrit/server/events/EventBroker.java b/java/com/google/gerrit/server/events/EventBroker.java
index 4001a48..2697da5 100644
--- a/java/com/google/gerrit/server/events/EventBroker.java
+++ b/java/com/google/gerrit/server/events/EventBroker.java
@@ -22,7 +22,6 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.registration.DynamicItem;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.GerritInstanceId;
@@ -170,9 +169,8 @@
return false;
}
- permissionBackend.user(user).project(project).check(ProjectPermission.ACCESS);
- return true;
- } catch (AuthException | PermissionBackendException e) {
+ return permissionBackend.user(user).project(project).test(ProjectPermission.ACCESS);
+ } catch (PermissionBackendException e) {
return false;
}
}
@@ -185,15 +183,10 @@
if (!pe.isPresent() || !pe.get().statePermitsRead()) {
return false;
}
- try {
- permissionBackend
- .user(user)
- .change(notesFactory.createChecked(change))
- .check(ChangePermission.READ);
- return true;
- } catch (AuthException e) {
- return false;
- }
+ return permissionBackend
+ .user(user)
+ .change(notesFactory.createChecked(change))
+ .test(ChangePermission.READ);
}
protected boolean isVisibleTo(BranchNameKey branchName, CurrentUser user)
@@ -203,12 +196,7 @@
return false;
}
- try {
- permissionBackend.user(user).ref(branchName).check(RefPermission.READ);
- return true;
- } catch (AuthException e) {
- return false;
- }
+ return permissionBackend.user(user).ref(branchName).test(RefPermission.READ);
}
protected boolean isVisibleTo(Event event, CurrentUser user) throws PermissionBackendException {
diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
index 85d7db0..718eec2 100644
--- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
@@ -325,9 +325,7 @@
/** Determine if the user can upload commits. */
public Capable canUpload() throws IOException, PermissionBackendException {
- try {
- perm.check(ProjectPermission.PUSH_AT_LEAST_ONE_REF);
- } catch (AuthException e) {
+ if (!perm.test(ProjectPermission.PUSH_AT_LEAST_ONE_REF)) {
return new Capable("Upload denied for project '" + projectState.getName() + "'");
}
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 8049df4..5f19758 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -639,10 +639,10 @@
}
if (!sboAuthor && !sboCommitter && !sboMe) {
try {
- perm.check(RefPermission.FORGE_COMMITTER);
- } catch (AuthException denied) {
- throw new CommitValidationException(
- "not Signed-off-by author/committer/uploader in message footer", denied);
+ if (!perm.test(RefPermission.FORGE_COMMITTER)) {
+ throw new CommitValidationException(
+ "not Signed-off-by author/committer/uploader in message footer");
+ }
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_COMMITTER");
throw new CommitValidationException("internal auth error");
@@ -673,11 +673,11 @@
return Collections.emptyList();
}
try {
- perm.check(RefPermission.FORGE_AUTHOR);
+ if (!perm.test(RefPermission.FORGE_AUTHOR)) {
+ throw new CommitValidationException(
+ "invalid author", invalidEmail("author", author, user, urlFormatter));
+ }
return Collections.emptyList();
- } catch (AuthException e) {
- throw new CommitValidationException(
- "invalid author", invalidEmail("author", author, user, urlFormatter), e);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_AUTHOR");
throw new CommitValidationException("internal auth error");
@@ -706,11 +706,11 @@
return Collections.emptyList();
}
try {
- perm.check(RefPermission.FORGE_COMMITTER);
+ if (!perm.test(RefPermission.FORGE_COMMITTER)) {
+ throw new CommitValidationException(
+ "invalid committer", invalidEmail("committer", committer, user, urlFormatter));
+ }
return Collections.emptyList();
- } catch (AuthException e) {
- throw new CommitValidationException(
- "invalid committer", invalidEmail("committer", committer, user, urlFormatter), e);
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_COMMITTER");
throw new CommitValidationException("internal auth error");
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java
index c514969..40ce671 100644
--- a/java/com/google/gerrit/server/git/validators/MergeValidators.java
+++ b/java/com/google/gerrit/server/git/validators/MergeValidators.java
@@ -195,9 +195,9 @@
if (!oldParent.equals(newParent)) {
if (!allowProjectOwnersToChangeParent) {
try {
- permissionBackend.user(caller).check(GlobalPermission.ADMINISTRATE_SERVER);
- } catch (AuthException e) {
- throw new MergeValidationException(SET_BY_ADMIN, e);
+ if (!permissionBackend.user(caller).test(GlobalPermission.ADMINISTRATE_SERVER)) {
+ throw new MergeValidationException(SET_BY_ADMIN);
+ }
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log("Cannot check ADMINISTRATE_SERVER");
throw new MergeValidationException("validation unavailable", e);
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index 24c4d6d..534da0d 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -70,7 +70,7 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.diff.DiffAlgorithm;
import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.diff.EditList;
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index 39b0f90..4f10528 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -27,7 +27,6 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
@@ -319,17 +318,14 @@
}
if (visibleChangesCache.isVisible(id)) {
- try {
- // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
- permissionBackendForProject
- .ref(visibleChangesCache.getBranchNameKey(id).branch())
- .check(RefPermission.READ_PRIVATE_CHANGES);
- logger.atFinest().log("Foreign change edit ref is visible: %s", name);
- return true;
- } catch (AuthException e) {
- logger.atFinest().log("Foreign change edit ref is not visible: %s", name);
- return false;
- }
+ // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
+ boolean canRead =
+ permissionBackendForProject
+ .ref(visibleChangesCache.getBranchNameKey(id).branch())
+ .test(RefPermission.READ_PRIVATE_CHANGES);
+ logger.atFinest().log(
+ "Foreign change edit ref is " + (canRead ? "visible" : "invisible") + ": %s", name);
+ return canRead;
}
logger.atFinest().log("Change %d of change edit ref %s is not visible", id.get(), name);
@@ -347,23 +343,14 @@
}
private boolean canReadRef(String ref) throws PermissionBackendException {
- try {
- permissionBackendForProject.ref(ref).check(RefPermission.READ);
- } catch (AuthException e) {
- return false;
- }
- return projectState.statePermitsRead();
+ return permissionBackendForProject.ref(ref).test(RefPermission.READ)
+ && projectState.statePermitsRead();
}
private boolean checkProjectPermission(
PermissionBackend.ForProject forProject, ProjectPermission perm)
throws PermissionBackendException {
- try {
- forProject.check(perm);
- } catch (AuthException e) {
- return false;
- }
- return true;
+ return forProject.test(perm);
}
/**
@@ -394,12 +381,7 @@
} catch (StorageException e) {
throw new PermissionBackendException("can't construct change notes", e);
}
- try {
- permissionBackendForProject.change(notes).check(ChangePermission.READ);
- return true;
- } catch (AuthException e) {
- return false;
- }
+ return permissionBackendForProject.change(notes).test(ChangePermission.READ);
}
@AutoValue
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 1191db8..fea2827 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -246,10 +246,9 @@
Set<Project.NameKey> allowed = Sets.newHashSetWithExpectedSize(projects.size());
for (Project.NameKey project : projects) {
try {
- project(project).check(perm);
- allowed.add(project);
- } catch (AuthException e) {
- // Do not include this project in allowed.
+ if (project(project).test(perm)) {
+ allowed.add(project);
+ }
} catch (PermissionBackendException e) {
if (e.getCause() instanceof RepositoryNotFoundException) {
logger.atWarning().withCause(e).log(
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index 1203049..664d867 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -165,9 +165,8 @@
boolean isAdmin() {
try {
- permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
- return true;
- } catch (AuthException | PermissionBackendException e) {
+ return permissionBackend.user(user).test(GlobalPermission.ADMINISTRATE_SERVER);
+ } catch (PermissionBackendException e) {
return false;
}
}
diff --git a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
index cc6387b..c2d1139 100644
--- a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
+++ b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
@@ -25,7 +25,6 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.NoSuchGroupException;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.GroupControl;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -170,17 +169,14 @@
return true;
}
- try {
- // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
- projectControl
- .asForProject()
- .ref(cd.change().getDest().branch())
- .check(RefPermission.READ_PRIVATE_CHANGES);
- logger.atFinest().log("Foreign change edit ref is visible: %s", refName);
- return true;
- } catch (AuthException e) {
- logger.atFinest().log("Foreign change edit ref is not visible: %s", refName);
- return false;
- }
+ // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
+ boolean canRead =
+ projectControl
+ .asForProject()
+ .ref(cd.change().getDest().branch())
+ .test(RefPermission.READ_PRIVATE_CHANGES);
+ logger.atFinest().log(
+ "Foreign change edit ref is " + (canRead ? "visible" : "invisible") + ": %s", refName);
+ return canRead;
}
}
diff --git a/java/com/google/gerrit/server/permissions/VisibleChangesCache.java b/java/com/google/gerrit/server/permissions/VisibleChangesCache.java
index 2e47576..552f4f6 100644
--- a/java/com/google/gerrit/server/permissions/VisibleChangesCache.java
+++ b/java/com/google/gerrit/server/permissions/VisibleChangesCache.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeNotes.Factory.ChangeNotesResult;
@@ -113,11 +112,8 @@
if (!projectState.statePermitsRead()) {
continue;
}
- try {
- permissionBackendForProject.change(cd).check(ChangePermission.READ);
+ if (permissionBackendForProject.change(cd).test(ChangePermission.READ)) {
visibleChanges.put(cd.getId(), cd.change().getDest());
- } catch (AuthException e) {
- // Do nothing.
}
}
} catch (StorageException e) {
@@ -158,11 +154,8 @@
return null;
}
- try {
- permissionBackendForProject.change(r.notes()).check(ChangePermission.READ);
+ if (permissionBackendForProject.change(r.notes()).test(ChangePermission.READ)) {
return r.notes();
- } catch (AuthException e) {
- // Skip.
}
return null;
}
diff --git a/java/com/google/gerrit/server/project/CreateRefControl.java b/java/com/google/gerrit/server/project/CreateRefControl.java
index 2e76949..0d015d4 100644
--- a/java/com/google/gerrit/server/project/CreateRefControl.java
+++ b/java/com/google/gerrit/server/project/CreateRefControl.java
@@ -124,13 +124,10 @@
Project.NameKey project,
PermissionBackend.ForRef forRef)
throws AuthException, PermissionBackendException, IOException {
- try {
- // If the user has update (push) permission, they can create the ref regardless
- // of whether they are pushing any new objects along with the create.
- forRef.check(RefPermission.UPDATE);
+ // If the user has update (push) permission, they can create the ref regardless
+ // of whether they are pushing any new objects along with the create.
+ if (forRef.test(RefPermission.UPDATE)) {
return;
- } catch (AuthException denied) {
- // Fall through to check reachability.
}
if (reachable.fromRefs(
project,
diff --git a/java/com/google/gerrit/server/project/RemoveReviewerControl.java b/java/com/google/gerrit/server/project/RemoveReviewerControl.java
index 0336e8e..1bc309c 100644
--- a/java/com/google/gerrit/server/project/RemoveReviewerControl.java
+++ b/java/com/google/gerrit/server/project/RemoveReviewerControl.java
@@ -108,30 +108,10 @@
// owner and site admin can remove anyone
PermissionBackend.WithUser withUser = permissionBackend.user(currentUser);
PermissionBackend.ForProject forProject = withUser.project(change.getProject());
- if (check(forProject.ref(change.getDest().branch()), RefPermission.WRITE_CONFIG)
- || check(withUser, GlobalPermission.ADMINISTRATE_SERVER)) {
+ if (forProject.ref(change.getDest().branch()).test(RefPermission.WRITE_CONFIG)
+ || withUser.test(GlobalPermission.ADMINISTRATE_SERVER)) {
return true;
}
return false;
}
-
- private static boolean check(PermissionBackend.ForRef forRef, RefPermission perm)
- throws PermissionBackendException {
- try {
- forRef.check(perm);
- return true;
- } catch (AuthException e) {
- return false;
- }
- }
-
- private static boolean check(PermissionBackend.WithUser withUser, GlobalPermission perm)
- throws PermissionBackendException {
- try {
- withUser.check(perm);
- return true;
- } catch (AuthException e) {
- return false;
- }
- }
}
diff --git a/java/com/google/gerrit/server/query/account/CanSeeChangePredicate.java b/java/com/google/gerrit/server/query/account/CanSeeChangePredicate.java
index 0252a06..e293285 100644
--- a/java/com/google/gerrit/server/query/account/CanSeeChangePredicate.java
+++ b/java/com/google/gerrit/server/query/account/CanSeeChangePredicate.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.query.account;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.index.query.PostFilterPredicate;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -36,15 +35,12 @@
@Override
public boolean match(AccountState accountState) {
try {
- permissionBackend
+ return permissionBackend
.absentUser(accountState.account().id())
.change(changeNotes)
- .check(ChangePermission.READ);
- return true;
+ .test(ChangePermission.READ);
} catch (PermissionBackendException e) {
throw new StorageException("Failed to check if account can see change", e);
- } catch (AuthException e) {
- return false;
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
index e3e0312..ac72d15 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
@@ -17,7 +17,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Change;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.index.query.IsVisibleToPredicate;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
@@ -86,8 +85,12 @@
Optional.of(user)
.filter(u -> u instanceof GroupBackedUser || u instanceof InternalUser)
.orElseGet(anonymousUserProvider::get));
+
try {
- withUser.change(cd).check(ChangePermission.READ);
+ if (!withUser.change(cd).test(ChangePermission.READ)) {
+ logger.atFine().log("Filter out non-visisble change: %s", cd);
+ return false;
+ }
} catch (PermissionBackendException e) {
Throwable cause = e.getCause();
if (cause instanceof RepositoryNotFoundException) {
@@ -97,9 +100,6 @@
return false;
}
throw new StorageException("unable to check permissions on change " + cd.getId(), e);
- } catch (AuthException e) {
- logger.atFine().log("Filter out non-visisble change: %s", cd);
- return false;
}
cd.cacheVisibleTo(user);
diff --git a/java/com/google/gerrit/server/restapi/BUILD b/java/com/google/gerrit/server/restapi/BUILD
index f70379b..2864391 100644
--- a/java/com/google/gerrit/server/restapi/BUILD
+++ b/java/com/google/gerrit/server/restapi/BUILD
@@ -32,7 +32,7 @@
"//lib/auto:auto-value",
"//lib/auto:auto-value-annotations",
"//lib/commons:compress",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
"//lib/errorprone:annotations",
"//lib/flogger:api",
"//lib/guice",
diff --git a/java/com/google/gerrit/server/restapi/account/Capabilities.java b/java/com/google/gerrit/server/restapi/account/Capabilities.java
index 3d719ff9..469f05d 100644
--- a/java/com/google/gerrit/server/restapi/account/Capabilities.java
+++ b/java/com/google/gerrit/server/restapi/account/Capabilities.java
@@ -71,12 +71,10 @@
}
GlobalOrPluginPermission perm = parse(id);
- try {
- permissionBackend.absentUser(target.getAccountId()).check(perm);
+ if (permissionBackend.absentUser(target.getAccountId()).test(perm)) {
return new AccountResource.Capability(target, globalOrPluginPermissionName(perm));
- } catch (AuthException e) {
- throw new ResourceNotFoundException(id, e);
}
+ throw new ResourceNotFoundException(id);
}
private GlobalOrPluginPermission parse(IdString id) throws ResourceNotFoundException {
diff --git a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
index e6b4eee..c671562 100644
--- a/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
+++ b/java/com/google/gerrit/server/restapi/account/QueryAccounts.java
@@ -21,7 +21,6 @@
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.AccountVisibility;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.Response;
@@ -186,11 +185,8 @@
if (modifyAccountCapabilityChecked) {
fillOptions.add(FillOptions.SECONDARY_EMAILS);
} else {
- try {
- permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
+ if (permissionBackend.currentUser().test(GlobalPermission.MODIFY_ACCOUNT)) {
fillOptions.add(FillOptions.SECONDARY_EMAILS);
- } catch (AuthException e) {
- // Do nothing.
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
index 572f704..a0c5b16 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
@@ -17,7 +17,6 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.registration.DynamicMap;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -123,9 +122,7 @@
}
private boolean canRead(ChangeNotes notes) throws PermissionBackendException {
- try {
- permissionBackend.currentUser().change(notes).check(ChangePermission.READ);
- } catch (AuthException e) {
+ if (!permissionBackend.currentUser().change(notes).test(ChangePermission.READ)) {
return false;
}
Optional<ProjectState> projectState = projectCache.get(notes.getProjectName());
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 1a0f2b6..835fd5a 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -16,8 +16,10 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
@@ -53,6 +55,7 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Map;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
@@ -126,6 +129,7 @@
.create(rsrc.getNotes(), rsrc.getPatchSet(), findBaseRev(repo, rw, rsrc, input))
.setForceContentMerge(true)
.setAllowConflicts(input.allowConflicts)
+ .setValidationOptions(getValidateOptionsAsMultimap(input.validationOptions))
.setFireRevisionCreated(true);
// TODO(dborowitz): Why no notification? This seems wrong; dig up blame.
bu.setNotify(NotifyResolver.Result.none());
@@ -246,6 +250,20 @@
return description;
}
+ private static ImmutableListMultimap<String, String> getValidateOptionsAsMultimap(
+ @Nullable Map<String, String> validationOptions) {
+ if (validationOptions == null) {
+ return ImmutableListMultimap.of();
+ }
+
+ ImmutableListMultimap.Builder<String, String> validationOptionsBuilder =
+ ImmutableListMultimap.builder();
+ validationOptions
+ .entrySet()
+ .forEach(e -> validationOptionsBuilder.put(e.getKey(), e.getValue()));
+ return validationOptionsBuilder.build();
+ }
+
public static class CurrentRevision implements RestModifyView<ChangeResource, RebaseInput> {
private final PatchSetUtil psUtil;
private final Rebase rebase;
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 383eda0..8f29d82 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -92,7 +92,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
-import org.apache.commons.lang.RandomStringUtils;
+import org.apache.commons.lang3.RandomStringUtils;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
index cc81aac..d8d51d4 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
@@ -56,7 +56,7 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
-import org.apache.commons.lang.mutable.MutableDouble;
+import org.apache.commons.lang3.mutable.MutableDouble;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index c2f9b85..4782729 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -257,7 +257,13 @@
return "Change " + c.getId() + " is marked work in progress";
}
try {
- MergeOp.checkSubmitRequirements(c);
+ // The data in the change index may be stale (e.g. if submit requirements have been
+ // changed). For that one change for which the submit action is computed, use the
+ // freshly loaded ChangeData instance 'cd' instead of the potentially stale ChangeData
+ // instance 'c' that was loaded from the index. This makes a difference if the ChangeSet
+ // 'cs' only contains this one single change. If the ChangeSet contains further changes
+ // those may still be stale.
+ MergeOp.checkSubmitRequirements(cd.getId().equals(c.getId()) ? cd : c);
} catch (ResourceConflictException e) {
return "Change " + c.getId() + " is not ready: " + e.getMessage();
}
@@ -317,14 +323,6 @@
String submitProblems = problemsForSubmittingChangeset(cd, cs, resource.getUser());
- // Recheck mergeability rather than using value stored in the index, which may be stale.
- // TODO(dborowitz): This is ugly; consider providing a way to not read stored fields from the
- // index in the first place.
- // cd.setMergeable(null);
- // That was done in unmergeableChanges which was called by problemsForSubmittingChangeset, so
- // now it is safe to read from the cache, as it yields the same result.
- Boolean enabled = cd.isMergeable();
-
if (submitProblems != null) {
return new UiAction.Description()
.setLabel(treatWithTopic ? submitTopicLabel : (cs.size() > 1) ? labelWithParents : label)
@@ -333,6 +331,14 @@
.setEnabled(false);
}
+ // Recheck mergeability rather than using value stored in the index, which may be stale.
+ // TODO(dborowitz): This is ugly; consider providing a way to not read stored fields from the
+ // index in the first place.
+ // cd.setMergeable(null);
+ // That was done in unmergeableChanges which was called by problemsForSubmittingChangeset, so
+ // now it is safe to read from the cache, as it yields the same result.
+ Boolean enabled = cd.isMergeable();
+
if (treatWithTopic) {
Map<String, String> params =
ImmutableMap.of(
diff --git a/java/com/google/gerrit/server/submit/SubmoduleCommits.java b/java/com/google/gerrit/server/submit/SubmoduleCommits.java
index 1312a4b..37df66b 100644
--- a/java/com/google/gerrit/server/submit/SubmoduleCommits.java
+++ b/java/com/google/gerrit/server/submit/SubmoduleCommits.java
@@ -36,7 +36,7 @@
import java.util.List;
import java.util.Objects;
import java.util.Optional;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.dircache.DirCache;
import org.eclipse.jgit.dircache.DirCacheBuilder;
import org.eclipse.jgit.dircache.DirCacheEditor;
diff --git a/java/com/google/gerrit/server/util/MostSpecificComparator.java b/java/com/google/gerrit/server/util/MostSpecificComparator.java
index ac33902..8f94c48 100644
--- a/java/com/google/gerrit/server/util/MostSpecificComparator.java
+++ b/java/com/google/gerrit/server/util/MostSpecificComparator.java
@@ -17,7 +17,7 @@
import com.google.gerrit.entities.AccessSection;
import com.google.gerrit.server.project.RefPattern;
import java.util.Comparator;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
/**
* Order the Ref Pattern by the most specific. This sort is done by:
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index dbf129a..1a79b53 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -163,7 +163,11 @@
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.testing.TestChangeETagComputation;
+import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.ChangeMessageModifier;
+import com.google.gerrit.server.git.validators.CommitValidationException;
+import com.google.gerrit.server.git.validators.CommitValidationListener;
+import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.index.change.ChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
@@ -1014,6 +1018,31 @@
}
@Test
+ public void rebaseWithValidationOptions() throws Exception {
+ // Create two changes both with the same parent
+ PushOneCommit.Result r = createChange();
+ testRepo.reset("HEAD~1");
+ PushOneCommit.Result r2 = createChange();
+
+ // Approve and submit the first change
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
+
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.validationOptions = ImmutableMap.of("key", "value");
+
+ TestCommitValidationListener testCommitValidationListener = new TestCommitValidationListener();
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(testCommitValidationListener)) {
+ // Rebase the second change
+ gApi.changes().id(r2.getChangeId()).current().rebase(rebaseInput);
+ assertThat(testCommitValidationListener.receiveEvent.pushOptions)
+ .containsExactly("key", "value");
+ }
+ }
+
+ @Test
public void deleteNewChangeAsAdmin() throws Exception {
deleteChangeAsUser(admin, admin);
}
@@ -4707,4 +4736,15 @@
private void voteLabel(String changeId, String labelName, int score) throws RestApiException {
gApi.changes().id(changeId).current().review(new ReviewInput().label(labelName, score));
}
+
+ private static class TestCommitValidationListener implements CommitValidationListener {
+ public CommitReceivedEvent receiveEvent;
+
+ @Override
+ public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+ throws CommitValidationException {
+ this.receiveEvent = receiveEvent;
+ return ImmutableList.of();
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
index ff76546..c9a57d0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
@@ -56,6 +56,7 @@
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelDefinitionInput;
import com.google.gerrit.extensions.common.LegacySubmitRequirementInfo;
@@ -75,6 +76,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import java.util.stream.IntStream;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -957,6 +959,126 @@
}
@Test
+ public void submitRequirementThatOverridesParentSubmitRequirementTakesEffectImmediately()
+ throws Exception {
+ // Define submit requirement in root project that ignores self approvals from the uploader.
+ configSubmitRequirement(
+ allProjects,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX,user=non_uploader"))
+ .setAllowOverrideInChildProjects(true)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Apply a self approval from the uploader.
+ voteLabel(changeId, "Code-Review", 2);
+
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ // Code-Review+2 is ignored since it's a self approval from the uploader
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ false);
+ // Legacy requirement is coming from the label MaxWithBlock function. Already satisfied since it
+ // doesn't ignore self approvals.
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+
+ // since the change is not submittable we expect the submit action to be not returned
+ assertThat(gApi.changes().id(changeId).current().actions()).doesNotContainKey("submit");
+
+ // Override submit requirement in project (allow uploaders to self approve).
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .setAllowOverrideInChildProjects(true)
+ .build());
+
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ // the self approval from the uploader is no longer ignored, hence the submit requirement is
+ // satisfied now
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ false);
+
+ // since the change is submittable now we expect the submit action to be returned
+ Map<String, ActionInfo> actions = gApi.changes().id(changeId).current().actions();
+ assertThat(actions).containsKey("submit");
+ ActionInfo submitAction = actions.get("submit");
+ assertThat(submitAction.enabled).isTrue();
+ }
+
+ @Test
+ public void
+ submitRequirementThatOverridesParentSubmitRequirementTakesEffectImmediately_staleIndex()
+ throws Exception {
+ // Define submit requirement in root project that ignores self approvals from the uploader.
+ configSubmitRequirement(
+ allProjects,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX,user=non_uploader"))
+ .setAllowOverrideInChildProjects(true)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Apply a self approval from the uploader.
+ voteLabel(changeId, "Code-Review", 2);
+
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ // Code-Review+2 is ignored since it's a self approval from the uploader
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.UNSATISFIED, /* isLegacy= */ false);
+ // Legacy requirement is coming from the label MaxWithBlock function. Already satisfied since it
+ // doesn't ignore self approvals.
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+
+ // since the change is not submittable we expect the submit action to be not returned
+ assertThat(gApi.changes().id(changeId).current().actions()).doesNotContainKey("submit");
+
+ // disable change index writes so that the change in the index gets stale when the new submit
+ // requirement is added
+ disableChangeIndexWrites();
+ try {
+ // Override submit requirement in project (allow uploaders to self approve).
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .setAllowOverrideInChildProjects(true)
+ .build());
+
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ // the self approval from the uploader is no longer ignored, hence the submit requirement is
+ // satisfied now
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ false);
+
+ // since the change is submittable now we expect the submit action to be returned
+ Map<String, ActionInfo> actions = gApi.changes().id(changeId).current().actions();
+ assertThat(actions).containsKey("submit");
+ ActionInfo submitAction = actions.get("submit");
+ assertThat(submitAction.enabled).isTrue();
+ } finally {
+ enableChangeIndexWrites();
+ }
+ }
+
+ @Test
public void submitRequirement_partiallyOverriddenSRIsIgnored() throws Exception {
// Create build-cop-override label
LabelDefinitionInput input = new LabelDefinitionInput();
diff --git a/javatests/com/google/gerrit/acceptance/git/BUILD b/javatests/com/google/gerrit/acceptance/git/BUILD
index 13311e3..db73f3f 100644
--- a/javatests/com/google/gerrit/acceptance/git/BUILD
+++ b/javatests/com/google/gerrit/acceptance/git/BUILD
@@ -10,7 +10,7 @@
":submodule_util",
"//java/com/google/gerrit/git",
"//java/com/google/gerrit/server/git/receive/testing",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
],
) for f in glob(["*IT.java"])]
diff --git a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
index 14a8e98..ef2ca95 100644
--- a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
@@ -34,7 +34,7 @@
import com.google.gerrit.testing.ConfigSuite;
import com.google.inject.Inject;
import java.util.ArrayDeque;
-import org.apache.commons.lang.RandomStringUtils;
+import org.apache.commons.lang3.RandomStringUtils;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/BUILD b/javatests/com/google/gerrit/acceptance/rest/project/BUILD
index edcb1f9..1fcc69a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/BUILD
+++ b/javatests/com/google/gerrit/acceptance/rest/project/BUILD
@@ -10,7 +10,7 @@
":project",
":push_tag_util",
":refassert",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
],
) for f in glob(["*IT.java"])]
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/ListChildProjectsIT.java b/javatests/com/google/gerrit/acceptance/rest/project/ListChildProjectsIT.java
index 3c8357b..b2ececc 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/ListChildProjectsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/ListChildProjectsIT.java
@@ -32,7 +32,7 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.inject.Inject;
-import org.apache.commons.lang.RandomStringUtils;
+import org.apache.commons.lang3.RandomStringUtils;
import org.junit.Test;
@NoHttpd
diff --git a/javatests/com/google/gerrit/acceptance/rest/util/BUILD b/javatests/com/google/gerrit/acceptance/rest/util/BUILD
index 1d3fe65..9b16eb1 100644
--- a/javatests/com/google/gerrit/acceptance/rest/util/BUILD
+++ b/javatests/com/google/gerrit/acceptance/rest/util/BUILD
@@ -7,6 +7,6 @@
visibility = ["//visibility:public"],
deps = [
"//java/com/google/gerrit/acceptance:lib",
- "//lib/commons:lang",
+ "//lib/commons:lang3",
],
)
diff --git a/javatests/com/google/gerrit/acceptance/rest/util/RestCall.java b/javatests/com/google/gerrit/acceptance/rest/util/RestCall.java
index 7b0002c..91cf15a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/util/RestCall.java
+++ b/javatests/com/google/gerrit/acceptance/rest/util/RestCall.java
@@ -18,7 +18,7 @@
import com.google.auto.value.AutoValue;
import java.util.Optional;
-import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang3.StringUtils;
import org.junit.Ignore;
/** Data container for test REST requests. */
diff --git a/lib/commons/BUILD b/lib/commons/BUILD
index 38b1b6d..091ea07 100644
--- a/lib/commons/BUILD
+++ b/lib/commons/BUILD
@@ -15,12 +15,6 @@
)
java_library(
- name = "lang",
- data = ["//lib:LICENSE-Apache2.0"],
- exports = ["@commons-lang//jar"],
-)
-
-java_library(
name = "lang3",
data = ["//lib:LICENSE-Apache2.0"],
exports = ["@commons-lang3//jar"],
diff --git a/plugins/BUILD b/plugins/BUILD
index 1271f04..7862b1c 100644
--- a/plugins/BUILD
+++ b/plugins/BUILD
@@ -72,7 +72,6 @@
"//lib/auto:auto-value-gson",
"//lib/commons:compress",
"//lib/commons:dbcp",
- "//lib/commons:lang",
"//lib/commons:lang3",
"//lib/dropwizard:dropwizard-core",
"//lib/flogger:api",
diff --git a/plugins/gitiles b/plugins/gitiles
index a0709a4..b62b109 160000
--- a/plugins/gitiles
+++ b/plugins/gitiles
@@ -1 +1 @@
-Subproject commit a0709a402ee1d4fe3921fd81e575ec48a053cc9f
+Subproject commit b62b1098cfc566f5edb9e9a3fed8be20210675f5
diff --git a/plugins/replication b/plugins/replication
index 98926b4..220ce68 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 98926b44a199b5a7049232f6c3b3758267368f8f
+Subproject commit 220ce6829d4b8382374b08045dc4a1459db8f001
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement.ts
index 2a614a7..f97799c 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement.ts
@@ -33,9 +33,12 @@
getAllUniqueApprovals,
getRequirements,
hasNeutralStatus,
+ hasVotes,
iconForStatus,
} from '../../../utils/label-util';
import {sharedStyles} from '../../../styles/shared-styles';
+import {ifDefined} from 'lit/directives/if-defined';
+import {capitalizeFirstLetter} from '../../../utils/string-util';
@customElement('gr-change-list-column-requirement')
export class GrChangeListColumnRequirement extends LitElement {
@@ -67,7 +70,10 @@
}
override render() {
- return html`<div class="container ${this.computeClass()}">
+ return html`<div
+ class="container ${this.computeClass()}"
+ title="${ifDefined(this.computeLabelTitle())}"
+ >
${this.renderContent()}
</div>`;
}
@@ -112,6 +118,7 @@
return html`<gr-vote-chip
.vote="${worstVote}"
.label="${labelInfo}"
+ tooltip-with-who-voted
></gr-vote-chip>`;
}
}
@@ -133,6 +140,40 @@
return '';
}
+ private computeLabelTitle() {
+ if (!this.labelName) return;
+ const requirements = this.getRequirement(this.labelName);
+ if (requirements.length === 0) return 'Requirement not applicable';
+
+ const requirement = requirements[0];
+ if (requirement.status === SubmitRequirementStatus.UNSATISFIED) {
+ const requirementLabels = extractAssociatedLabels(
+ requirement,
+ 'onlySubmittability'
+ );
+ const allLabels = this.change?.labels ?? {};
+ const associatedLabels = Object.keys(allLabels).filter(label =>
+ requirementLabels.includes(label)
+ );
+ const requirementWithoutLabelToVoteOn = associatedLabels.length === 0;
+ if (requirementWithoutLabelToVoteOn) {
+ const status = capitalizeFirstLetter(requirement.status.toLowerCase());
+ return status;
+ }
+
+ const everyAssociatedLabelsIsWithoutVotes = associatedLabels.every(
+ label => !hasVotes(allLabels[label])
+ );
+ if (everyAssociatedLabelsIsWithoutVotes) {
+ return 'No votes';
+ } else {
+ return; // there is a vote with tooltip, so undefined label title
+ }
+ } else {
+ return capitalizeFirstLetter(requirement.status.toLowerCase());
+ }
+ }
+
private getRequirement(labelName: string) {
const requirements = getRequirements(this.change).filter(
sr => sr.name === labelName
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement_test.ts
index 698eb0b..11b48be 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirement/gr-change-list-column-requirement_test.ts
@@ -67,13 +67,16 @@
>
</gr-change-list-column-requirement>`
);
- expect(element).shadowDom.to.equal(/* HTML */ ` <div class="container">
- <iron-icon
- class="check-circle-filled"
- icon="gr-icons:check-circle-filled"
- >
- </iron-icon>
- </div>`);
+ expect(element).shadowDom.to.equal(
+ /* HTML */
+ ` <div class="container" title="Satisfied">
+ <iron-icon
+ class="check-circle-filled"
+ icon="gr-icons:check-circle-filled"
+ >
+ </iron-icon>
+ </div>`
+ );
});
test('show worst vote when state is not satisfied', async () => {
@@ -87,8 +90,8 @@
const label: DetailedLabelInfo = {
values: VALUES_2,
all: [
- {value: -1, _account_id: 777 as AccountId},
- {value: 1, _account_id: 324 as AccountId},
+ {value: -1, _account_id: 777 as AccountId, name: 'Reviewer'},
+ {value: 1, _account_id: 324 as AccountId, name: 'Reviewer 2'},
],
};
const submitRequirement: SubmitRequirementResultInfo = {
@@ -114,16 +117,22 @@
>
</gr-change-list-column-requirement>`
);
- expect(element).shadowDom.to.equal(/* HTML */ ` <div class="container">
- <gr-vote-chip></gr-vote-chip>
- </div>`);
+ expect(element).shadowDom.to.equal(
+ /* HTML */
+ ` <div class="container">
+ <gr-vote-chip tooltip-with-who-voted=""></gr-vote-chip>
+ </div>`
+ );
const voteChip = queryAndAssert(element, 'gr-vote-chip');
- expect(voteChip).shadowDom.to.equal(/* HTML */ ` <gr-tooltip-content
- class="container"
- has-tooltip=""
- title="bad"
- >
- <div class="negative vote-chip">-1</div>
- </gr-tooltip-content>`);
+ expect(voteChip).shadowDom.to.equal(
+ /* HTML */
+ ` <gr-tooltip-content
+ class="container"
+ has-tooltip=""
+ title="Reviewer: bad"
+ >
+ <div class="negative vote-chip">-1</div>
+ </gr-tooltip-content>`
+ );
});
});
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary.ts
index 24409c6..2272133 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary.ts
@@ -122,8 +122,8 @@
}
renderState(icon: string, aggregation: string | TemplateResult) {
- return html`<span class="${icon}"
- ><gr-submit-requirement-dashboard-hovercard .change=${this.change}>
+ return html`<span class="${icon}" role="button" tabindex="0">
+ <gr-submit-requirement-dashboard-hovercard .change=${this.change}>
</gr-submit-requirement-dashboard-hovercard>
<iron-icon class="${icon}" icon="gr-icons:${icon}" role="img"></iron-icon
>${aggregation}</span
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary_test.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary_test.ts
index 6dc9af1..9021e46 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary_test.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-column-requirements-summary/gr-change-list-column-requirements-summary_test.ts
@@ -72,7 +72,11 @@
html`<gr-change-list-column-requirements-summary .change=${change}>
</gr-change-list-column-requirements-summary>`
);
- expect(element).shadowDom.to.equal(/* HTML */ ` <span class="block">
+ expect(element).shadowDom.to.equal(/* HTML */ ` <span
+ class="block"
+ role="button"
+ tabindex="0"
+ >
<gr-submit-requirement-dashboard-hovercard>
</gr-submit-requirement-dashboard-hovercard>
<iron-icon class="block" icon="gr-icons:block" role="img"></iron-icon>
@@ -92,7 +96,11 @@
html`<gr-change-list-column-requirements-summary .change=${change}>
</gr-change-list-column-requirements-summary>`
);
- expect(element).shadowDom.to.equal(/* HTML */ ` <span class="block">
+ expect(element).shadowDom.to.equal(/* HTML */ ` <span
+ class="block"
+ role="button"
+ tabindex="0"
+ >
<gr-submit-requirement-dashboard-hovercard>
</gr-submit-requirement-dashboard-hovercard>
<iron-icon class="block" icon="gr-icons:block" role="img"></iron-icon>
diff --git a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
index d2c0060..604dc51 100644
--- a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
@@ -153,10 +153,10 @@
}
private renderDownloadCommands() {
- if (!this.schemes.length) return;
+ const cssClass = this.schemes.length ? '' : 'hidden';
return html`
- <section>
+ <section class=${cssClass}>
<gr-download-commands
id="downloadCommands"
.commands=${this.computeDownloadCommands()}
@@ -216,7 +216,7 @@
}
override willUpdate(changedProperties: PropertyValues) {
- if (changedProperties.has('schemes')) {
+ if (changedProperties.has('change') || changedProperties.has('patchNum')) {
this.schemesChanged();
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
index 8216f7b..46e600e 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements.ts
@@ -42,7 +42,7 @@
orderSubmitRequirements,
} from '../../../utils/label-util';
import {fontStyles} from '../../../styles/gr-font-styles';
-import {charsOnly} from '../../../utils/string-util';
+import {capitalizeFirstLetter, charsOnly} from '../../../utils/string-util';
import {subscribe} from '../../lit/subscription-controller';
import {CheckRun} from '../../../models/checks/checks-model';
import {getResultsOf, hasResultsOf} from '../../../models/checks/checks-util';
@@ -193,9 +193,8 @@
}
renderRequirement(requirement: SubmitRequirementResultInfo, index: number) {
- return html`
- <tr id="requirement-${index}-${charsOnly(requirement.name)}">
- <td>${this.renderStatus(requirement.status)}</td>
+ const row = html`
+ <td>${this.renderStatus(requirement.status)}</td>
<td class="name">
<gr-limited-text
class="name"
@@ -212,6 +211,22 @@
</td>
</tr>
`;
+
+ if (this.disableHovercards) {
+ // when hovercards are disabled, we don't make line focusable (tabindex)
+ // since otherwise there is no action associated with the line
+ return html`<tr>
+ ${row}
+ </tr>`;
+ } else {
+ return html`<tr
+ id="requirement-${index}-${charsOnly(requirement.name)}"
+ role="button"
+ tabindex="0"
+ >
+ ${row}
+ </tr>`;
+ }
}
renderEndpoint(
@@ -264,6 +279,12 @@
const checksChips = this.renderChecks(requirement);
+ const requirementWithoutLabelToVoteOn = associatedLabels.length === 0;
+ if (requirementWithoutLabelToVoteOn) {
+ const status = capitalizeFirstLetter(requirement.status.toLowerCase());
+ return checksChips || html`${status}`;
+ }
+
if (everyAssociatedLabelsIsWithoutVotes) {
return checksChips || html`No votes`;
}
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts
index 3e02afb..0487316 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirements/gr-submit-requirements_test.ts
@@ -34,6 +34,7 @@
suite('gr-submit-requirements tests', () => {
let element: GrSubmitRequirements;
+ let change: ParsedChangeInfo;
setup(async () => {
const submitRequirement: SubmitRequirementResultInfo = {
...createSubmitRequirementResultInfo(),
@@ -43,7 +44,7 @@
expression: 'label:Verified=MAX -label:Verified=MIN',
},
};
- const change: ParsedChangeInfo = {
+ change = {
...createParsedChange(),
submit_requirements: [
submitRequirement,
@@ -84,7 +85,7 @@
</tr>
</thead>
<tbody>
- <tr id="requirement-0-Verified">
+ <tr id="requirement-0-Verified" role="button" tabindex="0">
<td>
<iron-icon
aria-label="satisfied"
@@ -115,6 +116,48 @@
`);
});
+ suite('votes-cell', () => {
+ setup(async () => {
+ element.disableEndpoints = true;
+ await element.updateComplete;
+ });
+ test('with vote', () => {
+ const votesCell = element.shadowRoot?.querySelectorAll('.votes-cell');
+ expect(votesCell?.[0]).dom.equal(/* HTML */ `
+ <div class="votes-cell">
+ <gr-vote-chip> </gr-vote-chip>
+ </div>
+ `);
+ });
+
+ test('no votes', async () => {
+ const modifiedChange = {...change};
+ modifiedChange.labels = {
+ Verified: {
+ ...createDetailedLabelInfo(),
+ },
+ };
+ element.change = modifiedChange;
+ await element.updateComplete;
+ const votesCell = element.shadowRoot?.querySelectorAll('.votes-cell');
+ expect(votesCell?.[0]).dom.equal(/* HTML */ `
+ <div class="votes-cell">No votes</div>
+ `);
+ });
+
+ test('without label to vote on', async () => {
+ const modifiedChange = {...change};
+ modifiedChange.submit_requirements![0]!.submittability_expression_result!.expression =
+ 'hasfooter:"Release-Notes"';
+ element.change = modifiedChange;
+ await element.updateComplete;
+ const votesCell = element.shadowRoot?.querySelectorAll('.votes-cell');
+ expect(votesCell?.[0]).dom.equal(/* HTML */ `
+ <div class="votes-cell">Satisfied</div>
+ `);
+ });
+ });
+
test('calculateEndpointName()', () => {
assert.equal(
element.calculateEndpointName('code-owners~CodeOwnerSub'),
diff --git a/polygerrit-ui/app/elements/checks/gr-hovercard-run_test.ts b/polygerrit-ui/app/elements/checks/gr-hovercard-run_test.ts
index 352219a..e217a79 100644
--- a/polygerrit-ui/app/elements/checks/gr-hovercard-run_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-hovercard-run_test.ts
@@ -33,7 +33,7 @@
});
teardown(() => {
- element.hide(new MouseEvent('click'));
+ element.mouseClickHide(new MouseEvent('click'));
});
test('hovercard is shown', () => {
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
index b638bc1..48273ef 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
@@ -448,7 +448,7 @@
.then(() => {
this.dispatchEventThroughTarget('hide-alert');
});
- this.hide(e);
+ this.mouseClickHide(e);
}
private handleClickRemoveFromAttentionSet(e: MouseEvent) {
@@ -479,7 +479,7 @@
.then(() => {
this.dispatchEventThroughTarget('hide-alert');
});
- this.hide(e);
+ this.mouseClickHide(e);
}
private reportingDetails() {
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.ts
index 1d67500..7e99bb7 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account_test.ts
@@ -66,12 +66,12 @@
>
</gr-hovercard-account>`
);
- await element.show();
+ await element.show({});
await element.updateComplete;
});
teardown(async () => {
- await element.hide(new MouseEvent('click'));
+ await element.mouseClickHide(new MouseEvent('click'));
await element.updateComplete;
});
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-impl.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-impl.ts
index ff0c7f8..bcb2629 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-impl.ts
@@ -157,7 +157,6 @@
import {ErrorCallback} from '../../../api/rest';
import {addDraftProp, DraftInfo} from '../../../utils/comment-util';
import {BaseScheduler} from '../../../services/scheduler/scheduler';
-import {RetryScheduler} from '../../../services/scheduler/retry-scheduler';
import {MaxInFlightScheduler} from '../../../services/scheduler/max-in-flight-scheduler';
const MAX_PROJECT_RESULTS = 25;
@@ -283,19 +282,11 @@
}
function createReadScheduler() {
- return new RetryScheduler<Response>(
- new MaxInFlightScheduler<Response>(new BaseScheduler<Response>(), 10),
- 3,
- 50
- );
+ return new MaxInFlightScheduler<Response>(new BaseScheduler<Response>(), 10);
}
function createWriteScheduler() {
- return new RetryScheduler<Response>(
- new MaxInFlightScheduler<Response>(new BaseScheduler<Response>(), 5),
- 3,
- 50
- );
+ return new MaxInFlightScheduler<Response>(new BaseScheduler<Response>(), 5);
}
@customElement('gr-rest-api-service-impl')
diff --git a/polygerrit-ui/app/elements/shared/gr-vote-chip/gr-vote-chip.ts b/polygerrit-ui/app/elements/shared/gr-vote-chip/gr-vote-chip.ts
index 487145f..d89ed65 100644
--- a/polygerrit-ui/app/elements/shared/gr-vote-chip/gr-vote-chip.ts
+++ b/polygerrit-ui/app/elements/shared/gr-vote-chip/gr-vote-chip.ts
@@ -58,6 +58,9 @@
@property()
displayValue?: string;
+ @property({type: Boolean, attribute: 'tooltip-with-who-voted'})
+ tooltipWithWhoVoted = false;
+
private readonly flagsService = getAppContext().flagsService;
static override get styles() {
@@ -186,6 +189,13 @@
if (!this.label || !isDetailedLabelInfo(this.label)) {
return '';
}
- return this.label.values?.[valueString(this.vote?.value)] ?? '';
+ const voteDescription =
+ this.label.values?.[valueString(this.vote?.value)] ?? '';
+
+ if (this.tooltipWithWhoVoted && this.vote) {
+ return `${this.vote?.name}: ${voteDescription}`;
+ } else {
+ return voteDescription;
+ }
}
}
diff --git a/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin.ts b/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin.ts
index 0ae88dd..815266c 100644
--- a/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin.ts
+++ b/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin.ts
@@ -23,6 +23,8 @@
import {hovercardStyles} from '../../styles/gr-hovercard-styles';
import {sharedStyles} from '../../styles/shared-styles';
import {DependencyRequestEvent} from '../../models/dependency';
+import {addShortcut, Key} from '../../utils/dom-util';
+import {ShortcutController} from '../../elements/lit/shortcut-controller';
interface ReloadEventDetail {
clearPatchset?: boolean;
@@ -36,6 +38,12 @@
*/
const containerId = 'gr-hovercard-container';
+export interface MouseKeyboardOrFocusEvent {
+ keyboardEvent?: KeyboardEvent;
+ mouseEvent?: MouseEvent;
+ focusEvent?: FocusEvent;
+}
+
export function getHovercardContainer(
options: {createIfNotExists: boolean} = {createIfNotExists: false}
): HTMLElement | null {
@@ -127,6 +135,10 @@
isScheduledToHide?: boolean;
+ openedByKeyboard = false;
+
+ private targetCleanups: Array<() => void> = [];
+
static get styles() {
return [sharedStyles, hovercardStyles];
}
@@ -136,9 +148,13 @@
super(...args);
// show the hovercard if mouse moves to hovercard
// this will cancel pending hide as well
- this.addEventListener('mouseenter', this.show);
+ this.addEventListener('mouseenter', () => this.show);
// when leave hovercard, hide it immediately
- this.addEventListener('mouseleave', this.hide);
+ this.addEventListener('mouseleave', () => this.hide);
+ const keyboardController = new ShortcutController(this);
+ keyboardController.addGlobal({key: Key.ESC}, (e: KeyboardEvent) =>
+ this.hide({keyboardEvent: e})
+ );
}
override connectedCallback() {
@@ -165,20 +181,34 @@
// trigger the hovercard, which can annoying for the user, for example
// when added reviewer chips appear in the reply dialog via keyboard
// interaction.
- this._target?.addEventListener('mousemove', this.debounceShow);
- this._target?.addEventListener('focus', this.debounceShow);
- this._target?.addEventListener('mouseleave', this.debounceHide);
- this._target?.addEventListener('blur', this.debounceHide);
- this._target?.addEventListener('click', this.hide);
+ this._target?.addEventListener('mousemove', this.mouseDebounceShow);
+ this._target?.addEventListener('mouseleave', this.mouseDebounceHide);
+ this._target?.addEventListener('blur', this.focusDebounceHide);
+ this._target?.addEventListener('click', this.mouseClickHide);
+ if (this._target) {
+ this.targetCleanups.push(
+ addShortcut(this._target, {key: Key.ENTER}, (e: KeyboardEvent) => {
+ this.show({keyboardEvent: e});
+ })
+ );
+ this.targetCleanups.push(
+ addShortcut(this._target, {key: Key.SPACE}, (e: KeyboardEvent) => {
+ this.show({keyboardEvent: e});
+ })
+ );
+ }
this.addEventListener('request-dependency', this.resolveDep);
}
private removeTargetEventListeners() {
- this._target?.removeEventListener('mousemove', this.debounceShow);
- this._target?.removeEventListener('focus', this.debounceShow);
- this._target?.removeEventListener('mouseleave', this.debounceHide);
- this._target?.removeEventListener('blur', this.debounceHide);
- this._target?.removeEventListener('click', this.hide);
+ this._target?.removeEventListener('mousemove', this.mouseDebounceShow);
+ this._target?.removeEventListener('mouseleave', this.mouseDebounceHide);
+ this._target?.removeEventListener('blur', this.focusDebounceHide);
+ this._target?.removeEventListener('click', this.mouseClickHide);
+ for (const cleanup of this.targetCleanups) {
+ cleanup();
+ }
+ this.targetCleanups = [];
this.removeEventListener('request-dependency', this.resolveDep);
}
@@ -195,7 +225,19 @@
}
}
- readonly debounceHide = () => {
+ readonly mouseDebounceHide = (e: MouseEvent) => {
+ this.debounceHide({mouseEvent: e});
+ };
+
+ readonly mouseDebounceShow = (e: MouseEvent) => {
+ this.debounceShow({mouseEvent: e});
+ };
+
+ readonly focusDebounceHide = (e: FocusEvent) => {
+ this.debounceHide({focusEvent: e});
+ };
+
+ readonly debounceHide = (props: MouseKeyboardOrFocusEvent) => {
this.cancelShowTask();
if (!this._isShowing || this.isScheduledToHide) return;
this.isScheduledToHide = true;
@@ -205,7 +247,7 @@
// This happens when hide immediately through click or mouse leave
// on the hovercard
if (!this.isScheduledToHide) return;
- this.hide();
+ this.hide(props);
},
HIDE_DELAY_MS
);
@@ -277,29 +319,43 @@
);
};
+ mouseClickHide = (e: MouseEvent) => {
+ // If the user is clicking on a link and still hovering over the hovercard
+ // or the user is returning from the hovercard but now hovering over the
+ // target (to stop an annoying flicker effect), just return.
+ if (
+ e &&
+ (e.relatedTarget === this ||
+ (e.target === this && e.relatedTarget === this._target))
+ ) {
+ return;
+ }
+ // We allow hiding hovercards on clicks outside even if the keyboard was
+ // the reason that it was displayed.
+ this.hide({mouseEvent: e});
+ };
+
/**
* Hides/closes the hovercard. This occurs when the user triggers the
* `mouseleave` event on the hovercard's `target` element (as long as the
- * user is not hovering over the hovercard).
+ * user is not hovering over the hovercard). If event is not specified
+ * in props, code assumes mouseEvent
*/
- readonly hide = (e?: MouseEvent) => {
+ readonly hide = (props: MouseKeyboardOrFocusEvent) => {
this.cancelHideTask();
this.cancelShowTask();
if (!this._isShowing) {
return;
}
-
- // If the user is now hovering over the hovercard or the user is returning
- // from the hovercard but now hovering over the target (to stop an annoying
- // flicker effect), just return.
- if (e) {
- if (
- e.relatedTarget === this ||
- (e.target === this && e.relatedTarget === this._target)
- ) {
- return;
+ if (!props?.keyboardEvent && this.openedByKeyboard) return;
+ if (this.openedByKeyboard) {
+ if (this._target) {
+ this._target.focus();
}
}
+ // Make sure to reset the keyboard variable so new shows will not
+ // assume keyboard is the reason for opening the hovercard.
+ this.openedByKeyboard = false;
// Mark that the hovercard is not visible and do not allow focusing
this._isShowing = false;
@@ -321,14 +377,14 @@
/**
* Shows/opens the hovercard with a fixed delay.
*/
- readonly debounceShow = () => {
- this.debounceShowBy(SHOW_DELAY_MS);
+ readonly debounceShow = (props: MouseKeyboardOrFocusEvent) => {
+ this.debounceShowBy(SHOW_DELAY_MS, props);
};
/**
* Shows/opens the hovercard with the given delay.
*/
- debounceShowBy(delayMs: number) {
+ debounceShowBy(delayMs: number, props: MouseKeyboardOrFocusEvent) {
this.cancelHideTask();
if (this._isShowing || this.isScheduledToShow) return;
this.isScheduledToShow = true;
@@ -337,7 +393,7 @@
() => {
// This happens when the mouse leaves the target before the delay is over.
if (!this.isScheduledToShow) return;
- this.show();
+ this.show(props);
},
delayMs
);
@@ -352,11 +408,16 @@
/**
* Shows/opens the hovercard. This occurs when the user triggers the
- * `mousenter` event on the hovercard's `target` element.
+ * `mousenter` event on the hovercard's `target` element or when a user
+ * presses enter/space on the hovercard's `target` element. If event is not
+ * specified in props, code assumes mouseEvent
*/
- readonly show = async () => {
+ readonly show = async (props: MouseKeyboardOrFocusEvent) => {
this.cancelHideTask();
this.cancelShowTask();
+ // If we are calling show again because of a mouse reason, then keep
+ // the keyboard valuable set.
+ this.openedByKeyboard = this.openedByKeyboard || !!props?.keyboardEvent;
if (this._isShowing || !this.container) {
return;
}
@@ -379,6 +440,9 @@
});
this.updatePosition();
this.classList.remove(HIDE_CLASS);
+ if (props?.keyboardEvent) {
+ this.focus();
+ }
};
updatePosition() {
@@ -490,15 +554,16 @@
_target: HTMLElement | null;
_isShowing: boolean;
dispatchEventThroughTarget(eventName: string, detail?: unknown): void;
- show(): void;
+ show(props: MouseKeyboardOrFocusEvent): void;
// Used for tests
- hide(e: MouseEvent): void;
+ mouseClickHide(e: MouseEvent): void;
+ hide(props: MouseKeyboardOrFocusEvent): void;
container: HTMLElement | null;
hideTask?: DelayedTask;
showTask?: DelayedTask;
position: string;
- debounceShowBy(delayMs: number): void;
+ debounceShowBy(delayMs: number, props: MouseKeyboardOrFocusEvent): void;
updatePosition(): void;
isScheduledToShow?: boolean;
isScheduledToHide?: boolean;
diff --git a/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin_test.ts b/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin_test.ts
index e6b63e6..7bba8c5 100644
--- a/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin_test.ts
+++ b/polygerrit-ui/app/mixins/hovercard-mixin/hovercard-mixin_test.ts
@@ -19,7 +19,8 @@
import {HovercardMixin} from './hovercard-mixin.js';
import {html, LitElement} from 'lit';
import {customElement} from 'lit/decorators';
-import {MockPromise, mockPromise} from '../../test/test-utils.js';
+import {MockPromise, mockPromise, pressKey} from '../../test/test-utils.js';
+import {findActiveElement, Key} from '../../utils/dom-util.js';
const base = HovercardMixin(LitElement);
@@ -60,7 +61,8 @@
});
teardown(() => {
- element.hide(new MouseEvent('click'));
+ pressKey(element, Key.ESC);
+ element.mouseClickHide(new MouseEvent('click'));
button?.remove();
});
@@ -95,7 +97,7 @@
});
test('hide', () => {
- element.hide(new MouseEvent('click'));
+ element.mouseClickHide(new MouseEvent('click'));
const style = getComputedStyle(element);
assert.isFalse(element._isShowing);
assert.isFalse(element.classList.contains('hovered'));
@@ -104,7 +106,7 @@
});
test('show', async () => {
- await element.show();
+ await element.show({});
await element.updateComplete;
const style = getComputedStyle(element);
assert.isTrue(element._isShowing);
@@ -114,14 +116,14 @@
});
test('debounceShow does not show immediately', async () => {
- element.debounceShowBy(100);
+ element.debounceShowBy(100, {});
setTimeout(() => testPromise.resolve(), 0);
await testPromise;
assert.isFalse(element._isShowing);
});
test('debounceShow shows after delay', async () => {
- element.debounceShowBy(1);
+ element.debounceShowBy(1, {});
setTimeout(() => testPromise.resolve(), 10);
await testPromise;
assert.isTrue(element._isShowing);
@@ -174,4 +176,52 @@
assert.isFalse(element.isScheduledToShow);
assert.isFalse(element._isShowing);
});
+
+ test('do not show on focus', async () => {
+ const button = document.querySelector('button');
+ button?.focus();
+ await element.updateComplete;
+ assert.isNotTrue(element.isScheduledToShow);
+ assert.isFalse(element._isShowing);
+ });
+
+ test('show on pressing enter when focused', async () => {
+ const button = document.querySelector('button')!;
+ button.focus();
+ await element.updateComplete;
+ pressKey(button, Key.ENTER);
+ await element.updateComplete;
+ assert.isTrue(element._isShowing);
+ });
+
+ test('show on pressing space when focused', async () => {
+ const button = document.querySelector('button')!;
+ button.focus();
+ await element.updateComplete;
+ pressKey(button, Key.SPACE);
+ await element.updateComplete;
+ assert.isTrue(element._isShowing);
+ });
+
+ test('when on pressing enter, focus is moved to hovercard', async () => {
+ const button = document.querySelector('button')!;
+ button.focus();
+ await element.updateComplete;
+ await element.show({keyboardEvent: new KeyboardEvent('enter')});
+ await element.updateComplete;
+ assert.isTrue(element._isShowing);
+ const activeElement = findActiveElement(document);
+ assert.isTrue(activeElement === element);
+ });
+
+ test('when on mouseEvent, focus is not moved to hovercard', async () => {
+ const button = document.querySelector('button')!;
+ button.focus();
+ await element.updateComplete;
+ await element.show({mouseEvent: new MouseEvent('enter')});
+ await element.updateComplete;
+ assert.isTrue(element._isShowing);
+ const activeElement = findActiveElement(document);
+ assert.isTrue(activeElement !== element);
+ });
});
diff --git a/polygerrit-ui/app/rules.bzl b/polygerrit-ui/app/rules.bzl
index 01bb31f..f69ede4 100644
--- a/polygerrit-ui/app/rules.bzl
+++ b/polygerrit-ui/app/rules.bzl
@@ -26,6 +26,7 @@
config_file = ":rollup.config.js",
entry_point = entry_point,
rollup_bin = "//tools/node_tools:rollup-bin",
+ silent = True,
sourcemap = "hidden",
deps = [
"@tools_npm//rollup-plugin-node-resolve",
diff --git a/polygerrit-ui/app/utils/string-util.ts b/polygerrit-ui/app/utils/string-util.ts
index 0b217ec..0a0928e 100644
--- a/polygerrit-ui/app/utils/string-util.ts
+++ b/polygerrit-ui/app/utils/string-util.ts
@@ -47,3 +47,7 @@
export function convertToString(key?: unknown) {
return key !== undefined ? String(key) : '';
}
+
+export function capitalizeFirstLetter(str: string) {
+ return str.charAt(0).toUpperCase() + str.slice(1);
+}
diff --git a/tools/bzl/js.bzl b/tools/bzl/js.bzl
index c8d6e4b..8025dab 100644
--- a/tools/bzl/js.bzl
+++ b/tools/bzl/js.bzl
@@ -110,6 +110,7 @@
entry_point = entry_point,
format = "iife",
rollup_bin = "//tools/node_tools:rollup-bin",
+ silent = True,
sourcemap = "hidden",
config_file = "//plugins:rollup.config.js",
deps = [
diff --git a/tools/deps.bzl b/tools/deps.bzl
index 62049e7..d243957 100644
--- a/tools/deps.bzl
+++ b/tools/deps.bzl
@@ -14,7 +14,7 @@
AUTO_VALUE_GSON_VERSION = "1.3.1"
PROLOG_VERS = "1.4.4"
PROLOG_REPO = GERRIT
-GITILES_VERS = "0.4-1"
+GITILES_VERS = "1.0.0"
GITILES_REPO = GERRIT
# When updating Bouncy Castle, also update it in bazlets.
@@ -155,12 +155,6 @@
)
maven_jar(
- name = "commons-lang",
- artifact = "commons-lang:commons-lang:2.6",
- sha1 = "0ce1edb914c94ebc388f086c6827e8bdeec71ac2",
- )
-
- maven_jar(
name = "commons-lang3",
artifact = "org.apache.commons:commons-lang3:3.8.1",
sha1 = "6505a72a097d9270f7a9e7bf42c4238283247755",
@@ -544,14 +538,14 @@
artifact = "com.google.gitiles:blame-cache:" + GITILES_VERS,
attach_source = False,
repository = GITILES_REPO,
- sha1 = "0df80c6b8822147e1f116fd7804b8a0de544f402",
+ sha1 = "f46833f8aa6f33ce3e443c8a414c295559eaf43e",
)
maven_jar(
name = "gitiles-servlet",
artifact = "com.google.gitiles:gitiles-servlet:" + GITILES_VERS,
repository = GITILES_REPO,
- sha1 = "60870897d22b840e65623fd024eabd9cc9706ebe",
+ sha1 = "90e107da00c2cd32490dd9ae8e3fb1ee095ea675",
)
# prettify must match the version used in Gitiles
diff --git a/tools/node_tools/node_modules_licenses/BUILD b/tools/node_tools/node_modules_licenses/BUILD
index b88ec24..32b4250 100644
--- a/tools/node_tools/node_modules_licenses/BUILD
+++ b/tools/node_tools/node_modules_licenses/BUILD
@@ -27,6 +27,7 @@
entry_point = "license-map-generator.ts",
format = "cjs",
rollup_bin = "//tools/node_tools:rollup-bin",
+ silent = True,
deps = [
":licenses-map",
"@tools_npm//rollup",
diff --git a/tools/node_tools/polygerrit_app_preprocessor/BUILD b/tools/node_tools/polygerrit_app_preprocessor/BUILD
index fa3ce56..ed7ef71 100644
--- a/tools/node_tools/polygerrit_app_preprocessor/BUILD
+++ b/tools/node_tools/polygerrit_app_preprocessor/BUILD
@@ -21,6 +21,7 @@
entry_point = "preprocessor.ts",
format = "cjs",
rollup_bin = "//tools/node_tools:rollup-bin",
+ silent = True,
deps = [
":preprocessor",
"@tools_npm//rollup-plugin-node-resolve",
@@ -33,6 +34,7 @@
entry_point = "links-updater.ts",
format = "cjs",
rollup_bin = "//tools/node_tools:rollup-bin",
+ silent = True,
deps = [
":preprocessor",
"@tools_npm//rollup-plugin-node-resolve",
