Merge "Throttle star requests."
diff --git a/.gitignore b/.gitignore
index 53bc9f6..0bbcaba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,6 +31,8 @@
/infer-out
/local.properties
/node_modules/
+/polygerrit-ui/node_modules/
+/polygerrit-ui/app/node_modules/
/package-lock.json
/plugins/*
/polygerrit-ui/coverage/
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 9a40b27..f2f5a66 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -800,22 +800,6 @@
Users without this access right who are able to upload changes can
still do the revert locally and upload the revert commit as a new change.
-[[category_remove_label]]
-=== Remove Label (Remove Vote)
-
-For every configured label `My-Name` in the project, there is a
-corresponding permission `removeLabel-My-Name` with a range corresponding to
-the defined values. For these values, the users are permitted to remove
-other users' votes from a change.
-
-Change owners can always remove zero or positive votes (even without
-having the `Remove Vote` access right assigned).
-
-Project owners and site administrators can always remove any vote (even
-without having the `Remove Vote` access right assigned).
-
-Users without this access right can still remove their own votes.
-
[[category_remove_reviewer]]
=== Remove Reviewer
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 2f144c6..359361c 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -824,26 +824,6 @@
"+2"
]
},
- "removable_labels": {
- "Code-Review": {
- "-1": [
- {
- "_account_id": 1000096,
- "name": "John Doe",
- "email": "john.doe@example.com",
- "username": "jdoe"
- }
- ],
- "+1": [
- {
- "_account_id": 1000097,
- "name": "Jane Roe",
- "email": "jane.roe@example.com",
- "username": "jroe"
- }
- ]
- }
- },
"removable_reviewers": [
{
"_account_id": 1000096,
@@ -7091,13 +7071,6 @@
A map of the permitted labels that maps a label name to the list of
values that are allowed for that label. +
Only set if link:#detailed-labels[detailed labels] are requested.
-|`removable_labels` |optional|
-A map of the removable labels that maps a label name to the map of
-values and reviewers (
-link:rest-api-accounts.html#account-info[AccountInfo] entities)
-that are allowed to be removed from the change. +
-Only set if link:#labels[labels] or
-link:#detailed-labels[detailed labels] are requested.
|`removable_reviewers`|optional|
The reviewers that can be removed by the calling user as a list of
link:rest-api-accounts.html#account-info[AccountInfo] entities. +
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index ccf74d1..5b4a9e5 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1393,17 +1393,6 @@
}
}
- protected void assertOnlyRemovableLabel(
- ChangeInfo info, String labelId, String labelValue, TestAccount reviewer) {
- assertThat(info.removableLabels).hasSize(1);
- assertThat(info.removableLabels).containsKey(labelId);
- assertThat(info.removableLabels.get(labelId)).hasSize(1);
- assertThat(info.removableLabels.get(labelId)).containsKey(labelValue);
- assertThat(info.removableLabels.get(labelId).get(labelValue)).hasSize(1);
- assertThat(info.removableLabels.get(labelId).get(labelValue).get(0).email)
- .isEqualTo(reviewer.email());
- }
-
protected void assertPermissions(
Project.NameKey project,
GroupReference groupReference,
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
index 69139ce..b1cd506 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -197,13 +197,8 @@
PermissionRule.Builder rule = newRule(projectConfig, p.group());
rule.setAction(p.action());
rule.setRange(p.min(), p.max());
- String permissionName;
- if (p.isAddPermission()) {
- permissionName =
- p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
- } else {
- permissionName = Permission.forRemoveLabel(p.name());
- }
+ String permissionName =
+ p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
projectConfig.upsertAccessSection(
p.ref(), as -> as.upsertPermission(permissionName).add(rule));
}
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
index 5634c78..9a9a21a 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
@@ -162,34 +162,12 @@
/** Starts a builder for allowing a label permission. */
public static TestLabelPermission.Builder allowLabel(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(true)
- .action(PermissionRule.Action.ALLOW);
+ return TestLabelPermission.builder().name(name).action(PermissionRule.Action.ALLOW);
}
/** Starts a builder for denying a label permission. */
public static TestLabelPermission.Builder blockLabel(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(true)
- .action(PermissionRule.Action.BLOCK);
- }
-
- /** Starts a builder for allowing a remove-label permission. */
- public static TestLabelPermission.Builder allowLabelRemoval(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(false)
- .action(PermissionRule.Action.ALLOW);
- }
-
- /** Starts a builder for denying a remove-label permission. */
- public static TestLabelPermission.Builder blockLabelRemoval(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(false)
- .action(PermissionRule.Action.BLOCK);
+ return TestLabelPermission.builder().name(name).action(PermissionRule.Action.BLOCK);
}
/** Records a label permission to be updated. */
@@ -213,8 +191,6 @@
abstract boolean impersonation();
- abstract boolean isAddPermission();
-
/** Builder for {@link TestLabelPermission}. */
@AutoValue.Builder
public abstract static class Builder {
@@ -232,8 +208,6 @@
abstract Builder max(int max);
- abstract Builder isAddPermission(boolean isAddPermission);
-
/** Sets the minimum and maximum values for the permission. */
public Builder range(int min, int max) {
checkArgument(min != 0 || max != 0, "empty range");
@@ -269,12 +243,6 @@
return TestPermissionKey.builder().name(Permission.forLabel(name));
}
- /** Starts a builder for describing a label removal permission key for deletion. */
- public static TestPermissionKey.Builder labelRemovalPermissionKey(String name) {
- checkLabelName(name);
- return TestPermissionKey.builder().name(Permission.forRemoveLabel(name));
- }
-
/** Starts a builder for describing a capability key for deletion. */
public static TestPermissionKey.Builder capabilityKey(String name) {
return TestPermissionKey.builder().name(name).section(GLOBAL_CAPABILITIES);
diff --git a/java/com/google/gerrit/entities/Permission.java b/java/com/google/gerrit/entities/Permission.java
index d029fad..6d2fa32 100644
--- a/java/com/google/gerrit/entities/Permission.java
+++ b/java/com/google/gerrit/entities/Permission.java
@@ -43,7 +43,6 @@
public static final String FORGE_SERVER = "forgeServerAsCommitter";
public static final String LABEL = "label-";
public static final String LABEL_AS = "labelAs-";
- public static final String REMOVE_LABEL = "removeLabel-";
public static final String OWNER = "owner";
public static final String PUSH = "push";
public static final String PUSH_MERGE = "pushMerge";
@@ -61,7 +60,6 @@
private static final List<String> NAMES_LC;
private static final int LABEL_INDEX;
private static final int LABEL_AS_INDEX;
- private static final int REMOVE_LABEL_INDEX;
static {
NAMES_LC = new ArrayList<>();
@@ -81,7 +79,6 @@
NAMES_LC.add(FORGE_SERVER.toLowerCase());
NAMES_LC.add(LABEL.toLowerCase());
NAMES_LC.add(LABEL_AS.toLowerCase());
- NAMES_LC.add(REMOVE_LABEL.toLowerCase());
NAMES_LC.add(OWNER.toLowerCase());
NAMES_LC.add(PUSH.toLowerCase());
NAMES_LC.add(PUSH_MERGE.toLowerCase());
@@ -96,19 +93,15 @@
LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL);
LABEL_AS_INDEX = NAMES_LC.indexOf(Permission.LABEL_AS.toLowerCase());
- REMOVE_LABEL_INDEX = NAMES_LC.indexOf(Permission.REMOVE_LABEL.toLowerCase());
}
/** Returns true if the name is recognized as a permission name. */
public static boolean isPermission(String varName) {
- return isLabel(varName)
- || isLabelAs(varName)
- || isRemoveLabel(varName)
- || NAMES_LC.contains(varName.toLowerCase());
+ return isLabel(varName) || isLabelAs(varName) || NAMES_LC.contains(varName.toLowerCase());
}
public static boolean hasRange(String varName) {
- return isLabel(varName) || isLabelAs(varName) || isRemoveLabel(varName);
+ return isLabel(varName) || isLabelAs(varName);
}
/** Returns true if the permission name is actually for a review label. */
@@ -121,11 +114,6 @@
return var.startsWith(LABEL_AS) && LABEL_AS.length() < var.length();
}
- /** Returns true if the permission is for impersonated review labels. */
- public static boolean isRemoveLabel(String var) {
- return var.startsWith(REMOVE_LABEL) && REMOVE_LABEL.length() < var.length();
- }
-
/** Returns permission name for the given review label. */
public static String forLabel(String labelName) {
return LABEL + labelName;
@@ -136,19 +124,12 @@
return LABEL_AS + labelName;
}
- /** Returns permission name to remove a label for another user. */
- public static String forRemoveLabel(String labelName) {
- return REMOVE_LABEL + labelName;
- }
-
@Nullable
public static String extractLabel(String varName) {
if (isLabel(varName)) {
return varName.substring(LABEL.length());
} else if (isLabelAs(varName)) {
return varName.substring(LABEL_AS.length());
- } else if (isRemoveLabel(varName)) {
- return varName.substring(REMOVE_LABEL.length());
}
return null;
}
@@ -224,8 +205,6 @@
return LABEL_INDEX;
} else if (isLabelAs(a.getName())) {
return LABEL_AS_INDEX;
- } else if (isRemoveLabel(a.getName())) {
- return REMOVE_LABEL_INDEX;
}
int index = NAMES_LC.indexOf(a.getName().toLowerCase());
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index a865187..40ae2ec 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -105,7 +105,6 @@
public Map<String, ActionInfo> actions;
public Map<String, LabelInfo> labels;
public Map<String, Collection<String>> permittedLabels;
- public Map<String, Map<String, List<AccountInfo>>> removableLabels;
public Collection<AccountInfo> removableReviewers;
public Map<ReviewerState, Collection<AccountInfo>> reviewers;
public Map<ReviewerState, Collection<AccountInfo>> pendingReviewers;
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
index 51c35dc..24182cc 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
@@ -150,17 +150,16 @@
/** Returns {@code null} if nothing has been added to {@code oldCollection} */
@Nullable
private static ImmutableList<?> getAddedForCollection(
- @Nullable Collection<?> oldCollection, Collection<?> newCollection) {
- ImmutableList<?> notInOldCollection = getAdditionsForCollection(oldCollection, newCollection);
+ Collection<?> oldCollection, Collection<?> newCollection) {
+ ImmutableList<?> notInOldCollection = getAdditions(oldCollection, newCollection);
return notInOldCollection.isEmpty() ? null : notInOldCollection;
}
@Nullable
- private static ImmutableList<Object> getAdditionsForCollection(
- @Nullable Collection<?> oldCollection, Collection<?> newCollection) {
- if (oldCollection == null) {
- return ImmutableList.copyOf(newCollection);
- }
+ private static ImmutableList<Object> getAdditions(
+ Collection<?> oldCollection, Collection<?> newCollection) {
+ if (oldCollection == null)
+ return newCollection != null ? ImmutableList.copyOf(newCollection) : null;
Map<Object, List<Object>> duplicatesMap = newCollection.stream().collect(groupingBy(v -> v));
oldCollection.forEach(
@@ -174,18 +173,7 @@
/** Returns {@code null} if nothing has been added to {@code oldMap} */
@Nullable
- private static ImmutableMap<Object, Object> getAddedForMap(
- @Nullable Map<?, ?> oldMap, Map<?, ?> newMap) {
- ImmutableMap<Object, Object> notInOldMap = getAdditionsForMap(oldMap, newMap);
- return notInOldMap.isEmpty() ? null : notInOldMap;
- }
-
- @Nullable
- private static ImmutableMap<Object, Object> getAdditionsForMap(
- @Nullable Map<?, ?> oldMap, Map<?, ?> newMap) {
- if (oldMap == null) {
- return ImmutableMap.copyOf(newMap);
- }
+ private static ImmutableMap<Object, Object> getAddedForMap(Map<?, ?> oldMap, Map<?, ?> newMap) {
ImmutableMap.Builder<Object, Object> additionsBuilder = ImmutableMap.builder();
for (Map.Entry<?, ?> entry : newMap.entrySet()) {
Object added = getAdded(oldMap.get(entry.getKey()), entry.getValue());
@@ -193,7 +181,8 @@
additionsBuilder.put(entry.getKey(), added);
}
}
- return additionsBuilder.build();
+ ImmutableMap<Object, Object> additions = additionsBuilder.build();
+ return additions.isEmpty() ? null : additions;
}
private static Object get(Field field, Object obj) {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index d26af7b..500bb77 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -688,7 +688,6 @@
!cd.change().isAbandoned()
? labelsJson.permittedLabels(user.getAccountId(), cd)
: ImmutableMap.of();
- out.removableLabels = labelsJson.removableLabels(accountLoader, user, cd);
}
}
diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java
index 06e41ff..cfa15ae 100644
--- a/java/com/google/gerrit/server/change/LabelsJson.java
+++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -36,18 +36,15 @@
import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.SubmitRecord;
-import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.VotingRangeInfo;
import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.DeleteVoteControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -72,12 +69,10 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final PermissionBackend permissionBackend;
- private final DeleteVoteControl deleteVoteControl;
@Inject
- LabelsJson(PermissionBackend permissionBackend, DeleteVoteControl deleteVoteControl) {
+ LabelsJson(PermissionBackend permissionBackend) {
this.permissionBackend = permissionBackend;
- this.deleteVoteControl = deleteVoteControl;
}
/**
@@ -138,45 +133,6 @@
return permitted.asMap();
}
- /**
- * Returns A map of all labels that the provided user has permission to remove.
- *
- * @param accountLoader to load the reviewers' data with.
- * @param user a Gerrit user.
- * @param cd {@link ChangeData} corresponding to a specific gerrit change.
- * @return A Map of {@code labelName} -> {Map of {@code value} -> List of {@link AccountInfo}}
- * that the user can remove votes from.
- */
- Map<String, Map<String, List<AccountInfo>>> removableLabels(
- AccountLoader accountLoader, CurrentUser user, ChangeData cd)
- throws PermissionBackendException {
- if (cd.change().isMerged()) {
- return new HashMap<>();
- }
-
- Map<String, Map<String, List<AccountInfo>>> res = new HashMap<>();
- LabelTypes labelTypes = cd.getLabelTypes();
- for (PatchSetApproval approval : cd.currentApprovals()) {
- Optional<LabelType> labelType = labelTypes.byLabel(approval.labelId());
- if (!labelType.isPresent()) {
- continue;
- }
- if (!deleteVoteControl.testDeleteVotePermissions(
- user, cd.notes(), approval, labelType.get())) {
- continue;
- }
- if (!res.containsKey(approval.label())) {
- res.put(approval.label(), new HashMap<>());
- }
- String labelValue = LabelValue.formatValue(approval.value());
- if (!res.get(approval.label()).containsKey(labelValue)) {
- res.get(approval.label()).put(labelValue, new ArrayList<>());
- }
- res.get(approval.label()).get(labelValue).add(accountLoader.get(approval.accountId()));
- }
- return res;
- }
-
private static void clearOnlyZerosEntries(SetMultimap<String, String> permitted) {
List<String> toClear = Lists.newArrayListWithCapacity(permitted.keySet().size());
for (Map.Entry<String, Collection<String>> e : permitted.asMap().entrySet()) {
@@ -261,10 +217,10 @@
}
}
- private Map<String, Short> currentLabels(@Nullable Account.Id accountId, ChangeData cd) {
+ private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd) {
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa : cd.currentApprovals()) {
- if (accountId == null || psa.accountId().equals(accountId)) {
+ if (psa.accountId().equals(accountId)) {
result.put(psa.label(), psa.value());
}
}
diff --git a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java b/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
deleted file mode 100644
index 622f0cf..0000000
--- a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
+++ /dev/null
@@ -1,155 +0,0 @@
-// Copyright (C) 2022 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.permissions;
-
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
-import static java.util.Objects.requireNonNull;
-
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.server.util.LabelVote;
-
-/** Abstract permission representing a label. */
-public abstract class AbstractLabelPermission implements ChangePermissionOrLabel {
- public enum ForUser {
- SELF,
- ON_BEHALF_OF
- }
-
- protected final ForUser forUser;
- protected final String name;
-
- /**
- * Construct a reference to an abstract label permission.
- *
- * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
- * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
- */
- public AbstractLabelPermission(ForUser forUser, String name) {
- this.forUser = requireNonNull(forUser, "ForUser");
- this.name = LabelType.checkName(name);
- }
-
- /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
- public ForUser forUser() {
- return forUser;
- }
-
- /** Returns name of the label, e.g. {@code "Code-Review"}. */
- public String label() {
- return name;
- }
-
- protected abstract String permissionPrefix();
-
- protected String permissionName() {
- if (forUser == ON_BEHALF_OF) {
- return permissionPrefix() + "As";
- }
- return permissionPrefix();
- }
-
- @Override
- public final String describeForException() {
- if (forUser == ON_BEHALF_OF) {
- return permissionPrefix() + " on behalf of " + name;
- }
- return permissionPrefix() + " " + name;
- }
-
- @Override
- public final int hashCode() {
- return (permissionPrefix() + name).hashCode();
- }
-
- @Override
- @SuppressWarnings("EqualsGetClass")
- public final boolean equals(Object other) {
- if (this.getClass().isAssignableFrom(other.getClass())) {
- AbstractLabelPermission b = (AbstractLabelPermission) other;
- return forUser == b.forUser && name.equals(b.name);
- }
- return false;
- }
-
- @Override
- public final String toString() {
- return permissionName() + "[" + name + ']';
- }
-
- /** A {@link AbstractLabelPermission} at a specific value. */
- public abstract static class WithValue implements ChangePermissionOrLabel {
- private final ForUser forUser;
- private final LabelVote label;
-
- /**
- * Construct a reference to an abstract label permission at a specific value.
- *
- * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
- * @param label label name and vote.
- */
- public WithValue(ForUser forUser, LabelVote label) {
- this.forUser = requireNonNull(forUser, "ForUser");
- this.label = requireNonNull(label, "LabelVote");
- }
-
- /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
- public ForUser forUser() {
- return forUser;
- }
-
- /** Returns name of the label, e.g. {@code "Code-Review"}. */
- public String label() {
- return label.label();
- }
-
- /** Returns specific value of the label, e.g. 1 or 2. */
- public short value() {
- return label.value();
- }
-
- public abstract String permissionName();
-
- @Override
- public final String describeForException() {
- if (forUser == ON_BEHALF_OF) {
- return permissionName() + " on behalf of " + label.formatWithEquals();
- }
- return permissionName() + " " + label.formatWithEquals();
- }
-
- @Override
- public final int hashCode() {
- return (permissionName() + label).hashCode();
- }
-
- @Override
- @SuppressWarnings("EqualsGetClass")
- public final boolean equals(Object other) {
- if (this.getClass().isAssignableFrom(other.getClass())) {
- AbstractLabelPermission.WithValue b = (AbstractLabelPermission.WithValue) other;
- return forUser == b.forUser && label.equals(b.label);
- }
- return false;
- }
-
- @Override
- public final String toString() {
- if (forUser == ON_BEHALF_OF) {
- return permissionName() + "As[" + label.format() + ']';
- }
- return permissionName() + "[" + label.format() + ']';
- }
- }
-}
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 6f7d761..8d432c8 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -14,8 +14,8 @@
package com.google.gerrit.server.permissions;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.labelPermissionName;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@@ -240,10 +240,10 @@
private boolean can(ChangePermissionOrLabel perm) throws PermissionBackendException {
if (perm instanceof ChangePermission) {
return can((ChangePermission) perm);
- } else if (perm instanceof AbstractLabelPermission) {
- return can((AbstractLabelPermission) perm);
- } else if (perm instanceof AbstractLabelPermission.WithValue) {
- return can((AbstractLabelPermission.WithValue) perm);
+ } else if (perm instanceof LabelPermission) {
+ return can((LabelPermission) perm);
+ } else if (perm instanceof LabelPermission.WithValue) {
+ return can((LabelPermission.WithValue) perm);
}
throw new PermissionBackendException(perm + " unsupported");
}
@@ -288,11 +288,11 @@
throw new PermissionBackendException(perm + " unsupported");
}
- private boolean can(AbstractLabelPermission perm) {
+ private boolean can(LabelPermission perm) {
return !label(labelPermissionName(perm)).isEmpty();
}
- private boolean can(AbstractLabelPermission.WithValue perm) {
+ private boolean can(LabelPermission.WithValue perm) {
PermissionRange r = label(labelPermissionName(perm));
if (perm.forUser() == ON_BEHALF_OF && r.isEmpty()) {
return false;
diff --git a/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java b/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
index f59ba02..2824efd 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
@@ -16,5 +16,5 @@
import com.google.gerrit.extensions.api.access.GerritPermission;
-/** A {@link ChangePermission} or a {@link AbstractLabelPermission}. */
+/** A {@link ChangePermission} or a {@link LabelPermission}. */
public interface ChangePermissionOrLabel extends GerritPermission {}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
index 89f0493..9d69d9b 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -24,7 +24,7 @@
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.extensions.api.access.PluginProjectPermission;
-import com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser;
+import com.google.gerrit.server.permissions.LabelPermission.ForUser;
import java.util.EnumSet;
import java.util.Optional;
import java.util.Set;
@@ -160,29 +160,19 @@
return Optional.ofNullable(CHANGE_PERMISSIONS.inverse().get(permissionName));
}
- public static String labelPermissionName(AbstractLabelPermission labelPermission) {
- if (labelPermission instanceof LabelPermission) {
- if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
- return Permission.forLabelAs(labelPermission.label());
- }
- return Permission.forLabel(labelPermission.label());
- } else if (labelPermission instanceof LabelRemovalPermission) {
- return Permission.forRemoveLabel(labelPermission.label());
+ public static String labelPermissionName(LabelPermission labelPermission) {
+ if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
+ return Permission.forLabelAs(labelPermission.label());
}
- throw new IllegalStateException("invalid AbstractLabelPermission subtype");
+ return Permission.forLabel(labelPermission.label());
}
// TODO(dborowitz): Can these share a common superinterface?
- public static String labelPermissionName(AbstractLabelPermission.WithValue labelPermission) {
- if (labelPermission instanceof LabelPermission.WithValue) {
- if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
- return Permission.forLabelAs(labelPermission.label());
- }
- return Permission.forLabel(labelPermission.label());
- } else if (labelPermission instanceof LabelRemovalPermission.WithValue) {
- return Permission.forRemoveLabel(labelPermission.label());
+ public static String labelPermissionName(LabelPermission.WithValue labelPermission) {
+ if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
+ return Permission.forLabelAs(labelPermission.label());
}
- throw new IllegalStateException("invalid AbstractLabelPermission.WithValue subtype");
+ return Permission.forLabel(labelPermission.label());
}
private DefaultPermissionMappings() {}
diff --git a/java/com/google/gerrit/server/permissions/LabelPermission.java b/java/com/google/gerrit/server/permissions/LabelPermission.java
index 4652364..c266caa 100644
--- a/java/com/google/gerrit/server/permissions/LabelPermission.java
+++ b/java/com/google/gerrit/server/permissions/LabelPermission.java
@@ -14,14 +14,24 @@
package com.google.gerrit.server.permissions;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.SELF;
+import static java.util.Objects.requireNonNull;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.server.util.LabelVote;
/** Permission representing a label. */
-public class LabelPermission extends AbstractLabelPermission {
+public class LabelPermission implements ChangePermissionOrLabel {
+ public enum ForUser {
+ SELF,
+ ON_BEHALF_OF;
+ }
+
+ private final ForUser forUser;
+ private final String name;
+
/**
* Construct a reference to a label permission.
*
@@ -57,16 +67,55 @@
* @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
*/
public LabelPermission(ForUser forUser, String name) {
- super(forUser, name);
+ this.forUser = requireNonNull(forUser, "ForUser");
+ this.name = LabelType.checkName(name);
+ }
+
+ /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
+ public ForUser forUser() {
+ return forUser;
+ }
+
+ /** Returns name of the label, e.g. {@code "Code-Review"}. */
+ public String label() {
+ return name;
}
@Override
- public String permissionPrefix() {
- return "label";
+ public String describeForException() {
+ if (forUser == ON_BEHALF_OF) {
+ return "label on behalf of " + name;
+ }
+ return "label " + name;
+ }
+
+ @Override
+ public int hashCode() {
+ return name.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (other instanceof LabelPermission) {
+ LabelPermission b = (LabelPermission) other;
+ return forUser == b.forUser && name.equals(b.name);
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ if (forUser == ON_BEHALF_OF) {
+ return "LabelAs[" + name + ']';
+ }
+ return "Label[" + name + ']';
}
/** A {@link LabelPermission} at a specific value. */
- public static class WithValue extends AbstractLabelPermission.WithValue {
+ public static class WithValue implements ChangePermissionOrLabel {
+ private final ForUser forUser;
+ private final LabelVote label;
+
/**
* Construct a reference to a label at a specific value.
*
@@ -146,12 +195,53 @@
* @param label label name and vote.
*/
public WithValue(ForUser forUser, LabelVote label) {
- super(forUser, label);
+ this.forUser = requireNonNull(forUser, "ForUser");
+ this.label = requireNonNull(label, "LabelVote");
+ }
+
+ /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
+ public ForUser forUser() {
+ return forUser;
+ }
+
+ /** Returns name of the label, e.g. {@code "Code-Review"}. */
+ public String label() {
+ return label.label();
+ }
+
+ /** Returns specific value of the label, e.g. 1 or 2. */
+ public short value() {
+ return label.value();
}
@Override
- public String permissionName() {
- return "label";
+ public String describeForException() {
+ if (forUser == ON_BEHALF_OF) {
+ return "label on behalf of " + label.formatWithEquals();
+ }
+ return "label " + label.formatWithEquals();
+ }
+
+ @Override
+ public int hashCode() {
+ return label.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (other instanceof WithValue) {
+ WithValue b = (WithValue) other;
+ return forUser == b.forUser && label.equals(b.label);
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ if (forUser == ON_BEHALF_OF) {
+ return "LabelAs[" + label.format() + ']';
+ }
+ return "Label[" + label.format() + ']';
}
}
}
diff --git a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
deleted file mode 100644
index 2553601..0000000
--- a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
+++ /dev/null
@@ -1,94 +0,0 @@
-// Copyright (C) 2022 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.permissions;
-
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF;
-
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.LabelValue;
-import com.google.gerrit.server.util.LabelVote;
-
-/** Permission representing a label removal. */
-public class LabelRemovalPermission extends AbstractLabelPermission {
- /**
- * Construct a reference to a label removal permission.
- *
- * @param type type description of the label.
- */
- public LabelRemovalPermission(LabelType type) {
- this(type.getName());
- }
-
- /**
- * Construct a reference to a label removal permission.
- *
- * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
- */
- public LabelRemovalPermission(String name) {
- super(SELF, name);
- }
-
- @Override
- public String permissionPrefix() {
- return "removeLabel";
- }
-
- /** A {@link LabelRemovalPermission} at a specific value. */
- public static class WithValue extends AbstractLabelPermission.WithValue {
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param type description of the label.
- * @param value numeric score assigned to the label.
- */
- public WithValue(LabelType type, LabelValue value) {
- this(type.getName(), value.getValue());
- }
-
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param type description of the label.
- * @param value numeric score assigned to the label.
- */
- public WithValue(LabelType type, short value) {
- this(type.getName(), value);
- }
-
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
- * @param value numeric score assigned to the label.
- */
- public WithValue(String name, short value) {
- this(LabelVote.create(name, value));
- }
-
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param label label name and vote.
- */
- public WithValue(LabelVote label) {
- super(SELF, label);
- }
-
- @Override
- public String permissionName() {
- return "removeLabel";
- }
- }
-}
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java
index eb5e053..fea2827 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -474,18 +474,6 @@
}
/**
- * Test which values of a label the user may be able to remove.
- *
- * @param label definition of the label to test values of.
- * @return set containing values the user may be able to use; may be empty if none.
- * @throws PermissionBackendException if failure consulting backend configuration.
- */
- public Set<LabelRemovalPermission.WithValue> testRemoval(LabelType label)
- throws PermissionBackendException {
- return test(removalValuesOf(requireNonNull(label, "LabelType")));
- }
-
- /**
* Test which values of a group of labels the user may be able to set.
*
* @param types definition of the labels to test values of.
@@ -498,29 +486,10 @@
return test(types.stream().flatMap(t -> valuesOf(t).stream()).collect(toSet()));
}
- /**
- * Test which values of a group of labels the user may be able to remove.
- *
- * @param types definition of the labels to test values of.
- * @return set containing values the user may be able to use; may be empty if none.
- * @throws PermissionBackendException if failure consulting backend configuration.
- */
- public Set<LabelRemovalPermission.WithValue> testLabelRemovals(Collection<LabelType> types)
- throws PermissionBackendException {
- requireNonNull(types, "LabelType");
- return test(types.stream().flatMap(t -> removalValuesOf(t).stream()).collect(toSet()));
- }
-
private static Set<LabelPermission.WithValue> valuesOf(LabelType label) {
return label.getValues().stream()
.map(v -> new LabelPermission.WithValue(label, v))
.collect(toSet());
}
-
- private static Set<LabelRemovalPermission.WithValue> removalValuesOf(LabelType label) {
- return label.getValues().stream()
- .map(v -> new LabelRemovalPermission.WithValue(label, v))
- .collect(toSet());
- }
}
}
diff --git a/java/com/google/gerrit/server/project/DeleteVoteControl.java b/java/com/google/gerrit/server/project/DeleteVoteControl.java
deleted file mode 100644
index 93c0451..0000000
--- a/java/com/google/gerrit/server/project/DeleteVoteControl.java
+++ /dev/null
@@ -1,83 +0,0 @@
-// Copyright (C) 2022 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.project;
-
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.PatchSetApproval;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.LabelRemovalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.RefPermission;
-import com.google.inject.Inject;
-import java.util.Set;
-
-public class DeleteVoteControl {
- private final PermissionBackend permissionBackend;
-
- @Inject
- public DeleteVoteControl(PermissionBackend permissionBackend) {
- this.permissionBackend = permissionBackend;
- }
-
- public void checkDeleteVotePermissions(
- CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
- throws AuthException, PermissionBackendException {
- if (testDeleteVotePermissions(user, notes, approval, labelType)) {
- return;
- }
- throw new AuthException(
- new LabelRemovalPermission.WithValue(labelType, approval.value()).describeForException()
- + " not permitted");
- }
-
- public boolean testDeleteVotePermissions(
- CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
- throws PermissionBackendException {
- if (canRemoveReviewerWithoutRemoveLabelPermission(
- notes.getChange(), user, approval.accountId(), approval.value())) {
- return true;
- }
- // Test if the user is allowed to remove vote of the given label type and value.
- Set<LabelRemovalPermission.WithValue> allowed =
- permissionBackend.user(user).change(notes).testRemoval(labelType);
- return allowed.contains(new LabelRemovalPermission.WithValue(labelType, approval.value()));
- }
-
- private boolean canRemoveReviewerWithoutRemoveLabelPermission(
- Change change, CurrentUser user, Account.Id reviewer, int value)
- throws PermissionBackendException {
- if (user.isIdentifiedUser()) {
- Account.Id aId = user.getAccountId();
- if (aId.equals(reviewer)) {
- return true; // A user can always remove their own votes.
- } else if (aId.equals(change.getOwner()) && 0 <= value) {
- return true; // The change owner may remove any zero or positive score.
- }
- }
-
- // Users with the remove reviewer permission, the branch owner, project
- // owner and site admin can remove anyone
- PermissionBackend.WithUser withUser = permissionBackend.user(user);
- PermissionBackend.ForProject forProject = withUser.project(change.getProject());
- return forProject.ref(change.getDest().branch()).test(RefPermission.WRITE_CONFIG)
- || withUser.test(GlobalPermission.ADMINISTRATE_SERVER);
- }
-}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
index 239b485..a0fc121 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -38,8 +38,8 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.DeleteVoteControl;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
@@ -75,7 +75,7 @@
private final VoteDeleted voteDeleted;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
- private final DeleteVoteControl deleteVoteControl;
+ private final RemoveReviewerControl removeReviewerControl;
private final MessageIdGenerator messageIdGenerator;
private final String label;
@@ -96,7 +96,7 @@
ChangeMessagesUtil cmUtil,
VoteDeleted voteDeleted,
DeleteVoteSender.Factory deleteVoteSenderFactory,
- DeleteVoteControl deleteVoteControl,
+ RemoveReviewerControl removeReviewerControl,
MessageIdGenerator messageIdGenerator,
@Assisted Project.NameKey projectName,
@Assisted AccountState reviewerToDeleteVoteFor,
@@ -109,7 +109,7 @@
this.cmUtil = cmUtil;
this.voteDeleted = voteDeleted;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
- this.deleteVoteControl = deleteVoteControl;
+ this.removeReviewerControl = removeReviewerControl;
this.messageIdGenerator = messageIdGenerator;
this.projectName = projectName;
@@ -143,8 +143,12 @@
newApprovals.put(a.label(), a.value());
continue;
} else if (enforcePermissions) {
- deleteVoteControl.checkDeleteVotePermissions(
- ctx.getUser(), ctx.getNotes(), a, labelTypes.byLabel(a.labelId()).get());
+ // For regular users, check if they are allowed to remove the vote.
+ try {
+ removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+ } catch (AuthException e) {
+ throw new AuthException("delete vote not permitted", e);
+ }
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.label(), (short) 0);
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 22eb32c..259e71d 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -17,7 +17,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.groupingBy;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 92d19bb..211baeb 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -26,10 +26,8 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.entities.RefNames.changeMetaRef;
@@ -2268,16 +2266,6 @@
@Test
public void deleteVote() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2321,16 +2309,6 @@
@Test
public void deleteVoteNotifyNone() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2348,16 +2326,6 @@
@Test
public void deleteVoteWithReason() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2379,16 +2347,6 @@
@Test
public void deleteVoteNotifyAccount() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2448,66 +2406,7 @@
.id(r.getChangeId())
.reviewer(admin.id().toString())
.deleteVote(LabelId.CODE_REVIEW));
- assertThat(thrown).hasMessageThat().contains("removeLabel Code-Review=+2 not permitted");
- }
-
- @Test
- public void deleteVoteAlwaysPermittedForSelfVotes() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabel(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .add(
- blockLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
- PushOneCommit.Result r = createChange();
- String changeId = r.getChangeId();
-
- requestScopeOperations.setApiUser(user.id());
- gApi.changes().id(changeId).revision(r.getCommit().name()).review(ReviewInput.approve());
-
- gApi.changes()
- .id(r.getChangeId())
- .reviewer(user.id().toString())
- .deleteVote(LabelId.CODE_REVIEW);
- }
-
- @Test
- public void deleteVoteAlwaysPermittedForAdmin() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabel(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .add(
- blockLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
- PushOneCommit.Result r = createChange();
- String changeId = r.getChangeId();
-
- requestScopeOperations.setApiUser(user.id());
- gApi.changes().id(changeId).revision(r.getCommit().name()).review(ReviewInput.approve());
-
- requestScopeOperations.setApiUser(admin.id());
- gApi.changes()
- .id(r.getChangeId())
- .reviewer(user.id().toString())
- .deleteVote(LabelId.CODE_REVIEW);
+ assertThat(thrown).hasMessageThat().contains("delete vote not permitted");
}
@Test
@@ -3376,7 +3275,6 @@
assertThat(change.status).isEqualTo(ChangeStatus.NEW);
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
- assertThat(change.removableLabels).isEmpty();
// add new label and assert that it's returned for existing changes
AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
@@ -3391,7 +3289,6 @@
.project(project)
.forUpdate()
.add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
- .add(allowLabelRemoval(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
.update();
change = gApi.changes().id(r.getChangeId()).get();
@@ -3407,9 +3304,6 @@
.id(r.getChangeId())
.revision(r.getCommit().name())
.review(new ReviewInput().label(verified.getName(), verified.getMax().getValue()));
- change = gApi.changes().id(r.getChangeId()).get();
- assertPermitted(change, LabelId.VERIFIED, -1, 0, 1);
- assertOnlyRemovableLabel(change, LabelId.VERIFIED, "+1", admin);
try (ProjectConfigUpdate u = updateProject(project)) {
// remove label and assert that it's no longer returned for existing
@@ -3429,7 +3323,6 @@
change = gApi.changes().id(r.getChangeId()).get();
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
- assertThat(change.removableLabels).isEmpty();
// abandon the change and see that the returned labels stay the same
// while all permitted labels disappear.
@@ -3438,7 +3331,6 @@
assertThat(change.status).isEqualTo(ChangeStatus.ABANDONED);
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels).isEmpty();
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3555,7 +3447,6 @@
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW, LabelId.VERIFIED);
assertPermitted(change, LabelId.CODE_REVIEW, 2);
assertPermitted(change, LabelId.VERIFIED, 1);
- assertThat(change.removableLabels).isEmpty();
// remove label and assert that it's no longer returned for existing
// changes, even if there is an approval for it
@@ -3573,7 +3464,6 @@
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertPermitted(change, LabelId.CODE_REVIEW, 2);
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3670,7 +3560,6 @@
.containsExactly(LabelId.CODE_REVIEW, "Non-Author-Code-Review");
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertPermitted(change, LabelId.CODE_REVIEW, 0, 1, 2);
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3686,7 +3575,6 @@
assertThat(change.submissionId).isNotNull();
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertPermitted(change, LabelId.CODE_REVIEW, 0, 1, 2);
- assertThat(change.removableLabels).isEmpty();
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index e707949..0291f33 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -22,7 +22,6 @@
import static com.google.gerrit.acceptance.PushOneCommit.PATCH_FILE_ONLY;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
@@ -235,15 +234,6 @@
revision(r).review(ReviewInput.recommend());
requestScopeOperations.setApiUser(admin.id());
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
gApi.changes().id(changeId).reviewer(user.username()).deleteVote(LabelId.CODE_REVIEW);
Optional<ApprovalInfo> crUser =
get(changeId, DETAILED_LABELS).labels.get(LabelId.CODE_REVIEW).all.stream()
@@ -2011,15 +2001,6 @@
recommend(r.getChangeId());
requestScopeOperations.setApiUser(admin.id());
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
gApi.changes()
.id(r.getChangeId())
.current()
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 7e8ff62..ea52690 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.extensions.restapi.testing.AttentionSetUpdateSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -1945,16 +1944,6 @@
@Test
public void deleteVotesDoesNotAffectAttentionSetWhenIgnoreAutomaticRulesIsSet() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
@@ -1979,16 +1968,6 @@
@Test
public void deleteVotesOfOthersAddThemToAttentionSet() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
index c29b265..c57d285 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -15,16 +15,13 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
-import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
-import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.LabelId;
@@ -39,26 +36,11 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import org.junit.Before;
import org.junit.Test;
public class DeleteVoteIT extends AbstractDaemonTest {
- @Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
- @Before
- public void allowVoteDeletion() {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
- }
-
@Test
public void deleteVoteOnChange() throws Exception {
deleteVote(false);
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index b1277c0..b68afc5 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ABANDONED_CHANGES;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ALL_COMMENTS;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.NEW_CHANGES;
@@ -99,11 +98,6 @@
.add(allow(Permission.SUBMIT_AS).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.ABANDON).ref("refs/*").group(REGISTERED_USERS))
.add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, +2))
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/*")
- .group(REGISTERED_USERS)
- .range(-2, +2))
.update();
}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
index 661802e..c1a7627 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -18,14 +18,11 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.capabilityKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.deny;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelRemovalPermissionKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.common.data.GlobalCapability.ADMINISTRATE_SERVER;
import static com.google.gerrit.common.data.GlobalCapability.DEFAULT_MAX_QUERY_LIMIT;
@@ -446,73 +443,6 @@
}
@Test
- public void addAllowLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .update();
-
- Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "-1..+2 group global:Registered-Users");
- }
-
- @Test
- public void addBlockLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(blockLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .update();
-
- Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "block -1..+2 group global:Registered-Users");
- }
-
- @Test
- public void addAllowExclusiveLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .setExclusiveGroup(labelRemovalPermissionKey("Code-Review").ref("refs/foo"), true)
- .update();
-
- Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly(
- "removeLabel-Code-Review", "-1..+2 group global:Registered-Users",
- "exclusiveGroupPermissions", "removeLabel-Code-Review");
-
- projectOperations
- .project(key)
- .forUpdate()
- .setExclusiveGroup(labelRemovalPermissionKey("Code-Review").ref("refs/foo"), false)
- .update();
-
- config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "-1..+2 group global:Registered-Users");
- }
-
- @Test
public void addAllowCapability() throws Exception {
Config config = projectOperations.project(allProjects).getConfig();
assertThat(config)
@@ -610,31 +540,6 @@
}
@Test
- public void removeLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(PROJECT_OWNERS).range(-2, 1))
- .update();
- assertThat(projectOperations.project(key).getConfig())
- .subsectionValues("access", "refs/foo")
- .containsExactly(
- "removeLabel-Code-Review", "-1..+2 group global:Registered-Users",
- "removeLabel-Code-Review", "-2..+1 group global:Project-Owners");
-
- projectOperations
- .project(key)
- .forUpdate()
- .remove(labelRemovalPermissionKey("Code-Review").ref("refs/foo").group(REGISTERED_USERS))
- .update();
- assertThat(projectOperations.project(key).getConfig())
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "-2..+1 group global:Project-Owners");
- }
-
- @Test
public void removeCapability() throws Exception {
projectOperations
.allProjectsForUpdate()
diff --git a/javatests/com/google/gerrit/entities/PermissionTest.java b/javatests/com/google/gerrit/entities/PermissionTest.java
index d25d833..3175671 100644
--- a/javatests/com/google/gerrit/entities/PermissionTest.java
+++ b/javatests/com/google/gerrit/entities/PermissionTest.java
@@ -36,7 +36,6 @@
assertThat(Permission.isPermission(Permission.LABEL + LabelId.CODE_REVIEW)).isTrue();
assertThat(Permission.isPermission(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isTrue();
- assertThat(Permission.isPermission(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isTrue();
assertThat(Permission.isPermission(LabelId.CODE_REVIEW)).isFalse();
}
@@ -57,7 +56,6 @@
assertThat(Permission.isLabel(Permission.LABEL + LabelId.CODE_REVIEW)).isTrue();
assertThat(Permission.isLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isFalse();
- assertThat(Permission.isLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isFalse();
assertThat(Permission.isLabel(LabelId.CODE_REVIEW)).isFalse();
}
@@ -68,22 +66,10 @@
assertThat(Permission.isLabelAs(Permission.LABEL + LabelId.CODE_REVIEW)).isFalse();
assertThat(Permission.isLabelAs(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isTrue();
- assertThat(Permission.isLabelAs(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isFalse();
assertThat(Permission.isLabelAs(LabelId.CODE_REVIEW)).isFalse();
}
@Test
- public void isRemoveLabel() {
- assertThat(Permission.isRemoveLabel(Permission.ABANDON)).isFalse();
- assertThat(Permission.isRemoveLabel("no-permission")).isFalse();
-
- assertThat(Permission.isRemoveLabel(Permission.LABEL + LabelId.CODE_REVIEW)).isFalse();
- assertThat(Permission.isRemoveLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isFalse();
- assertThat(Permission.isRemoveLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isTrue();
- assertThat(Permission.isRemoveLabel(LabelId.CODE_REVIEW)).isFalse();
- }
-
- @Test
public void forLabel() {
assertThat(Permission.forLabel(LabelId.CODE_REVIEW))
.isEqualTo(Permission.LABEL + LabelId.CODE_REVIEW);
@@ -96,19 +82,11 @@
}
@Test
- public void forRemoveLabel() {
- assertThat(Permission.forRemoveLabel(LabelId.CODE_REVIEW))
- .isEqualTo(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW);
- }
-
- @Test
public void extractLabel() {
assertThat(Permission.extractLabel(Permission.LABEL + LabelId.CODE_REVIEW))
.isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.extractLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isEqualTo(LabelId.CODE_REVIEW);
- assertThat(Permission.extractLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.extractLabel(LabelId.CODE_REVIEW)).isNull();
assertThat(Permission.extractLabel(Permission.ABANDON)).isNull();
}
@@ -125,10 +103,6 @@
Permission.canBeOnAllProjects(
AccessSection.ALL, Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isTrue();
- assertThat(
- Permission.canBeOnAllProjects(
- AccessSection.ALL, Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isTrue();
assertThat(Permission.canBeOnAllProjects("refs/heads/*", Permission.ABANDON)).isTrue();
assertThat(Permission.canBeOnAllProjects("refs/heads/*", Permission.OWNER)).isTrue();
@@ -139,10 +113,6 @@
Permission.canBeOnAllProjects(
"refs/heads/*", Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isTrue();
- assertThat(
- Permission.canBeOnAllProjects(
- "refs/heads/*", Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isTrue();
}
@Test
@@ -156,8 +126,6 @@
.isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.create(Permission.LABEL_AS + LabelId.CODE_REVIEW).getLabel())
.isEqualTo(LabelId.CODE_REVIEW);
- assertThat(Permission.create(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW).getLabel())
- .isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.create(LabelId.CODE_REVIEW).getLabel()).isNull();
assertThat(Permission.create(Permission.ABANDON).getLabel()).isNull();
}
diff --git a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
index f45d33b..7ed236a 100644
--- a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
+++ b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
@@ -48,7 +48,6 @@
assertThat(diff.added().messages).isNull();
assertThat(diff.added().reviewers).isNull();
assertThat(diff.added().hashtags).isNull();
- assertThat(diff.added().removableLabels).isNull();
assertThat(diff.removed()._number).isNull();
assertThat(diff.removed().branch).isNull();
assertThat(diff.removed().project).isNull();
@@ -57,7 +56,6 @@
assertThat(diff.removed().messages).isNull();
assertThat(diff.removed().reviewers).isNull();
assertThat(diff.removed().hashtags).isNull();
- assertThat(diff.removed().removableLabels).isNull();
}
@Test
@@ -317,295 +315,6 @@
}
@Test
- public void getDiff_removableLabelsEmpty_returnsNullRemovableLabels() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- oldChangeInfo.removableLabels = ImmutableMap.of();
- newChangeInfo.removableLabels = ImmutableMap.of();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsNullAndEmpty_returnsEmptyRemovableLabels() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- newChangeInfo.removableLabels = ImmutableMap.of();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isEmpty();
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsEmptyAndNull_returnsEmptyRemovableLabels() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- oldChangeInfo.removableLabels = ImmutableMap.of();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels).isEmpty();
- }
-
- @Test
- public void getDiff_removableLabelsLabelAdded() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "Cow";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "Pig";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "Cat";
- AccountInfo acc4 = new AccountInfo();
- acc4.name = "Dog";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
- newChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)),
- "Verified",
- ImmutableMap.of("-1", ImmutableList.of(acc4)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc4))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsLabelRemoved() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "Cow";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "Pig";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "Cat";
- AccountInfo acc4 = new AccountInfo();
- acc4.name = "Dog";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)),
- "Verified",
- ImmutableMap.of("-1", ImmutableList.of(acc4)));
- newChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc4))));
- }
-
- @Test
- public void getDiff_removableLabelsVoteAdded() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "acc3";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc2, acc3))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsVoteRemoved() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "acc3";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc2, acc3))));
- }
-
- @Test
- public void getDiff_removableLabelsAccountAdded() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1, acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsAccountRemoved() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1, acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsAccountChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
- public void getDiff_removableLabelsScoreChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc1)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc1))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
- public void getDiff_removableLabelsLabelChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Verified", ImmutableMap.of("+1", ImmutableList.of(acc1)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
- public void getDiff_removableLabelsLabelScoreAndAccountChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
public void getDiff_assertCanConstructAllChangeInfoReferences() throws Exception {
buildObjectWithFullFields(ChangeInfo.class);
}
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list.ts b/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list.ts
index 27fe620..d3562f2 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list.ts
@@ -16,7 +16,11 @@
import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, query, property, state} from 'lit/decorators.js';
import {assertIsDefined} from '../../../utils/common-util';
-import {AdminViewState} from '../../../models/views/admin';
+import {
+ AdminChildView,
+ AdminViewState,
+ createAdminUrl,
+} from '../../../models/views/admin';
import {createGroupUrl} from '../../../models/views/group';
import {whenVisible} from '../../../utils/dom-util';
import {modalStyles} from '../../../styles/gr-modal-styles';
@@ -29,8 +33,6 @@
@customElement('gr-admin-group-list')
export class GrAdminGroupList extends LitElement {
- readonly path = '/admin/groups';
-
@query('#createModal') private createModal?: HTMLDialogElement;
@query('#createNewModal') private createNewModal?: GrCreateGroupDialog;
@@ -87,7 +89,7 @@
.itemsPerPage=${this.groupsPerPage}
.loading=${this.loading}
.offset=${this.offset}
- .path=${this.path}
+ .path=${createAdminUrl({adminView: AdminChildView.GROUPS})}
@create-clicked=${() => this.handleCreateClicked()}
>
<table id="list" class="genericList">
@@ -167,18 +169,14 @@
*
* private but used in test
*/
- maybeOpenCreateModal(params?: AdminViewState) {
+ async maybeOpenCreateModal(params?: AdminViewState) {
if (params?.openCreateModal) {
- assertIsDefined(this.createModal, 'createModal');
- this.createModal.showModal();
+ await this.updateComplete;
+ if (!this.createModal?.open) this.createModal?.showModal();
}
}
- /**
- * Generates groups link (/admin/groups/<uuid>)
- *
- * private but used in test
- */
+ // private but used in test
computeGroupUrl(encodedId: string) {
const groupId = decodeURIComponent(encodedId) as GroupId;
return createGroupUrl({groupId});
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list_test.ts b/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list_test.ts
index 1b128aa..e9b7ea0 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-admin-group-list/gr-admin-group-list_test.ts
@@ -120,17 +120,17 @@
assert.equal(element.groups.slice(0, SHOWN_ITEMS_COUNT).length, 25);
});
- test('maybeOpenCreateModal', () => {
+ test('maybeOpenCreateModal', async () => {
const modalOpen = sinon.stub(
queryAndAssert<HTMLDialogElement>(element, '#createModal'),
'showModal'
);
- element.maybeOpenCreateModal();
+ await element.maybeOpenCreateModal();
assert.isFalse(modalOpen.called);
- element.maybeOpenCreateModal(undefined);
+ await element.maybeOpenCreateModal(undefined);
assert.isFalse(modalOpen.called);
value.openCreateModal = true;
- element.maybeOpenCreateModal(value);
+ await element.maybeOpenCreateModal(value);
assert.isTrue(modalOpen.called);
});
});
diff --git a/polygerrit-ui/app/elements/admin/gr-create-group-dialog/gr-create-group-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-group-dialog/gr-create-group-dialog.ts
index 4808d00..8d5689c 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-group-dialog/gr-create-group-dialog.ts
+++ b/polygerrit-ui/app/elements/admin/gr-create-group-dialog/gr-create-group-dialog.ts
@@ -6,9 +6,8 @@
import '@polymer/iron-input/iron-input';
import '../../../styles/gr-form-styles';
import '../../../styles/shared-styles';
-import {encodeURL, getBaseUrl} from '../../../utils/url-util';
import {page} from '../../../utils/page-wrapper-utils';
-import {GroupName} from '../../../types/common';
+import {GroupId, GroupName} from '../../../types/common';
import {getAppContext} from '../../../services/app-context';
import {formStyles} from '../../../styles/gr-form-styles';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -16,6 +15,7 @@
import {customElement, query, property} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
import {fireEvent} from '../../../utils/event-util';
+import {createGroupUrl} from '../../../models/views/group';
declare global {
interface HTMLElementTagNameMap {
@@ -75,10 +75,6 @@
fireEvent(this, 'has-new-group-name');
}
- private computeGroupUrl(groupId: string) {
- return getBaseUrl() + '/admin/groups/' + encodeURL(groupId, true);
- }
-
override focus() {
this.input.focus();
}
@@ -89,7 +85,9 @@
if (groupRegistered.status !== 201) return;
return this.restApiService.getGroupConfig(name).then(group => {
if (!group) return;
- page.show(this.computeGroupUrl(String(group.group_id!)));
+ const groupId = String(group.group_id!) as GroupId;
+ // TODO: Use navigation service instead of `page.show()` directly.
+ page.show(createGroupUrl({groupId}));
});
});
}
diff --git a/polygerrit-ui/app/elements/admin/gr-create-repo-dialog/gr-create-repo-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-repo-dialog/gr-create-repo-dialog.ts
index a90abbd..bb59ccc 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-repo-dialog/gr-create-repo-dialog.ts
+++ b/polygerrit-ui/app/elements/admin/gr-create-repo-dialog/gr-create-repo-dialog.ts
@@ -7,7 +7,6 @@
import '../../shared/gr-autocomplete/gr-autocomplete';
import '../../shared/gr-button/gr-button';
import '../../shared/gr-select/gr-select';
-import {encodeURL, getBaseUrl} from '../../../utils/url-util';
import {page} from '../../../utils/page-wrapper-utils';
import {
BranchName,
@@ -24,6 +23,7 @@
import {customElement, query, property, state} from 'lit/decorators.js';
import {fireEvent} from '../../../utils/event-util';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {createRepoUrl} from '../../../models/views/repo';
declare global {
interface HTMLElementTagNameMap {
@@ -183,10 +183,6 @@
`;
}
- _computeRepoUrl(repoName: string) {
- return getBaseUrl() + '/admin/repos/' + encodeURL(repoName, true);
- }
-
override focus() {
this.input?.focus();
}
@@ -199,7 +195,8 @@
);
if (repoRegistered.status === 201) {
this.repoCreated = true;
- page.show(this._computeRepoUrl(this.repoConfig.name));
+ // TODO: Use navigation service instead of `page.show()` directly.
+ page.show(createRepoUrl({repo: this.repoConfig.name}));
}
return repoRegistered;
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.ts b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.ts
index 389e7c4..52e0b3f 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import '../gr-access-section/gr-access-section';
-import {encodeURL, getBaseUrl, singleDecodeURL} from '../../../utils/url-util';
+import {singleDecodeURL} from '../../../utils/url-util';
import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {toSortedPermissionsArray} from '../../../utils/access-util';
import {
@@ -44,6 +44,7 @@
import {resolve} from '../../../models/dependency';
import {createChangeUrl} from '../../../models/views/change';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {createRepoUrl, RepoDetailView} from '../../../models/views/repo';
const NOTHING_TO_SAVE = 'No changes to save.';
@@ -726,10 +727,10 @@
computeParentHref() {
if (!this.inheritsFrom?.name) return '';
- return `${getBaseUrl()}/admin/repos/${encodeURL(
- this.inheritsFrom.name,
- true
- )},access`;
+ return createRepoUrl({
+ repo: this.inheritsFrom.name,
+ detail: RepoDetailView.ACCESS,
+ });
}
private handleEditInheritFromTextChanged(e: ValueChangedEvent) {
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
index 3b193a6..7eef7a4 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
@@ -442,6 +442,9 @@
}
private getPath(repo?: RepoName, detailType?: RepoDetailView) {
+ // TODO: Replace with `createRepoUrl()`, but be aware that `encodeURL()`
+ // gets `false` as a second parameter here. The router pattern in gr-router
+ // does not handle the filter URLs, if the repo is not encoded!
return `/admin/repos/${encodeURL(repo ?? '', false)},${detailType}`;
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list_test.ts b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list_test.ts
index 28fc751..ec1bb82 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list_test.ts
@@ -2434,6 +2434,18 @@
});
suite('create new', () => {
+ setup(async () => {
+ stubRestApi('getRepoBranches').resolves(createBranchesList(3));
+
+ element.params = {
+ view: GerritView.REPO,
+ repo: 'test' as RepoName,
+ detail: RepoDetailView.BRANCHES,
+ };
+ await element.paramsChanged();
+ await element.updateComplete;
+ });
+
test('handleCreateClicked called when create-click fired', () => {
const handleCreateClickedStub = sinon.stub(
element,
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts b/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts
index c6c6efd..2fd7e79 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts
@@ -6,23 +6,23 @@
import '../../shared/gr-dialog/gr-dialog';
import '../../shared/gr-list-view/gr-list-view';
import '../gr-create-repo-dialog/gr-create-repo-dialog';
-import {
- RepoName,
- ProjectInfoWithName,
- WebLinkInfo,
-} from '../../../types/common';
+import {ProjectInfoWithName, WebLinkInfo} from '../../../types/common';
import {GrCreateRepoDialog} from '../gr-create-repo-dialog/gr-create-repo-dialog';
import {RepoState, SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {fireTitleChange} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
-import {encodeURL, getBaseUrl} from '../../../utils/url-util';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, property, query, state} from 'lit/decorators.js';
-import {AdminViewState} from '../../../models/views/admin';
+import {
+ AdminChildView,
+ AdminViewState,
+ createAdminUrl,
+} from '../../../models/views/admin';
import {createSearchUrl} from '../../../models/views/search';
import {modalStyles} from '../../../styles/gr-modal-styles';
+import {createRepoUrl} from '../../../models/views/repo';
declare global {
interface HTMLElementTagNameMap {
@@ -32,8 +32,6 @@
@customElement('gr-repo-list')
export class GrRepoList extends LitElement {
- readonly path = '/admin/repos';
-
@query('#createModal') private createModal?: HTMLDialogElement;
@query('#createNewModal') private createNewModal?: GrCreateRepoDialog;
@@ -103,7 +101,7 @@
.items=${this.repos}
.loading=${this.loading}
.offset=${this.offset}
- .path=${this.path}
+ .path=${createAdminUrl({adminView: AdminChildView.REPOS})}
@create-clicked=${() => this.handleCreateClicked()}
>
<table id="list" class="genericList">
@@ -157,11 +155,11 @@
return html`
<tr class="table">
<td class="name">
- <a href=${this.computeRepoUrl(item.name)}>${item.name}</a>
+ <a href=${createRepoUrl({repo: item.name})}>${item.name}</a>
</td>
<td class="repositoryBrowser">${this.renderWebLinks(item)}</td>
<td class="changesLink">
- <a href=${this.computeChangesLink(item.name)}>view all</a>
+ <a href=${createSearchUrl({repo: item.name})}>view all</a>
</td>
<td class="readOnly">
${item.state === RepoState.READ_ONLY ? 'Y' : ''}
@@ -210,14 +208,6 @@
}
}
- private computeRepoUrl(name: string) {
- return `${getBaseUrl()}${this.path}/${encodeURL(name, true)}`;
- }
-
- private computeChangesLink(name: string) {
- return createSearchUrl({repo: name as RepoName});
- }
-
private async getCreateRepoCapability() {
const account = await this.restApiService.getAccount();
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
index 7abfce9..faaee0b 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts
@@ -282,12 +282,14 @@
// private but used in test
handleNextPage() {
if (!this.nextArrow || !this.changesPerPage) return;
+ // TODO: Use navigation service instead of `page.show()` directly.
page.show(this.computeNavLink(1));
}
// private but used in test
handlePreviousPage() {
if (!this.prevArrow || !this.changesPerPage) return;
+ // TODO: Use navigation service instead of `page.show()` directly.
page.show(this.computeNavLink(-1));
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 3fff1d4..bcf6937 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -60,6 +60,7 @@
RepoViewState,
} from '../../../models/views/repo';
import {
+ createGroupUrl,
GroupDetailView,
GroupViewModel,
GroupViewState,
@@ -1077,7 +1078,8 @@
}
handleGroupInfoRoute(ctx: PageContext) {
- this.redirect('/admin/groups/' + encodeURIComponent(ctx.params[0]));
+ const groupId = ctx.params[0] as GroupId;
+ this.redirect(createGroupUrl({groupId}));
}
handleGroupSelfRedirectRoute(_: PageContext) {
@@ -1161,6 +1163,8 @@
}
}
+ // TODO: Change the route pattern to match `repo` and `detailView`
+ // separately, and then use `createRepoUrl()` here.
this.redirect(`/admin/repos/${params}`);
}
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index e0933e8..eceb05e 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -73,6 +73,7 @@
import {createDashboardUrl} from '../models/views/dashboard';
import {userModelToken} from '../models/user/user-model';
import {modalStyles} from '../styles/gr-modal-styles';
+import {AdminChildView, createAdminUrl} from '../models/views/admin';
import {ChangeChildView, changeViewModelToken} from '../models/views/change';
interface ErrorInfo {
@@ -215,6 +216,16 @@
createSearchUrl({query: 'is:watched is:open'})
)
);
+ this.shortcuts.addAbstract(Shortcut.GO_TO_REPOS, () =>
+ this.getNavigation().setUrl(
+ createAdminUrl({adminView: AdminChildView.REPOS})
+ )
+ );
+ this.shortcuts.addAbstract(Shortcut.GO_TO_GROUPS, () =>
+ this.getNavigation().setUrl(
+ createAdminUrl({adminView: AdminChildView.GROUPS})
+ )
+ );
subscribe(
this,
diff --git a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
index fd43869..449c041 100644
--- a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
+++ b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
@@ -177,9 +177,11 @@
() => {
if (!this.isConnected || !this.path) return;
if (filter) {
+ // TODO: Use navigation service instead of `page.show()` directly.
page.show(`${this.path}/q/filter:${encodeURL(filter, false)}`);
return;
}
+ // TODO: Use navigation service instead of `page.show()` directly.
page.show(this.path);
},
REQUEST_DEBOUNCE_INTERVAL_MS
diff --git a/polygerrit-ui/app/models/views/admin.ts b/polygerrit-ui/app/models/views/admin.ts
index 2ad95a2..3380637 100644
--- a/polygerrit-ui/app/models/views/admin.ts
+++ b/polygerrit-ui/app/models/views/admin.ts
@@ -4,6 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {GerritView} from '../../services/router/router-model';
+import {getBaseUrl} from '../../utils/url-util';
import {define} from '../dependency';
import {Model} from '../model';
import {ViewState} from './base';
@@ -21,6 +22,17 @@
offset?: number | string;
}
+export function createAdminUrl(state: Omit<AdminViewState, 'view'>) {
+ switch (state.adminView) {
+ case AdminChildView.REPOS:
+ return `${getBaseUrl()}/admin/repos`;
+ case AdminChildView.GROUPS:
+ return `${getBaseUrl()}/admin/groups`;
+ case AdminChildView.PLUGINS:
+ return `${getBaseUrl()}/admin/plugins`;
+ }
+}
+
export const adminViewModelToken = define<AdminViewModel>('admin-view-model');
export class AdminViewModel extends Model<AdminViewState | undefined> {
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
index da61c41..9ca2213 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
@@ -35,6 +35,8 @@
GO_TO_MERGED_CHANGES = 'GO_TO_MERGED_CHANGES',
GO_TO_ABANDONED_CHANGES = 'GO_TO_ABANDONED_CHANGES',
GO_TO_WATCHED_CHANGES = 'GO_TO_WATCHED_CHANGES',
+ GO_TO_REPOS = 'GO_TO_REPOS',
+ GO_TO_GROUPS = 'GO_TO_GROUPS',
CURSOR_NEXT_CHANGE = 'CURSOR_NEXT_CHANGE',
CURSOR_PREV_CHANGE = 'CURSOR_PREV_CHANGE',
@@ -167,6 +169,16 @@
{key: 'w', combo: ComboKey.G}
);
describe(
+ Shortcut.GO_TO_REPOS,
+ ShortcutSection.EVERYWHERE,
+ 'Go to Repositories',
+ {key: 'r', combo: ComboKey.G}
+ );
+ describe(Shortcut.GO_TO_GROUPS, ShortcutSection.EVERYWHERE, 'Go to Groups', {
+ key: 'g',
+ combo: ComboKey.G,
+ });
+ describe(
Shortcut.TOGGLE_CHECKBOX,
ShortcutSection.ACTIONS,
'Toggle checkbox',
diff --git a/polygerrit-ui/app/utils/admin-nav-util.ts b/polygerrit-ui/app/utils/admin-nav-util.ts
index 6f81d57..7916799 100644
--- a/polygerrit-ui/app/utils/admin-nav-util.ts
+++ b/polygerrit-ui/app/utils/admin-nav-util.ts
@@ -12,7 +12,7 @@
import {hasOwnProperty} from './common-util';
import {GerritView} from '../services/router/router-model';
import {MenuLink} from '../api/admin';
-import {AdminChildView} from '../models/views/admin';
+import {AdminChildView, createAdminUrl} from '../models/views/admin';
import {createGroupUrl, GroupDetailView} from '../models/views/group';
import {createRepoUrl, RepoDetailView} from '../models/views/repo';
@@ -20,7 +20,7 @@
{
name: 'Repositories',
noBaseUrl: true,
- url: '/admin/repos',
+ url: createAdminUrl({adminView: AdminChildView.REPOS}),
view: 'gr-repo-list' as GerritView,
viewableToAll: true,
},
@@ -28,7 +28,7 @@
name: 'Groups',
section: 'Groups',
noBaseUrl: true,
- url: '/admin/groups',
+ url: createAdminUrl({adminView: AdminChildView.GROUPS}),
view: 'gr-admin-group-list' as GerritView,
},
{
@@ -36,7 +36,7 @@
capability: 'viewPlugins',
section: 'Plugins',
noBaseUrl: true,
- url: '/admin/plugins',
+ url: createAdminUrl({adminView: AdminChildView.PLUGINS}),
view: 'gr-plugin-list' as GerritView,
},
];
