Merge changes I119f7594,I2c8398d6 * changes: Revert "Add project permission for removing votes/labels." Revert "Fix ChangeInfoDiffer.getAddedForMap method"
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); }