Merge "Add code to generate ProtocolBuffers from Gerrit API POJO definitions"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index f2f5a66..185fa07 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -800,6 +800,22 @@
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
@@ -1354,10 +1370,11 @@
[[capability_createProject]]
=== Create Project
-Allow project creation. This capability allows the granted group to
-either link:cmd-create-project.html[create new git projects via ssh]
-or via the web UI.
+Allow project creation.
+This capability allows the granted group to create projects via the web UI, via
+link:rest-api-projects.html#create-project][REST] and via
+link:cmd-create-project.html[SSH].
[[capability_emailReviewers]]
=== Email Reviewers
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 359361c..2f144c6 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -824,6 +824,26 @@
"+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,
@@ -7071,6 +7091,13 @@
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/Documentation/user-search.txt b/Documentation/user-search.txt
index 565c491..e12c27c 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -43,6 +43,17 @@
For more predictable results, use explicit search operators as described
in the following section.
+[IMPORTANT]
+--
+The change search API is backed by a secondary index and might sometimes return
+stale results if the re-indexing operation failed for a change update.
+
+Please also note that changes are not re-indexed if the project configuration
+is updated with newly added or modified
+link:config-submit-requirements.html[submit requirements].
+--
+
+
[[search-operators]]
== Search Operators
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 5b4a9e5..ccf74d1 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1393,6 +1393,17 @@
}
}
+ 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 b1cd506..69139ce 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -197,8 +197,13 @@
PermissionRule.Builder rule = newRule(projectConfig, p.group());
rule.setAction(p.action());
rule.setRange(p.min(), p.max());
- String permissionName =
- p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
+ String permissionName;
+ if (p.isAddPermission()) {
+ permissionName =
+ p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
+ } else {
+ permissionName = Permission.forRemoveLabel(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 9a9a21a..5634c78 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
@@ -162,12 +162,34 @@
/** Starts a builder for allowing a label permission. */
public static TestLabelPermission.Builder allowLabel(String name) {
- return TestLabelPermission.builder().name(name).action(PermissionRule.Action.ALLOW);
+ return TestLabelPermission.builder()
+ .name(name)
+ .isAddPermission(true)
+ .action(PermissionRule.Action.ALLOW);
}
/** Starts a builder for denying a label permission. */
public static TestLabelPermission.Builder blockLabel(String name) {
- return TestLabelPermission.builder().name(name).action(PermissionRule.Action.BLOCK);
+ 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);
}
/** Records a label permission to be updated. */
@@ -191,6 +213,8 @@
abstract boolean impersonation();
+ abstract boolean isAddPermission();
+
/** Builder for {@link TestLabelPermission}. */
@AutoValue.Builder
public abstract static class Builder {
@@ -208,6 +232,8 @@
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");
@@ -243,6 +269,12 @@
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 6d2fa32..d029fad 100644
--- a/java/com/google/gerrit/entities/Permission.java
+++ b/java/com/google/gerrit/entities/Permission.java
@@ -43,6 +43,7 @@
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";
@@ -60,6 +61,7 @@
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<>();
@@ -79,6 +81,7 @@
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());
@@ -93,15 +96,19 @@
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) || NAMES_LC.contains(varName.toLowerCase());
+ return isLabel(varName)
+ || isLabelAs(varName)
+ || isRemoveLabel(varName)
+ || NAMES_LC.contains(varName.toLowerCase());
}
public static boolean hasRange(String varName) {
- return isLabel(varName) || isLabelAs(varName);
+ return isLabel(varName) || isLabelAs(varName) || isRemoveLabel(varName);
}
/** Returns true if the permission name is actually for a review label. */
@@ -114,6 +121,11 @@
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;
@@ -124,12 +136,19 @@
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;
}
@@ -205,6 +224,8 @@
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 40ae2ec..a865187 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -105,6 +105,7 @@
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 24182cc..51c35dc 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
@@ -150,16 +150,17 @@
/** Returns {@code null} if nothing has been added to {@code oldCollection} */
@Nullable
private static ImmutableList<?> getAddedForCollection(
- Collection<?> oldCollection, Collection<?> newCollection) {
- ImmutableList<?> notInOldCollection = getAdditions(oldCollection, newCollection);
+ @Nullable Collection<?> oldCollection, Collection<?> newCollection) {
+ ImmutableList<?> notInOldCollection = getAdditionsForCollection(oldCollection, newCollection);
return notInOldCollection.isEmpty() ? null : notInOldCollection;
}
@Nullable
- private static ImmutableList<Object> getAdditions(
- Collection<?> oldCollection, Collection<?> newCollection) {
- if (oldCollection == null)
- return newCollection != null ? ImmutableList.copyOf(newCollection) : null;
+ private static ImmutableList<Object> getAdditionsForCollection(
+ @Nullable Collection<?> oldCollection, Collection<?> newCollection) {
+ if (oldCollection == null) {
+ return ImmutableList.copyOf(newCollection);
+ }
Map<Object, List<Object>> duplicatesMap = newCollection.stream().collect(groupingBy(v -> v));
oldCollection.forEach(
@@ -173,7 +174,18 @@
/** Returns {@code null} if nothing has been added to {@code oldMap} */
@Nullable
- private static ImmutableMap<Object, Object> getAddedForMap(Map<?, ?> oldMap, Map<?, ?> newMap) {
+ 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);
+ }
ImmutableMap.Builder<Object, Object> additionsBuilder = ImmutableMap.builder();
for (Map.Entry<?, ?> entry : newMap.entrySet()) {
Object added = getAdded(oldMap.get(entry.getKey()), entry.getValue());
@@ -181,8 +193,7 @@
additionsBuilder.put(entry.getKey(), added);
}
}
- ImmutableMap<Object, Object> additions = additionsBuilder.build();
- return additions.isEmpty() ? null : additions;
+ return additionsBuilder.build();
}
private static Object get(Field field, Object obj) {
diff --git a/java/com/google/gerrit/httpd/raw/StaticModule.java b/java/com/google/gerrit/httpd/raw/StaticModule.java
index 129d961..961bf9b 100644
--- a/java/com/google/gerrit/httpd/raw/StaticModule.java
+++ b/java/com/google/gerrit/httpd/raw/StaticModule.java
@@ -79,7 +79,6 @@
"/dashboard/*",
"/groups/self",
"/settings/*",
- "/topic/*",
"/Documentation/q/*");
/**
diff --git a/java/com/google/gerrit/index/Index.java b/java/com/google/gerrit/index/Index.java
index f2aafcf9..870d827 100644
--- a/java/com/google/gerrit/index/Index.java
+++ b/java/com/google/gerrit/index/Index.java
@@ -156,4 +156,14 @@
default boolean isEnabled() {
return true;
}
+
+ /**
+ * Rewriter that should be invoked on queries to this index.
+ *
+ * <p>The default implementation does not do anything. Should be overridden by implementation, if
+ * needed.
+ */
+ default IndexRewriter<V> getIndexRewriter() {
+ return (in, opts) -> in;
+ }
}
diff --git a/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java b/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
index ef2c3f5..05c23e1 100644
--- a/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
+++ b/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.index.SchemaUtil.schema;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.index.IndexedField;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.SchemaDefinitions;
@@ -38,7 +39,7 @@
ProjectField.PARENT_NAME_FIELD,
ProjectField.NAME_PART_FIELD,
ProjectField.ANCESTOR_NAME_FIELD),
- ImmutableList.of(
+ ImmutableList.<IndexedField<ProjectData, ?>.SearchSpec>of(
ProjectField.NAME_SPEC,
ProjectField.DESCRIPTION_SPEC,
ProjectField.PARENT_NAME_SPEC,
@@ -50,8 +51,8 @@
schema(
V1,
ImmutableList.of(ProjectField.REF_STATE),
- ImmutableList.of(ProjectField.STATE_FIELD),
- ImmutableList.of(ProjectField.STATE_SPEC));
+ ImmutableList.<IndexedField<ProjectData, ?>>of(ProjectField.STATE_FIELD),
+ ImmutableList.<IndexedField<ProjectData, ?>.SearchSpec>of(ProjectField.STATE_SPEC));
// Bump Lucene version requires reindexing
@Deprecated static final Schema<ProjectData> V3 = schema(V2);
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index f237006..1c8bbc3 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -268,7 +268,9 @@
limit,
getRequestedFields());
logger.atFine().log("Query options: %s", opts);
- Predicate<T> pred = rewriter.rewrite(q, opts);
+ // Apply index-specific rewrite first
+ Predicate<T> pred = indexes.getSearchIndex().getIndexRewriter().rewrite(q, opts);
+ pred = rewriter.rewrite(pred, opts);
if (enforceVisibility) {
pred = enforceVisibility(pred);
}
diff --git a/java/com/google/gerrit/server/IdentifiedUser.java b/java/com/google/gerrit/server/IdentifiedUser.java
index 65a81f7..eda6e09 100644
--- a/java/com/google/gerrit/server/IdentifiedUser.java
+++ b/java/com/google/gerrit/server/IdentifiedUser.java
@@ -100,30 +100,30 @@
enablePeerIPInReflogRecord,
Providers.of(null),
state,
- null);
+ /* realUser= */ null);
}
public IdentifiedUser create(Account.Id id) {
- return create(null, id);
+ return create(/* remotePeer= */ null, id);
}
@VisibleForTesting
@UsedAt(UsedAt.Project.GOOGLE)
public IdentifiedUser forTest(Account.Id id, PropertyMap properties) {
- return runAs(null, id, null, properties);
+ return runAs(/* remotePeer= */ null, id, /* caller= */ null, properties);
}
- public IdentifiedUser create(SocketAddress remotePeer, Account.Id id) {
- return runAs(remotePeer, id, null);
+ public IdentifiedUser create(@Nullable SocketAddress remotePeer, Account.Id id) {
+ return runAs(remotePeer, id, /* caller= */ null);
}
public IdentifiedUser runAs(
- SocketAddress remotePeer, Account.Id id, @Nullable CurrentUser caller) {
+ @Nullable SocketAddress remotePeer, Account.Id id, @Nullable CurrentUser caller) {
return runAs(remotePeer, id, caller, PropertyMap.EMPTY);
}
private IdentifiedUser runAs(
- SocketAddress remotePeer,
+ @Nullable SocketAddress remotePeer,
Account.Id id,
@Nullable CurrentUser caller,
PropertyMap properties) {
@@ -244,7 +244,7 @@
AccountCache accountCache,
GroupBackend groupBackend,
Boolean enablePeerIPInReflogRecord,
- @Nullable Provider<SocketAddress> remotePeerProvider,
+ Provider<SocketAddress> remotePeerProvider,
AccountState state,
@Nullable CurrentUser realUser) {
this(
@@ -270,7 +270,7 @@
AccountCache accountCache,
GroupBackend groupBackend,
Boolean enablePeerIPInReflogRecord,
- @Nullable Provider<SocketAddress> remotePeerProvider,
+ Provider<SocketAddress> remotePeerProvider,
Account.Id id,
@Nullable CurrentUser realUser,
PropertyMap properties) {
diff --git a/java/com/google/gerrit/server/account/AccountControl.java b/java/com/google/gerrit/server/account/AccountControl.java
index c80059b..ca63565 100644
--- a/java/com/google/gerrit/server/account/AccountControl.java
+++ b/java/com/google/gerrit/server/account/AccountControl.java
@@ -82,12 +82,12 @@
* accounts.
*/
@UsedAt(UsedAt.Project.PLUGIN_CODE_OWNERS)
- public AccountControl get(IdentifiedUser identifiedUser) {
+ public AccountControl get(CurrentUser user) {
return new AccountControl(
permissionBackend,
projectCache,
groupControlFactory,
- identifiedUser,
+ user,
userFactory,
accountVisibility);
}
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 65eb332..fcfc805 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -132,12 +132,18 @@
private final String input;
private final ImmutableList<AccountState> list;
private final ImmutableList<AccountState> filteredInactive;
+ private final CurrentUser searchedAsUser;
@VisibleForTesting
- Result(String input, List<AccountState> list, List<AccountState> filteredInactive) {
+ Result(
+ String input,
+ List<AccountState> list,
+ List<AccountState> filteredInactive,
+ CurrentUser searchedAsUser) {
this.input = requireNonNull(input);
this.list = canonicalize(list);
this.filteredInactive = canonicalize(filteredInactive);
+ this.searchedAsUser = requireNonNull(searchedAsUser);
}
private ImmutableList<AccountState> canonicalize(List<AccountState> list) {
@@ -180,13 +186,21 @@
}
}
- public IdentifiedUser asUniqueUser() throws UnresolvableAccountException {
+ private void ensureSelfIsUniqueIdentifiedUser() throws UnresolvableAccountException {
ensureUnique();
+ if (!searchedAsUser.isIdentifiedUser()) {
+ throw new UnresolvableAccountException(this);
+ }
+ }
+
+ public IdentifiedUser asUniqueUser() throws UnresolvableAccountException {
if (isSelf()) {
+ ensureSelfIsUniqueIdentifiedUser();
// In the special case of "self", use the exact IdentifiedUser from the request context, to
// preserve the peer address and any other per-request state.
- return self.get().asIdentifiedUser();
+ return searchedAsUser.asIdentifiedUser();
}
+ ensureUnique();
return userFactory.create(asUnique());
}
@@ -194,11 +208,10 @@
throws UnresolvableAccountException {
ensureUnique();
if (isSelf()) {
- // TODO(dborowitz): This preserves old behavior, but it seems wrong to discard the caller.
- return self.get().asIdentifiedUser();
+ return searchedAsUser.asIdentifiedUser();
}
return userFactory.runAs(
- null, list.get(0).account().id(), requireNonNull(caller).getRealUser());
+ /* remotePeer= */ null, list.get(0).account().id(), requireNonNull(caller).getRealUser());
}
@VisibleForTesting
@@ -221,16 +234,57 @@
return false;
}
+ /**
+ * Searches can be done on behalf of either the current user or another provided user. The
+ * results of some searchers, such as BySelf, are affected by the context user.
+ */
+ default boolean requiresContextUser() {
+ return false;
+ }
+
Optional<I> tryParse(String input) throws IOException;
- Stream<AccountState> search(I input) throws IOException, ConfigInvalidException;
+ /**
+ * This method should be implemented for every searcher which doesn't require a context user.
+ *
+ * @param input to search for
+ * @return stream of the matching accounts
+ * @throws IOException by some subclasses
+ * @throws ConfigInvalidException by some subclasses
+ */
+ default Stream<AccountState> search(I input) throws IOException, ConfigInvalidException {
+ throw new IllegalStateException("search(I) default implementation should never be called.");
+ }
+
+ /**
+ * This method should be implemented for every searcher which requires a context user.
+ *
+ * @param input to search for
+ * @param asUser the context user for the search
+ * @return stream of the matching accounts
+ * @throws IOException by some subclasses
+ * @throws ConfigInvalidException by some subclasses
+ */
+ default Stream<AccountState> search(I input, CurrentUser asUser)
+ throws IOException, ConfigInvalidException {
+ if (!requiresContextUser()) {
+ return search(input);
+ }
+ throw new IllegalStateException(
+ "The searcher requires a context user, but doesn't implement search(input, asUser).");
+ }
boolean shortCircuitIfNoResults();
- default Optional<Stream<AccountState>> trySearch(String input)
+ default Optional<Stream<AccountState>> trySearch(String input, CurrentUser asUser)
throws IOException, ConfigInvalidException {
Optional<I> parsed = tryParse(input);
- return parsed.isPresent() ? Optional.of(search(parsed.get())) : Optional.empty();
+ if (parsed.isEmpty()) {
+ return Optional.empty();
+ }
+ return requiresContextUser()
+ ? Optional.of(search(parsed.get(), asUser))
+ : Optional.of(search(parsed.get()));
}
}
@@ -251,7 +305,7 @@
}
}
- private class BySelf extends StringSearcher {
+ private static class BySelf extends StringSearcher {
@Override
public boolean callerShouldFilterOutInactiveCandidates() {
return false;
@@ -263,17 +317,21 @@
}
@Override
+ public boolean requiresContextUser() {
+ return true;
+ }
+
+ @Override
protected boolean matches(String input) {
return "self".equals(input) || "me".equals(input);
}
@Override
- public Stream<AccountState> search(String input) {
- CurrentUser user = self.get();
- if (!user.isIdentifiedUser()) {
+ public Stream<AccountState> search(String input, CurrentUser asUser) {
+ if (!asUser.isIdentifiedUser()) {
return Stream.empty();
}
- return Stream.of(user.asIdentifiedUser().state());
+ return Stream.of(asUser.asIdentifiedUser().state());
}
@Override
@@ -400,9 +458,20 @@
}
private class ByFullName implements Searcher<AccountState> {
+ boolean allowSkippingVisibilityCheck = true;
+
+ ByFullName() {
+ super();
+ }
+
+ ByFullName(boolean allowSkippingVisibilityCheck) {
+ this();
+ this.allowSkippingVisibilityCheck = allowSkippingVisibilityCheck;
+ }
+
@Override
public boolean callerMayAssumeCandidatesAreVisible() {
- return true; // Rely on enforceVisibility from the index.
+ return allowSkippingVisibilityCheck;
}
@Override
@@ -424,9 +493,25 @@
}
private class ByDefaultSearch extends StringSearcher {
+ boolean allowSkippingVisibilityCheck = true;
+
+ ByDefaultSearch() {
+ super();
+ }
+
+ ByDefaultSearch(boolean allowSkippingVisibilityCheck) {
+ this();
+ this.allowSkippingVisibilityCheck = allowSkippingVisibilityCheck;
+ }
+
@Override
public boolean callerMayAssumeCandidatesAreVisible() {
- return true; // Rely on enforceVisibility from the index.
+ return allowSkippingVisibilityCheck;
+ }
+
+ @Override
+ public boolean requiresContextUser() {
+ return true;
}
@Override
@@ -435,14 +520,14 @@
}
@Override
- public Stream<AccountState> search(String input) {
+ public Stream<AccountState> search(String input, CurrentUser asUser) {
// At this point we have no clue. Just perform a whole bunch of suggestions and pray we come
// up with a reasonable result list.
// TODO(dborowitz): This doesn't match the documentation; consider whether it's possible to be
// more strict here.
boolean canSeeSecondaryEmails = false;
try {
- if (permissionBackend.user(self.get()).test(GlobalPermission.MODIFY_ACCOUNT)) {
+ if (permissionBackend.user(asUser).test(GlobalPermission.MODIFY_ACCOUNT)) {
canSeeSecondaryEmails = true;
}
} catch (PermissionBackendException e) {
@@ -477,6 +562,18 @@
.addAll(nameOrEmailSearchers)
.build();
+ private final ImmutableList<Searcher<?>> forcedVisibilitySearchers =
+ ImmutableList.of(
+ new ByNameAndEmail(),
+ new ByEmail(),
+ new FromRealm(),
+ new ByFullName(false),
+ new ByDefaultSearch(false),
+ new BySelf(),
+ new ByExactAccountId(),
+ new ByParenthesizedAccountId(),
+ new ByUsername());
+
private final AccountCache accountCache;
private final AccountControl.Factory accountControlFactory;
private final Emails emails;
@@ -538,12 +635,63 @@
* @throws IOException if an error occurs.
*/
public Result resolve(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::isActive);
+ return searchImpl(
+ input, searchers, self.get(), this::currentUserCanSeePredicate, AccountResolver::isActive);
}
public Result resolve(String input, Predicate<AccountState> accountActivityPredicate)
throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::canSeePredicate, accountActivityPredicate);
+ return searchImpl(
+ input, searchers, self.get(), this::currentUserCanSeePredicate, accountActivityPredicate);
+ }
+
+ /**
+ * Resolves all accounts matching the input string, visible to the provided user.
+ *
+ * <p>The following input formats are recognized:
+ *
+ * <ul>
+ * <li>The strings {@code "self"} and {@code "me"}, if the provided user is an {@link
+ * IdentifiedUser}. In this case, may return exactly one inactive account.
+ * <li>A bare account ID ({@code "18419"}). In this case, may return exactly one inactive
+ * account. This case short-circuits if the input matches.
+ * <li>An account ID in parentheses following a full name ({@code "Full Name (18419)"}). This
+ * case short-circuits if the input matches.
+ * <li>A username ({@code "username"}).
+ * <li>A full name and email address ({@code "Full Name <email@example>"}). This case
+ * short-circuits if the input matches.
+ * <li>An email address ({@code "email@example"}. This case short-circuits if the input matches.
+ * <li>An account name recognized by the configured {@link Realm#lookup(String)} Realm}.
+ * <li>A full name ({@code "Full Name"}).
+ * <li>As a fallback, a {@link
+ * com.google.gerrit.server.query.account.AccountPredicates#defaultPredicate(Schema,
+ * boolean, String) default search} against the account index.
+ * </ul>
+ *
+ * @param asUser user to resolve the users by.
+ * @param input input string.
+ * @param forceVisibilityCheck whether to force all searchers to check for visibility.
+ * @return a result describing matching accounts. Never null even if the result set is empty.
+ * @throws ConfigInvalidException if an error occurs.
+ * @throws IOException if an error occurs.
+ */
+ public Result resolveAsUser(CurrentUser asUser, String input, boolean forceVisibilityCheck)
+ throws ConfigInvalidException, IOException {
+ return resolveAsUser(asUser, input, AccountResolver::isActive, forceVisibilityCheck);
+ }
+
+ public Result resolveAsUser(
+ CurrentUser asUser,
+ String input,
+ Predicate<AccountState> accountActivityPredicate,
+ boolean forceVisibilityCheck)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(
+ input,
+ forceVisibilityCheck ? forcedVisibilitySearchers : searchers,
+ asUser,
+ new ProvidedUserCanSeePredicate(asUser),
+ accountActivityPredicate);
}
/**
@@ -556,22 +704,23 @@
* instead will be stored as a link to the corresponding Gerrit Account.
*/
public Result resolveIncludeInactive(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::allVisible);
+ return searchImpl(
+ input,
+ searchers,
+ self.get(),
+ this::currentUserCanSeePredicate,
+ AccountResolver::allVisible);
}
public Result resolveIncludeInactiveIgnoreVisibility(String input)
throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::allVisible);
+ return searchImpl(
+ input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::allVisible);
}
public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::isActive);
- }
-
- public Result resolveIgnoreVisibility(
- String input, Predicate<AccountState> accountActivityPredicate)
- throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, this::allVisiblePredicate, accountActivityPredicate);
+ return searchImpl(
+ input, searchers, self.get(), this::allVisiblePredicate, AccountResolver::isActive);
}
/**
@@ -600,7 +749,11 @@
@Deprecated
public Result resolveByNameOrEmail(String input) throws ConfigInvalidException, IOException {
return searchImpl(
- input, nameOrEmailSearchers, this::canSeePredicate, AccountResolver::isActive);
+ input,
+ nameOrEmailSearchers,
+ self.get(),
+ this::currentUserCanSeePredicate,
+ AccountResolver::isActive);
}
/**
@@ -619,16 +772,26 @@
return searchImpl(
input,
ImmutableList.of(new ByNameAndEmail(), new ByEmail(), new ByFullName(), new ByUsername()),
- this::canSeePredicate,
+ self.get(),
+ this::currentUserCanSeePredicate,
AccountResolver::isActive);
}
- private Predicate<AccountState> canSeePredicate() {
- return this::canSee;
+ private Predicate<AccountState> currentUserCanSeePredicate() {
+ return accountControlFactory.get()::canSee;
}
- private boolean canSee(AccountState accountState) {
- return accountControlFactory.get().canSee(accountState);
+ private class ProvidedUserCanSeePredicate implements Supplier<Predicate<AccountState>> {
+ CurrentUser asUser;
+
+ ProvidedUserCanSeePredicate(CurrentUser asUser) {
+ this.asUser = asUser;
+ }
+
+ @Override
+ public Predicate<AccountState> get() {
+ return accountControlFactory.get(asUser.asIdentifiedUser())::canSee;
+ }
}
private Predicate<AccountState> allVisiblePredicate() {
@@ -648,14 +811,16 @@
Result searchImpl(
String input,
ImmutableList<Searcher<?>> searchers,
+ CurrentUser asUser,
Supplier<Predicate<AccountState>> visibilitySupplier,
Predicate<AccountState> accountActivityPredicate)
throws ConfigInvalidException, IOException {
+ requireNonNull(asUser);
visibilitySupplier = Suppliers.memoize(visibilitySupplier::get);
List<AccountState> inactive = new ArrayList<>();
for (Searcher<?> searcher : searchers) {
- Optional<Stream<AccountState>> maybeResults = searcher.trySearch(input);
+ Optional<Stream<AccountState>> maybeResults = searcher.trySearch(input, asUser);
if (!maybeResults.isPresent()) {
continue;
}
@@ -677,22 +842,25 @@
}
if (!list.isEmpty()) {
- return createResult(input, list);
+ return createResult(input, list, asUser);
}
if (searcher.shortCircuitIfNoResults()) {
// For a short-circuiting searcher, return results even if empty.
- return !inactive.isEmpty() ? emptyResult(input, inactive) : createResult(input, list);
+ return !inactive.isEmpty()
+ ? emptyResult(input, inactive, asUser)
+ : createResult(input, list, asUser);
}
}
- return emptyResult(input, inactive);
+ return emptyResult(input, inactive, asUser);
}
- private Result createResult(String input, List<AccountState> list) {
- return new Result(input, list, ImmutableList.of());
+ private Result createResult(String input, List<AccountState> list, CurrentUser searchedAsUser) {
+ return new Result(input, list, ImmutableList.of(), searchedAsUser);
}
- private Result emptyResult(String input, List<AccountState> inactive) {
- return new Result(input, ImmutableList.of(), inactive);
+ private Result emptyResult(
+ String input, List<AccountState> inactive, CurrentUser searchedAsUser) {
+ return new Result(input, ImmutableList.of(), inactive, searchedAsUser);
}
private Stream<AccountState> toAccountStates(Set<Account.Id> ids) {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 500bb77..912d202 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -146,7 +146,7 @@
public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_STRICT =
ChangeField.SUBMIT_RULE_OPTIONS_STRICT.toBuilder().build();
- static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD =
+ public static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD =
ImmutableSet.of(
ALL_COMMITS,
ALL_REVISIONS,
@@ -688,6 +688,7 @@
!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 cfa15ae..5555ba6 100644
--- a/java/com/google/gerrit/server/change/LabelsJson.java
+++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -36,15 +36,19 @@
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.project.RemoveReviewerControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -69,10 +73,17 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final PermissionBackend permissionBackend;
+ private final DeleteVoteControl deleteVoteControl;
+ private final RemoveReviewerControl removeReviewerControl;
@Inject
- LabelsJson(PermissionBackend permissionBackend) {
+ LabelsJson(
+ PermissionBackend permissionBackend,
+ DeleteVoteControl deleteVoteControl,
+ RemoveReviewerControl removeReviewerControl) {
this.permissionBackend = permissionBackend;
+ this.deleteVoteControl = deleteVoteControl;
+ this.removeReviewerControl = removeReviewerControl;
}
/**
@@ -133,6 +144,46 @@
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, approval, labelType.get())
+ || removeReviewerControl.testRemoveReviewer(
+ cd, user, approval.accountId(), approval.value()))) {
+ 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()) {
@@ -217,10 +268,10 @@
}
}
- private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd) {
+ private Map<String, Short> currentLabels(@Nullable Account.Id accountId, ChangeData cd) {
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa : cd.currentApprovals()) {
- if (psa.accountId().equals(accountId)) {
+ if (accountId == null || psa.accountId().equals(accountId)) {
result.put(psa.label(), psa.value());
}
}
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index dcbd1ae..8acc925 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
@@ -37,7 +35,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
@@ -70,30 +67,28 @@
this.rebaseFactory = rebaseFactory;
}
- public static void verifyRebasePreconditions(
- ProjectCache projectCache, PatchSetUtil patchSetUtil, RevWalk rw, RevisionResource rsrc)
- throws ResourceConflictException, IOException, AuthException, PermissionBackendException {
+ /**
+ * Checks whether the given change fulfills all preconditions to be rebased.
+ *
+ * <p>This method does not check whether the calling user is allowed to rebase the change.
+ */
+ public void verifyRebasePreconditions(RevWalk rw, ChangeNotes changeNotes, PatchSet patchSet)
+ throws ResourceConflictException, IOException {
// Not allowed to rebase if the current patch set is locked.
- patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes());
+ psUtil.checkPatchSetNotLocked(changeNotes);
- rsrc.permissions().check(ChangePermission.REBASE);
- projectCache
- .get(rsrc.getProject())
- .orElseThrow(illegalState(rsrc.getProject()))
- .checkStatePermitsWrite();
-
- if (!rsrc.getChange().isNew()) {
+ Change change = changeNotes.getChange();
+ if (!change.isNew()) {
throw new ResourceConflictException(
- String.format(
- "Change %s is %s", rsrc.getChange().getId(), ChangeUtil.status(rsrc.getChange())));
- } else if (!hasOneParent(rw, rsrc.getPatchSet())) {
+ String.format("Change %s is %s", change.getId(), ChangeUtil.status(change)));
+ }
+
+ if (!hasOneParent(rw, patchSet)) {
throw new ResourceConflictException(
String.format(
"Error rebasing %s. Cannot rebase %s",
- rsrc.getChange().getId(),
- countParents(rw, rsrc.getPatchSet()) > 1
- ? "merge commits"
- : "commit with no ancestor"));
+ change.getId(),
+ countParents(rw, patchSet) > 1 ? "merge commits" : "commit with no ancestor"));
}
}
diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index f4c211d..cbf47c5 100644
--- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -253,6 +253,7 @@
qb = WatcherChangeQueryBuilder.asUser(args.queryBuilder.get(), user);
p = qb.isVisible();
}
+ qb.forceAccountVisibilityCheck();
if (filter != null) {
Predicate<ChangeData> filterPredicate = qb.parse(filter);
diff --git a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java b/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
new file mode 100644
index 0000000..622f0cf
--- /dev/null
+++ b/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
@@ -0,0 +1,155 @@
+// 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 8d432c8..6f7d761 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 LabelPermission) {
- return can((LabelPermission) perm);
- } else if (perm instanceof LabelPermission.WithValue) {
- return can((LabelPermission.WithValue) perm);
+ } else if (perm instanceof AbstractLabelPermission) {
+ return can((AbstractLabelPermission) perm);
+ } else if (perm instanceof AbstractLabelPermission.WithValue) {
+ return can((AbstractLabelPermission.WithValue) perm);
}
throw new PermissionBackendException(perm + " unsupported");
}
@@ -288,11 +288,11 @@
throw new PermissionBackendException(perm + " unsupported");
}
- private boolean can(LabelPermission perm) {
+ private boolean can(AbstractLabelPermission perm) {
return !label(labelPermissionName(perm)).isEmpty();
}
- private boolean can(LabelPermission.WithValue perm) {
+ private boolean can(AbstractLabelPermission.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 2824efd..f59ba02 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 LabelPermission}. */
+/** A {@link ChangePermission} or a {@link AbstractLabelPermission}. */
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 9d69d9b..89f0493 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.LabelPermission.ForUser;
+import com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser;
import java.util.EnumSet;
import java.util.Optional;
import java.util.Set;
@@ -160,19 +160,29 @@
return Optional.ofNullable(CHANGE_PERMISSIONS.inverse().get(permissionName));
}
- public static String labelPermissionName(LabelPermission labelPermission) {
- if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
- return Permission.forLabelAs(labelPermission.label());
+ 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());
}
- return Permission.forLabel(labelPermission.label());
+ throw new IllegalStateException("invalid AbstractLabelPermission subtype");
}
// TODO(dborowitz): Can these share a common superinterface?
- public static String labelPermissionName(LabelPermission.WithValue labelPermission) {
- if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
- return Permission.forLabelAs(labelPermission.label());
+ 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());
}
- return Permission.forLabel(labelPermission.label());
+ throw new IllegalStateException("invalid AbstractLabelPermission.WithValue subtype");
}
private DefaultPermissionMappings() {}
diff --git a/java/com/google/gerrit/server/permissions/LabelPermission.java b/java/com/google/gerrit/server/permissions/LabelPermission.java
index c266caa..4652364 100644
--- a/java/com/google/gerrit/server/permissions/LabelPermission.java
+++ b/java/com/google/gerrit/server/permissions/LabelPermission.java
@@ -14,24 +14,14 @@
package com.google.gerrit.server.permissions;
-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 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. */
-public class LabelPermission implements ChangePermissionOrLabel {
- public enum ForUser {
- SELF,
- ON_BEHALF_OF;
- }
-
- private final ForUser forUser;
- private final String name;
-
+public class LabelPermission extends AbstractLabelPermission {
/**
* Construct a reference to a label permission.
*
@@ -67,55 +57,16 @@
* @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
*/
public LabelPermission(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;
+ super(forUser, name);
}
@Override
- 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 + ']';
+ public String permissionPrefix() {
+ return "label";
}
/** A {@link LabelPermission} at a specific value. */
- public static class WithValue implements ChangePermissionOrLabel {
- private final ForUser forUser;
- private final LabelVote label;
-
+ public static class WithValue extends AbstractLabelPermission.WithValue {
/**
* Construct a reference to a label at a specific value.
*
@@ -195,53 +146,12 @@
* @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();
+ super(forUser, label);
}
@Override
- 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() + ']';
+ public String permissionName() {
+ return "label";
}
}
}
diff --git a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
new file mode 100644
index 0000000..2553601
--- /dev/null
+++ b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
@@ -0,0 +1,94 @@
+// 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 fea2827..eb5e053 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -474,6 +474,18 @@
}
/**
+ * 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.
@@ -486,10 +498,29 @@
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
new file mode 100644
index 0000000..3f3f88a
--- /dev/null
+++ b/java/com/google/gerrit/server/project/DeleteVoteControl.java
@@ -0,0 +1,81 @@
+// 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.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.gerrit.server.query.change.ChangeData;
+import com.google.inject.Inject;
+import java.util.Set;
+
+public class DeleteVoteControl {
+ private final PermissionBackend permissionBackend;
+ private final ChangeData.Factory changeDataFactory;
+
+ @Inject
+ public DeleteVoteControl(
+ PermissionBackend permissionBackend, ChangeData.Factory changeDataFactory) {
+ this.permissionBackend = permissionBackend;
+ this.changeDataFactory = changeDataFactory;
+ }
+
+ public boolean testDeleteVotePermissions(
+ CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
+ throws PermissionBackendException {
+ return testDeleteVotePermissions(user, changeDataFactory.create(notes), approval, labelType);
+ }
+
+ public boolean testDeleteVotePermissions(
+ CurrentUser user, ChangeData cd, PatchSetApproval approval, LabelType labelType)
+ throws PermissionBackendException {
+ if (canRemoveReviewerWithoutRemoveLabelPermission(
+ cd.change(), 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(cd).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/project/ProjectCacheImpl.java b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
index 0afaa3f..6498d1b 100644
--- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
@@ -76,7 +76,6 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
-import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
@@ -301,20 +300,19 @@
@Override
public Set<AccountGroup.UUID> guessRelevantGroupUUIDs() {
try (Timer0.Context ignored = guessRelevantGroupsLatency.start()) {
- Stream<AccountGroup.UUID> configuredRelevantGroups =
- Arrays.stream(config.getStringList("groups", /* subsection= */ null, "relevantGroup"))
- .map(AccountGroup::uuid);
-
- Stream<AccountGroup.UUID> guessedRelevantGroups =
- inMemoryProjectCache.asMap().values().stream()
- .filter(Objects::nonNull)
- .flatMap(p -> p.getAllGroupUUIDs().stream())
- // getAllGroupUUIDs shouldn't really return null UUIDs, but harden
- // against them just in case there is a bug or corner case.
- .filter(id -> id != null && id.get() != null);
-
Set<AccountGroup.UUID> relevantGroupUuids =
- Streams.concat(configuredRelevantGroups, guessedRelevantGroups).collect(toSet());
+ Streams.concat(
+ Arrays.stream(
+ config.getStringList("groups", /* subsection= */ null, "relevantGroup"))
+ .map(AccountGroup::uuid),
+ all().stream()
+ .map(n -> inMemoryProjectCache.getIfPresent(n))
+ .filter(Objects::nonNull)
+ .flatMap(p -> p.getAllGroupUUIDs().stream())
+ // getAllGroupUUIDs shouldn't really return null UUIDs, but harden
+ // against them just in case there is a bug or corner case.
+ .filter(id -> id != null && id.get() != null))
+ .collect(toSet());
logger.atFine().log("relevant group UUIDs: %s", relevantGroupUuids);
return relevantGroupUuids;
}
diff --git a/java/com/google/gerrit/server/project/RemoveReviewerControl.java b/java/com/google/gerrit/server/project/RemoveReviewerControl.java
index 1bc309c..3fda87a 100644
--- a/java/com/google/gerrit/server/project/RemoveReviewerControl.java
+++ b/java/com/google/gerrit/server/project/RemoveReviewerControl.java
@@ -32,10 +32,12 @@
@Singleton
public class RemoveReviewerControl {
private final PermissionBackend permissionBackend;
+ private final ChangeData.Factory changeDataFactory;
@Inject
- RemoveReviewerControl(PermissionBackend permissionBackend) {
+ RemoveReviewerControl(PermissionBackend permissionBackend, ChangeData.Factory changeDataFactory) {
this.permissionBackend = permissionBackend;
+ this.changeDataFactory = changeDataFactory;
}
/**
@@ -64,6 +66,20 @@
/** Returns true if the user is allowed to remove this reviewer. */
public boolean testRemoveReviewer(
+ ChangeNotes notes, CurrentUser currentUser, PatchSetApproval approval)
+ throws PermissionBackendException {
+ return testRemoveReviewer(notes, currentUser, approval.accountId(), approval.value());
+ }
+
+ /** Returns true if the user is allowed to remove this reviewer. */
+ public boolean testRemoveReviewer(
+ ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+ throws PermissionBackendException {
+ return testRemoveReviewer(changeDataFactory.create(notes), currentUser, reviewer, value);
+ }
+
+ /** Returns true if the user is allowed to remove this reviewer. */
+ public boolean testRemoveReviewer(
ChangeData cd, CurrentUser currentUser, Account.Id reviewer, int value)
throws PermissionBackendException {
if (canRemoveReviewerWithoutPermissionCheck(
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index da75057..ca18ab2 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -488,6 +488,7 @@
private final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
private Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
+ private boolean forceAccountVisibilityCheck = false;
private static final Splitter RULE_SPLITTER = Splitter.on("=");
private static final Splitter PLUGIN_SPLITTER = Splitter.on("_");
@@ -518,6 +519,11 @@
return args;
}
+ /** Whether to force account visibility check when searching for changes by account(s). */
+ public void forceAccountVisibilityCheck() {
+ forceAccountVisibilityCheck = true;
+ }
+
@Operator
public Predicate<ChangeData> age(String value) {
return new AgePredicate(value);
@@ -545,14 +551,14 @@
@Operator
public Predicate<ChangeData> mergedBefore(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
return new BeforePredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_BEFORE, value);
}
@Operator
public Predicate<ChangeData> mergedAfter(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
return new AfterPredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_AFTER, value);
}
@@ -635,7 +641,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
return new IsAttentionPredicate();
}
@@ -678,7 +684,7 @@
}
if ("uploader".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
return ChangePredicates.uploader(self());
}
@@ -701,7 +707,7 @@
}
if ("merge".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.MERGE_SPEC, "is:merge");
+ checkOperatorAvailable(ChangeField.MERGE_SPEC, "is:merge");
return new BooleanPredicate(ChangeField.MERGE_SPEC);
}
@@ -710,7 +716,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
return new IsAttentionPredicate();
}
@@ -723,7 +729,7 @@
}
if ("pure-revert".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
+ checkOperatorAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
return ChangePredicates.pureRevert("1");
}
@@ -739,12 +745,12 @@
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.NOT_READY)),
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.RULE_ERROR)));
}
- checkFieldAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
+ checkOperatorAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
return new IsSubmittablePredicate();
}
if ("started".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.STARTED_SPEC, "is:started");
+ checkOperatorAvailable(ChangeField.STARTED_SPEC, "is:started");
return new BooleanPredicate(ChangeField.STARTED_SPEC);
}
@@ -753,7 +759,7 @@
}
if ("cherrypick".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
return new BooleanPredicate(ChangeField.CHERRY_PICK_SPEC);
}
@@ -888,7 +894,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
+ checkOperatorAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
return ChangePredicates.fuzzyHashtag(hashtag);
}
@@ -898,7 +904,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
+ checkOperatorAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
return ChangePredicates.prefixHashtag(hashtag);
}
@@ -924,7 +930,7 @@
return ChangePredicates.exactTopic(name);
}
- checkFieldAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
+ checkOperatorAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
return ChangePredicates.prefixTopic(name);
}
@@ -990,7 +996,7 @@
@Operator
public Predicate<ChangeData> hasfooter(String footerName) throws QueryParseException {
- checkFieldAvailable(ChangeField.FOOTER_NAME, "hasfooter");
+ checkOperatorAvailable(ChangeField.FOOTER_NAME, "hasfooter");
return ChangePredicates.hasFooter(footerName);
}
@@ -1141,10 +1147,9 @@
@Operator
public Predicate<ChangeData> message(String text) throws QueryParseException {
if (text.startsWith("^")) {
- if (!args.index.getSchema().hasField(ChangeField.COMMIT_MESSAGE_EXACT)) {
- throw new QueryParseException(
- "'message' operator with regular expression is not supported on this gerrit host");
- }
+ checkFieldAvailable(
+ ChangeField.COMMIT_MESSAGE_EXACT,
+ "'message' operator with regular expression is not supported on this gerrit host");
return new RegexMessagePredicate(text);
}
return ChangePredicates.message(text);
@@ -1152,13 +1157,14 @@
@Operator
public Predicate<ChangeData> subject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
+ checkOperatorAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
return ChangePredicates.subject(value);
}
@Operator
public Predicate<ChangeData> prefixsubject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
+ checkOperatorAvailable(
+ ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
return ChangePredicates.prefixSubject(value);
}
@@ -1246,7 +1252,7 @@
@Operator
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
return uploader(parseAccount(who, (AccountState s) -> true));
}
@@ -1261,7 +1267,7 @@
@Operator
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
return attention(parseAccount(who, (AccountState s) -> true));
}
@@ -1306,7 +1312,7 @@
@Operator
public Predicate<ChangeData> uploaderin(String group) throws QueryParseException, IOException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
if (g == null) {
@@ -1572,8 +1578,8 @@
@Operator
public Predicate<ChangeData> cherryPickOf(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
if (Ints.tryParse(value) != null) {
return ChangePredicates.cherryPickOf(Change.id(Ints.tryParse(value)));
}
@@ -1645,11 +1651,16 @@
return Predicate.or(predicates);
}
- protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String operator)
+ private void checkOperatorAvailable(SchemaField<ChangeData, ?> field, String operator)
+ throws QueryParseException {
+ checkFieldAvailable(
+ field, String.format("'%s' operator is not supported on this gerrit host", operator));
+ }
+
+ protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String errorMessage)
throws QueryParseException {
if (!args.index.getSchema().hasField(field)) {
- throw new QueryParseException(
- String.format("'%s' operator is not supported on this gerrit host", operator));
+ throw new QueryParseException(errorMessage);
}
}
@@ -1700,7 +1711,9 @@
private Set<Account.Id> parseAccount(String who)
throws QueryParseException, IOException, ConfigInvalidException {
try {
- return args.accountResolver.resolve(who).asNonEmptyIdSet();
+ return args.accountResolver
+ .resolveAsUser(args.getUser(), who, forceAccountVisibilityCheck)
+ .asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
@@ -1713,7 +1726,9 @@
String who, java.util.function.Predicate<AccountState> activityFilter)
throws QueryParseException, IOException, ConfigInvalidException {
try {
- return args.accountResolver.resolve(who, activityFilter).asNonEmptyIdSet();
+ return args.accountResolver
+ .resolveAsUser(args.getUser(), who, activityFilter, forceAccountVisibilityCheck)
+ .asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index ccd645b..3edad69 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -26,6 +26,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
@@ -103,6 +104,7 @@
return query(ChangePredicates.idStr(id));
}
+ @UsedAt(UsedAt.Project.GOOGLE)
public List<ChangeData> byLegacyChangeIds(Collection<Change.Id> ids) {
List<Predicate<ChangeData>> preds = new ArrayList<>(ids.size());
for (Change.Id id : ids) {
@@ -115,15 +117,6 @@
return query(byBranchKeyPred(branch, key));
}
- public List<ChangeData> byBranchKeyOpen(Project.NameKey project, String branch, Change.Key key) {
- return query(and(byBranchKeyPred(BranchNameKey.create(project, branch), key), open()));
- }
-
- public static Predicate<ChangeData> byBranchKeyOpenPred(
- Project.NameKey project, String branch, Change.Key key) {
- return and(byBranchKeyPred(BranchNameKey.create(project, branch), key), open());
- }
-
private static Predicate<ChangeData> byBranchKeyPred(BranchNameKey branch, Change.Key key) {
return and(ref(branch), project(branch.project()), change(key));
}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
index a0fc121..0e1a218 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -20,6 +20,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
@@ -37,7 +38,9 @@
import com.google.gerrit.server.mail.send.DeleteVoteSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
+import com.google.gerrit.server.permissions.LabelRemovalPermission;
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;
@@ -75,6 +78,7 @@
private final VoteDeleted voteDeleted;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
+ private final DeleteVoteControl deleteVoteControl;
private final RemoveReviewerControl removeReviewerControl;
private final MessageIdGenerator messageIdGenerator;
@@ -96,8 +100,9 @@
ChangeMessagesUtil cmUtil,
VoteDeleted voteDeleted,
DeleteVoteSender.Factory deleteVoteSenderFactory,
- RemoveReviewerControl removeReviewerControl,
+ DeleteVoteControl deleteVoteControl,
MessageIdGenerator messageIdGenerator,
+ RemoveReviewerControl removeReviewerControl,
@Assisted Project.NameKey projectName,
@Assisted AccountState reviewerToDeleteVoteFor,
@Assisted String label,
@@ -109,6 +114,7 @@
this.cmUtil = cmUtil;
this.voteDeleted = voteDeleted;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
+ this.deleteVoteControl = deleteVoteControl;
this.removeReviewerControl = removeReviewerControl;
this.messageIdGenerator = messageIdGenerator;
@@ -143,12 +149,7 @@
newApprovals.put(a.label(), a.value());
continue;
} else if (enforcePermissions) {
- // 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);
- }
+ checkPermissions(ctx, labelTypes.byLabel(a.labelId()).get(), a);
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.label(), (short) 0);
@@ -209,4 +210,21 @@
user.isIdentifiedUser() ? user.asIdentifiedUser().state() : null,
ctx.getWhen());
}
+
+ private void checkPermissions(ChangeContext ctx, LabelType labelType, PatchSetApproval approval)
+ throws PermissionBackendException, AuthException {
+ boolean permitted =
+ removeReviewerControl.testRemoveReviewer(ctx.getNotes(), ctx.getUser(), approval)
+ || deleteVoteControl.testDeleteVotePermissions(
+ ctx.getUser(), ctx.getNotes(), approval, labelType);
+ if (!permitted) {
+ throw new AuthException(
+ "Delete vote not permitted.",
+ new AuthException(
+ "Both "
+ + new LabelRemovalPermission.WithValue(labelType, approval.value())
+ .describeForException()
+ + " and remove-reviewer are not permitted"));
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 259e71d..22eb32c 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.LabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.AbstractLabelPermission.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/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 1a8f07a..8a8d2ca 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -87,6 +87,13 @@
@Override
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
+ rsrc.permissions().check(ChangePermission.REBASE);
+
+ projectCache
+ .get(rsrc.getProject())
+ .orElseThrow(illegalState(rsrc.getProject()))
+ .checkStatePermitsWrite();
+
Change change = rsrc.getChange();
try (Repository repo = repoManager.openRepository(change.getProject());
ObjectInserter oi = repo.newObjectInserter();
@@ -94,7 +101,7 @@
RevWalk rw = CodeReviewCommit.newRevWalk(reader);
BatchUpdate bu =
updateFactory.create(change.getProject(), rsrc.getUser(), TimeUtil.now())) {
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, rsrc);
+ rebaseUtil.verifyRebasePreconditions(rw, rsrc.getNotes(), rsrc.getPatchSet());
RebaseChangeOp rebaseOp =
rebaseUtil.getRebaseOp(
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 4754c69..786bba7 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -113,7 +113,11 @@
@Override
public Response<RebaseChainInfo> apply(ChangeResource tipRsrc, RebaseInput input)
throws IOException, PermissionBackendException, RestApiException, UpdateException {
+ tipRsrc.permissions().check(ChangePermission.REBASE);
+
Project.NameKey project = tipRsrc.getProject();
+ projectCache.get(project).orElseThrow(illegalState(project)).checkStatePermitsWrite();
+
CurrentUser user = tipRsrc.getUser();
List<Change.Id> upToDateAncestors = new ArrayList<>();
@@ -136,7 +140,8 @@
RevisionResource revRsrc =
new RevisionResource(changeResourceFactory.create(changeData, user), ps);
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, revRsrc);
+ revRsrc.permissions().check(ChangePermission.REBASE);
+ rebaseUtil.verifyRebasePreconditions(rw, changeData.notes(), ps);
boolean isUpToDate = false;
RebaseChangeOp rebaseOp = null;
diff --git a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
index 491b5cd..f3c741f 100644
--- a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
+++ b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
@@ -37,6 +37,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
+import java.util.stream.Collectors;
import javax.inject.Inject;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
@@ -216,12 +217,19 @@
Arrays.asList(
cfg.getStringList(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_VALUE)))
.build();
+ ImmutableList<String> refPatterns =
+ ImmutableList.<String>builder()
+ .addAll(
+ Arrays.asList(
+ cfg.getStringList(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_BRANCH)))
+ .build();
LabelAttributes attributes =
LabelAttributes.create(
function == null ? "MaxWithBlock" : function,
canOverride,
ignoreSelfApproval,
- values);
+ values,
+ refPatterns);
labelTypes.put(labelName, attributes);
}
return labelTypes;
@@ -320,6 +328,15 @@
default:
break;
}
+ if (!attributes.refPatterns().isEmpty()) {
+ builder.setApplicabilityExpression(
+ SubmitRequirementExpression.of(
+ String.join(
+ " OR ",
+ attributes.refPatterns().stream()
+ .map(b -> "branch:\\\"" + b + "\\\"")
+ .collect(Collectors.toList()))));
+ }
return builder.build();
}
@@ -435,13 +452,16 @@
abstract ImmutableList<String> values();
+ abstract ImmutableList<String> refPatterns();
+
static LabelAttributes create(
String function,
boolean canOverride,
boolean ignoreSelfApproval,
- ImmutableList<String> values) {
+ ImmutableList<String> values,
+ ImmutableList<String> refPatterns) {
return new AutoValue_MigrateLabelFunctionsToSubmitRequirement_LabelAttributes(
- function, canOverride, ignoreSelfApproval, values);
+ function, canOverride, ignoreSelfApproval, values, refPatterns);
}
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 211baeb..215d1e8 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -28,6 +28,7 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
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;
@@ -160,6 +161,7 @@
import com.google.gerrit.index.query.PostFilterPredicate;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.account.AccountControl;
+import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.testing.TestChangeETagComputation;
@@ -2406,7 +2408,68 @@
.id(r.getChangeId())
.reviewer(admin.id().toString())
.deleteVote(LabelId.CODE_REVIEW));
- assertThat(thrown).hasMessageThat().contains("delete vote not permitted");
+ assertThat(thrown).hasMessageThat().contains("Delete vote 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))
+ .add(block(Permission.REMOVE_REVIEWER).ref("refs/heads/*").group(REGISTERED_USERS))
+ .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))
+ .add(block(Permission.REMOVE_REVIEWER).ref("refs/heads/*").group(REGISTERED_USERS))
+ .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);
}
@Test
@@ -2996,7 +3059,11 @@
.review(ReviewInput.approve());
gApi.changes().id(r1.getChangeId()).revision(r1.getCommit().name()).submit();
- createChange();
+ PushOneCommit.Result change = createChange();
+ // Populate change with a reasonable set of fields. We can't exhaustively
+ // test all possible variations, but can try to cover a reasonable set.
+ approve(change.getChangeId());
+ gApi.changes().id(change.getChangeId()).addReviewer(user.email());
requestScopeOperations.setApiUser(user.id());
try (AutoCloseable ignored = disableNoteDb()) {
@@ -3011,6 +3078,34 @@
}
@Test
+ public void nonLazyloadQueryOptionsDoNotTouchDatabase() throws Exception {
+ requestScopeOperations.setApiUser(admin.id());
+ PushOneCommit.Result r1 = createChange();
+ gApi.changes()
+ .id(r1.getChangeId())
+ .revision(r1.getCommit().name())
+ .review(ReviewInput.approve());
+ gApi.changes().id(r1.getChangeId()).revision(r1.getCommit().name()).submit();
+
+ PushOneCommit.Result change = createChange();
+ // Populate change with a reasonable set of fields. We can't exhaustively
+ // test all possible variations, but can try to cover a reasonable set.
+ approve(change.getChangeId());
+ gApi.changes().id(change.getChangeId()).addReviewer(user.email());
+
+ requestScopeOperations.setApiUser(user.id());
+ try (AutoCloseable ignored = disableNoteDb()) {
+ assertThat(
+ gApi.changes()
+ .query()
+ .withQuery("project:{" + project.get() + "} (status:open OR status:closed)")
+ .withOptions(EnumSet.complementOf(EnumSet.copyOf(ChangeJson.REQUIRE_LAZY_LOAD)))
+ .get())
+ .hasSize(2);
+ }
+ }
+
+ @Test
public void votable() throws Exception {
PushOneCommit.Result r = createChange();
String triplet = project.get() + "~master~" + r.getChangeId();
@@ -3275,6 +3370,7 @@
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();
@@ -3304,6 +3400,9 @@
.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
@@ -3323,6 +3422,7 @@
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.
@@ -3331,6 +3431,7 @@
assertThat(change.status).isEqualTo(ChangeStatus.ABANDONED);
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels).isEmpty();
+ assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3447,6 +3548,7 @@
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
@@ -3464,6 +3566,7 @@
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
@@ -3560,6 +3663,7 @@
.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
@@ -3575,6 +3679,7 @@
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
@@ -4281,8 +4386,12 @@
ListChangesOption.SKIP_DIFFSTAT);
PushOneCommit.Result change = createChange();
- int number = gApi.changes().id(change.getChangeId()).get()._number;
+ // Populate change with a reasonable set of fields. We can't exhaustively
+ // test all possible variations, but can try to cover a reasonable set.
+ approve(change.getChangeId());
+ gApi.changes().id(change.getChangeId()).addReviewer(user.email());
+ int number = gApi.changes().id(change.getChangeId()).get()._number;
try (AutoCloseable ignored = changeIndexOperations.disableReadsAndWrites()) {
assertThat(gApi.changes().id(project.get(), number).get(options).changeId)
.isEqualTo(change.getChangeId());
diff --git a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
index 74bfe0f..9d37497 100644
--- a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -260,6 +261,96 @@
}
@Test
+ public void migrateBlockingLabel_withBranchAttribute() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("refs/heads/master"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_withMultipleBranchAttributes() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("refs/heads/master", "refs/heads/develop"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" "
+ + "OR branch:\\\"refs/heads/develop\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_withRegexBranchAttribute() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("^refs/heads/main-.*"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"^refs/heads/main-.*\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_withRegexAndNonRegexBranchAttributes() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("refs/heads/master", "^refs/heads/main-.*"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" "
+ + "OR branch:\\\"^refs/heads/main-.*\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
public void migrationIsIdempotent() throws Exception {
String oldRefsConfigId;
try (Repository repo = repoManager.openRepository(project)) {
@@ -381,6 +472,21 @@
gApi.projects().name(project.get()).label(labelName).create(input);
}
+ private void createLabelWithBranch(
+ String labelName,
+ String function,
+ boolean ignoreSelfApproval,
+ ImmutableList<String> refPatterns)
+ throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.name = labelName;
+ input.function = function;
+ input.ignoreSelfApproval = ignoreSelfApproval;
+ input.values = ImmutableMap.of("+1", "Looks Good", " 0", "Don't Know", "-1", "Looks Bad");
+ input.branches = refPatterns;
+ gApi.projects().name(project.get()).label(labelName).create(input);
+ }
+
@CanIgnoreReturnValue
private SubmitRequirementApi createSubmitRequirement(
String name, String submitExpression, boolean canOverride) throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
index c57d285..016b1e6 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -15,16 +15,24 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
+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.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval;
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.TestAccount;
+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;
+import com.google.gerrit.entities.Permission;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -39,56 +47,141 @@
import org.junit.Test;
public class DeleteVoteIT extends AbstractDaemonTest {
+ @Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@Test
- public void deleteVoteOnChange() throws Exception {
- deleteVote(false);
+ public void deleteVoteOnChange_withRemoveLabelPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabelRemoval(LabelId.CODE_REVIEW)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ verifyDeleteVote(false);
}
@Test
- public void deleteVoteOnRevision() throws Exception {
- deleteVote(true);
+ public void deleteVoteOnChange_withRemoveReviewerPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ blockLabelRemoval(LabelId.CODE_REVIEW)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .add(allow(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ verifyDeleteVote(false);
}
- private void deleteVote(boolean onRevisionLevel) throws Exception {
+ @Test
+ public void deleteVoteOnChange_noPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ blockLabelRemoval(LabelId.CODE_REVIEW)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ verifyCannotDeleteVote(false);
+ }
+
+ @Test
+ public void deleteVoteOnRevision_withRemoveLabelPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabelRemoval(LabelId.CODE_REVIEW)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ verifyDeleteVote(true);
+ }
+
+ @Test
+ public void deleteVoteOnRevision_withRemoveReviewerPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ blockLabelRemoval(LabelId.CODE_REVIEW)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .add(allow(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ verifyDeleteVote(true);
+ }
+
+ @Test
+ public void deleteVoteOnRevision_noPermission() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ blockLabelRemoval(LabelId.CODE_REVIEW)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .range(-2, 2))
+ .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+ verifyCannotDeleteVote(true);
+ }
+
+ private void verifyDeleteVote(boolean onRevisionLevel) throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
PushOneCommit.Result r2 = amendChange(r.getChangeId());
- requestScopeOperations.setApiUser(user.id());
+ requestScopeOperations.setApiUser(admin.id());
+ recommend(r.getChangeId());
+
+ TestAccount user2 = accountCreator.user2();
+ requestScopeOperations.setApiUser(user2.id());
recommend(r.getChangeId());
sender.clear();
- String endPoint =
+ String deleteAdminVoteEndPoint =
"/changes/"
+ r.getChangeId()
+ (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "")
+ "/reviewers/"
- + user.id().toString()
+ + admin.id().toString()
+ "/votes/Code-Review";
- RestResponse response = adminRestSession.delete(endPoint);
+ RestResponse response = userRestSession.delete(deleteAdminVoteEndPoint);
response.assertNoContent();
List<FakeEmailSender.Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
FakeEmailSender.Message msg = messages.get(0);
- assertThat(msg.rcpt()).containsExactly(user.getNameEmail());
- assertThat(msg.body()).contains(admin.fullName() + " has removed a vote from this change.");
+ assertThat(msg.rcpt()).containsExactly(admin.getNameEmail(), user2.getNameEmail());
+ assertThat(msg.body()).contains(user.fullName() + " has removed a vote from this change.");
assertThat(msg.body())
- .contains("Removed Code-Review+1 by " + user.fullName() + " <" + user.email() + ">\n");
+ .contains("Removed Code-Review+1 by " + admin.fullName() + " <" + admin.email() + ">\n");
- endPoint =
+ String viewVotesEndPoint =
"/changes/"
+ r.getChangeId()
+ (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "")
+ "/reviewers/"
- + user.id().toString()
+ + admin.id().toString()
+ "/votes";
- response = adminRestSession.get(endPoint);
+ response = userRestSession.get(viewVotesEndPoint);
response.assertOK();
Map<String, Short> m =
@@ -99,14 +192,38 @@
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
ChangeMessageInfo message = Iterables.getLast(c.messages);
- assertThat(message.author._accountId).isEqualTo(admin.id().get());
+ assertThat(message.author._accountId).isEqualTo(user.id().get());
assertThat(message.message)
.isEqualTo(
String.format(
"Removed Code-Review+1 by %s\n",
- AccountTemplateUtil.getAccountTemplate(user.id())));
+ AccountTemplateUtil.getAccountTemplate(admin.id())));
assertThat(getReviewers(c.reviewers.get(REVIEWER)))
- .containsExactlyElementsIn(ImmutableSet.of(admin.id(), user.id()));
+ .containsExactlyElementsIn(ImmutableSet.of(user2.id(), admin.id()));
+ }
+
+ private void verifyCannotDeleteVote(boolean onRevisionLevel) throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+
+ PushOneCommit.Result r2 = amendChange(r.getChangeId());
+
+ requestScopeOperations.setApiUser(admin.id());
+ recommend(r.getChangeId());
+
+ sender.clear();
+ String deleteAdminVoteEndPoint =
+ "/changes/"
+ + r.getChangeId()
+ + (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "")
+ + "/reviewers/"
+ + admin.id().toString()
+ + "/votes/Code-Review";
+
+ RestResponse response = userRestSession.delete(deleteAdminVoteEndPoint);
+ response.assertForbidden();
+
+ assertThat(sender.getMessages()).isEmpty();
}
private Iterable<Account.Id> getReviewers(Collection<AccountInfo> r) {
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 2123ac2..15baa78 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -1262,7 +1262,7 @@
+ c
+ "/comment/"
+ ps1List.get(0).id
- + " \n"
+ + " :\n"
+ "PS1, Line 1: initial\n"
+ "what happened to this?\n"
+ "\n"
@@ -1274,7 +1274,7 @@
+ c
+ "/comment/"
+ ps1List.get(1).id
- + " \n"
+ + " :\n"
+ "PS1, Line 1: boring\n"
+ "Is it that bad?\n"
+ "\n"
@@ -1288,7 +1288,7 @@
+ c
+ "/comment/"
+ ps2List.get(0).id
- + " \n"
+ + " :\n"
+ "PS2, Line 1: initial content\n"
+ "comment 1 on base\n"
+ "\n"
@@ -1300,7 +1300,7 @@
+ c
+ "/comment/"
+ ps2List.get(1).id
- + " \n"
+ + " :\n"
+ "PS2, Line 2: \n"
+ "comment 2 on base\n"
+ "\n"
@@ -1312,7 +1312,7 @@
+ c
+ "/comment/"
+ ps2List.get(2).id
- + " \n"
+ + " :\n"
+ "PS2, Line 1: interesting\n"
+ "better now\n"
+ "\n"
@@ -1324,7 +1324,7 @@
+ c
+ "/comment/"
+ ps2List.get(3).id
- + " \n"
+ + " :\n"
+ "PS2, Line 2: cntent\n"
+ "typo: content\n"
+ "\n"
diff --git a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
index 16617b4..cf1eee0 100644
--- a/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.AccountGroup;
@@ -400,6 +401,111 @@
}
@Test
+ public void watchOwner() throws Exception {
+ String watchedProject = projectOperations.newProject().create().get();
+ requestScopeOperations.setApiUser(user.id());
+
+ // watch keyword in project as user
+ watch(watchedProject, "owner:admin");
+
+ // push a change with keyword -> should trigger email notification
+ requestScopeOperations.setApiUser(admin.id());
+ TestRepository<InMemoryRepository> watchedRepo =
+ cloneProject(Project.nameKey(watchedProject), admin);
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), watchedRepo, "subject", "a.txt", "a1")
+ .to("refs/for/master");
+ r.assertOkStatus();
+
+ // assert email notification for user
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(user.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void watchNonVisibleOwner() throws Exception {
+ String watchedProject = projectOperations.newProject().create().get();
+ requestScopeOperations.setApiUser(user.id());
+
+ // watch keyword in project as user
+ watch(watchedProject, "owner:admin");
+
+ // Verify that 'user' can't see 'admin'
+ assertThatAccountIsNotVisible(admin);
+
+ // push a change with keyword -> should trigger email notification
+ requestScopeOperations.setApiUser(admin.id());
+ TestRepository<InMemoryRepository> watchedRepo =
+ cloneProject(Project.nameKey(watchedProject), admin);
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), watchedRepo, "subject", "a.txt", "a1")
+ .to("refs/for/master");
+ r.assertOkStatus();
+
+ // assert no email notifications for user
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
+ @Test
+ public void watchChangesCommentedBySelf() throws Exception {
+ String watchedProject = projectOperations.newProject().create().get();
+ requestScopeOperations.setApiUser(user.id());
+
+ // user watches all changes that have a comment by themselves
+ watch(watchedProject, "commentby:self");
+
+ // pushing a change as admin should not trigger an email to user
+ requestScopeOperations.setApiUser(admin.id());
+ TestRepository<InMemoryRepository> watchedRepo =
+ cloneProject(Project.nameKey(watchedProject), admin);
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), watchedRepo, "subject", "a.txt", "a1")
+ .to("refs/for/master");
+ r.assertOkStatus();
+ assertThat(sender.getMessages()).isEmpty();
+
+ // commenting by admin should not trigger an email to user
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message = "A Comment";
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+ assertThat(sender.getMessages()).isEmpty();
+
+ // commenting by user matches the project watch, but doesn't send an email to user because
+ // CC_ON_OWN_COMMENTS is false by default, so the user is removed from the TO list, but an email
+ // is sent to the admin user
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ Message m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(admin.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
+
+ // commenting by admin now triggers an email to user because the change has a comment by user
+ // and hence matches the project watch
+ requestScopeOperations.setApiUser(admin.id());
+ gApi.changes().id(r.getChangeId()).current().review(reviewInput);
+ messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ m = messages.get(0);
+ assertThat(m.rcpt()).containsExactly(user.getNameEmail());
+ assertThat(m.body()).contains("Change subject: subject\n");
+ assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+ sender.clear();
+ }
+
+ @Test
public void watchAllProjects() throws Exception {
String anyProject = projectOperations.newProject().create().get();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
index c1a7627..661802e 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -18,11 +18,14 @@
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;
@@ -443,6 +446,73 @@
}
@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)
@@ -540,6 +610,31 @@
}
@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 3175671..d25d833 100644
--- a/javatests/com/google/gerrit/entities/PermissionTest.java
+++ b/javatests/com/google/gerrit/entities/PermissionTest.java
@@ -36,6 +36,7 @@
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();
}
@@ -56,6 +57,7 @@
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();
}
@@ -66,10 +68,22 @@
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);
@@ -82,11 +96,19 @@
}
@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();
}
@@ -103,6 +125,10 @@
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();
@@ -113,6 +139,10 @@
Permission.canBeOnAllProjects(
"refs/heads/*", Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isTrue();
+ assertThat(
+ Permission.canBeOnAllProjects(
+ "refs/heads/*", Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
+ .isTrue();
}
@Test
@@ -126,6 +156,8 @@
.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 7ed236a..f45d33b 100644
--- a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
+++ b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
@@ -48,6 +48,7 @@
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();
@@ -56,6 +57,7 @@
assertThat(diff.removed().messages).isNull();
assertThat(diff.removed().reviewers).isNull();
assertThat(diff.removed().hashtags).isNull();
+ assertThat(diff.removed().removableLabels).isNull();
}
@Test
@@ -315,6 +317,295 @@
}
@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/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
index 3658834..37728f7 100644
--- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
@@ -23,6 +23,8 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountResolver.Result;
import com.google.gerrit.server.account.AccountResolver.Searcher;
import com.google.gerrit.server.account.AccountResolver.StringSearcher;
@@ -35,6 +37,8 @@
import org.junit.Test;
public class AccountResolverTest {
+ private final CurrentUser user = new AnonymousUser();
+
private static class TestSearcher extends StringSearcher {
private final String pattern;
private final boolean shortCircuit;
@@ -282,7 +286,7 @@
AccountResolver resolver = newAccountResolver();
assertThat(
new UnresolvableAccountException(
- resolver.new Result("foo", ImmutableList.of(), ImmutableList.of())))
+ resolver.new Result("foo", ImmutableList.of(), ImmutableList.of(), user)))
.hasMessageThat()
.isEqualTo("Account 'foo' not found");
}
@@ -292,7 +296,7 @@
AccountResolver resolver = newAccountResolver();
UnresolvableAccountException e =
new UnresolvableAccountException(
- resolver.new Result("self", ImmutableList.of(), ImmutableList.of()));
+ resolver.new Result("self", ImmutableList.of(), ImmutableList.of(), user));
assertThat(e.isSelf()).isTrue();
assertThat(e).hasMessageThat().isEqualTo("Resolving account 'self' requires login");
}
@@ -302,7 +306,7 @@
AccountResolver resolver = newAccountResolver();
UnresolvableAccountException e =
new UnresolvableAccountException(
- resolver.new Result("me", ImmutableList.of(), ImmutableList.of()));
+ resolver.new Result("me", ImmutableList.of(), ImmutableList.of(), user));
assertThat(e.isSelf()).isTrue();
assertThat(e).hasMessageThat().isEqualTo("Resolving account 'me' requires login");
}
@@ -314,7 +318,10 @@
new UnresolvableAccountException(
resolver
.new Result(
- "foo", ImmutableList.of(newAccount(3), newAccount(1)), ImmutableList.of())))
+ "foo",
+ ImmutableList.of(newAccount(3), newAccount(1)),
+ ImmutableList.of(),
+ user)))
.hasMessageThat()
.isEqualTo(
"Account 'foo' is ambiguous (at most 3 shown):\n1: Anonymous Name (1)\n3: Anonymous Name (3)");
@@ -329,7 +336,8 @@
.new Result(
"foo",
ImmutableList.of(),
- ImmutableList.of(newInactiveAccount(3), newInactiveAccount(1)))))
+ ImmutableList.of(newInactiveAccount(3), newInactiveAccount(1)),
+ user)))
.hasMessageThat()
.isEqualTo(
"Account 'foo' only matches inactive accounts. To use an inactive account, retry"
@@ -352,10 +360,11 @@
Supplier<Predicate<AccountState>> visibilitySupplier,
Predicate<AccountState> activityPredicate)
throws Exception {
- return newAccountResolver().searchImpl(input, searchers, visibilitySupplier, activityPredicate);
+ return newAccountResolver()
+ .searchImpl(input, searchers, user, visibilitySupplier, activityPredicate);
}
- private static AccountResolver newAccountResolver() {
+ private AccountResolver newAccountResolver() {
return new AccountResolver(null, null, null, null, null, null, null, null, "Anonymous Name");
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 61b5e55..9cd002e 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -825,7 +825,8 @@
ImmutableList.of(", ", ":\"", ",", "!@#$%^\0&*):\" \n: \r\"#$@,. :");
for (String strangeTag : strangeTags) {
Change c = newChange();
- CurrentUser otherUserAsOwner = userFactory.runAs(null, changeOwner.getAccountId(), otherUser);
+ CurrentUser otherUserAsOwner =
+ userFactory.runAs(/* remotePeer= */ null, changeOwner.getAccountId(), otherUser);
ChangeUpdate update = newUpdate(c, otherUserAsOwner);
update.putApproval(LabelId.CODE_REVIEW, (short) 2);
update.setTag(strangeTag);
diff --git a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
index b53de89..25f2f98 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -354,7 +354,8 @@
@Test
public void realUser() throws Exception {
Change c = newChange();
- CurrentUser ownerAsOtherUser = userFactory.runAs(null, otherUserId, changeOwner);
+ CurrentUser ownerAsOtherUser =
+ userFactory.runAs(/* remotePeer= */ null, otherUserId, changeOwner);
ChangeUpdate update = newUpdate(c, ownerAsOtherUser);
update.setChangeMessage("Message on behalf of other user");
update.commit();
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 5e6803e..527e78e 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -399,7 +399,9 @@
IdentifiedUser impersonatedChangeOwner =
this.userFactory.runAs(
- null, changeOwner.getAccountId(), requireNonNull(otherUser).getRealUser());
+ /* remotePeer= */ null,
+ changeOwner.getAccountId(),
+ requireNonNull(otherUser).getRealUser());
ChangeUpdate impersonatedChangeMessageUpdate = newUpdate(c, impersonatedChangeOwner);
impersonatedChangeMessageUpdate.setChangeMessage("Other comment on behalf of");
impersonatedChangeMessageUpdate.commit();
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts
index ce9ce2d..e3b3ad4 100644
--- a/polygerrit-ui/app/api/diff.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -201,7 +201,7 @@
syntax_highlighting?: boolean;
tab_size: number;
font_size: number;
- // TODO: Missing documentation
+ // Hides the FILE and LOST diff rows. Default is TRUE.
show_file_comment_button?: boolean;
line_wrapping?: boolean;
}
diff --git a/polygerrit-ui/app/constants/constants.ts b/polygerrit-ui/app/constants/constants.ts
index 89c7622..f915432 100644
--- a/polygerrit-ui/app/constants/constants.ts
+++ b/polygerrit-ui/app/constants/constants.ts
@@ -319,6 +319,4 @@
export const RELOAD_DASHBOARD_INTERVAL_MS = 10 * 1000;
-export const SHOWN_ITEMS_COUNT = 25;
-
export const WAITING = 'Waiting';
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 d3562f2..893a997 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
@@ -10,7 +10,6 @@
import {GrCreateGroupDialog} from '../gr-create-group-dialog/gr-create-group-dialog';
import {fireTitleChange} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
-import {SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
import {LitElement, PropertyValues, css, html} from 'lit';
@@ -43,21 +42,19 @@
/**
* Offset of currently visible query results.
*/
- @state() private offset = 0;
+ @state() offset = 0;
- @state() private hasNewGroupName = false;
+ @state() hasNewGroupName = false;
- @state() private createNewCapability = false;
+ @state() createNewCapability = false;
- // private but used in test
@state() groups: GroupInfo[] = [];
- @state() private groupsPerPage = 25;
+ @state() groupsPerPage = 25;
- // private but used in test
@state() loading = true;
- @state() private filter = '';
+ @state() filter = '';
private readonly restApiService = getAppContext().restApiService;
@@ -108,7 +105,7 @@
</tbody>
<tbody class=${this.loading ? 'loading' : ''}>
${this.groups
- .slice(0, SHOWN_ITEMS_COUNT)
+ .slice(0, this.groupsPerPage)
.map(group => this.renderGroupList(group))}
</tbody>
</table>
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 e9b7ea0..fe5aa22 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
@@ -15,7 +15,6 @@
import {GerritView} from '../../../services/router/router-model';
import {GrListView} from '../../shared/gr-list-view/gr-list-view';
import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
-import {SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {fixture, html, assert} from '@open-wc/testing';
import {AdminChildView, AdminViewState} from '../../../models/views/admin';
@@ -117,7 +116,9 @@
});
test('groups', () => {
- assert.equal(element.groups.slice(0, SHOWN_ITEMS_COUNT).length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.groupsPerPage);
});
test('maybeOpenCreateModal', async () => {
@@ -145,7 +146,9 @@
});
test('groups', () => {
- assert.equal(element.groups.slice(0, SHOWN_ITEMS_COUNT).length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.groupsPerPage);
});
});
diff --git a/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts b/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts
index 383b4a7..944cd7a 100644
--- a/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list.ts
@@ -9,7 +9,6 @@
import {getAppContext} from '../../../services/app-context';
import {ErrorCallback} from '../../../api/rest';
import {encodeURL, getBaseUrl} from '../../../utils/url-util';
-import {SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
import {LitElement, PropertyValues, css, html} from 'lit';
@@ -34,17 +33,15 @@
/**
* Offset of currently visible query results.
*/
- @state() private offset = 0;
+ @state() offset = 0;
- // private but used in test
@state() plugins?: PluginInfoWithName[];
- @state() private pluginsPerPage = 25;
+ @state() pluginsPerPage = 25;
- // private but used in test
@state() loading = true;
- @state() private filter = '';
+ @state() filter = '';
private readonly restApiService = getAppContext().restApiService;
@@ -107,7 +104,7 @@
return html`
<tbody>
${this.plugins
- ?.slice(0, SHOWN_ITEMS_COUNT)
+ ?.slice(0, this.pluginsPerPage)
.map(plugin => this.renderPluginList(plugin))}
</tbody>
`;
diff --git a/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list_test.ts b/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list_test.ts
index 4057e52..34c88be 100644
--- a/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-plugin-list/gr-plugin-list_test.ts
@@ -17,7 +17,6 @@
import {PluginInfo} from '../../../types/common';
import {GerritView} from '../../../services/router/router-model';
import {PageErrorEvent} from '../../../types/events';
-import {SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {fixture, html, assert} from '@open-wc/testing';
import {AdminChildView, AdminViewState} from '../../../models/views/admin';
@@ -334,7 +333,9 @@
});
test('plugins', () => {
- assert.equal(element.plugins!.slice(0, SHOWN_ITEMS_COUNT).length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.pluginsPerPage);
});
});
@@ -348,7 +349,9 @@
});
test('plugins', () => {
- assert.equal(element.plugins!.slice(0, SHOWN_ITEMS_COUNT).length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.pluginsPerPage);
});
});
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 7eef7a4..981cbe4 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
@@ -25,7 +25,6 @@
import {firePageError} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
import {ErrorCallback} from '../../../api/rest';
-import {SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {formStyles} from '../../../styles/gr-form-styles';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -51,36 +50,30 @@
@property({type: Object})
params?: RepoViewState;
- // private but used in test
@state() detailType?: RepoDetailView.BRANCHES | RepoDetailView.TAGS;
- // private but used in test
@state() isOwner = false;
- @state() private loggedIn = false;
+ @state() loggedIn = false;
- @state() private offset = 0;
+ @state() offset = 0;
- // private but used in test
@state() repo?: RepoName;
- // private but used in test
@state() items?: BranchInfo[] | TagInfo[];
- @state() private readonly itemsPerPage = 25;
+ @state() readonly itemsPerPage = 25;
- @state() private loading = true;
+ @state() loading = true;
- @state() private filter?: string;
+ @state() filter?: string;
- @state() private refName?: GitRef;
+ @state() refName?: GitRef;
- @state() private newItemName = false;
+ @state() newItemName = false;
- // private but used in test
@state() isEditing = false;
- // private but used in test
@state() revisedRef?: GitRef;
private readonly restApiService = getAppContext().restApiService;
@@ -185,7 +178,7 @@
</tbody>
<tbody class=${this.loading ? 'loading' : ''}>
${this.items
- ?.slice(0, SHOWN_ITEMS_COUNT)
+ ?.slice(0, this.itemsPerPage)
.map((item, index) => this.renderItemList(item, index))}
</tbody>
</table>
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 ec1bb82..9b9cf29 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
@@ -32,7 +32,6 @@
import {PageErrorEvent} from '../../../types/events';
import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
import {GrListView} from '../../shared/gr-list-view/gr-list-view';
-import {SHOWN_ITEMS_COUNT} from '../../../constants/constants';
import {fixture, html, assert} from '@open-wc/testing';
import {RepoDetailView} from '../../../models/views/repo';
@@ -2391,7 +2390,9 @@
});
test('items', () => {
- assert.equal(element.items!.slice(0, SHOWN_ITEMS_COUNT)!.length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.itemsPerPage);
});
});
@@ -2411,7 +2412,9 @@
});
test('items', () => {
- assert.equal(element.items!.slice(0, SHOWN_ITEMS_COUNT)!.length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.itemsPerPage);
});
});
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 2fd7e79..055cb30 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
@@ -8,7 +8,7 @@
import '../gr-create-repo-dialog/gr-create-repo-dialog';
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 {RepoState} from '../../../constants/constants';
import {fireTitleChange} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
import {tableStyles} from '../../../styles/gr-table-styles';
@@ -39,23 +39,18 @@
@property({type: Object})
params?: AdminViewState;
- // private but used in test
@state() offset = 0;
- @state() private newRepoName = false;
+ @state() newRepoName = false;
- @state() private createNewCapability = false;
+ @state() createNewCapability = false;
- // private but used in test
@state() repos: ProjectInfoWithName[] = [];
- // private but used in test
@state() reposPerPage = 25;
- // private but used in test
@state() loading = true;
- // private but used in test
@state() filter = '';
private readonly restApiService = getAppContext().restApiService;
@@ -147,7 +142,7 @@
}
private renderRepoList() {
- const shownRepos = this.repos.slice(0, SHOWN_ITEMS_COUNT);
+ const shownRepos = this.repos.slice(0, this.reposPerPage);
return shownRepos.map(item => this.renderRepo(item));
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list_test.ts b/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list_test.ts
index 5b65942..65f5a8c 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list_test.ts
@@ -17,7 +17,7 @@
ProjectInfoWithName,
RepoName,
} from '../../../types/common';
-import {RepoState, SHOWN_ITEMS_COUNT} from '../../../constants/constants';
+import {RepoState} from '../../../api/rest-api';
import {GerritView} from '../../../services/router/router-model';
import {GrDialog} from '../../shared/gr-dialog/gr-dialog';
import {GrListView} from '../../shared/gr-list-view/gr-list-view';
@@ -614,7 +614,9 @@
});
test('shownRepos', () => {
- assert.equal(element.repos.slice(0, SHOWN_ITEMS_COUNT).length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.reposPerPage);
});
test('maybeOpenCreateModal', () => {
@@ -645,7 +647,9 @@
});
test('shownRepos', () => {
- assert.equal(element.repos.slice(0, SHOWN_ITEMS_COUNT).length, 25);
+ const table = queryAndAssert(element, 'table');
+ const rows = table.querySelectorAll('tr.table');
+ assert.equal(rows.length, element.reposPerPage);
});
});
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
index 8d6d89a..3599224 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
@@ -25,7 +25,7 @@
RepoState,
SubmitType,
} from '../../../constants/constants';
-import {hasOwnProperty} from '../../../utils/common-util';
+import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
import {firePageError, fireTitleChange} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
import {WebLinkInfo} from '../../../types/diff';
@@ -36,8 +36,9 @@
import {sharedStyles} from '../../../styles/shared-styles';
import {BindValueChangeEvent} from '../../../types/events';
import {deepClone} from '../../../utils/deep-util';
-import {LitElement, PropertyValues, css, html} from 'lit';
+import {LitElement, PropertyValues, css, html, nothing} from 'lit';
import {customElement, property, state} from 'lit/decorators.js';
+import {when} from 'lit/directives/when.js';
import {subscribe} from '../../lit/subscription-controller';
import {createSearchUrl} from '../../../models/views/search';
import {userModelToken} from '../../../models/user/user-model';
@@ -150,16 +151,6 @@
color: var(--deemphasized-text-color);
content: ' *';
}
- .loading,
- .hide {
- display: none;
- }
- #loading.loading {
- display: block;
- }
- #loading:not(.loading) {
- display: none;
- }
#options .repositorySettings {
display: none;
}
@@ -187,49 +178,48 @@
>
</div>
</div>
- <div id="loading" class=${this.loading ? 'loading' : ''}>
- Loading...
- </div>
- <div id="loadedContent" class=${this.loading ? 'loading' : ''}>
- ${this.renderDownloadCommands()}
- <h2
- id="configurations"
- class="heading-2 ${configChanged ? 'edited' : ''}"
- >
- Configurations
- </h2>
- <div id="form">
- <fieldset>
- ${this.renderDescription()} ${this.renderRepoOptions()}
- ${this.renderPluginConfig()}
- <gr-button
- ?disabled=${this.readOnly || !configChanged}
- @click=${this.handleSaveRepoConfig}
- >Save changes</gr-button
- >
- </fieldset>
- <gr-endpoint-decorator name="repo-config">
- <gr-endpoint-param
- name="repoName"
- .value=${this.repo}
- ></gr-endpoint-param>
- <gr-endpoint-param
- name="readOnly"
- .value=${this.readOnly}
- ></gr-endpoint-param>
- </gr-endpoint-decorator>
- </div>
- </div>
+ ${when(
+ this.loading || !this.repoConfig,
+ () => html`<div id="loading">Loading...</div>`,
+ () => html`<div id="loadedContent">
+ ${this.renderDownloadCommands()}
+ <h2
+ id="configurations"
+ class="heading-2 ${configChanged ? 'edited' : ''}"
+ >
+ Configurations
+ </h2>
+ <div id="form">
+ <fieldset>
+ ${this.renderDescription()} ${this.renderRepoOptions()}
+ ${this.renderPluginConfig()}
+ <gr-button
+ ?disabled=${this.readOnly || !configChanged}
+ @click=${this.handleSaveRepoConfig}
+ >Save changes</gr-button
+ >
+ </fieldset>
+ <gr-endpoint-decorator name="repo-config">
+ <gr-endpoint-param
+ name="repoName"
+ .value=${this.repo}
+ ></gr-endpoint-param>
+ <gr-endpoint-param
+ name="readOnly"
+ .value=${this.readOnly}
+ ></gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </div>
+ </div>`
+ )}
</div>
`;
}
private renderDownloadCommands() {
+ if (!this.schemes.length) return nothing;
return html`
- <div
- id="downloadContent"
- class=${!this.schemes || !this.schemes.length ? 'hide' : ''}
- >
+ <div id="downloadContent">
<h2 id="download" class="heading-2">Download</h2>
<fieldset>
<gr-download-commands
@@ -252,6 +242,7 @@
}
private renderDescription() {
+ assertIsDefined(this.repoConfig, 'repoConfig');
return html`
<h3 id="Description" class="heading-3">Description</h3>
<fieldset>
@@ -263,7 +254,7 @@
rows="4"
monospace
?disabled=${this.readOnly}
- .text=${this.repoConfig?.description ?? ''}
+ .text=${this.repoConfig.description ?? ''}
@text-changed=${this.handleDescriptionTextChanged}
></gr-textarea>
</fieldset>
@@ -725,8 +716,9 @@
private renderPluginConfig() {
const pluginData = this.computePluginData();
+ if (!pluginData.length) return nothing;
return html` <div
- class="pluginConfig ${!pluginData || !pluginData.length ? 'hide' : ''}"
+ class="pluginConfig"
@plugin-config-changed=${this.handlePluginConfigChanged}
>
<h3 class="heading-3">Plugins</h3>
@@ -762,6 +754,12 @@
// private but used in test
async loadRepo() {
if (!this.repo) return Promise.resolve();
+ this.repoConfig = undefined;
+ this.originalConfig = undefined;
+ this.loading = true;
+ this.weblinks = [];
+ this.schemesObj = undefined;
+ this.readOnly = true;
const promises = [];
@@ -1121,6 +1119,7 @@
private handleDescriptionTextChanged(e: BindValueChangeEvent) {
if (!this.repoConfig || this.loading) return;
+ if (this.repoConfig.description === e.detail.value) return;
this.repoConfig = {
...this.repoConfig,
description: e.detail.value,
@@ -1130,6 +1129,7 @@
private handleStateSelectBindValueChanged(e: BindValueChangeEvent) {
if (!this.repoConfig || this.loading) return;
+ if (this.repoConfig.state === e.detail.value) return;
this.repoConfig = {
...this.repoConfig,
state: e.detail.value as RepoState,
@@ -1139,6 +1139,7 @@
private handleSubmitTypeSelectBindValueChanged(e: BindValueChangeEvent) {
if (!this.repoConfig || this.loading) return;
+ if (this.repoConfig.submit_type === e.detail.value) return;
this.repoConfig = {
...this.repoConfig,
submit_type: e.detail.value as SubmitType,
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
index c013c9e..4deb99a 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
@@ -157,14 +157,17 @@
element = await fixture(html`<gr-repo></gr-repo>`);
});
- test('render', () => {
+ test('render', async () => {
+ element.repo = REPO as RepoName;
+ await element.loadRepo();
+ await element.updateComplete;
// prettier and shadowDom assert do not agree about span.title wrapping
assert.shadowDom.equal(
element,
/* prettier-ignore */ /* HTML */ `
<div class="gr-form-styles main read-only">
<div class="info">
- <h1 class="heading-1" id="Title"></h1>
+ <h1 class="heading-1" id="Title">test-repo</h1>
<hr />
<div>
<a href="">
@@ -178,7 +181,7 @@
Browse
</gr-button>
</a>
- <a href="">
+ <a href="/q/project:test-repo">
<gr-button
aria-disabled="false"
link=""
@@ -190,15 +193,7 @@
</a>
</div>
</div>
- <div class="loading" id="loading">Loading...</div>
- <div class="loading" id="loadedContent">
- <div class="hide" id="downloadContent">
- <h2 class="heading-2" id="download">Download</h2>
- <fieldset>
- <gr-download-commands id="downloadCommands">
- </gr-download-commands>
- </fieldset>
- </div>
+ <div id="loadedContent">
<h2 class="heading-2" id="configurations">Configurations</h2>
<div id="form">
<fieldset>
@@ -266,7 +261,7 @@
</span>
</section>
<section
- class="repositorySettings"
+ class="repositorySettings showConfig"
id="enableSignedPushSettings"
>
<span class="title"> Enable signed push </span>
@@ -277,7 +272,7 @@
</span>
</section>
<section
- class="repositorySettings"
+ class="repositorySettings showConfig"
id="requireSignedPushSettings"
>
<span class="title"> Require signed push </span>
@@ -379,9 +374,6 @@
</span>
</section>
</fieldset>
- <div class="hide pluginConfig">
- <h3 class="heading-3">Plugins</h3>
- </div>
<gr-button
aria-disabled="true"
disabled=""
@@ -398,7 +390,51 @@
</div>
</div>
</div>
- `
+ `,
+ {ignoreTags: ['option']}
+ );
+ });
+
+ test('render loading', async () => {
+ element.repo = REPO as RepoName;
+ element.loading = true;
+ await element.updateComplete;
+ // prettier and shadowDom assert do not agree about span.title wrapping
+ assert.shadowDom.equal(
+ element,
+ /* prettier-ignore */ /* HTML */ `
+ <div class="gr-form-styles main read-only">
+ <div class="info">
+ <h1 class="heading-1" id="Title">test-repo</h1>
+ <hr />
+ <div>
+ <a href="">
+ <gr-button
+ aria-disabled="true"
+ disabled=""
+ link=""
+ role="button"
+ tabindex="-1"
+ >
+ Browse
+ </gr-button>
+ </a>
+ <a href="/q/project:test-repo">
+ <gr-button
+ aria-disabled="false"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ View Changes
+ </gr-button>
+ </a>
+ </div>
+ </div>
+ <div id="loading">Loading...</div>
+ </div>
+ `,
+ {ignoreTags: ['option']}
);
});
@@ -451,55 +487,22 @@
assert.isTrue(requestUpdateStub.called);
});
- test('loading displays before repo config is loaded', () => {
- assert.isTrue(
- queryAndAssert<HTMLDivElement>(element, '#loading').classList.contains(
- 'loading'
- )
- );
- assert.isFalse(
- getComputedStyle(queryAndAssert<HTMLDivElement>(element, '#loading'))
- .display === 'none'
- );
- assert.isTrue(
- queryAndAssert<HTMLDivElement>(
- element,
- '#loadedContent'
- ).classList.contains('loading')
- );
- assert.isTrue(
- getComputedStyle(
- queryAndAssert<HTMLDivElement>(element, '#loadedContent')
- ).display === 'none'
- );
- });
-
- test('download commands visibility', async () => {
- element.loading = false;
- await element.updateComplete;
- assert.isTrue(
- queryAndAssert<HTMLDivElement>(
- element,
- '#downloadContent'
- ).classList.contains('hide')
- );
- assert.isTrue(
- getComputedStyle(
- queryAndAssert<HTMLDivElement>(element, '#downloadContent')
- ).display === 'none'
- );
+ test('render download commands', async () => {
+ element.repo = REPO as RepoName;
+ await element.loadRepo();
element.schemesObj = SCHEMES;
await element.updateComplete;
- assert.isFalse(
- queryAndAssert<HTMLDivElement>(
- element,
- '#downloadContent'
- ).classList.contains('hide')
- );
- assert.isFalse(
- getComputedStyle(
- queryAndAssert<HTMLDivElement>(element, '#downloadContent')
- ).display === 'none'
+ const content = queryAndAssert<HTMLDivElement>(element, '#downloadContent');
+ assert.dom.equal(
+ content,
+ /* HTML */ `
+ <div id="downloadContent">
+ <h2 class="heading-2" id="download">Download</h2>
+ <fieldset>
+ <gr-download-commands id="downloadCommands"></gr-download-commands>
+ </fieldset>
+ </div>
+ `
);
});
@@ -715,9 +718,9 @@
Promise.resolve(new Response())
);
- const button = queryAll<GrButton>(element, 'gr-button')[2];
-
await element.loadRepo();
+
+ const button = queryAll<GrButton>(element, 'gr-button')[2];
assert.isTrue(button.hasAttribute('disabled'));
assert.isFalse(
queryAndAssert<HTMLHeadingElement>(
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index c0ed3b3..d4627e2 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1256,7 +1256,14 @@
flatten
down-arrow
class="showCopyLinkDialogButton"
- @click=${() => this.copyLinksDropdown?.toggleDropdown()}
+ @click=${(e: MouseEvent) => {
+ // We don't want to handle clicks on the star or the <a> link.
+ // Calling `stopPropagation()` from the click handler of <a> is not an
+ // option, because then the click does not reach the top-level page.js
+ // click handler and would result is a full page reload.
+ if ((e.target as HTMLElement)?.nodeName !== 'GR-BUTTON') return;
+ this.copyLinksDropdown?.toggleDropdown();
+ }}
><gr-change-star
id="changeStar"
.change=${this.change}
@@ -1267,10 +1274,7 @@
<a
class="changeNumber"
aria-label=${`Change ${this.change?._number}`}
- @click=${(e: MouseEvent) => {
- fireReload(this, true);
- e.stopPropagation();
- }}
+ href=${ifDefined(this.computeChangeUrl(true))}
>${this.change?._number}</a
>
</gr-button>
@@ -2075,9 +2079,7 @@
// Private but used in tests.
viewStateChanged() {
- // viewState is set by gr-router in handleChangeRoute method and is never
- // set to undefined
- assertIsDefined(this.viewState, 'viewState');
+ if (!this.viewState) return;
if (this.isChangeObsolete()) {
// Tell the app element that we are not going to handle the new change
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
index 0a21da4..efc6efe 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
@@ -8,11 +8,21 @@
import {LitElement, css, html, PropertyValues, nothing} from 'lit';
import {customElement, property, state} from 'lit/decorators.js';
import {RunResult} from '../../models/checks/checks-model';
-import {createFixAction, iconFor} from '../../models/checks/checks-util';
+import {
+ createFixAction,
+ createPleaseFixComment,
+ iconFor,
+} from '../../models/checks/checks-util';
import {modifierPressed} from '../../utils/dom-util';
import './gr-checks-results';
import './gr-hovercard-run';
import {fontStyles} from '../../styles/gr-font-styles';
+import {Action} from '../../api/checks';
+import {assertIsDefined} from '../../utils/common-util';
+import {resolve} from '../../models/dependency';
+import {commentsModelToken} from '../../models/comments/comments-model';
+import {subscribe} from '../lit/subscription-controller';
+import {changeModelToken} from '../../models/change/change-model';
@customElement('gr-diff-check-result')
export class GrDiffCheckResult extends LitElement {
@@ -32,6 +42,13 @@
@state()
isExpandable = false;
+ @state()
+ isOwner = false;
+
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
+ private readonly getCommentsModel = resolve(this, commentsModelToken);
+
static override get styles() {
return [
fontStyles,
@@ -114,6 +131,15 @@
];
}
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getChangeModel().isOwner$,
+ x => (this.isOwner = x)
+ );
+ }
+
override render() {
if (!this.result) return;
const cat = this.result.category.toLowerCase();
@@ -182,14 +208,39 @@
private renderActions() {
if (!this.isExpanded) return nothing;
- return html`<div class="actions">${this.renderFixButton()}</div>`;
+ return html`<div class="actions">
+ ${this.renderPleaseFixButton()}${this.renderShowFixButton()}
+ </div>`;
}
- private renderFixButton() {
+ private renderPleaseFixButton() {
+ if (this.isOwner) return nothing;
+ const action: Action = {
+ name: 'Please Fix',
+ callback: () => {
+ assertIsDefined(this.result, 'result');
+ this.getCommentsModel().saveDraft(createPleaseFixComment(this.result));
+ return undefined;
+ },
+ };
+ return html`
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ .action=${action}
+ ></gr-checks-action>
+ `;
+ }
+
+ private renderShowFixButton() {
const action = createFixAction(this, this.result);
if (!action) return nothing;
return html`
- <gr-checks-action context="diff-fix" .action=${action}></gr-checks-action>
+ <gr-checks-action
+ id="show-fix"
+ context="diff-fix"
+ .action=${action}
+ ></gr-checks-action>
`;
}
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
index 3892c9a..0377e0e 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
@@ -7,6 +7,7 @@
import {fakeRun1} from '../../models/checks/checks-fakes';
import {RunResult} from '../../models/checks/checks-model';
import '../../test/common-test-setup';
+import {queryAndAssert} from '../../utils/common-util';
import './gr-diff-check-result';
import {GrDiffCheckResult} from './gr-diff-check-result';
@@ -50,4 +51,30 @@
`
);
});
+
+ test('renders expanded', async () => {
+ element.result = {...fakeRun1, ...fakeRun1.results?.[2]} as RunResult;
+ element.isExpanded = true;
+ await element.updateComplete;
+
+ const details = queryAndAssert(element, 'div.details');
+ assert.dom.equal(
+ details,
+ /* HTML */ `
+ <div class="details">
+ <gr-result-expanded hidecodepointers=""></gr-result-expanded>
+ <div class="actions">
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ <gr-checks-action
+ id="show-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ </div>
+ </div>
+ `
+ );
+ });
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index a9cbcbd..c74993f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -732,7 +732,7 @@
changedProperties.has('focusLineNum') ||
changedProperties.has('leftSide')
) {
- this.initLineOfInterestAndCursor();
+ this.initCursor();
}
if (
changedProperties.has('change') ||
@@ -774,6 +774,7 @@
.change=${this.change}
.patchRange=${this.patchRange}
.file=${file}
+ .lineOfInterest=${this.getLineOfInterest()}
.path=${this.path}
.projectName=${this.change?.project}
@is-blame-loaded-changed=${this.onIsBlameLoadedChanged}
@@ -1350,13 +1351,6 @@
return {path: fileList[idx]};
}
- // Private but used in tests.
- initLineOfInterestAndCursor() {
- if (!this.diffHost) return;
- this.diffHost.lineOfInterest = this.getLineOfInterest();
- this.initCursor();
- }
-
private updateUrlToDiffUrl(lineNum?: number, leftSide?: boolean) {
if (!this.change) return;
if (!this.patchNum) return;
@@ -1454,10 +1448,10 @@
}
// Private but used in tests.
- handleFileChange(e: CustomEvent) {
- const path = e.detail.value;
+ handleFileChange(e: ValueChangedEvent<string>) {
+ const path: string = e.detail.value;
if (path === this.path) return;
- this.getChangeModel().navigateToDiff(path);
+ this.getChangeModel().navigateToDiff({path});
}
// Private but used in tests.
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index 1999a82..889e9dd 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -1315,7 +1315,7 @@
test('hash is determined from viewState', async () => {
assertIsDefined(element.diffHost);
sinon.stub(element.diffHost, 'reload');
- const initLineStub = sinon.stub(element, 'initLineOfInterestAndCursor');
+ const initLineStub = sinon.stub(element, 'initCursor');
element.focusLineNum = 123;
@@ -1819,7 +1819,7 @@
test('File change should trigger setUrl once', async () => {
element.files = getFilesFromFileList(['file1', 'file2', 'file3']);
- sinon.stub(element, 'initLineOfInterestAndCursor');
+ sinon.stub(element, 'initCursor');
// Load file1
viewModel.setState({
@@ -1842,6 +1842,7 @@
new CustomEvent('value-change', {detail: {value: 'file2'}})
);
assert.isTrue(navToDiffStub.calledOnce);
+ assert.deepEqual(navToDiffStub.lastCall.firstArg, {path: 'file2'});
// This is to mock the param change triggered by above navigate
viewModel.setState({
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 449c041..dad802a 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
@@ -121,30 +121,18 @@
</div>
<slot></slot>
<nav>
- Page ${this.computePage(this.offset, this.itemsPerPage)}
+ Page ${this.computePage()}
<a
id="prevArrow"
- href=${this.computeNavLink(
- this.offset,
- -1,
- this.itemsPerPage,
- this.filter,
- this.path
- )}
+ href=${this.computeNavLink(-1)}
?hidden=${this.loading || this.offset === 0}
>
<gr-icon icon="chevron_left"></gr-icon>
</a>
<a
id="nextArrow"
- href=${this.computeNavLink(
- this.offset,
- 1,
- this.itemsPerPage,
- this.filter,
- this.path
- )}
- ?hidden=${this.hideNextArrow(this.loading, this.items)}
+ href=${this.computeNavLink(1)}
+ ?hidden=${this.hideNextArrow()}
>
<gr-icon icon="chevron_right"></gr-icon>
</a>
@@ -193,19 +181,13 @@
}
// private but used in test
- computeNavLink(
- offset: number,
- direction: number,
- itemsPerPage: number,
- filter: string | undefined,
- path = ''
- ) {
+ computeNavLink(direction: number) {
// Offset could be a string when passed from the router.
- offset = +(offset || 0);
- const newOffset = Math.max(0, offset + itemsPerPage * direction);
- let href = getBaseUrl() + path;
- if (filter) {
- href += '/q/filter:' + encodeURL(filter, false);
+ const offset = +(this.offset || 0);
+ const newOffset = Math.max(0, offset + this.itemsPerPage * direction);
+ let href = getBaseUrl() + (this.path ?? '');
+ if (this.filter) {
+ href += '/q/filter:' + encodeURL(this.filter, false);
}
if (newOffset > 0) {
href += `,${newOffset}`;
@@ -214,11 +196,9 @@
}
// private but used in test
- hideNextArrow(loading?: boolean, items?: unknown[]) {
- if (loading || !items || !items.length) {
- return true;
- }
- const lastPage = items.length < this.itemsPerPage + 1;
+ hideNextArrow() {
+ if (this.loading || !this.items?.length) return true;
+ const lastPage = this.items.length < this.itemsPerPage + 1;
return lastPage;
}
@@ -226,8 +206,8 @@
// to either support a decimal or make it go to the nearest
// whole number (e.g 3).
// private but used in test
- computePage(offset: number, itemsPerPage: number) {
- return offset / itemsPerPage + 1;
+ computePage() {
+ return this.offset / this.itemsPerPage + 1;
}
private handleFilterBindValueChanged(e: BindValueChangeEvent) {
diff --git a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts
index bbbef72..bf94e8f 100644
--- a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view_test.ts
@@ -57,36 +57,25 @@
});
test('computeNavLink', () => {
- const offset = 25;
- const projectsPerPage = 25;
- let filter = 'test';
- const path = '/admin/projects';
+ element.offset = 25;
+ element.itemsPerPage = 25;
+ element.filter = 'test';
+ element.path = '/admin/projects';
stubBaseUrl('');
- assert.equal(
- element.computeNavLink(offset, 1, projectsPerPage, filter, path),
- '/admin/projects/q/filter:test,50'
- );
+ assert.equal(element.computeNavLink(1), '/admin/projects/q/filter:test,50');
- assert.equal(
- element.computeNavLink(offset, -1, projectsPerPage, filter, path),
- '/admin/projects/q/filter:test'
- );
+ assert.equal(element.computeNavLink(-1), '/admin/projects/q/filter:test');
- assert.equal(
- element.computeNavLink(offset, 1, projectsPerPage, undefined, path),
- '/admin/projects,50'
- );
+ element.filter = undefined;
+ assert.equal(element.computeNavLink(1), '/admin/projects,50');
- assert.equal(
- element.computeNavLink(offset, -1, projectsPerPage, undefined, path),
- '/admin/projects'
- );
+ assert.equal(element.computeNavLink(-1), '/admin/projects');
- filter = 'plugins/';
+ element.filter = 'plugins/';
assert.equal(
- element.computeNavLink(offset, 1, projectsPerPage, filter, path),
+ element.computeNavLink(1),
'/admin/projects/q/filter:plugins%252F,50'
);
});
@@ -113,19 +102,19 @@
test('next button', async () => {
element.itemsPerPage = 25;
- let projects = new Array(26);
+ element.items = new Array(26);
+ element.loading = false;
await element.updateComplete;
- let loading;
- assert.isFalse(element.hideNextArrow(loading, projects));
- loading = true;
- assert.isTrue(element.hideNextArrow(loading, projects));
- loading = false;
- assert.isFalse(element.hideNextArrow(loading, projects));
- projects = [];
- assert.isTrue(element.hideNextArrow(loading, projects));
- projects = new Array(4);
- assert.isTrue(element.hideNextArrow(loading, projects));
+ assert.isFalse(element.hideNextArrow());
+ element.loading = true;
+ assert.isTrue(element.hideNextArrow());
+ element.loading = false;
+ assert.isFalse(element.hideNextArrow());
+ element.items = [];
+ assert.isTrue(element.hideNextArrow());
+ element.items = new Array(4);
+ assert.isTrue(element.hideNextArrow());
});
test('prev button', async () => {
@@ -186,20 +175,40 @@
test('next/prev links change when path changes', async () => {
const BRANCHES_PATH = '/path/to/branches';
const TAGS_PATH = '/path/to/tags';
- const computeNavLinkStub = sinon.stub(element, 'computeNavLink');
element.offset = 0;
element.itemsPerPage = 25;
element.filter = '';
element.path = BRANCHES_PATH;
await element.updateComplete;
- assert.equal(computeNavLinkStub.lastCall.args[4], BRANCHES_PATH);
+
+ assert.dom.equal(
+ queryAndAssert(element, 'nav a'),
+ /* HTML */ `
+ <a hidden="" href="${BRANCHES_PATH}" id="prevArrow">
+ <gr-icon icon="chevron_left"> </gr-icon>
+ </a>
+ `
+ );
+
element.path = TAGS_PATH;
await element.updateComplete;
- assert.equal(computeNavLinkStub.lastCall.args[4], TAGS_PATH);
+
+ assert.dom.equal(
+ queryAndAssert(element, 'nav a'),
+ /* HTML */ `
+ <a hidden="" href="${TAGS_PATH}" id="prevArrow">
+ <gr-icon icon="chevron_left"> </gr-icon>
+ </a>
+ `
+ );
});
test('computePage', () => {
- assert.equal(element.computePage(0, 25), 1);
- assert.equal(element.computePage(50, 25), 3);
+ element.offset = 0;
+ element.itemsPerPage = 25;
+ assert.equal(element.computePage(), 1);
+ element.offset = 50;
+ element.itemsPerPage = 25;
+ assert.equal(element.computePage(), 3);
});
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
index 28e83ae..b952a3d 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
@@ -76,6 +76,9 @@
const pairs = this.getLinePairs();
const responsiveMode = getResponsiveMode(this.diffPrefs, this.renderPrefs);
+ const hideFileCommentButton =
+ this.diffPrefs?.show_file_comment_button === false ||
+ this.renderPrefs?.show_file_comment_button === false;
const body = html`
<tbody class=${diffClasses(...extras)}>
${this.renderContextControls()} ${this.renderMoveControls()}
@@ -92,6 +95,7 @@
.tabSize=${this.diffPrefs?.tab_size ?? 2}
.unifiedDiff=${this.isUnifiedDiff()}
.responsiveMode=${responsiveMode}
+ .hideFileCommentButton=${hideFileCommentButton}
>
</gr-diff-row>
`;
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index 6a5933c..ba43eb4 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -14,12 +14,13 @@
Replacement,
RunStatus,
} from '../../api/checks';
-import {PatchSetNumber} from '../../api/rest-api';
+import {PatchSetNumber, RevisionPatchSetNum} from '../../api/rest-api';
+import {CommentSide} from '../../constants/constants';
import {FixSuggestionInfo, FixReplacementInfo} from '../../types/common';
import {OpenFixPreviewEventDetail} from '../../types/events';
import {isDefined} from '../../types/types';
-import {PROVIDED_FIX_ID} from '../../utils/comment-util';
-import {assert, assertNever} from '../../utils/common-util';
+import {PROVIDED_FIX_ID, UnsavedInfo} from '../../utils/comment-util';
+import {assert, assertIsDefined, assertNever} from '../../utils/common-util';
import {fire} from '../../utils/event-util';
import {CheckResult, CheckRun, RunResult} from './checks-model';
@@ -86,6 +87,27 @@
}
}
+function pleaseFixMessage(result: RunResult) {
+ return `Please fix this ${result.category} reported by ${result.checkName}: ${result.summary}
+
+${result.message}`;
+}
+
+export function createPleaseFixComment(result: RunResult): UnsavedInfo {
+ const pointer = result.codePointers?.[0];
+ assertIsDefined(pointer, 'codePointer');
+ return {
+ __unsaved: true,
+ path: pointer.path,
+ patch_set: result.patchset as RevisionPatchSetNum,
+ side: CommentSide.REVISION,
+ line: pointer.range.end_line ?? pointer.range.start_line,
+ range: pointer.range,
+ message: pleaseFixMessage(result),
+ unresolved: true,
+ };
+}
+
export function createFixAction(
target: EventTarget,
result?: RunResult
diff --git a/polygerrit-ui/app/styles/gr-page-nav-styles.ts b/polygerrit-ui/app/styles/gr-page-nav-styles.ts
index b6e8f60..963b2a2 100644
--- a/polygerrit-ui/app/styles/gr-page-nav-styles.ts
+++ b/polygerrit-ui/app/styles/gr-page-nav-styles.ts
@@ -16,13 +16,16 @@
border-top: 1px solid transparent;
display: block;
padding: 0 var(--spacing-xl);
- }
- .navStyles li a {
- display: block;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
+ .navStyles li a {
+ display: block;
+ /* overflow and text-overflow are not inherited, must repeat them */
+ overflow: hidden;
+ text-overflow: ellipsis;
+ }
.navStyles .subsectionItem {
padding-left: var(--spacing-xxl);
}
diff --git a/polygerrit-ui/app/utils/attention-set-util.ts b/polygerrit-ui/app/utils/attention-set-util.ts
index 77834bd..4404e59 100644
--- a/polygerrit-ui/app/utils/attention-set-util.ts
+++ b/polygerrit-ui/app/utils/attention-set-util.ts
@@ -3,7 +3,12 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {AccountInfo, ChangeInfo, ServerInfo} from '../types/common';
+import {
+ AccountInfo,
+ ChangeInfo,
+ DetailedLabelInfo,
+ ServerInfo,
+} from '../types/common';
import {ParsedChangeInfo} from '../types/types';
import {
getAccountTemplate,
@@ -13,6 +18,7 @@
} from './account-util';
import {CommentThread, isMentionedThread, isUnresolved} from './comment-util';
import {hasOwnProperty} from './common-util';
+import {getCodeReviewLabel} from './label-util';
export function canHaveAttention(account?: AccountInfo): boolean {
return !!account?._account_id && !isServiceUser(account);
@@ -101,9 +107,10 @@
/**
* Sort order:
* 1. The user themselves
- * 2. Human users in the attention set.
- * 3. Other human users.
- * 4. Service users.
+ * 2. Users in the attention set first.
+ * 3. Human users first.
+ * 4. Users that have voted first in this order of vote values:
+ * -2, -1, +2, +1, 0 or no vote.
*/
export function sortReviewers(
r1: AccountInfo,
@@ -117,7 +124,22 @@
}
const a1 = hasAttention(r1, change) ? 1 : 0;
const a2 = hasAttention(r2, change) ? 1 : 0;
- const s1 = isServiceUser(r1) ? -2 : 0;
- const s2 = isServiceUser(r2) ? -2 : 0;
- return a2 - a1 + s2 - s1;
+ if (a2 - a1 !== 0) return a2 - a1;
+
+ const s1 = isServiceUser(r1) ? -1 : 0;
+ const s2 = isServiceUser(r2) ? -1 : 0;
+ if (s2 - s1 !== 0) return s2 - s1;
+
+ const crLabel = getCodeReviewLabel(change?.labels ?? {}) as DetailedLabelInfo;
+ let v1 =
+ crLabel?.all?.find(vote => vote._account_id === r1._account_id)?.value ?? 0;
+ let v2 =
+ crLabel?.all?.find(vote => vote._account_id === r2._account_id)?.value ?? 0;
+ // We want negative votes getting a higher score than positive votes, so
+ // we choose 10 as a random number that is higher than all positive votes that
+ // are in use, and then add the absolute value of the vote to that.
+ // So -2 becomes 12.
+ if (v1 < 0) v1 = 10 - v1;
+ if (v2 < 0) v2 = 10 - v2;
+ return v2 - v1;
}
diff --git a/polygerrit-ui/app/utils/attention-set-util_test.ts b/polygerrit-ui/app/utils/attention-set-util_test.ts
index 8092a6e..5bd1924 100644
--- a/polygerrit-ui/app/utils/attention-set-util_test.ts
+++ b/polygerrit-ui/app/utils/attention-set-util_test.ts
@@ -6,9 +6,11 @@
import '../test/common-test-setup';
import {
createAccountDetailWithIdNameAndEmail,
+ createAccountWithId,
createChange,
createComment,
createCommentThread,
+ createParsedChange,
createServerInfo,
} from '../test/test-data-generators';
import {
@@ -22,9 +24,10 @@
getMentionedReason,
getReason,
hasAttention,
+ sortReviewers,
} from './attention-set-util';
import {DefaultDisplayNameConfig} from '../api/rest-api';
-import {AccountsVisibility} from '../constants/constants';
+import {AccountsVisibility, AccountTag} from '../constants/constants';
import {assert} from '@open-wc/testing';
const KERMIT: AccountInfo = {
@@ -101,6 +104,45 @@
assert.equal(getReason(config, OTHER_ACCOUNT, change), 'Added by kermit');
});
+ test('sortReviewers', () => {
+ const a1 = createAccountWithId(1);
+ a1.tags = [AccountTag.SERVICE_USER];
+ const a2 = createAccountWithId(2);
+ a2.tags = [AccountTag.SERVICE_USER];
+ const a3 = createAccountWithId(3);
+ const a4 = createAccountWithId(4);
+ const a5 = createAccountWithId(5);
+ const a6 = createAccountWithId(6);
+ const a7 = createAccountWithId(7);
+
+ const reviewers = [a1, a2, a3, a4, a5, a6, a7];
+ const change = {
+ ...createParsedChange(),
+ attention_set: {'6': {account: a6}},
+ labels: {
+ 'Code-Review': {
+ all: [
+ {...a2, value: 1},
+ {...a4, value: 1},
+ {...a5, value: -1},
+ ],
+ },
+ },
+ };
+ assert.sameOrderedMembers(
+ reviewers.sort((r1, r2) => sortReviewers(r1, r2, change, a7)),
+ [
+ a7, // self
+ a6, // is in the attention set
+ a5, // human user, has voted -1
+ a4, // human user, has voted +1
+ a3, // human user, has not voted
+ a2, // service user, has voted
+ a1, // service user, has not voted
+ ]
+ );
+ });
+
test('getMentionReason', () => {
let comment = {
...createComment(),
diff --git a/resources/com/google/gerrit/server/mail/Comment.soy b/resources/com/google/gerrit/server/mail/Comment.soy
index 98ab4b2..4b621b5 100644
--- a/resources/com/google/gerrit/server/mail/Comment.soy
+++ b/resources/com/google/gerrit/server/mail/Comment.soy
@@ -77,13 +77,8 @@
{for $line, $index in $comment.lines}
{if $index == 0}
{if $comment.startLine != 0}
- {$comment.link}
+ {$comment.link}{sp}:{\n}
{/if}
-
- // Insert a space before the newline so that Gmail does not mistakenly
- // link the following line with the file link. See issue 9201.
- {sp}{\n}
-
{$comment.linePrefix}
{else}
{$comment.linePrefixEmpty}
