Merge "AndPredicate#compare: Remove unnecessary calculation"
diff --git a/.gitignore b/.gitignore
index 53bc9f6..0bbcaba 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,6 +31,8 @@
/infer-out
/local.properties
/node_modules/
+/polygerrit-ui/node_modules/
+/polygerrit-ui/app/node_modules/
/package-lock.json
/plugins/*
/polygerrit-ui/coverage/
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 9a40b27..f2f5a66 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -800,22 +800,6 @@
Users without this access right who are able to upload changes can
still do the revert locally and upload the revert commit as a new change.
-[[category_remove_label]]
-=== Remove Label (Remove Vote)
-
-For every configured label `My-Name` in the project, there is a
-corresponding permission `removeLabel-My-Name` with a range corresponding to
-the defined values. For these values, the users are permitted to remove
-other users' votes from a change.
-
-Change owners can always remove zero or positive votes (even without
-having the `Remove Vote` access right assigned).
-
-Project owners and site administrators can always remove any vote (even
-without having the `Remove Vote` access right assigned).
-
-Users without this access right can still remove their own votes.
-
[[category_remove_reviewer]]
=== Remove Reviewer
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 65275bd..359361c 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -824,26 +824,6 @@
"+2"
]
},
- "removable_labels": {
- "Code-Review": {
- "-1": [
- {
- "_account_id": 1000096,
- "name": "John Doe",
- "email": "john.doe@example.com",
- "username": "jdoe"
- }
- ],
- "+1": [
- {
- "_account_id": 1000097,
- "name": "Jane Roe",
- "email": "jane.roe@example.com",
- "username": "jroe"
- }
- ]
- }
- },
"removable_reviewers": [
{
"_account_id": 1000096,
@@ -7091,13 +7071,6 @@
A map of the permitted labels that maps a label name to the list of
values that are allowed for that label. +
Only set if link:#detailed-labels[detailed labels] are requested.
-|`removable_labels` |optional|
-A map of the removable labels that maps a label name to the map of
-values and reviewers (
-link:rest-api-accounts.html#account-info[AccountInfo] entities)
-that are allowed to be removed from the change. +
-Only set if link:#labels[labels] or
-link:#detailed-labels[detailed labels] are requested.
|`removable_reviewers`|optional|
The reviewers that can be removed by the calling user as a list of
link:rest-api-accounts.html#account-info[AccountInfo] entities. +
@@ -7216,11 +7189,16 @@
Whether the new change should be set to work in progress.
|`base_change` |optional|
A link:#change-id[\{change-id\}] that identifies the base change for a create
-change operation. Mutually exclusive with `base_commit`.
+change operation. +
+Mutually exclusive with `base_commit`. +
+If neither `base_commit` nor `base_change` are set, the target branch tip will
+be used as the parent commit.
|`base_commit` |optional|
A 40-digit hex SHA-1 of the commit which will be the parent commit of the newly
-created change. If set, it must be a merged commit on the destination branch.
-Mutually exclusive with `base_change`.
+created change. If set, it must be a merged commit on the destination branch. +
+Mutually exclusive with `base_change`. +
+If neither `base_commit` nor `base_change` are set, the target branch tip will
+be used as the parent commit.
|`new_branch` |optional, default to `false`|
Allow creating a new branch when set to `true`. Using this option is
only possible for non-merge commits (if the `merge` field is not set).
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index ccf74d1..5b4a9e5 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1393,17 +1393,6 @@
}
}
- protected void assertOnlyRemovableLabel(
- ChangeInfo info, String labelId, String labelValue, TestAccount reviewer) {
- assertThat(info.removableLabels).hasSize(1);
- assertThat(info.removableLabels).containsKey(labelId);
- assertThat(info.removableLabels.get(labelId)).hasSize(1);
- assertThat(info.removableLabels.get(labelId)).containsKey(labelValue);
- assertThat(info.removableLabels.get(labelId).get(labelValue)).hasSize(1);
- assertThat(info.removableLabels.get(labelId).get(labelValue).get(0).email)
- .isEqualTo(reviewer.email());
- }
-
protected void assertPermissions(
Project.NameKey project,
GroupReference groupReference,
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
index 69139ce..b1cd506 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -197,13 +197,8 @@
PermissionRule.Builder rule = newRule(projectConfig, p.group());
rule.setAction(p.action());
rule.setRange(p.min(), p.max());
- String permissionName;
- if (p.isAddPermission()) {
- permissionName =
- p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
- } else {
- permissionName = Permission.forRemoveLabel(p.name());
- }
+ String permissionName =
+ p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
projectConfig.upsertAccessSection(
p.ref(), as -> as.upsertPermission(permissionName).add(rule));
}
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
index 5634c78..9a9a21a 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
@@ -162,34 +162,12 @@
/** Starts a builder for allowing a label permission. */
public static TestLabelPermission.Builder allowLabel(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(true)
- .action(PermissionRule.Action.ALLOW);
+ return TestLabelPermission.builder().name(name).action(PermissionRule.Action.ALLOW);
}
/** Starts a builder for denying a label permission. */
public static TestLabelPermission.Builder blockLabel(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(true)
- .action(PermissionRule.Action.BLOCK);
- }
-
- /** Starts a builder for allowing a remove-label permission. */
- public static TestLabelPermission.Builder allowLabelRemoval(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(false)
- .action(PermissionRule.Action.ALLOW);
- }
-
- /** Starts a builder for denying a remove-label permission. */
- public static TestLabelPermission.Builder blockLabelRemoval(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(false)
- .action(PermissionRule.Action.BLOCK);
+ return TestLabelPermission.builder().name(name).action(PermissionRule.Action.BLOCK);
}
/** Records a label permission to be updated. */
@@ -213,8 +191,6 @@
abstract boolean impersonation();
- abstract boolean isAddPermission();
-
/** Builder for {@link TestLabelPermission}. */
@AutoValue.Builder
public abstract static class Builder {
@@ -232,8 +208,6 @@
abstract Builder max(int max);
- abstract Builder isAddPermission(boolean isAddPermission);
-
/** Sets the minimum and maximum values for the permission. */
public Builder range(int min, int max) {
checkArgument(min != 0 || max != 0, "empty range");
@@ -269,12 +243,6 @@
return TestPermissionKey.builder().name(Permission.forLabel(name));
}
- /** Starts a builder for describing a label removal permission key for deletion. */
- public static TestPermissionKey.Builder labelRemovalPermissionKey(String name) {
- checkLabelName(name);
- return TestPermissionKey.builder().name(Permission.forRemoveLabel(name));
- }
-
/** Starts a builder for describing a capability key for deletion. */
public static TestPermissionKey.Builder capabilityKey(String name) {
return TestPermissionKey.builder().name(name).section(GLOBAL_CAPABILITIES);
diff --git a/java/com/google/gerrit/entities/Permission.java b/java/com/google/gerrit/entities/Permission.java
index d029fad..6d2fa32 100644
--- a/java/com/google/gerrit/entities/Permission.java
+++ b/java/com/google/gerrit/entities/Permission.java
@@ -43,7 +43,6 @@
public static final String FORGE_SERVER = "forgeServerAsCommitter";
public static final String LABEL = "label-";
public static final String LABEL_AS = "labelAs-";
- public static final String REMOVE_LABEL = "removeLabel-";
public static final String OWNER = "owner";
public static final String PUSH = "push";
public static final String PUSH_MERGE = "pushMerge";
@@ -61,7 +60,6 @@
private static final List<String> NAMES_LC;
private static final int LABEL_INDEX;
private static final int LABEL_AS_INDEX;
- private static final int REMOVE_LABEL_INDEX;
static {
NAMES_LC = new ArrayList<>();
@@ -81,7 +79,6 @@
NAMES_LC.add(FORGE_SERVER.toLowerCase());
NAMES_LC.add(LABEL.toLowerCase());
NAMES_LC.add(LABEL_AS.toLowerCase());
- NAMES_LC.add(REMOVE_LABEL.toLowerCase());
NAMES_LC.add(OWNER.toLowerCase());
NAMES_LC.add(PUSH.toLowerCase());
NAMES_LC.add(PUSH_MERGE.toLowerCase());
@@ -96,19 +93,15 @@
LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL);
LABEL_AS_INDEX = NAMES_LC.indexOf(Permission.LABEL_AS.toLowerCase());
- REMOVE_LABEL_INDEX = NAMES_LC.indexOf(Permission.REMOVE_LABEL.toLowerCase());
}
/** Returns true if the name is recognized as a permission name. */
public static boolean isPermission(String varName) {
- return isLabel(varName)
- || isLabelAs(varName)
- || isRemoveLabel(varName)
- || NAMES_LC.contains(varName.toLowerCase());
+ return isLabel(varName) || isLabelAs(varName) || NAMES_LC.contains(varName.toLowerCase());
}
public static boolean hasRange(String varName) {
- return isLabel(varName) || isLabelAs(varName) || isRemoveLabel(varName);
+ return isLabel(varName) || isLabelAs(varName);
}
/** Returns true if the permission name is actually for a review label. */
@@ -121,11 +114,6 @@
return var.startsWith(LABEL_AS) && LABEL_AS.length() < var.length();
}
- /** Returns true if the permission is for impersonated review labels. */
- public static boolean isRemoveLabel(String var) {
- return var.startsWith(REMOVE_LABEL) && REMOVE_LABEL.length() < var.length();
- }
-
/** Returns permission name for the given review label. */
public static String forLabel(String labelName) {
return LABEL + labelName;
@@ -136,19 +124,12 @@
return LABEL_AS + labelName;
}
- /** Returns permission name to remove a label for another user. */
- public static String forRemoveLabel(String labelName) {
- return REMOVE_LABEL + labelName;
- }
-
@Nullable
public static String extractLabel(String varName) {
if (isLabel(varName)) {
return varName.substring(LABEL.length());
} else if (isLabelAs(varName)) {
return varName.substring(LABEL_AS.length());
- } else if (isRemoveLabel(varName)) {
- return varName.substring(REMOVE_LABEL.length());
}
return null;
}
@@ -224,8 +205,6 @@
return LABEL_INDEX;
} else if (isLabelAs(a.getName())) {
return LABEL_AS_INDEX;
- } else if (isRemoveLabel(a.getName())) {
- return REMOVE_LABEL_INDEX;
}
int index = NAMES_LC.indexOf(a.getName().toLowerCase());
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index a865187..40ae2ec 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -105,7 +105,6 @@
public Map<String, ActionInfo> actions;
public Map<String, LabelInfo> labels;
public Map<String, Collection<String>> permittedLabels;
- public Map<String, Map<String, List<AccountInfo>>> removableLabels;
public Collection<AccountInfo> removableReviewers;
public Map<ReviewerState, Collection<AccountInfo>> reviewers;
public Map<ReviewerState, Collection<AccountInfo>> pendingReviewers;
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
index 51c35dc..24182cc 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
@@ -150,17 +150,16 @@
/** Returns {@code null} if nothing has been added to {@code oldCollection} */
@Nullable
private static ImmutableList<?> getAddedForCollection(
- @Nullable Collection<?> oldCollection, Collection<?> newCollection) {
- ImmutableList<?> notInOldCollection = getAdditionsForCollection(oldCollection, newCollection);
+ Collection<?> oldCollection, Collection<?> newCollection) {
+ ImmutableList<?> notInOldCollection = getAdditions(oldCollection, newCollection);
return notInOldCollection.isEmpty() ? null : notInOldCollection;
}
@Nullable
- private static ImmutableList<Object> getAdditionsForCollection(
- @Nullable Collection<?> oldCollection, Collection<?> newCollection) {
- if (oldCollection == null) {
- return ImmutableList.copyOf(newCollection);
- }
+ private static ImmutableList<Object> getAdditions(
+ Collection<?> oldCollection, Collection<?> newCollection) {
+ if (oldCollection == null)
+ return newCollection != null ? ImmutableList.copyOf(newCollection) : null;
Map<Object, List<Object>> duplicatesMap = newCollection.stream().collect(groupingBy(v -> v));
oldCollection.forEach(
@@ -174,18 +173,7 @@
/** Returns {@code null} if nothing has been added to {@code oldMap} */
@Nullable
- private static ImmutableMap<Object, Object> getAddedForMap(
- @Nullable Map<?, ?> oldMap, Map<?, ?> newMap) {
- ImmutableMap<Object, Object> notInOldMap = getAdditionsForMap(oldMap, newMap);
- return notInOldMap.isEmpty() ? null : notInOldMap;
- }
-
- @Nullable
- private static ImmutableMap<Object, Object> getAdditionsForMap(
- @Nullable Map<?, ?> oldMap, Map<?, ?> newMap) {
- if (oldMap == null) {
- return ImmutableMap.copyOf(newMap);
- }
+ private static ImmutableMap<Object, Object> getAddedForMap(Map<?, ?> oldMap, Map<?, ?> newMap) {
ImmutableMap.Builder<Object, Object> additionsBuilder = ImmutableMap.builder();
for (Map.Entry<?, ?> entry : newMap.entrySet()) {
Object added = getAdded(oldMap.get(entry.getKey()), entry.getValue());
@@ -193,7 +181,8 @@
additionsBuilder.put(entry.getKey(), added);
}
}
- return additionsBuilder.build();
+ ImmutableMap<Object, Object> additions = additionsBuilder.build();
+ return additions.isEmpty() ? null : additions;
}
private static Object get(Field field, Object obj) {
diff --git a/java/com/google/gerrit/index/project/ProjectField.java b/java/com/google/gerrit/index/project/ProjectField.java
index 3114b4c..e050f53 100644
--- a/java/com/google/gerrit/index/project/ProjectField.java
+++ b/java/com/google/gerrit/index/project/ProjectField.java
@@ -15,14 +15,12 @@
package com.google.gerrit.index.project;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.index.FieldDef.exact;
-import static com.google.gerrit.index.FieldDef.fullText;
-import static com.google.gerrit.index.FieldDef.prefix;
import static com.google.gerrit.index.FieldDef.storedOnly;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.IndexedField;
import com.google.gerrit.index.RefState;
import com.google.gerrit.index.SchemaUtil;
@@ -38,23 +36,53 @@
.toByteArray(project.getNameKey());
}
- public static final FieldDef<ProjectData, String> NAME =
- exact("name").stored().build(p -> p.getProject().getName());
+ public static final IndexedField<ProjectData, String> NAME_FIELD =
+ IndexedField.<ProjectData>stringBuilder("RepoName")
+ .required()
+ .size(200)
+ .stored()
+ .build(p -> p.getProject().getName());
- public static final FieldDef<ProjectData, String> DESCRIPTION =
- fullText("description").stored().build(p -> p.getProject().getDescription());
+ public static final IndexedField<ProjectData, String>.SearchSpec NAME_SPEC =
+ NAME_FIELD.exact("name");
- public static final FieldDef<ProjectData, String> PARENT_NAME =
- exact("parent_name").build(p -> p.getProject().getParentName());
+ public static final IndexedField<ProjectData, String> DESCRIPTION_FIELD =
+ IndexedField.<ProjectData>stringBuilder("Description")
+ .stored()
+ .build(p -> p.getProject().getDescription());
- public static final FieldDef<ProjectData, Iterable<String>> NAME_PART =
- prefix("name_part").buildRepeatable(p -> SchemaUtil.getNameParts(p.getProject().getName()));
+ public static final IndexedField<ProjectData, String>.SearchSpec DESCRIPTION_SPEC =
+ DESCRIPTION_FIELD.fullText("description");
- public static final FieldDef<ProjectData, String> STATE =
- exact("state").stored().build(p -> p.getProject().getState().name());
+ public static final IndexedField<ProjectData, String> PARENT_NAME_FIELD =
+ IndexedField.<ProjectData>stringBuilder("ParentName")
+ .build(p -> p.getProject().getParentName());
- public static final FieldDef<ProjectData, Iterable<String>> ANCESTOR_NAME =
- exact("ancestor_name").buildRepeatable(ProjectData::getParentNames);
+ public static final IndexedField<ProjectData, String>.SearchSpec PARENT_NAME_SPEC =
+ PARENT_NAME_FIELD.exact("parent_name");
+
+ public static final IndexedField<ProjectData, Iterable<String>> NAME_PART_FIELD =
+ IndexedField.<ProjectData>iterableStringBuilder("NamePart")
+ .size(200)
+ .build(p -> SchemaUtil.getNameParts(p.getProject().getName()));
+
+ public static final IndexedField<ProjectData, Iterable<String>>.SearchSpec NAME_PART_SPEC =
+ NAME_PART_FIELD.prefix("name_part");
+
+ public static final IndexedField<ProjectData, String> STATE_FIELD =
+ IndexedField.<ProjectData>stringBuilder("State")
+ .stored()
+ .build(p -> p.getProject().getState().name());
+
+ public static final IndexedField<ProjectData, String>.SearchSpec STATE_SPEC =
+ STATE_FIELD.exact("state");
+
+ public static final IndexedField<ProjectData, Iterable<String>> ANCESTOR_NAME_FIELD =
+ IndexedField.<ProjectData>iterableStringBuilder("AncestorName")
+ .build(ProjectData::getParentNames);
+
+ public static final IndexedField<ProjectData, Iterable<String>>.SearchSpec ANCESTOR_NAME_SPEC =
+ ANCESTOR_NAME_FIELD.exact("ancestor_name");
/**
* All values of all refs that were used in the course of indexing this document. This covers
diff --git a/java/com/google/gerrit/index/project/ProjectIndex.java b/java/com/google/gerrit/index/project/ProjectIndex.java
index 8687544..0aa7393 100644
--- a/java/com/google/gerrit/index/project/ProjectIndex.java
+++ b/java/com/google/gerrit/index/project/ProjectIndex.java
@@ -31,7 +31,7 @@
@Override
default Predicate<ProjectData> keyPredicate(Project.NameKey nameKey) {
- return new ProjectPredicate(ProjectField.NAME, nameKey.get());
+ return new ProjectPredicate(ProjectField.NAME_SPEC, nameKey.get());
}
Function<ProjectData, Project.NameKey> ENTITY_TO_KEY = (p) -> p.getProject().getNameKey();
diff --git a/java/com/google/gerrit/index/project/ProjectPredicate.java b/java/com/google/gerrit/index/project/ProjectPredicate.java
index 11875ef..0eaf2b6 100644
--- a/java/com/google/gerrit/index/project/ProjectPredicate.java
+++ b/java/com/google/gerrit/index/project/ProjectPredicate.java
@@ -14,12 +14,12 @@
package com.google.gerrit.index.project;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.IndexPredicate;
/** Predicate that is mapped to a field in the project index. */
public class ProjectPredicate extends IndexPredicate<ProjectData> {
- public ProjectPredicate(FieldDef<ProjectData, ?> def, String value) {
+ public ProjectPredicate(SchemaField<ProjectData, ?> def, String value) {
super(def, value);
}
}
diff --git a/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java b/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
index 0619566..ef2c3f5 100644
--- a/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
+++ b/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.index.SchemaUtil.schema;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.SchemaDefinitions;
@@ -31,14 +32,26 @@
static final Schema<ProjectData> V1 =
schema(
/* version= */ 1,
- ProjectField.NAME,
- ProjectField.DESCRIPTION,
- ProjectField.PARENT_NAME,
- ProjectField.NAME_PART,
- ProjectField.ANCESTOR_NAME);
+ ImmutableList.of(
+ ProjectField.NAME_FIELD,
+ ProjectField.DESCRIPTION_FIELD,
+ ProjectField.PARENT_NAME_FIELD,
+ ProjectField.NAME_PART_FIELD,
+ ProjectField.ANCESTOR_NAME_FIELD),
+ ImmutableList.of(
+ ProjectField.NAME_SPEC,
+ ProjectField.DESCRIPTION_SPEC,
+ ProjectField.PARENT_NAME_SPEC,
+ ProjectField.NAME_PART_SPEC,
+ ProjectField.ANCESTOR_NAME_SPEC));
@Deprecated
- static final Schema<ProjectData> V2 = schema(V1, ProjectField.STATE, ProjectField.REF_STATE);
+ static final Schema<ProjectData> V2 =
+ schema(
+ V1,
+ ImmutableList.of(ProjectField.REF_STATE),
+ ImmutableList.of(ProjectField.STATE_FIELD),
+ ImmutableList.of(ProjectField.STATE_SPEC));
// Bump Lucene version requires reindexing
@Deprecated static final Schema<ProjectData> V3 = schema(V2);
diff --git a/java/com/google/gerrit/lucene/LuceneProjectIndex.java b/java/com/google/gerrit/lucene/LuceneProjectIndex.java
index fae854e..911d91f 100644
--- a/java/com/google/gerrit/lucene/LuceneProjectIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneProjectIndex.java
@@ -15,7 +15,7 @@
package com.google.gerrit.lucene;
import static com.google.common.collect.Iterables.getOnlyElement;
-import static com.google.gerrit.index.project.ProjectField.NAME;
+import static com.google.gerrit.index.project.ProjectField.NAME_SPEC;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
@@ -58,14 +58,14 @@
implements ProjectIndex {
private static final String PROJECTS = "projects";
- private static final String NAME_SORT_FIELD = sortFieldName(NAME);
+ private static final String NAME_SORT_FIELD = sortFieldName(NAME_SPEC);
private static Term idTerm(ProjectData projectState) {
return idTerm(projectState.getProject().getNameKey());
}
private static Term idTerm(Project.NameKey nameKey) {
- return QueryBuilder.stringTerm(NAME.getName(), nameKey.get());
+ return QueryBuilder.stringTerm(NAME_SPEC.getName(), nameKey.get());
}
private final GerritIndexWriterConfig indexWriterConfig;
@@ -110,7 +110,7 @@
void add(Document doc, Values<ProjectData> values) {
// Add separate DocValues field for the field that is needed for sorting.
SchemaField<ProjectData, ?> f = values.getField();
- if (f == NAME) {
+ if (f == NAME_SPEC) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(NAME_SORT_FIELD, new BytesRef(value)));
}
@@ -156,7 +156,7 @@
@Nullable
@Override
protected ProjectData fromDocument(Document doc) {
- Project.NameKey nameKey = Project.nameKey(doc.getField(NAME.getName()).stringValue());
+ Project.NameKey nameKey = Project.nameKey(doc.getField(NAME_SPEC.getName()).stringValue());
return projectCache.get().get(nameKey).map(ProjectState::toProjectData).orElse(null);
}
}
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index d26af7b..500bb77 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -688,7 +688,6 @@
!cd.change().isAbandoned()
? labelsJson.permittedLabels(user.getAccountId(), cd)
: ImmutableMap.of();
- out.removableLabels = labelsJson.removableLabels(accountLoader, user, cd);
}
}
diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java
index 06e41ff..cfa15ae 100644
--- a/java/com/google/gerrit/server/change/LabelsJson.java
+++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -36,18 +36,15 @@
import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.SubmitRecord;
-import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.VotingRangeInfo;
import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.DeleteVoteControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -72,12 +69,10 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final PermissionBackend permissionBackend;
- private final DeleteVoteControl deleteVoteControl;
@Inject
- LabelsJson(PermissionBackend permissionBackend, DeleteVoteControl deleteVoteControl) {
+ LabelsJson(PermissionBackend permissionBackend) {
this.permissionBackend = permissionBackend;
- this.deleteVoteControl = deleteVoteControl;
}
/**
@@ -138,45 +133,6 @@
return permitted.asMap();
}
- /**
- * Returns A map of all labels that the provided user has permission to remove.
- *
- * @param accountLoader to load the reviewers' data with.
- * @param user a Gerrit user.
- * @param cd {@link ChangeData} corresponding to a specific gerrit change.
- * @return A Map of {@code labelName} -> {Map of {@code value} -> List of {@link AccountInfo}}
- * that the user can remove votes from.
- */
- Map<String, Map<String, List<AccountInfo>>> removableLabels(
- AccountLoader accountLoader, CurrentUser user, ChangeData cd)
- throws PermissionBackendException {
- if (cd.change().isMerged()) {
- return new HashMap<>();
- }
-
- Map<String, Map<String, List<AccountInfo>>> res = new HashMap<>();
- LabelTypes labelTypes = cd.getLabelTypes();
- for (PatchSetApproval approval : cd.currentApprovals()) {
- Optional<LabelType> labelType = labelTypes.byLabel(approval.labelId());
- if (!labelType.isPresent()) {
- continue;
- }
- if (!deleteVoteControl.testDeleteVotePermissions(
- user, cd.notes(), approval, labelType.get())) {
- continue;
- }
- if (!res.containsKey(approval.label())) {
- res.put(approval.label(), new HashMap<>());
- }
- String labelValue = LabelValue.formatValue(approval.value());
- if (!res.get(approval.label()).containsKey(labelValue)) {
- res.get(approval.label()).put(labelValue, new ArrayList<>());
- }
- res.get(approval.label()).get(labelValue).add(accountLoader.get(approval.accountId()));
- }
- return res;
- }
-
private static void clearOnlyZerosEntries(SetMultimap<String, String> permitted) {
List<String> toClear = Lists.newArrayListWithCapacity(permitted.keySet().size());
for (Map.Entry<String, Collection<String>> e : permitted.asMap().entrySet()) {
@@ -261,10 +217,10 @@
}
}
- private Map<String, Short> currentLabels(@Nullable Account.Id accountId, ChangeData cd) {
+ private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd) {
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa : cd.currentApprovals()) {
- if (accountId == null || psa.accountId().equals(accountId)) {
+ if (psa.accountId().equals(accountId)) {
result.put(psa.label(), psa.value());
}
}
diff --git a/java/com/google/gerrit/server/index/IndexUtils.java b/java/com/google/gerrit/server/index/IndexUtils.java
index 213094e..352d376 100644
--- a/java/com/google/gerrit/server/index/IndexUtils.java
+++ b/java/com/google/gerrit/server/index/IndexUtils.java
@@ -116,9 +116,9 @@
*/
public static Set<String> projectFields(QueryOptions opts) {
Set<String> fs = opts.fields();
- return fs.contains(ProjectField.NAME.getName())
+ return fs.contains(ProjectField.NAME_SPEC.getName())
? fs
- : Sets.union(fs, ImmutableSet.of(ProjectField.NAME.getName()));
+ : Sets.union(fs, ImmutableSet.of(ProjectField.NAME_SPEC.getName()));
}
private IndexUtils() {
diff --git a/java/com/google/gerrit/server/index/project/StalenessChecker.java b/java/com/google/gerrit/server/index/project/StalenessChecker.java
index 9c44c00..9f6bb31 100644
--- a/java/com/google/gerrit/server/index/project/StalenessChecker.java
+++ b/java/com/google/gerrit/server/index/project/StalenessChecker.java
@@ -40,7 +40,7 @@
*/
public class StalenessChecker {
private static final ImmutableSet<String> FIELDS =
- ImmutableSet.of(ProjectField.NAME.getName(), ProjectField.REF_STATE.getName());
+ ImmutableSet.of(ProjectField.NAME_SPEC.getName(), ProjectField.REF_STATE.getName());
private final ProjectCache projectCache;
private final ProjectIndexCollection indexes;
diff --git a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java b/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
deleted file mode 100644
index 622f0cf..0000000
--- a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
+++ /dev/null
@@ -1,155 +0,0 @@
-// Copyright (C) 2022 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.permissions;
-
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
-import static java.util.Objects.requireNonNull;
-
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.server.util.LabelVote;
-
-/** Abstract permission representing a label. */
-public abstract class AbstractLabelPermission implements ChangePermissionOrLabel {
- public enum ForUser {
- SELF,
- ON_BEHALF_OF
- }
-
- protected final ForUser forUser;
- protected final String name;
-
- /**
- * Construct a reference to an abstract label permission.
- *
- * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
- * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
- */
- public AbstractLabelPermission(ForUser forUser, String name) {
- this.forUser = requireNonNull(forUser, "ForUser");
- this.name = LabelType.checkName(name);
- }
-
- /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
- public ForUser forUser() {
- return forUser;
- }
-
- /** Returns name of the label, e.g. {@code "Code-Review"}. */
- public String label() {
- return name;
- }
-
- protected abstract String permissionPrefix();
-
- protected String permissionName() {
- if (forUser == ON_BEHALF_OF) {
- return permissionPrefix() + "As";
- }
- return permissionPrefix();
- }
-
- @Override
- public final String describeForException() {
- if (forUser == ON_BEHALF_OF) {
- return permissionPrefix() + " on behalf of " + name;
- }
- return permissionPrefix() + " " + name;
- }
-
- @Override
- public final int hashCode() {
- return (permissionPrefix() + name).hashCode();
- }
-
- @Override
- @SuppressWarnings("EqualsGetClass")
- public final boolean equals(Object other) {
- if (this.getClass().isAssignableFrom(other.getClass())) {
- AbstractLabelPermission b = (AbstractLabelPermission) other;
- return forUser == b.forUser && name.equals(b.name);
- }
- return false;
- }
-
- @Override
- public final String toString() {
- return permissionName() + "[" + name + ']';
- }
-
- /** A {@link AbstractLabelPermission} at a specific value. */
- public abstract static class WithValue implements ChangePermissionOrLabel {
- private final ForUser forUser;
- private final LabelVote label;
-
- /**
- * Construct a reference to an abstract label permission at a specific value.
- *
- * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior.
- * @param label label name and vote.
- */
- public WithValue(ForUser forUser, LabelVote label) {
- this.forUser = requireNonNull(forUser, "ForUser");
- this.label = requireNonNull(label, "LabelVote");
- }
-
- /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
- public ForUser forUser() {
- return forUser;
- }
-
- /** Returns name of the label, e.g. {@code "Code-Review"}. */
- public String label() {
- return label.label();
- }
-
- /** Returns specific value of the label, e.g. 1 or 2. */
- public short value() {
- return label.value();
- }
-
- public abstract String permissionName();
-
- @Override
- public final String describeForException() {
- if (forUser == ON_BEHALF_OF) {
- return permissionName() + " on behalf of " + label.formatWithEquals();
- }
- return permissionName() + " " + label.formatWithEquals();
- }
-
- @Override
- public final int hashCode() {
- return (permissionName() + label).hashCode();
- }
-
- @Override
- @SuppressWarnings("EqualsGetClass")
- public final boolean equals(Object other) {
- if (this.getClass().isAssignableFrom(other.getClass())) {
- AbstractLabelPermission.WithValue b = (AbstractLabelPermission.WithValue) other;
- return forUser == b.forUser && label.equals(b.label);
- }
- return false;
- }
-
- @Override
- public final String toString() {
- if (forUser == ON_BEHALF_OF) {
- return permissionName() + "As[" + label.format() + ']';
- }
- return permissionName() + "[" + label.format() + ']';
- }
- }
-}
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 6f7d761..8d432c8 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -14,8 +14,8 @@
package com.google.gerrit.server.permissions;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.labelPermissionName;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@@ -240,10 +240,10 @@
private boolean can(ChangePermissionOrLabel perm) throws PermissionBackendException {
if (perm instanceof ChangePermission) {
return can((ChangePermission) perm);
- } else if (perm instanceof AbstractLabelPermission) {
- return can((AbstractLabelPermission) perm);
- } else if (perm instanceof AbstractLabelPermission.WithValue) {
- return can((AbstractLabelPermission.WithValue) perm);
+ } else if (perm instanceof LabelPermission) {
+ return can((LabelPermission) perm);
+ } else if (perm instanceof LabelPermission.WithValue) {
+ return can((LabelPermission.WithValue) perm);
}
throw new PermissionBackendException(perm + " unsupported");
}
@@ -288,11 +288,11 @@
throw new PermissionBackendException(perm + " unsupported");
}
- private boolean can(AbstractLabelPermission perm) {
+ private boolean can(LabelPermission perm) {
return !label(labelPermissionName(perm)).isEmpty();
}
- private boolean can(AbstractLabelPermission.WithValue perm) {
+ private boolean can(LabelPermission.WithValue perm) {
PermissionRange r = label(labelPermissionName(perm));
if (perm.forUser() == ON_BEHALF_OF && r.isEmpty()) {
return false;
diff --git a/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java b/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
index f59ba02..2824efd 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
@@ -16,5 +16,5 @@
import com.google.gerrit.extensions.api.access.GerritPermission;
-/** A {@link ChangePermission} or a {@link AbstractLabelPermission}. */
+/** A {@link ChangePermission} or a {@link LabelPermission}. */
public interface ChangePermissionOrLabel extends GerritPermission {}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
index 89f0493..9d69d9b 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -24,7 +24,7 @@
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.extensions.api.access.PluginProjectPermission;
-import com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser;
+import com.google.gerrit.server.permissions.LabelPermission.ForUser;
import java.util.EnumSet;
import java.util.Optional;
import java.util.Set;
@@ -160,29 +160,19 @@
return Optional.ofNullable(CHANGE_PERMISSIONS.inverse().get(permissionName));
}
- public static String labelPermissionName(AbstractLabelPermission labelPermission) {
- if (labelPermission instanceof LabelPermission) {
- if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
- return Permission.forLabelAs(labelPermission.label());
- }
- return Permission.forLabel(labelPermission.label());
- } else if (labelPermission instanceof LabelRemovalPermission) {
- return Permission.forRemoveLabel(labelPermission.label());
+ public static String labelPermissionName(LabelPermission labelPermission) {
+ if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
+ return Permission.forLabelAs(labelPermission.label());
}
- throw new IllegalStateException("invalid AbstractLabelPermission subtype");
+ return Permission.forLabel(labelPermission.label());
}
// TODO(dborowitz): Can these share a common superinterface?
- public static String labelPermissionName(AbstractLabelPermission.WithValue labelPermission) {
- if (labelPermission instanceof LabelPermission.WithValue) {
- if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
- return Permission.forLabelAs(labelPermission.label());
- }
- return Permission.forLabel(labelPermission.label());
- } else if (labelPermission instanceof LabelRemovalPermission.WithValue) {
- return Permission.forRemoveLabel(labelPermission.label());
+ public static String labelPermissionName(LabelPermission.WithValue labelPermission) {
+ if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
+ return Permission.forLabelAs(labelPermission.label());
}
- throw new IllegalStateException("invalid AbstractLabelPermission.WithValue subtype");
+ return Permission.forLabel(labelPermission.label());
}
private DefaultPermissionMappings() {}
diff --git a/java/com/google/gerrit/server/permissions/LabelPermission.java b/java/com/google/gerrit/server/permissions/LabelPermission.java
index 4652364..c266caa 100644
--- a/java/com/google/gerrit/server/permissions/LabelPermission.java
+++ b/java/com/google/gerrit/server/permissions/LabelPermission.java
@@ -14,14 +14,24 @@
package com.google.gerrit.server.permissions;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.SELF;
+import static java.util.Objects.requireNonNull;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.server.util.LabelVote;
/** Permission representing a label. */
-public class LabelPermission extends AbstractLabelPermission {
+public class LabelPermission implements ChangePermissionOrLabel {
+ public enum ForUser {
+ SELF,
+ ON_BEHALF_OF;
+ }
+
+ private final ForUser forUser;
+ private final String name;
+
/**
* Construct a reference to a label permission.
*
@@ -57,16 +67,55 @@
* @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
*/
public LabelPermission(ForUser forUser, String name) {
- super(forUser, name);
+ this.forUser = requireNonNull(forUser, "ForUser");
+ this.name = LabelType.checkName(name);
+ }
+
+ /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
+ public ForUser forUser() {
+ return forUser;
+ }
+
+ /** Returns name of the label, e.g. {@code "Code-Review"}. */
+ public String label() {
+ return name;
}
@Override
- public String permissionPrefix() {
- return "label";
+ public String describeForException() {
+ if (forUser == ON_BEHALF_OF) {
+ return "label on behalf of " + name;
+ }
+ return "label " + name;
+ }
+
+ @Override
+ public int hashCode() {
+ return name.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (other instanceof LabelPermission) {
+ LabelPermission b = (LabelPermission) other;
+ return forUser == b.forUser && name.equals(b.name);
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ if (forUser == ON_BEHALF_OF) {
+ return "LabelAs[" + name + ']';
+ }
+ return "Label[" + name + ']';
}
/** A {@link LabelPermission} at a specific value. */
- public static class WithValue extends AbstractLabelPermission.WithValue {
+ public static class WithValue implements ChangePermissionOrLabel {
+ private final ForUser forUser;
+ private final LabelVote label;
+
/**
* Construct a reference to a label at a specific value.
*
@@ -146,12 +195,53 @@
* @param label label name and vote.
*/
public WithValue(ForUser forUser, LabelVote label) {
- super(forUser, label);
+ this.forUser = requireNonNull(forUser, "ForUser");
+ this.label = requireNonNull(label, "LabelVote");
+ }
+
+ /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */
+ public ForUser forUser() {
+ return forUser;
+ }
+
+ /** Returns name of the label, e.g. {@code "Code-Review"}. */
+ public String label() {
+ return label.label();
+ }
+
+ /** Returns specific value of the label, e.g. 1 or 2. */
+ public short value() {
+ return label.value();
}
@Override
- public String permissionName() {
- return "label";
+ public String describeForException() {
+ if (forUser == ON_BEHALF_OF) {
+ return "label on behalf of " + label.formatWithEquals();
+ }
+ return "label " + label.formatWithEquals();
+ }
+
+ @Override
+ public int hashCode() {
+ return label.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (other instanceof WithValue) {
+ WithValue b = (WithValue) other;
+ return forUser == b.forUser && label.equals(b.label);
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ if (forUser == ON_BEHALF_OF) {
+ return "LabelAs[" + label.format() + ']';
+ }
+ return "Label[" + label.format() + ']';
}
}
}
diff --git a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
deleted file mode 100644
index 2553601..0000000
--- a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
+++ /dev/null
@@ -1,94 +0,0 @@
-// Copyright (C) 2022 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.permissions;
-
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF;
-
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.LabelValue;
-import com.google.gerrit.server.util.LabelVote;
-
-/** Permission representing a label removal. */
-public class LabelRemovalPermission extends AbstractLabelPermission {
- /**
- * Construct a reference to a label removal permission.
- *
- * @param type type description of the label.
- */
- public LabelRemovalPermission(LabelType type) {
- this(type.getName());
- }
-
- /**
- * Construct a reference to a label removal permission.
- *
- * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
- */
- public LabelRemovalPermission(String name) {
- super(SELF, name);
- }
-
- @Override
- public String permissionPrefix() {
- return "removeLabel";
- }
-
- /** A {@link LabelRemovalPermission} at a specific value. */
- public static class WithValue extends AbstractLabelPermission.WithValue {
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param type description of the label.
- * @param value numeric score assigned to the label.
- */
- public WithValue(LabelType type, LabelValue value) {
- this(type.getName(), value.getValue());
- }
-
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param type description of the label.
- * @param value numeric score assigned to the label.
- */
- public WithValue(LabelType type, short value) {
- this(type.getName(), value);
- }
-
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
- * @param value numeric score assigned to the label.
- */
- public WithValue(String name, short value) {
- this(LabelVote.create(name, value));
- }
-
- /**
- * Construct a reference to a label removal at a specific value.
- *
- * @param label label name and vote.
- */
- public WithValue(LabelVote label) {
- super(SELF, label);
- }
-
- @Override
- public String permissionName() {
- return "removeLabel";
- }
- }
-}
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java
index eb5e053..fea2827 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -474,18 +474,6 @@
}
/**
- * Test which values of a label the user may be able to remove.
- *
- * @param label definition of the label to test values of.
- * @return set containing values the user may be able to use; may be empty if none.
- * @throws PermissionBackendException if failure consulting backend configuration.
- */
- public Set<LabelRemovalPermission.WithValue> testRemoval(LabelType label)
- throws PermissionBackendException {
- return test(removalValuesOf(requireNonNull(label, "LabelType")));
- }
-
- /**
* Test which values of a group of labels the user may be able to set.
*
* @param types definition of the labels to test values of.
@@ -498,29 +486,10 @@
return test(types.stream().flatMap(t -> valuesOf(t).stream()).collect(toSet()));
}
- /**
- * Test which values of a group of labels the user may be able to remove.
- *
- * @param types definition of the labels to test values of.
- * @return set containing values the user may be able to use; may be empty if none.
- * @throws PermissionBackendException if failure consulting backend configuration.
- */
- public Set<LabelRemovalPermission.WithValue> testLabelRemovals(Collection<LabelType> types)
- throws PermissionBackendException {
- requireNonNull(types, "LabelType");
- return test(types.stream().flatMap(t -> removalValuesOf(t).stream()).collect(toSet()));
- }
-
private static Set<LabelPermission.WithValue> valuesOf(LabelType label) {
return label.getValues().stream()
.map(v -> new LabelPermission.WithValue(label, v))
.collect(toSet());
}
-
- private static Set<LabelRemovalPermission.WithValue> removalValuesOf(LabelType label) {
- return label.getValues().stream()
- .map(v -> new LabelRemovalPermission.WithValue(label, v))
- .collect(toSet());
- }
}
}
diff --git a/java/com/google/gerrit/server/project/DeleteVoteControl.java b/java/com/google/gerrit/server/project/DeleteVoteControl.java
deleted file mode 100644
index 93c0451..0000000
--- a/java/com/google/gerrit/server/project/DeleteVoteControl.java
+++ /dev/null
@@ -1,83 +0,0 @@
-// Copyright (C) 2022 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.project;
-
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.PatchSetApproval;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.LabelRemovalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.RefPermission;
-import com.google.inject.Inject;
-import java.util.Set;
-
-public class DeleteVoteControl {
- private final PermissionBackend permissionBackend;
-
- @Inject
- public DeleteVoteControl(PermissionBackend permissionBackend) {
- this.permissionBackend = permissionBackend;
- }
-
- public void checkDeleteVotePermissions(
- CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
- throws AuthException, PermissionBackendException {
- if (testDeleteVotePermissions(user, notes, approval, labelType)) {
- return;
- }
- throw new AuthException(
- new LabelRemovalPermission.WithValue(labelType, approval.value()).describeForException()
- + " not permitted");
- }
-
- public boolean testDeleteVotePermissions(
- CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
- throws PermissionBackendException {
- if (canRemoveReviewerWithoutRemoveLabelPermission(
- notes.getChange(), user, approval.accountId(), approval.value())) {
- return true;
- }
- // Test if the user is allowed to remove vote of the given label type and value.
- Set<LabelRemovalPermission.WithValue> allowed =
- permissionBackend.user(user).change(notes).testRemoval(labelType);
- return allowed.contains(new LabelRemovalPermission.WithValue(labelType, approval.value()));
- }
-
- private boolean canRemoveReviewerWithoutRemoveLabelPermission(
- Change change, CurrentUser user, Account.Id reviewer, int value)
- throws PermissionBackendException {
- if (user.isIdentifiedUser()) {
- Account.Id aId = user.getAccountId();
- if (aId.equals(reviewer)) {
- return true; // A user can always remove their own votes.
- } else if (aId.equals(change.getOwner()) && 0 <= value) {
- return true; // The change owner may remove any zero or positive score.
- }
- }
-
- // Users with the remove reviewer permission, the branch owner, project
- // owner and site admin can remove anyone
- PermissionBackend.WithUser withUser = permissionBackend.user(user);
- PermissionBackend.ForProject forProject = withUser.project(change.getProject());
- return forProject.ref(change.getDest().branch()).test(RefPermission.WRITE_CONFIG)
- || withUser.test(GlobalPermission.ADMINISTRATE_SERVER);
- }
-}
diff --git a/java/com/google/gerrit/server/project/ProjectCacheImpl.java b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
index 6498d1b..0afaa3f 100644
--- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
@@ -76,6 +76,7 @@
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;
@@ -300,19 +301,20 @@
@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(
- 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());
+ Streams.concat(configuredRelevantGroups, guessedRelevantGroups).collect(toSet());
logger.atFine().log("relevant group UUIDs: %s", relevantGroupUuids);
return relevantGroupUuids;
}
diff --git a/java/com/google/gerrit/server/query/project/ProjectPredicates.java b/java/com/google/gerrit/server/query/project/ProjectPredicates.java
index 8b4048f..a7b0743 100644
--- a/java/com/google/gerrit/server/query/project/ProjectPredicates.java
+++ b/java/com/google/gerrit/server/query/project/ProjectPredicates.java
@@ -25,23 +25,23 @@
/** Utility class to create predicates for project index queries. */
public class ProjectPredicates {
public static Predicate<ProjectData> name(Project.NameKey nameKey) {
- return new ProjectPredicate(ProjectField.NAME, nameKey.get());
+ return new ProjectPredicate(ProjectField.NAME_SPEC, nameKey.get());
}
public static Predicate<ProjectData> parent(Project.NameKey parentNameKey) {
- return new ProjectPredicate(ProjectField.PARENT_NAME, parentNameKey.get());
+ return new ProjectPredicate(ProjectField.PARENT_NAME_SPEC, parentNameKey.get());
}
public static Predicate<ProjectData> inname(String name) {
- return new ProjectPredicate(ProjectField.NAME_PART, name.toLowerCase(Locale.US));
+ return new ProjectPredicate(ProjectField.NAME_PART_SPEC, name.toLowerCase(Locale.US));
}
public static Predicate<ProjectData> description(String description) {
- return new ProjectPredicate(ProjectField.DESCRIPTION, description);
+ return new ProjectPredicate(ProjectField.DESCRIPTION_SPEC, description);
}
public static Predicate<ProjectData> state(ProjectState state) {
- return new ProjectPredicate(ProjectField.STATE, state.name());
+ return new ProjectPredicate(ProjectField.STATE_SPEC, state.name());
}
private ProjectPredicates() {}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
index 239b485..a0fc121 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -38,8 +38,8 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.project.DeleteVoteControl;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.RemoveReviewerControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
@@ -75,7 +75,7 @@
private final VoteDeleted voteDeleted;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
- private final DeleteVoteControl deleteVoteControl;
+ private final RemoveReviewerControl removeReviewerControl;
private final MessageIdGenerator messageIdGenerator;
private final String label;
@@ -96,7 +96,7 @@
ChangeMessagesUtil cmUtil,
VoteDeleted voteDeleted,
DeleteVoteSender.Factory deleteVoteSenderFactory,
- DeleteVoteControl deleteVoteControl,
+ RemoveReviewerControl removeReviewerControl,
MessageIdGenerator messageIdGenerator,
@Assisted Project.NameKey projectName,
@Assisted AccountState reviewerToDeleteVoteFor,
@@ -109,7 +109,7 @@
this.cmUtil = cmUtil;
this.voteDeleted = voteDeleted;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
- this.deleteVoteControl = deleteVoteControl;
+ this.removeReviewerControl = removeReviewerControl;
this.messageIdGenerator = messageIdGenerator;
this.projectName = projectName;
@@ -143,8 +143,12 @@
newApprovals.put(a.label(), a.value());
continue;
} else if (enforcePermissions) {
- deleteVoteControl.checkDeleteVotePermissions(
- ctx.getUser(), ctx.getNotes(), a, labelTypes.byLabel(a.labelId()).get());
+ // For regular users, check if they are allowed to remove the vote.
+ try {
+ removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+ } catch (AuthException e) {
+ throw new AuthException("delete vote not permitted", e);
+ }
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.label(), (short) 0);
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index a8ba052..259e71d 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -17,7 +17,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.groupingBy;
@@ -379,7 +379,8 @@
// Add the review ops.
logger.atFine().log("posting review");
PostReviewOp postReviewOp =
- postReviewOpFactory.create(projectState, revision.getPatchSet().id(), input);
+ postReviewOpFactory.create(
+ projectState, revision.getPatchSet().id(), input, revision.getAccountId());
bu.addOp(revision.getChange().getId(), postReviewOp);
// Adjust the attention set based on the input
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index b7d17f2..29e453b 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -97,7 +97,8 @@
public class PostReviewOp implements BatchUpdateOp {
interface Factory {
- PostReviewOp create(ProjectState projectState, PatchSet.Id psId, ReviewInput in);
+ PostReviewOp create(
+ ProjectState projectState, PatchSet.Id psId, ReviewInput in, Account.Id reviewerId);
}
/**
@@ -192,6 +193,7 @@
private final ProjectState projectState;
private final PatchSet.Id psId;
private final ReviewInput in;
+ private final Account.Id reviewerId;
private final boolean publishPatchSetLevelComment;
private IdentifiedUser user;
@@ -220,7 +222,8 @@
PluginSetContext<OnPostReview> onPostReviews,
@Assisted ProjectState projectState,
@Assisted PatchSet.Id psId,
- @Assisted ReviewInput in) {
+ @Assisted ReviewInput in,
+ @Assisted Account.Id reviewerId) {
this.approvalCopier = approvalCopier;
this.approvalsUtil = approvalsUtil;
this.publishCommentUtil = publishCommentUtil;
@@ -237,6 +240,7 @@
this.projectState = projectState;
this.psId = psId;
this.in = in;
+ this.reviewerId = reviewerId;
}
@Override
@@ -645,10 +649,11 @@
del.add(c);
update.putApproval(normName, (short) 0);
}
- // Only allow voting again if the vote is copied over from a past patch-set, or the
- // values are different.
+ // Only allow voting again the values are different, if the real account differs or if the
+ // vote is copied over from a past patch-set.
} else if (c != null
&& (c.value() != ent.getValue()
+ || !c.realAccountId().equals(reviewerId)
|| (inLabels.containsKey(c.label()) && isApprovalCopiedOver(c, ctx.getNotes())))) {
PatchSetApproval.Builder b =
c.toBuilder()
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 92d19bb..211baeb 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -26,10 +26,8 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.entities.RefNames.changeMetaRef;
@@ -2268,16 +2266,6 @@
@Test
public void deleteVote() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2321,16 +2309,6 @@
@Test
public void deleteVoteNotifyNone() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2348,16 +2326,6 @@
@Test
public void deleteVoteWithReason() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2379,16 +2347,6 @@
@Test
public void deleteVoteNotifyAccount() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2448,66 +2406,7 @@
.id(r.getChangeId())
.reviewer(admin.id().toString())
.deleteVote(LabelId.CODE_REVIEW));
- assertThat(thrown).hasMessageThat().contains("removeLabel Code-Review=+2 not permitted");
- }
-
- @Test
- public void deleteVoteAlwaysPermittedForSelfVotes() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabel(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .add(
- blockLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
- PushOneCommit.Result r = createChange();
- String changeId = r.getChangeId();
-
- requestScopeOperations.setApiUser(user.id());
- gApi.changes().id(changeId).revision(r.getCommit().name()).review(ReviewInput.approve());
-
- gApi.changes()
- .id(r.getChangeId())
- .reviewer(user.id().toString())
- .deleteVote(LabelId.CODE_REVIEW);
- }
-
- @Test
- public void deleteVoteAlwaysPermittedForAdmin() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabel(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .add(
- blockLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
- PushOneCommit.Result r = createChange();
- String changeId = r.getChangeId();
-
- requestScopeOperations.setApiUser(user.id());
- gApi.changes().id(changeId).revision(r.getCommit().name()).review(ReviewInput.approve());
-
- requestScopeOperations.setApiUser(admin.id());
- gApi.changes()
- .id(r.getChangeId())
- .reviewer(user.id().toString())
- .deleteVote(LabelId.CODE_REVIEW);
+ assertThat(thrown).hasMessageThat().contains("delete vote not permitted");
}
@Test
@@ -3376,7 +3275,6 @@
assertThat(change.status).isEqualTo(ChangeStatus.NEW);
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
- assertThat(change.removableLabels).isEmpty();
// add new label and assert that it's returned for existing changes
AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
@@ -3391,7 +3289,6 @@
.project(project)
.forUpdate()
.add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
- .add(allowLabelRemoval(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
.update();
change = gApi.changes().id(r.getChangeId()).get();
@@ -3407,9 +3304,6 @@
.id(r.getChangeId())
.revision(r.getCommit().name())
.review(new ReviewInput().label(verified.getName(), verified.getMax().getValue()));
- change = gApi.changes().id(r.getChangeId()).get();
- assertPermitted(change, LabelId.VERIFIED, -1, 0, 1);
- assertOnlyRemovableLabel(change, LabelId.VERIFIED, "+1", admin);
try (ProjectConfigUpdate u = updateProject(project)) {
// remove label and assert that it's no longer returned for existing
@@ -3429,7 +3323,6 @@
change = gApi.changes().id(r.getChangeId()).get();
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
- assertThat(change.removableLabels).isEmpty();
// abandon the change and see that the returned labels stay the same
// while all permitted labels disappear.
@@ -3438,7 +3331,6 @@
assertThat(change.status).isEqualTo(ChangeStatus.ABANDONED);
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels).isEmpty();
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3555,7 +3447,6 @@
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW, LabelId.VERIFIED);
assertPermitted(change, LabelId.CODE_REVIEW, 2);
assertPermitted(change, LabelId.VERIFIED, 1);
- assertThat(change.removableLabels).isEmpty();
// remove label and assert that it's no longer returned for existing
// changes, even if there is an approval for it
@@ -3573,7 +3464,6 @@
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertPermitted(change, LabelId.CODE_REVIEW, 2);
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3670,7 +3560,6 @@
.containsExactly(LabelId.CODE_REVIEW, "Non-Author-Code-Review");
assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertPermitted(change, LabelId.CODE_REVIEW, 0, 1, 2);
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3686,7 +3575,6 @@
assertThat(change.submissionId).isNotNull();
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertPermitted(change, LabelId.CODE_REVIEW, 0, 1, 2);
- assertThat(change.removableLabels).isEmpty();
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIndexerIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIndexerIT.java
index a625a70..93f91dd 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIndexerIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIndexerIT.java
@@ -54,7 +54,7 @@
@Inject private IndexOperations.Project projectIndexOperations;
private static final ImmutableSet<String> FIELDS =
- ImmutableSet.of(ProjectField.NAME.getName(), ProjectField.REF_STATE.getName());
+ ImmutableSet.of(ProjectField.NAME_SPEC.getName(), ProjectField.REF_STATE.getName());
@Test
public void indexProject_indexesRefStateOfProjectAndParents() throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index e707949..0291f33 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -22,7 +22,6 @@
import static com.google.gerrit.acceptance.PushOneCommit.PATCH_FILE_ONLY;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
@@ -235,15 +234,6 @@
revision(r).review(ReviewInput.recommend());
requestScopeOperations.setApiUser(admin.id());
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
gApi.changes().id(changeId).reviewer(user.username()).deleteVote(LabelId.CODE_REVIEW);
Optional<ApprovalInfo> crUser =
get(changeId, DETAILED_LABELS).labels.get(LabelId.CODE_REVIEW).all.stream()
@@ -2011,15 +2001,6 @@
recommend(r.getChangeId());
requestScopeOperations.setApiUser(admin.id());
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
gApi.changes()
.id(r.getChangeId())
.current()
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 804723b..3531d1c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
@@ -166,6 +166,202 @@
}
@Test
+ public void overrideImpersonatedVoteWithOtherImpersonatedVote_sameValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount realUser2 = admin2;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // realUser2 votes Code-Review+1 on behalf of impersonatedUser, this should override the
+ // impersonated Code-Review+1 of realUser with an impersonated Code-Review+1 of realUser2
+ requestScopeOperations.setApiUser(realUser2.id());
+ in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Another message on behalf of";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser2.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser2.id());
+ }
+
+ @Test
+ public void overrideImpersonatedVoteWithOtherImpersonatedVote_differentValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount realUser2 = admin2;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // realUser2 votes Code-Review-1 on behalf of impersonatedUser, this should override the
+ // impersonated Code-Review+1 of realUser with an impersonated Code-Review-1 of realUser2
+ requestScopeOperations.setApiUser(realUser2.id());
+ in = ReviewInput.dislike();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Another message on behalf of";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(-1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser2.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser2.id());
+ }
+
+ @Test
+ public void overrideImpersonatedVoteWithNonImpersonatedVote_sameValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // impersonatedUser votes Code-Review+1 themselves, this should override the impersonated
+ // Code-Review+1 with a non-impersonated Code-Review+1
+ requestScopeOperations.setApiUser(impersonatedUser.id());
+ in = ReviewInput.recommend();
+ in.message = "Message";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(impersonatedUser.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(impersonatedUser.id());
+ }
+
+ @Test
+ public void overrideImpersonatedVoteWithNonImpersonatedVote_differentValue() throws Exception {
+ allowCodeReviewOnBehalfOf();
+ TestAccount realUser = admin;
+ TestAccount impersonatedUser = user;
+ PushOneCommit.Result r = createChange();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+
+ // realUser votes Code-Review+1 on behalf of impersonatedUser
+ ReviewInput in = ReviewInput.recommend();
+ in.onBehalfOf = impersonatedUser.id().toString();
+ in.message = "Message on behalf of";
+ revision.review(in);
+
+ PatchSetApproval psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(1);
+ assertThat(psa.realAccountId()).isEqualTo(realUser.id());
+
+ ChangeData cd = r.getChange();
+ ChangeMessage m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(realUser.id());
+
+ // impersonatedUser votes Code-Review-1 themselves, this should override the impersonated
+ // Code-Review+1 with a non-impersonated Code-Review-1
+ requestScopeOperations.setApiUser(impersonatedUser.id());
+ in = ReviewInput.dislike();
+ in.message = "Message";
+ gApi.changes().id(r.getChangeId()).current().review(in);
+
+ psa = Iterables.getOnlyElement(r.getChange().approvals().values());
+ assertThat(psa.patchSetId().get()).isEqualTo(1);
+ assertThat(psa.label()).isEqualTo("Code-Review");
+ assertThat(psa.accountId()).isEqualTo(impersonatedUser.id());
+ assertThat(psa.value()).isEqualTo(-1);
+ assertThat(psa.realAccountId()).isEqualTo(impersonatedUser.id());
+
+ cd = r.getChange();
+ m = Iterables.getLast(cmUtil.byChange(cd.notes()));
+ assertThat(m.getMessage()).endsWith(in.message);
+ assertThat(m.getAuthor()).isEqualTo(impersonatedUser.id());
+ assertThat(m.getRealAuthor()).isEqualTo(impersonatedUser.id());
+ }
+
+ @Test
public void voteOnBehalfOfRequiresLabel() throws Exception {
allowCodeReviewOnBehalfOf();
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 7e8ff62..ea52690 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.extensions.restapi.testing.AttentionSetUpdateSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -1945,16 +1944,6 @@
@Test
public void deleteVotesDoesNotAffectAttentionSetWhenIgnoreAutomaticRulesIsSet() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
@@ -1979,16 +1968,6 @@
@Test
public void deleteVotesOfOthersAddThemToAttentionSet() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
index c29b265..c57d285 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -15,16 +15,13 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
-import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
-import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.LabelId;
@@ -39,26 +36,11 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import org.junit.Before;
import org.junit.Test;
public class DeleteVoteIT extends AbstractDaemonTest {
- @Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
- @Before
- public void allowVoteDeletion() {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
- }
-
@Test
public void deleteVoteOnChange() throws Exception {
deleteVote(false);
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index b1277c0..b68afc5 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ABANDONED_CHANGES;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ALL_COMMENTS;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.NEW_CHANGES;
@@ -99,11 +98,6 @@
.add(allow(Permission.SUBMIT_AS).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.ABANDON).ref("refs/*").group(REGISTERED_USERS))
.add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, +2))
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/*")
- .group(REGISTERED_USERS)
- .range(-2, +2))
.update();
}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
index 661802e..c1a7627 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -18,14 +18,11 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabel;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.capabilityKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.deny;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelRemovalPermissionKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.common.data.GlobalCapability.ADMINISTRATE_SERVER;
import static com.google.gerrit.common.data.GlobalCapability.DEFAULT_MAX_QUERY_LIMIT;
@@ -446,73 +443,6 @@
}
@Test
- public void addAllowLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .update();
-
- Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "-1..+2 group global:Registered-Users");
- }
-
- @Test
- public void addBlockLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(blockLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .update();
-
- Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "block -1..+2 group global:Registered-Users");
- }
-
- @Test
- public void addAllowExclusiveLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .setExclusiveGroup(labelRemovalPermissionKey("Code-Review").ref("refs/foo"), true)
- .update();
-
- Config config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly(
- "removeLabel-Code-Review", "-1..+2 group global:Registered-Users",
- "exclusiveGroupPermissions", "removeLabel-Code-Review");
-
- projectOperations
- .project(key)
- .forUpdate()
- .setExclusiveGroup(labelRemovalPermissionKey("Code-Review").ref("refs/foo"), false)
- .update();
-
- config = projectOperations.project(key).getConfig();
- assertThat(config).sections().containsExactly("access", "submit");
- assertThat(config).subsections("access").containsExactly("refs/foo");
- assertThat(config)
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "-1..+2 group global:Registered-Users");
- }
-
- @Test
public void addAllowCapability() throws Exception {
Config config = projectOperations.project(allProjects).getConfig();
assertThat(config)
@@ -610,31 +540,6 @@
}
@Test
- public void removeLabelRemovalPermission() throws Exception {
- Project.NameKey key = projectOperations.newProject().create();
- projectOperations
- .project(key)
- .forUpdate()
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2))
- .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(PROJECT_OWNERS).range(-2, 1))
- .update();
- assertThat(projectOperations.project(key).getConfig())
- .subsectionValues("access", "refs/foo")
- .containsExactly(
- "removeLabel-Code-Review", "-1..+2 group global:Registered-Users",
- "removeLabel-Code-Review", "-2..+1 group global:Project-Owners");
-
- projectOperations
- .project(key)
- .forUpdate()
- .remove(labelRemovalPermissionKey("Code-Review").ref("refs/foo").group(REGISTERED_USERS))
- .update();
- assertThat(projectOperations.project(key).getConfig())
- .subsectionValues("access", "refs/foo")
- .containsExactly("removeLabel-Code-Review", "-2..+1 group global:Project-Owners");
- }
-
- @Test
public void removeCapability() throws Exception {
projectOperations
.allProjectsForUpdate()
diff --git a/javatests/com/google/gerrit/entities/PermissionTest.java b/javatests/com/google/gerrit/entities/PermissionTest.java
index d25d833..3175671 100644
--- a/javatests/com/google/gerrit/entities/PermissionTest.java
+++ b/javatests/com/google/gerrit/entities/PermissionTest.java
@@ -36,7 +36,6 @@
assertThat(Permission.isPermission(Permission.LABEL + LabelId.CODE_REVIEW)).isTrue();
assertThat(Permission.isPermission(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isTrue();
- assertThat(Permission.isPermission(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isTrue();
assertThat(Permission.isPermission(LabelId.CODE_REVIEW)).isFalse();
}
@@ -57,7 +56,6 @@
assertThat(Permission.isLabel(Permission.LABEL + LabelId.CODE_REVIEW)).isTrue();
assertThat(Permission.isLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isFalse();
- assertThat(Permission.isLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isFalse();
assertThat(Permission.isLabel(LabelId.CODE_REVIEW)).isFalse();
}
@@ -68,22 +66,10 @@
assertThat(Permission.isLabelAs(Permission.LABEL + LabelId.CODE_REVIEW)).isFalse();
assertThat(Permission.isLabelAs(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isTrue();
- assertThat(Permission.isLabelAs(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isFalse();
assertThat(Permission.isLabelAs(LabelId.CODE_REVIEW)).isFalse();
}
@Test
- public void isRemoveLabel() {
- assertThat(Permission.isRemoveLabel(Permission.ABANDON)).isFalse();
- assertThat(Permission.isRemoveLabel("no-permission")).isFalse();
-
- assertThat(Permission.isRemoveLabel(Permission.LABEL + LabelId.CODE_REVIEW)).isFalse();
- assertThat(Permission.isRemoveLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isFalse();
- assertThat(Permission.isRemoveLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isTrue();
- assertThat(Permission.isRemoveLabel(LabelId.CODE_REVIEW)).isFalse();
- }
-
- @Test
public void forLabel() {
assertThat(Permission.forLabel(LabelId.CODE_REVIEW))
.isEqualTo(Permission.LABEL + LabelId.CODE_REVIEW);
@@ -96,19 +82,11 @@
}
@Test
- public void forRemoveLabel() {
- assertThat(Permission.forRemoveLabel(LabelId.CODE_REVIEW))
- .isEqualTo(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW);
- }
-
- @Test
public void extractLabel() {
assertThat(Permission.extractLabel(Permission.LABEL + LabelId.CODE_REVIEW))
.isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.extractLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isEqualTo(LabelId.CODE_REVIEW);
- assertThat(Permission.extractLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.extractLabel(LabelId.CODE_REVIEW)).isNull();
assertThat(Permission.extractLabel(Permission.ABANDON)).isNull();
}
@@ -125,10 +103,6 @@
Permission.canBeOnAllProjects(
AccessSection.ALL, Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isTrue();
- assertThat(
- Permission.canBeOnAllProjects(
- AccessSection.ALL, Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isTrue();
assertThat(Permission.canBeOnAllProjects("refs/heads/*", Permission.ABANDON)).isTrue();
assertThat(Permission.canBeOnAllProjects("refs/heads/*", Permission.OWNER)).isTrue();
@@ -139,10 +113,6 @@
Permission.canBeOnAllProjects(
"refs/heads/*", Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isTrue();
- assertThat(
- Permission.canBeOnAllProjects(
- "refs/heads/*", Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isTrue();
}
@Test
@@ -156,8 +126,6 @@
.isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.create(Permission.LABEL_AS + LabelId.CODE_REVIEW).getLabel())
.isEqualTo(LabelId.CODE_REVIEW);
- assertThat(Permission.create(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW).getLabel())
- .isEqualTo(LabelId.CODE_REVIEW);
assertThat(Permission.create(LabelId.CODE_REVIEW).getLabel()).isNull();
assertThat(Permission.create(Permission.ABANDON).getLabel()).isNull();
}
diff --git a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
index f45d33b..7ed236a 100644
--- a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
+++ b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
@@ -48,7 +48,6 @@
assertThat(diff.added().messages).isNull();
assertThat(diff.added().reviewers).isNull();
assertThat(diff.added().hashtags).isNull();
- assertThat(diff.added().removableLabels).isNull();
assertThat(diff.removed()._number).isNull();
assertThat(diff.removed().branch).isNull();
assertThat(diff.removed().project).isNull();
@@ -57,7 +56,6 @@
assertThat(diff.removed().messages).isNull();
assertThat(diff.removed().reviewers).isNull();
assertThat(diff.removed().hashtags).isNull();
- assertThat(diff.removed().removableLabels).isNull();
}
@Test
@@ -317,295 +315,6 @@
}
@Test
- public void getDiff_removableLabelsEmpty_returnsNullRemovableLabels() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- oldChangeInfo.removableLabels = ImmutableMap.of();
- newChangeInfo.removableLabels = ImmutableMap.of();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsNullAndEmpty_returnsEmptyRemovableLabels() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- newChangeInfo.removableLabels = ImmutableMap.of();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isEmpty();
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsEmptyAndNull_returnsEmptyRemovableLabels() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- oldChangeInfo.removableLabels = ImmutableMap.of();
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels).isEmpty();
- }
-
- @Test
- public void getDiff_removableLabelsLabelAdded() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "Cow";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "Pig";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "Cat";
- AccountInfo acc4 = new AccountInfo();
- acc4.name = "Dog";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
- newChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)),
- "Verified",
- ImmutableMap.of("-1", ImmutableList.of(acc4)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc4))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsLabelRemoved() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "Cow";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "Pig";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "Cat";
- AccountInfo acc4 = new AccountInfo();
- acc4.name = "Dog";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)),
- "Verified",
- ImmutableMap.of("-1", ImmutableList.of(acc4)));
- newChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc4))));
- }
-
- @Test
- public void getDiff_removableLabelsVoteAdded() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "acc3";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc2, acc3))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsVoteRemoved() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
- AccountInfo acc3 = new AccountInfo();
- acc3.name = "acc3";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of(
- "Code-Review",
- ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels).isNull();
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc2, acc3))));
- }
-
- @Test
- public void getDiff_removableLabelsAccountAdded() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1, acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsAccountRemoved() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1, acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels).isNull();
- }
-
- @Test
- public void getDiff_removableLabelsAccountChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
- public void getDiff_removableLabelsScoreChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc1)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc1))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
- public void getDiff_removableLabelsLabelChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Verified", ImmutableMap.of("+1", ImmutableList.of(acc1)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
- public void getDiff_removableLabelsLabelScoreAndAccountChanged() {
- ChangeInfo oldChangeInfo = new ChangeInfo();
- ChangeInfo newChangeInfo = new ChangeInfo();
- AccountInfo acc1 = new AccountInfo();
- acc1.name = "acc1";
- AccountInfo acc2 = new AccountInfo();
- acc2.name = "acc2";
-
- oldChangeInfo.removableLabels =
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)));
- newChangeInfo.removableLabels =
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc2)));
-
- ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo);
-
- assertThat(diff.added().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc2))));
- assertThat(diff.removed().removableLabels)
- .containsExactlyEntriesIn(
- ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))));
- }
-
- @Test
public void getDiff_assertCanConstructAllChangeInfoReferences() throws Exception {
buildObjectWithFullFields(ChangeInfo.class);
}
diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
index b263ab6..12bafd5 100644
--- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
+++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
@@ -440,10 +440,8 @@
return createGroupWithDescription(name, null, members);
}
- protected void deleteGroup(AccountGroup.UUID uuid) throws Exception {
- for (GroupIndex index : groupIndexes.getWriteIndexes()) {
- index.delete(uuid);
- }
+ protected GroupInfo createGroup(GroupInput in) throws Exception {
+ return gApi.groups().create(in).get();
}
protected GroupInfo createGroupWithDescription(
@@ -453,21 +451,27 @@
in.description = description;
in.members =
Arrays.asList(members).stream().map(a -> String.valueOf(a._accountId)).collect(toList());
- return gApi.groups().create(in).get();
+ return createGroup(in);
}
protected GroupInfo createGroupWithOwner(String name, GroupInfo ownerGroup) throws Exception {
GroupInput in = new GroupInput();
in.name = name;
in.ownerId = ownerGroup.id;
- return gApi.groups().create(in).get();
+ return createGroup(in);
}
protected GroupInfo createGroupThatIsVisibleToAll(String name) throws Exception {
GroupInput in = new GroupInput();
in.name = name;
in.visibleToAll = true;
- return gApi.groups().create(in).get();
+ return createGroup(in);
+ }
+
+ protected void deleteGroup(AccountGroup.UUID uuid) throws Exception {
+ for (GroupIndex index : groupIndexes.getWriteIndexes()) {
+ index.delete(uuid);
+ }
}
protected GroupInfo getGroup(AccountGroup.UUID uuid) throws Exception {
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts
index 0b1bd80..0cfbaa4 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-file-edit-dialog.ts
@@ -14,7 +14,7 @@
import {LitElement, html, nothing} from 'lit';
import {customElement, property, query, state} from 'lit/decorators.js';
import {resolve} from '../../../models/dependency';
-import {createEditUrl} from '../../../models/views/edit';
+import {createEditUrl} from '../../../models/views/change';
import {modalStyles} from '../../../styles/gr-modal-styles';
import {assertIsDefined} from '../../../utils/common-util';
import {when} from 'lit/directives/when.js';
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
index 2624677..11cfaab 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-commands/gr-repo-commands.ts
@@ -31,7 +31,7 @@
import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, query, property, state} from 'lit/decorators.js';
import {assertIsDefined} from '../../../utils/common-util';
-import {createEditUrl} from '../../../models/views/edit';
+import {createEditUrl} from '../../../models/views/change';
import {resolve} from '../../../models/dependency';
import {modalStyles} from '../../../styles/gr-modal-styles';
import {GrCreateFileEditDialog} from '../gr-create-change-dialog/gr-create-file-edit-dialog';
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 632ec4c..7eef7a4 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo-detail-list/gr-repo-detail-list.ts
@@ -11,6 +11,7 @@
import '../../shared/gr-list-view/gr-list-view';
import '../gr-create-pointer-dialog/gr-create-pointer-dialog';
import '../gr-confirm-delete-item-dialog/gr-confirm-delete-item-dialog';
+import {encodeURL} from '../../../utils/url-util';
import {GrCreatePointerDialog} from '../gr-create-pointer-dialog/gr-create-pointer-dialog';
import {
BranchInfo,
@@ -28,16 +29,12 @@
import {formStyles} from '../../../styles/gr-form-styles';
import {tableStyles} from '../../../styles/gr-table-styles';
import {sharedStyles} from '../../../styles/shared-styles';
-import {LitElement, PropertyValues, css, html, nothing} from 'lit';
+import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, query, property, state} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
import {assertIsDefined} from '../../../utils/common-util';
import {ifDefined} from 'lit/directives/if-defined.js';
-import {
- createRepoUrl,
- RepoDetailView,
- RepoViewState,
-} from '../../../models/views/repo';
+import {RepoDetailView, RepoViewState} from '../../../models/views/repo';
import {modalStyles} from '../../../styles/gr-modal-styles';
const PGP_START = '-----BEGIN PGP SIGNATURE-----';
@@ -142,7 +139,6 @@
}
override render() {
- if (!this.repo || !this.detailType) return nothing;
return html`
<gr-list-view
.createNew=${this.loggedIn}
@@ -151,7 +147,7 @@
.items=${this.items}
.loading=${this.loading}
.offset=${this.offset}
- .path=${createRepoUrl({repo: this.repo, detail: this.detailType})}
+ .path=${this.getPath(this.repo, this.detailType)}
@create-clicked=${() => {
this.handleCreateClicked();
}}
@@ -445,6 +441,13 @@
return Promise.reject(new Error('unknown detail type'));
}
+ private getPath(repo?: RepoName, detailType?: RepoDetailView) {
+ // TODO: Replace with `createRepoUrl()`, but be aware that `encodeURL()`
+ // gets `false` as a second parameter here. The router pattern in gr-router
+ // does not handle the filter URLs, if the repo is not encoded!
+ return `/admin/repos/${encodeURL(repo ?? '', false)},${detailType}`;
+ }
+
private computeWeblink(repo: ProjectInfo | BranchInfo | TagInfo) {
if (!repo.web_links) return [];
const webLinks = repo.web_links;
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 75854cc..e47b450 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1577,10 +1577,11 @@
base: e.detail.base,
allow_conflicts: e.detail.allowConflicts,
};
+ const rebaseChain = !!e.detail.rebaseChain;
this.fireAction(
- '/rebase',
+ rebaseChain ? '/rebase:chain' : '/rebase',
assertUIActionInfo(this.revisionActions.rebase),
- true,
+ rebaseChain ? false : true,
payload,
{allow_conflicts: payload.allow_conflicts}
);
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index c6bfd55..4602eac 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -625,7 +625,9 @@
};
assert.isTrue(fetchChangesStub.called);
element.handleRebaseConfirm(
- new CustomEvent('', {detail: {base: '1234', allowConflicts: false}})
+ new CustomEvent('', {
+ detail: {base: '1234', allowConflicts: false, rebaseChain: false},
+ })
);
assert.deepEqual(fireActionStub.lastCall.args, [
'/rebase',
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 c7473ca..c0ed3b3 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
@@ -176,9 +176,9 @@
changeViewModelToken,
ChangeViewState,
createChangeUrl,
+ createEditUrl,
} from '../../../models/views/change';
import {rootUrl} from '../../../utils/url-util';
-import {createEditUrl} from '../../../models/views/edit';
import {userModelToken} from '../../../models/user/user-model';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {modalStyles} from '../../../styles/gr-modal-styles';
@@ -2271,7 +2271,7 @@
private updateTitle(change?: ChangeInfo | ParsedChangeInfo) {
if (!change) return;
- const title = change.subject + ' (' + change.change_id.substr(0, 9) + ')';
+ const title = `${change.subject} (${change._number})`;
fireTitleChange(this, title);
}
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
index da61b60..b0dbda5 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
@@ -5,6 +5,7 @@
*/
import {css, html, LitElement, PropertyValues} from 'lit';
import {customElement, property, query, state} from 'lit/decorators.js';
+import {when} from 'lit/directives/when.js';
import {
NumericChangeId,
BranchName,
@@ -21,6 +22,7 @@
import {sharedStyles} from '../../../styles/shared-styles';
import {ValueChangedEvent} from '../../../types/events';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {KnownExperimentId} from '../../../services/flags/flags';
export interface RebaseChange {
name: string;
@@ -30,6 +32,7 @@
export interface ConfirmRebaseEventDetail {
base: string | null;
allowConflicts: boolean;
+ rebaseChain: boolean;
}
@customElement('gr-confirm-rebase-dialog')
@@ -85,11 +88,16 @@
@query('#rebaseAllowConflicts')
private rebaseAllowConflicts!: HTMLInputElement;
+ @query('#rebaseChain')
+ private rebaseChain?: HTMLInputElement;
+
@query('#parentInput')
parentInput!: GrAutocomplete;
private readonly restApiService = getAppContext().restApiService;
+ private readonly flagsService = getAppContext().flagsService;
+
constructor() {
super();
this.query = input => this.getChangeSuggestions(input);
@@ -221,6 +229,14 @@
>Allow rebase with conflicts</label
>
</div>
+ ${when(
+ this.flagsService.isEnabled(KnownExperimentId.REBASE_CHAIN),
+ () =>
+ html`<div>
+ <input id="rebaseChain" type="checkbox" />
+ <label for="rebaseChain">Rebase all ancestors</label>
+ </div>`
+ )}
</div>
</gr-dialog>
`;
@@ -326,6 +342,7 @@
const detail: ConfirmRebaseEventDetail = {
base: this.getSelectedBase(),
allowConflicts: this.rebaseAllowConflicts.checked,
+ rebaseChain: !!this.rebaseChain?.checked,
};
this.dispatchEvent(new CustomEvent('confirm', {detail}));
this.text = '';
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 08aecb9..d4defcb 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -78,9 +78,11 @@
import {incrementalRepeat} from '../../lit/incremental-repeat';
import {ifDefined} from 'lit/directives/if-defined.js';
import {HtmlPatched} from '../../../utils/lit-util';
-import {createDiffUrl} from '../../../models/views/diff';
-import {createEditUrl} from '../../../models/views/edit';
-import {createChangeUrl} from '../../../models/views/change';
+import {
+ createDiffUrl,
+ createEditUrl,
+ createChangeUrl,
+} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {FileMode, fileModeToString} from '../../../utils/file-util';
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 9d2e214..5792230 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -75,8 +75,7 @@
import {HtmlPatched} from '../../utils/lit-util';
import {DropdownItem} from '../shared/gr-dropdown-list/gr-dropdown-list';
import './gr-checks-attempt';
-import {createDiffUrl} from '../../models/views/diff';
-import {changeViewModelToken} from '../../models/views/change';
+import {createDiffUrl, changeViewModelToken} from '../../models/views/change';
/**
* Firing this event sets the regular expression of the results filter.
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
index b1ff749..833a91a 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
@@ -219,6 +219,7 @@
}
.titleText::after {
content: var(--header-title-content);
+ white-space: nowrap;
}
ul {
list-style: none;
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index ac54d35..bcf6937 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -70,6 +70,7 @@
ChangeViewModel,
ChangeViewState,
createChangeViewUrl,
+ createDiffUrl,
} from '../../../models/views/change';
import {
DashboardViewModel,
@@ -97,7 +98,6 @@
getPatchRangeForCommentUrl,
isInBaseOfPatchRange,
} from '../../../utils/comment-util';
-import {createDiffUrl} from '../../../models/views/diff';
import {isFileUnchanged} from '../../../embed/diff/gr-diff/gr-diff-utils';
const RoutePattern = {
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 65a5a23..a9cbcbd 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
@@ -84,8 +84,8 @@
import {sharedStyles} from '../../../styles/shared-styles';
import {ifDefined} from 'lit/directives/if-defined.js';
import {when} from 'lit/directives/when.js';
-import {createDiffUrl} from '../../../models/views/diff';
import {
+ createDiffUrl,
ChangeChildView,
changeViewModelToken,
} from '../../../models/views/change';
diff --git a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
index 84413c8..ec1e48e 100644
--- a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
+++ b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
@@ -29,7 +29,7 @@
import {customElement, property, query, state} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
import {IronInputElement} from '@polymer/iron-input/iron-input';
-import {createEditUrl} from '../../../models/views/edit';
+import {createEditUrl} from '../../../models/views/change';
import {resolve} from '../../../models/dependency';
import {modalStyles} from '../../../styles/gr-modal-styles';
import {whenVisible} from '../../../utils/dom-util';
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 4467f58..dd0fbca 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -72,8 +72,7 @@
import {whenRendered} from '../../../utils/dom-util';
import {Interaction} from '../../../constants/reporting';
import {HtmlPatched} from '../../../utils/lit-util';
-import {createDiffUrl} from '../../../models/views/diff';
-import {createChangeUrl} from '../../../models/views/change';
+import {createChangeUrl, createDiffUrl} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 090dfef..66beaf1 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -60,7 +60,7 @@
import {Interaction} from '../../../constants/reporting';
import {KnownExperimentId} from '../../../services/flags/flags';
import {isBase64FileContent} from '../../../api/rest-api';
-import {createDiffUrl} from '../../../models/views/diff';
+import {createDiffUrl} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {modalStyles} from '../../../styles/gr-modal-styles';
@@ -547,8 +547,9 @@
${this.renderDraftLabel()}
</div>
<div class="headerMiddle">${this.renderCollapsedContent()}</div>
- ${this.renderRunDetails()} ${this.renderDeleteButton()}
- ${this.renderPatchset()} ${this.renderDate()} ${this.renderToggle()}
+ ${this.renderSuggestEditButton()} ${this.renderRunDetails()}
+ ${this.renderDeleteButton()} ${this.renderPatchset()}
+ ${this.renderDate()} ${this.renderToggle()}
</div>
`;
}
@@ -777,10 +778,9 @@
return html`
<div class="rightActions">
${this.autoSaving ? html`. ` : ''}
- ${this.renderDiscardButton()} ${this.renderSuggestEditButton()}
- ${this.renderPreviewSuggestEditButton()} ${this.renderEditButton()}
- ${this.renderCancelButton()} ${this.renderSaveButton()}
- ${this.renderCopyLinkIcon()}
+ ${this.renderDiscardButton()} ${this.renderPreviewSuggestEditButton()}
+ ${this.renderEditButton()} ${this.renderCancelButton()}
+ ${this.renderSaveButton()} ${this.renderCopyLinkIcon()}
</div>
`;
}
@@ -809,6 +809,7 @@
return nothing;
}
if (
+ !this.editing ||
this.permanentEditingMode ||
this.comment?.path === SpecialFilePath.PATCHSET_LEVEL_COMMENTS
) {
@@ -1139,7 +1140,8 @@
fire(this, 'open-fix-preview', await this.createFixPreview());
}
- async createSuggestEdit() {
+ async createSuggestEdit(e: MouseEvent) {
+ e.stopPropagation();
const line = await this.getCommentedCode();
this.messageText += `${USER_SUGGESTION_START_PATTERN}${line}${'\n```'}`;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
index ec9c875..3390369 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
@@ -854,12 +854,12 @@
.initiallyCollapsed=${false}
></gr-comment>`
);
+ element.editing = true;
});
test('renders suggest fix button', () => {
assert.dom.equal(
queryAndAssert(element, 'gr-button.suggestEdit'),
/* HTML */ `<gr-button
- aria-disabled="false"
class="action suggestEdit"
link=""
role="button"
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 2bde847..446822f 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -38,10 +38,13 @@
import {UserModel} from '../user/user-model';
import {define} from '../dependency';
import {isOwner} from '../../utils/change-util';
-import {ChangeViewModel, createChangeUrl} from '../views/change';
-import {createDiffUrl} from '../views/diff';
+import {
+ ChangeViewModel,
+ createChangeUrl,
+ createDiffUrl,
+ createEditUrl,
+} from '../views/change';
import {NavigationService} from '../../elements/core/gr-navigation/gr-navigation';
-import {createEditUrl} from '../views/edit';
export enum LoadingStatus {
NOT_LOADED = 'NOT_LOADED',
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index e3570d1..a206037 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -10,6 +10,7 @@
BasePatchSetNum,
ChangeInfo,
PatchSetNumber,
+ EDIT,
} from '../../api/rest-api';
import {Tab} from '../../constants/constants';
import {GerritView} from '../../services/router/router-model';
@@ -25,8 +26,6 @@
import {define} from '../dependency';
import {Model} from '../model';
import {ViewState} from './base';
-import {createDiffUrl} from './diff';
-import {createEditUrl} from './edit';
export enum ChangeChildView {
OVERVIEW = 'OVERVIEW',
@@ -143,11 +142,8 @@
...obj,
childView: ChangeChildView.OVERVIEW,
});
- let range = getPatchRangeExpression(state);
- if (range.length) {
- range = '/' + range;
- }
- let suffix = `${range}`;
+
+ let suffix = '';
const queries = [];
if (state.checksPatchset && state.checksPatchset > 0) {
queries.push(`checksPatchset=${state.checksPatchset}`);
@@ -180,7 +176,7 @@
suffix += ',edit';
}
if (state.commentId) {
- suffix = suffix + `/comments/${state.commentId}`;
+ suffix += `/comments/${state.commentId}`;
}
if (queries.length > 0) {
suffix += '?' + queries.join('&');
@@ -188,12 +184,67 @@
if (state.messageHash) {
suffix += state.messageHash;
}
- if (state.repo) {
- const encodedProject = encodeURL(state.repo, true);
- return `${getBaseUrl()}/c/${encodedProject}/+/${state.changeNum}${suffix}`;
- } else {
- return `${getBaseUrl()}/c/${state.changeNum}${suffix}`;
+
+ return `${createChangeUrlCommon(state)}${suffix}`;
+}
+
+export function createDiffUrl(
+ obj: CreateChangeUrlObject | Omit<ChangeViewState, 'view' | 'childView'>
+) {
+ const state: ChangeViewState = objToState({
+ ...obj,
+ childView: ChangeChildView.DIFF,
+ });
+
+ const path = `/${encodeURL(state.diffView?.path ?? '', true)}`;
+
+ let suffix = '';
+ // TODO: Move creating of comment URLs to a separate function. We are
+ // "abusing" the `commentId` property, which should only be used for pointing
+ // to comment in the COMMENTS tab of the OVERVIEW page.
+ if (state.commentId) {
+ suffix += `comment/${state.commentId}/`;
}
+
+ if (state.diffView?.lineNum) {
+ suffix += '#';
+ if (state.diffView?.leftSide) {
+ suffix += 'b';
+ }
+ suffix += state.diffView.lineNum;
+ }
+
+ return `${createChangeUrlCommon(state)}${path}${suffix}`;
+}
+
+export function createEditUrl(
+ obj: Omit<ChangeViewState, 'view' | 'childView'>
+): string {
+ const state: ChangeViewState = objToState({
+ ...obj,
+ childView: ChangeChildView.DIFF,
+ patchNum: obj.patchNum ?? EDIT,
+ });
+
+ const path = `/${encodeURL(state.editView?.path ?? '', true)}`;
+ const line = state.editView?.lineNum;
+ const suffix = line ? `#${line}` : '';
+
+ return `${createChangeUrlCommon(state)}${path},edit${suffix}`;
+}
+
+/**
+ * The shared part of creating a change URL between OVERVIEW, DIFF and EDIT
+ * child views.
+ */
+function createChangeUrlCommon(state: ChangeViewState) {
+ let range = getPatchRangeExpression(state);
+ if (range.length) range = '/' + range;
+
+ let repo = '';
+ if (state.repo) repo = `${encodeURL(state.repo, true)}/+/`;
+
+ return `${getBaseUrl()}/c/${repo}${state.changeNum}${range}`;
}
export const changeViewModelToken =
diff --git a/polygerrit-ui/app/models/views/change_test.ts b/polygerrit-ui/app/models/views/change_test.ts
index ca6f104..837e362 100644
--- a/polygerrit-ui/app/models/views/change_test.ts
+++ b/polygerrit-ui/app/models/views/change_test.ts
@@ -10,8 +10,17 @@
RevisionPatchSetNum,
} from '../../api/rest-api';
import '../../test/common-test-setup';
-import {createChangeViewState} from '../../test/test-data-generators';
-import {createChangeUrl, ChangeViewState} from './change';
+import {
+ createChangeViewState,
+ createDiffViewState,
+ createEditViewState,
+} from '../../test/test-data-generators';
+import {
+ createChangeUrl,
+ createDiffUrl,
+ createEditUrl,
+ ChangeViewState,
+} from './change';
suite('change view state tests', () => {
test('createChangeUrl()', () => {
@@ -67,4 +76,75 @@
};
assert.equal(createChangeUrl(state), '/c/x%252B/y%252B/z%252B/w/+/42');
});
+
+ test('createDiffUrl', () => {
+ const params: ChangeViewState = {
+ ...createDiffViewState(),
+ patchNum: 12 as RevisionPatchSetNum,
+ diffView: {path: 'x+y/path.cpp'},
+ };
+ assert.equal(
+ createDiffUrl(params),
+ '/c/test-project/+/42/12/x%252By/path.cpp'
+ );
+
+ window.CANONICAL_PATH = '/base';
+ assert.equal(createDiffUrl(params).substring(0, 5), '/base');
+ window.CANONICAL_PATH = undefined;
+
+ params.repo = 'test' as RepoName;
+ assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%252By/path.cpp');
+
+ params.basePatchNum = 6 as BasePatchSetNum;
+ assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%252By/path.cpp');
+
+ params.diffView = {
+ path: 'foo bar/my+file.txt%',
+ };
+ params.patchNum = 2 as RevisionPatchSetNum;
+ delete params.basePatchNum;
+ assert.equal(
+ createDiffUrl(params),
+ '/c/test/+/42/2/foo+bar/my%252Bfile.txt%2525'
+ );
+
+ params.diffView = {
+ path: 'file.cpp',
+ lineNum: 123,
+ };
+ assert.equal(createDiffUrl(params), '/c/test/+/42/2/file.cpp#123');
+
+ params.diffView = {
+ path: 'file.cpp',
+ lineNum: 123,
+ leftSide: true,
+ };
+ assert.equal(createDiffUrl(params), '/c/test/+/42/2/file.cpp#b123');
+ });
+
+ test('diff with repo name encoding', () => {
+ const params: ChangeViewState = {
+ ...createDiffViewState(),
+ patchNum: 12 as RevisionPatchSetNum,
+ repo: 'x+/y' as RepoName,
+ diffView: {path: 'x+y/path.cpp'},
+ };
+ assert.equal(createDiffUrl(params), '/c/x%252B/y/+/42/12/x%252By/path.cpp');
+ });
+
+ test('createEditUrl', () => {
+ const params: ChangeViewState = {
+ ...createEditViewState(),
+ patchNum: 12 as RevisionPatchSetNum,
+ editView: {path: 'x+y/path.cpp' as RepoName, lineNum: 31},
+ };
+ assert.equal(
+ createEditUrl(params),
+ '/c/test-project/+/42/12/x%252By/path.cpp,edit#31'
+ );
+
+ window.CANONICAL_PATH = '/base';
+ assert.equal(createEditUrl(params).substring(0, 5), '/base');
+ window.CANONICAL_PATH = undefined;
+ });
});
diff --git a/polygerrit-ui/app/models/views/diff.ts b/polygerrit-ui/app/models/views/diff.ts
deleted file mode 100644
index 961c9d5..0000000
--- a/polygerrit-ui/app/models/views/diff.ts
+++ /dev/null
@@ -1,52 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {
- encodeURL,
- getBaseUrl,
- getPatchRangeExpression,
-} from '../../utils/url-util';
-import {
- ChangeChildView,
- ChangeViewState,
- CreateChangeUrlObject,
- objToState,
-} from './change';
-
-// TODO: Move to change.ts.
-export function createDiffUrl(
- obj: CreateChangeUrlObject | Omit<ChangeViewState, 'view' | 'childView'>
-) {
- const state: ChangeViewState = objToState({
- ...obj,
- childView: ChangeChildView.DIFF,
- });
- let range = getPatchRangeExpression(state);
- if (range.length) range = '/' + range;
-
- let suffix = `${range}/${encodeURL(state.diffView?.path ?? '', true)}`;
-
- if (state.diffView?.lineNum) {
- suffix += '#';
- if (state.diffView?.leftSide) {
- suffix += 'b';
- }
- suffix += state.diffView.lineNum;
- }
-
- // TODO: Move creating of comment URLs to a separate function. We are
- // "abusing" the `commentId` property, which should only be used for pointing
- // to comment in the COMMENTS tab of the OVERVIEW page.
- if (state.commentId) {
- suffix = `/comment/${state.commentId}` + suffix;
- }
-
- if (state.repo) {
- const encodedProject = encodeURL(state.repo, true);
- return `${getBaseUrl()}/c/${encodedProject}/+/${state.changeNum}${suffix}`;
- } else {
- return `${getBaseUrl()}/c/${state.changeNum}${suffix}`;
- }
-}
diff --git a/polygerrit-ui/app/models/views/diff_test.ts b/polygerrit-ui/app/models/views/diff_test.ts
deleted file mode 100644
index 851bed7..0000000
--- a/polygerrit-ui/app/models/views/diff_test.ts
+++ /dev/null
@@ -1,72 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {assert} from '@open-wc/testing';
-import {
- BasePatchSetNum,
- RepoName,
- RevisionPatchSetNum,
-} from '../../api/rest-api';
-import '../../test/common-test-setup';
-import {createDiffViewState} from '../../test/test-data-generators';
-import {ChangeViewState} from './change';
-import {createDiffUrl} from './diff';
-
-suite('diff view state tests', () => {
- test('createDiffUrl', () => {
- const params: ChangeViewState = {
- ...createDiffViewState(),
- patchNum: 12 as RevisionPatchSetNum,
- diffView: {path: 'x+y/path.cpp'},
- };
- assert.equal(
- createDiffUrl(params),
- '/c/test-project/+/42/12/x%252By/path.cpp'
- );
-
- window.CANONICAL_PATH = '/base';
- assert.equal(createDiffUrl(params).substring(0, 5), '/base');
- window.CANONICAL_PATH = undefined;
-
- params.repo = 'test' as RepoName;
- assert.equal(createDiffUrl(params), '/c/test/+/42/12/x%252By/path.cpp');
-
- params.basePatchNum = 6 as BasePatchSetNum;
- assert.equal(createDiffUrl(params), '/c/test/+/42/6..12/x%252By/path.cpp');
-
- params.diffView = {
- path: 'foo bar/my+file.txt%',
- };
- params.patchNum = 2 as RevisionPatchSetNum;
- delete params.basePatchNum;
- assert.equal(
- createDiffUrl(params),
- '/c/test/+/42/2/foo+bar/my%252Bfile.txt%2525'
- );
-
- params.diffView = {
- path: 'file.cpp',
- lineNum: 123,
- };
- assert.equal(createDiffUrl(params), '/c/test/+/42/2/file.cpp#123');
-
- params.diffView = {
- path: 'file.cpp',
- lineNum: 123,
- leftSide: true,
- };
- assert.equal(createDiffUrl(params), '/c/test/+/42/2/file.cpp#b123');
- });
-
- test('diff with repo name encoding', () => {
- const params: ChangeViewState = {
- ...createDiffViewState(),
- patchNum: 12 as RevisionPatchSetNum,
- repo: 'x+/y' as RepoName,
- diffView: {path: 'x+y/path.cpp'},
- };
- assert.equal(createDiffUrl(params), '/c/x%252B/y/+/42/12/x%252By/path.cpp');
- });
-});
diff --git a/polygerrit-ui/app/models/views/edit.ts b/polygerrit-ui/app/models/views/edit.ts
deleted file mode 100644
index a12cd85..0000000
--- a/polygerrit-ui/app/models/views/edit.ts
+++ /dev/null
@@ -1,38 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {EDIT} from '../../api/rest-api';
-import {
- encodeURL,
- getBaseUrl,
- getPatchRangeExpression,
-} from '../../utils/url-util';
-import {ChangeViewState} from './change';
-
-// TODO: Move to change.ts.
-export function createEditUrl(
- state: Omit<ChangeViewState, 'view' | 'childView'>
-): string {
- if (state.patchNum === undefined) {
- state = {...state, patchNum: EDIT};
- }
- let range = getPatchRangeExpression(state);
- if (range.length) range = '/' + range;
-
- let suffix = `${range}/${encodeURL(state.editView?.path ?? '', true)}`;
- suffix += ',edit';
-
- if (state.editView?.lineNum) {
- suffix += '#';
- suffix += state.editView.lineNum;
- }
-
- if (state.repo) {
- const encodedProject = encodeURL(state.repo, true);
- return `${getBaseUrl()}/c/${encodedProject}/+/${state.changeNum}${suffix}`;
- } else {
- return `${getBaseUrl()}/c/${state.changeNum}${suffix}`;
- }
-}
diff --git a/polygerrit-ui/app/models/views/edit_test.ts b/polygerrit-ui/app/models/views/edit_test.ts
deleted file mode 100644
index be8fb70..0000000
--- a/polygerrit-ui/app/models/views/edit_test.ts
+++ /dev/null
@@ -1,29 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {assert} from '@open-wc/testing';
-import {RepoName, RevisionPatchSetNum} from '../../api/rest-api';
-import '../../test/common-test-setup';
-import {createEditViewState} from '../../test/test-data-generators';
-import {ChangeViewState} from './change';
-import {createEditUrl} from './edit';
-
-suite('edit view state tests', () => {
- test('createEditUrl', () => {
- const params: ChangeViewState = {
- ...createEditViewState(),
- patchNum: 12 as RevisionPatchSetNum,
- editView: {path: 'x+y/path.cpp' as RepoName, lineNum: 31},
- };
- assert.equal(
- createEditUrl(params),
- '/c/test-project/+/42/12/x%252By/path.cpp,edit#31'
- );
-
- window.CANONICAL_PATH = '/base';
- assert.equal(createEditUrl(params).substring(0, 5), '/base');
- window.CANONICAL_PATH = undefined;
- });
-});
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 572e107..2a5dff2 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -22,4 +22,5 @@
SUGGEST_EDIT = 'UiFeature__suggest_edit',
MENTION_USERS = 'UiFeature__mention_users',
RENDER_MARKDOWN = 'UiFeature__render_markdown',
+ REBASE_CHAIN = 'UiFeature__rebase_chain',
}
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
index 746ecf3..0d0c88f 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -143,7 +143,7 @@
import {ParsedChangeInfo} from '../../types/types';
import {ErrorCallback} from '../../api/rest';
import {addDraftProp, DraftInfo} from '../../utils/comment-util';
-import {BaseScheduler} from '../scheduler/scheduler';
+import {BaseScheduler, Scheduler} from '../scheduler/scheduler';
import {MaxInFlightScheduler} from '../scheduler/max-in-flight-scheduler';
import {escapeAndWrapSearchOperatorValue} from '../../utils/string-util';
@@ -270,6 +270,11 @@
function createWriteScheduler() {
return new MaxInFlightScheduler<Response>(new BaseScheduler<Response>(), 5);
}
+
+function createSerializingScheduler() {
+ return new MaxInFlightScheduler<Response>(new BaseScheduler<Response>(), 1);
+}
+
export class GrRestApiServiceImpl implements RestApiService, Finalizable {
readonly _cache = siteBasedCache; // Shared across instances.
@@ -286,6 +291,9 @@
// Private, but used in tests.
readonly _restApiHelper: GrRestApiHelper;
+ // Used to serialize requests for certain RPCs
+ readonly _serialScheduler: Scheduler<Response>;
+
constructor(private readonly authService: AuthService) {
this._restApiHelper = new GrRestApiHelper(
this._cache,
@@ -294,6 +302,7 @@
createReadScheduler(),
createWriteScheduler()
);
+ this._serialScheduler = createSerializingScheduler();
}
finalize() {}
@@ -2232,11 +2241,13 @@
return this.getFromProjectLookup(changeNum).then(project => {
const encodedRepoName = project ? encodeURIComponent(project) + '~' : '';
const url = `/accounts/self/starred.changes/${encodedRepoName}${changeNum}`;
- return this._restApiHelper.send({
- method: starred ? HttpMethod.PUT : HttpMethod.DELETE,
- url,
- anonymizedUrl: '/accounts/self/starred.changes/*',
- });
+ return this._serialScheduler.schedule(() =>
+ this._restApiHelper.send({
+ method: starred ? HttpMethod.PUT : HttpMethod.DELETE,
+ url,
+ anonymizedUrl: '/accounts/self/starred.changes/*',
+ })
+ );
});
}
