Merge "InternalChangeQuery: Remove unused methods"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 9a40b27..185fa07 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -1370,10 +1370,11 @@
[[capability_createProject]]
=== Create Project
-Allow project creation. This capability allows the granted group to
-either link:cmd-create-project.html[create new git projects via ssh]
-or via the web UI.
+Allow project creation.
+This capability allows the granted group to create projects via the web UI, via
+link:rest-api-projects.html#create-project][REST] and via
+link:cmd-create-project.html[SSH].
[[capability_emailReviewers]]
=== Email Reviewers
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index 565c491..e12c27c 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -43,6 +43,17 @@
For more predictable results, use explicit search operators as described
in the following section.
+[IMPORTANT]
+--
+The change search API is backed by a secondary index and might sometimes return
+stale results if the re-indexing operation failed for a change update.
+
+Please also note that changes are not re-indexed if the project configuration
+is updated with newly added or modified
+link:config-submit-requirements.html[submit requirements].
+--
+
+
[[search-operators]]
== Search Operators
diff --git a/java/com/google/gerrit/server/IdentifiedUser.java b/java/com/google/gerrit/server/IdentifiedUser.java
index 65a81f7..eda6e09 100644
--- a/java/com/google/gerrit/server/IdentifiedUser.java
+++ b/java/com/google/gerrit/server/IdentifiedUser.java
@@ -100,30 +100,30 @@
enablePeerIPInReflogRecord,
Providers.of(null),
state,
- null);
+ /* realUser= */ null);
}
public IdentifiedUser create(Account.Id id) {
- return create(null, id);
+ return create(/* remotePeer= */ null, id);
}
@VisibleForTesting
@UsedAt(UsedAt.Project.GOOGLE)
public IdentifiedUser forTest(Account.Id id, PropertyMap properties) {
- return runAs(null, id, null, properties);
+ return runAs(/* remotePeer= */ null, id, /* caller= */ null, properties);
}
- public IdentifiedUser create(SocketAddress remotePeer, Account.Id id) {
- return runAs(remotePeer, id, null);
+ public IdentifiedUser create(@Nullable SocketAddress remotePeer, Account.Id id) {
+ return runAs(remotePeer, id, /* caller= */ null);
}
public IdentifiedUser runAs(
- SocketAddress remotePeer, Account.Id id, @Nullable CurrentUser caller) {
+ @Nullable SocketAddress remotePeer, Account.Id id, @Nullable CurrentUser caller) {
return runAs(remotePeer, id, caller, PropertyMap.EMPTY);
}
private IdentifiedUser runAs(
- SocketAddress remotePeer,
+ @Nullable SocketAddress remotePeer,
Account.Id id,
@Nullable CurrentUser caller,
PropertyMap properties) {
@@ -244,7 +244,7 @@
AccountCache accountCache,
GroupBackend groupBackend,
Boolean enablePeerIPInReflogRecord,
- @Nullable Provider<SocketAddress> remotePeerProvider,
+ Provider<SocketAddress> remotePeerProvider,
AccountState state,
@Nullable CurrentUser realUser) {
this(
@@ -270,7 +270,7 @@
AccountCache accountCache,
GroupBackend groupBackend,
Boolean enablePeerIPInReflogRecord,
- @Nullable Provider<SocketAddress> remotePeerProvider,
+ Provider<SocketAddress> remotePeerProvider,
Account.Id id,
@Nullable CurrentUser realUser,
PropertyMap properties) {
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 389b292..fcfc805 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -211,7 +211,7 @@
return searchedAsUser.asIdentifiedUser();
}
return userFactory.runAs(
- null, list.get(0).account().id(), requireNonNull(caller).getRealUser());
+ /* remotePeer= */ null, list.get(0).account().id(), requireNonNull(caller).getRealUser());
}
@VisibleForTesting
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index dcbd1ae..8acc925 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
@@ -37,7 +35,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
@@ -70,30 +67,28 @@
this.rebaseFactory = rebaseFactory;
}
- public static void verifyRebasePreconditions(
- ProjectCache projectCache, PatchSetUtil patchSetUtil, RevWalk rw, RevisionResource rsrc)
- throws ResourceConflictException, IOException, AuthException, PermissionBackendException {
+ /**
+ * Checks whether the given change fulfills all preconditions to be rebased.
+ *
+ * <p>This method does not check whether the calling user is allowed to rebase the change.
+ */
+ public void verifyRebasePreconditions(RevWalk rw, ChangeNotes changeNotes, PatchSet patchSet)
+ throws ResourceConflictException, IOException {
// Not allowed to rebase if the current patch set is locked.
- patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes());
+ psUtil.checkPatchSetNotLocked(changeNotes);
- rsrc.permissions().check(ChangePermission.REBASE);
- projectCache
- .get(rsrc.getProject())
- .orElseThrow(illegalState(rsrc.getProject()))
- .checkStatePermitsWrite();
-
- if (!rsrc.getChange().isNew()) {
+ Change change = changeNotes.getChange();
+ if (!change.isNew()) {
throw new ResourceConflictException(
- String.format(
- "Change %s is %s", rsrc.getChange().getId(), ChangeUtil.status(rsrc.getChange())));
- } else if (!hasOneParent(rw, rsrc.getPatchSet())) {
+ String.format("Change %s is %s", change.getId(), ChangeUtil.status(change)));
+ }
+
+ if (!hasOneParent(rw, patchSet)) {
throw new ResourceConflictException(
String.format(
"Error rebasing %s. Cannot rebase %s",
- rsrc.getChange().getId(),
- countParents(rw, rsrc.getPatchSet()) > 1
- ? "merge commits"
- : "commit with no ancestor"));
+ change.getId(),
+ countParents(rw, patchSet) > 1 ? "merge commits" : "commit with no ancestor"));
}
}
diff --git a/java/com/google/gerrit/server/project/ProjectCacheImpl.java b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
index 0afaa3f..6498d1b 100644
--- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
@@ -76,7 +76,6 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
-import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
@@ -301,20 +300,19 @@
@Override
public Set<AccountGroup.UUID> guessRelevantGroupUUIDs() {
try (Timer0.Context ignored = guessRelevantGroupsLatency.start()) {
- Stream<AccountGroup.UUID> configuredRelevantGroups =
- Arrays.stream(config.getStringList("groups", /* subsection= */ null, "relevantGroup"))
- .map(AccountGroup::uuid);
-
- Stream<AccountGroup.UUID> guessedRelevantGroups =
- inMemoryProjectCache.asMap().values().stream()
- .filter(Objects::nonNull)
- .flatMap(p -> p.getAllGroupUUIDs().stream())
- // getAllGroupUUIDs shouldn't really return null UUIDs, but harden
- // against them just in case there is a bug or corner case.
- .filter(id -> id != null && id.get() != null);
-
Set<AccountGroup.UUID> relevantGroupUuids =
- Streams.concat(configuredRelevantGroups, guessedRelevantGroups).collect(toSet());
+ Streams.concat(
+ Arrays.stream(
+ config.getStringList("groups", /* subsection= */ null, "relevantGroup"))
+ .map(AccountGroup::uuid),
+ all().stream()
+ .map(n -> inMemoryProjectCache.getIfPresent(n))
+ .filter(Objects::nonNull)
+ .flatMap(p -> p.getAllGroupUUIDs().stream())
+ // getAllGroupUUIDs shouldn't really return null UUIDs, but harden
+ // against them just in case there is a bug or corner case.
+ .filter(id -> id != null && id.get() != null))
+ .collect(toSet());
logger.atFine().log("relevant group UUIDs: %s", relevantGroupUuids);
return relevantGroupUuids;
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 714516c..ca18ab2 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -551,14 +551,14 @@
@Operator
public Predicate<ChangeData> mergedBefore(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_BEFORE);
return new BeforePredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_BEFORE, value);
}
@Operator
public Predicate<ChangeData> mergedAfter(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
+ checkOperatorAvailable(ChangeField.MERGED_ON_SPEC, OPERATOR_MERGED_AFTER);
return new AfterPredicate(
ChangeField.MERGED_ON_SPEC, ChangeQueryBuilder.OPERATOR_MERGED_AFTER, value);
}
@@ -641,7 +641,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "has:attention");
return new IsAttentionPredicate();
}
@@ -684,7 +684,7 @@
}
if ("uploader".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "is:uploader");
return ChangePredicates.uploader(self());
}
@@ -707,7 +707,7 @@
}
if ("merge".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.MERGE_SPEC, "is:merge");
+ checkOperatorAvailable(ChangeField.MERGE_SPEC, "is:merge");
return new BooleanPredicate(ChangeField.MERGE_SPEC);
}
@@ -716,7 +716,7 @@
}
if ("attention".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "is:attention");
return new IsAttentionPredicate();
}
@@ -729,7 +729,7 @@
}
if ("pure-revert".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
+ checkOperatorAvailable(ChangeField.IS_PURE_REVERT_SPEC, "is:pure-revert");
return ChangePredicates.pureRevert("1");
}
@@ -745,12 +745,12 @@
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.NOT_READY)),
Predicate.not(new SubmittablePredicate(SubmitRecord.Status.RULE_ERROR)));
}
- checkFieldAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
+ checkOperatorAvailable(ChangeField.IS_SUBMITTABLE_SPEC, "is:submittable");
return new IsSubmittablePredicate();
}
if ("started".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.STARTED_SPEC, "is:started");
+ checkOperatorAvailable(ChangeField.STARTED_SPEC, "is:started");
return new BooleanPredicate(ChangeField.STARTED_SPEC);
}
@@ -759,7 +759,7 @@
}
if ("cherrypick".equalsIgnoreCase(value)) {
- checkFieldAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_SPEC, "is:cherrypick");
return new BooleanPredicate(ChangeField.CHERRY_PICK_SPEC);
}
@@ -894,7 +894,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
+ checkOperatorAvailable(ChangeField.FUZZY_HASHTAG, "inhashtag");
return ChangePredicates.fuzzyHashtag(hashtag);
}
@@ -904,7 +904,7 @@
return ChangePredicates.hashtag(hashtag);
}
- checkFieldAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
+ checkOperatorAvailable(ChangeField.PREFIX_HASHTAG, "prefixhashtag");
return ChangePredicates.prefixHashtag(hashtag);
}
@@ -930,7 +930,7 @@
return ChangePredicates.exactTopic(name);
}
- checkFieldAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
+ checkOperatorAvailable(ChangeField.PREFIX_TOPIC, "prefixtopic");
return ChangePredicates.prefixTopic(name);
}
@@ -996,7 +996,7 @@
@Operator
public Predicate<ChangeData> hasfooter(String footerName) throws QueryParseException {
- checkFieldAvailable(ChangeField.FOOTER_NAME, "hasfooter");
+ checkOperatorAvailable(ChangeField.FOOTER_NAME, "hasfooter");
return ChangePredicates.hasFooter(footerName);
}
@@ -1147,7 +1147,9 @@
@Operator
public Predicate<ChangeData> message(String text) throws QueryParseException {
if (text.startsWith("^")) {
- checkFieldAvailable(ChangeField.COMMIT_MESSAGE_EXACT, "messageexact");
+ checkFieldAvailable(
+ ChangeField.COMMIT_MESSAGE_EXACT,
+ "'message' operator with regular expression is not supported on this gerrit host");
return new RegexMessagePredicate(text);
}
return ChangePredicates.message(text);
@@ -1155,13 +1157,14 @@
@Operator
public Predicate<ChangeData> subject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
+ checkOperatorAvailable(ChangeField.SUBJECT_SPEC, ChangeQueryBuilder.FIELD_SUBJECT);
return ChangePredicates.subject(value);
}
@Operator
public Predicate<ChangeData> prefixsubject(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
+ checkOperatorAvailable(
+ ChangeField.PREFIX_SUBJECT_SPEC, ChangeQueryBuilder.FIELD_PREFIX_SUBJECT);
return ChangePredicates.prefixSubject(value);
}
@@ -1249,7 +1252,7 @@
@Operator
public Predicate<ChangeData> uploader(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploader");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploader");
return uploader(parseAccount(who, (AccountState s) -> true));
}
@@ -1264,7 +1267,7 @@
@Operator
public Predicate<ChangeData> attention(String who)
throws QueryParseException, IOException, ConfigInvalidException {
- checkFieldAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
+ checkOperatorAvailable(ChangeField.ATTENTION_SET_USERS, "attention");
return attention(parseAccount(who, (AccountState s) -> true));
}
@@ -1309,7 +1312,7 @@
@Operator
public Predicate<ChangeData> uploaderin(String group) throws QueryParseException, IOException {
- checkFieldAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
+ checkOperatorAvailable(ChangeField.UPLOADER_SPEC, "uploaderin");
GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
if (g == null) {
@@ -1575,8 +1578,8 @@
@Operator
public Predicate<ChangeData> cherryPickOf(String value) throws QueryParseException {
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
- checkFieldAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_CHANGE, "cherryPickOf");
+ checkOperatorAvailable(ChangeField.CHERRY_PICK_OF_PATCHSET, "cherryPickOf");
if (Ints.tryParse(value) != null) {
return ChangePredicates.cherryPickOf(Change.id(Ints.tryParse(value)));
}
@@ -1648,11 +1651,16 @@
return Predicate.or(predicates);
}
- protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String operator)
+ private void checkOperatorAvailable(SchemaField<ChangeData, ?> field, String operator)
+ throws QueryParseException {
+ checkFieldAvailable(
+ field, String.format("'%s' operator is not supported on this gerrit host", operator));
+ }
+
+ protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String errorMessage)
throws QueryParseException {
if (!args.index.getSchema().hasField(field)) {
- throw new QueryParseException(
- String.format("'%s' operator is not supported on this gerrit host", operator));
+ throw new QueryParseException(errorMessage);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 1a8f07a..8a8d2ca 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -87,6 +87,13 @@
@Override
public Response<ChangeInfo> apply(RevisionResource rsrc, RebaseInput input)
throws UpdateException, RestApiException, IOException, PermissionBackendException {
+ rsrc.permissions().check(ChangePermission.REBASE);
+
+ projectCache
+ .get(rsrc.getProject())
+ .orElseThrow(illegalState(rsrc.getProject()))
+ .checkStatePermitsWrite();
+
Change change = rsrc.getChange();
try (Repository repo = repoManager.openRepository(change.getProject());
ObjectInserter oi = repo.newObjectInserter();
@@ -94,7 +101,7 @@
RevWalk rw = CodeReviewCommit.newRevWalk(reader);
BatchUpdate bu =
updateFactory.create(change.getProject(), rsrc.getUser(), TimeUtil.now())) {
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, rsrc);
+ rebaseUtil.verifyRebasePreconditions(rw, rsrc.getNotes(), rsrc.getPatchSet());
RebaseChangeOp rebaseOp =
rebaseUtil.getRebaseOp(
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 4754c69..786bba7 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -113,7 +113,11 @@
@Override
public Response<RebaseChainInfo> apply(ChangeResource tipRsrc, RebaseInput input)
throws IOException, PermissionBackendException, RestApiException, UpdateException {
+ tipRsrc.permissions().check(ChangePermission.REBASE);
+
Project.NameKey project = tipRsrc.getProject();
+ projectCache.get(project).orElseThrow(illegalState(project)).checkStatePermitsWrite();
+
CurrentUser user = tipRsrc.getUser();
List<Change.Id> upToDateAncestors = new ArrayList<>();
@@ -136,7 +140,8 @@
RevisionResource revRsrc =
new RevisionResource(changeResourceFactory.create(changeData, user), ps);
- RebaseUtil.verifyRebasePreconditions(projectCache, patchSetUtil, rw, revRsrc);
+ revRsrc.permissions().check(ChangePermission.REBASE);
+ rebaseUtil.verifyRebasePreconditions(rw, changeData.notes(), ps);
boolean isUpToDate = false;
RebaseChangeOp rebaseOp = null;
diff --git a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
index 491b5cd..f3c741f 100644
--- a/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
+++ b/java/com/google/gerrit/server/schema/MigrateLabelFunctionsToSubmitRequirement.java
@@ -37,6 +37,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
+import java.util.stream.Collectors;
import javax.inject.Inject;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
@@ -216,12 +217,19 @@
Arrays.asList(
cfg.getStringList(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_VALUE)))
.build();
+ ImmutableList<String> refPatterns =
+ ImmutableList.<String>builder()
+ .addAll(
+ Arrays.asList(
+ cfg.getStringList(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_BRANCH)))
+ .build();
LabelAttributes attributes =
LabelAttributes.create(
function == null ? "MaxWithBlock" : function,
canOverride,
ignoreSelfApproval,
- values);
+ values,
+ refPatterns);
labelTypes.put(labelName, attributes);
}
return labelTypes;
@@ -320,6 +328,15 @@
default:
break;
}
+ if (!attributes.refPatterns().isEmpty()) {
+ builder.setApplicabilityExpression(
+ SubmitRequirementExpression.of(
+ String.join(
+ " OR ",
+ attributes.refPatterns().stream()
+ .map(b -> "branch:\\\"" + b + "\\\"")
+ .collect(Collectors.toList()))));
+ }
return builder.build();
}
@@ -435,13 +452,16 @@
abstract ImmutableList<String> values();
+ abstract ImmutableList<String> refPatterns();
+
static LabelAttributes create(
String function,
boolean canOverride,
boolean ignoreSelfApproval,
- ImmutableList<String> values) {
+ ImmutableList<String> values,
+ ImmutableList<String> refPatterns) {
return new AutoValue_MigrateLabelFunctionsToSubmitRequirement_LabelAttributes(
- function, canOverride, ignoreSelfApproval, values);
+ function, canOverride, ignoreSelfApproval, values, refPatterns);
}
}
}
diff --git a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
index 74bfe0f..9d37497 100644
--- a/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/pgm/MigrateLabelFunctionsToSubmitRequirementIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -260,6 +261,96 @@
}
@Test
+ public void migrateBlockingLabel_withBranchAttribute() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("refs/heads/master"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_withMultipleBranchAttributes() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("refs/heads/master", "refs/heads/develop"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" "
+ + "OR branch:\\\"refs/heads/develop\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_withRegexBranchAttribute() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("^refs/heads/main-.*"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"^refs/heads/main-.*\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
+ public void migrateBlockingLabel_withRegexAndNonRegexBranchAttributes() throws Exception {
+ createLabelWithBranch(
+ "Foo",
+ "MaxWithBlock",
+ /* ignoreSelfApproval= */ false,
+ ImmutableList.of("refs/heads/master", "^refs/heads/main-.*"));
+
+ assertNonExistentSr(/* srName = */ "Foo");
+
+ TestUpdateUI updateUI = runMigration(/* expectedResult= */ Status.MIGRATED);
+ assertThat(updateUI.newlyCreatedSrs).isEqualTo(1);
+ assertThat(updateUI.existingSrsMismatchingWithMigration).isEqualTo(0);
+
+ assertExistentSr(
+ /* srName */ "Foo",
+ /* applicabilityExpression= */ "branch:\\\"refs/heads/master\\\" "
+ + "OR branch:\\\"^refs/heads/main-.*\\\"",
+ /* submittabilityExpression= */ "label:Foo=MAX AND -label:Foo=MIN",
+ /* canOverride= */ true);
+ assertLabelFunction("Foo", "NoBlock");
+ }
+
+ @Test
public void migrationIsIdempotent() throws Exception {
String oldRefsConfigId;
try (Repository repo = repoManager.openRepository(project)) {
@@ -381,6 +472,21 @@
gApi.projects().name(project.get()).label(labelName).create(input);
}
+ private void createLabelWithBranch(
+ String labelName,
+ String function,
+ boolean ignoreSelfApproval,
+ ImmutableList<String> refPatterns)
+ throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.name = labelName;
+ input.function = function;
+ input.ignoreSelfApproval = ignoreSelfApproval;
+ input.values = ImmutableMap.of("+1", "Looks Good", " 0", "Don't Know", "-1", "Looks Bad");
+ input.branches = refPatterns;
+ gApi.projects().name(project.get()).label(labelName).create(input);
+ }
+
@CanIgnoreReturnValue
private SubmitRequirementApi createSubmitRequirement(
String name, String submitExpression, boolean canOverride) throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 2123ac2..15baa78 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -1262,7 +1262,7 @@
+ c
+ "/comment/"
+ ps1List.get(0).id
- + " \n"
+ + " :\n"
+ "PS1, Line 1: initial\n"
+ "what happened to this?\n"
+ "\n"
@@ -1274,7 +1274,7 @@
+ c
+ "/comment/"
+ ps1List.get(1).id
- + " \n"
+ + " :\n"
+ "PS1, Line 1: boring\n"
+ "Is it that bad?\n"
+ "\n"
@@ -1288,7 +1288,7 @@
+ c
+ "/comment/"
+ ps2List.get(0).id
- + " \n"
+ + " :\n"
+ "PS2, Line 1: initial content\n"
+ "comment 1 on base\n"
+ "\n"
@@ -1300,7 +1300,7 @@
+ c
+ "/comment/"
+ ps2List.get(1).id
- + " \n"
+ + " :\n"
+ "PS2, Line 2: \n"
+ "comment 2 on base\n"
+ "\n"
@@ -1312,7 +1312,7 @@
+ c
+ "/comment/"
+ ps2List.get(2).id
- + " \n"
+ + " :\n"
+ "PS2, Line 1: interesting\n"
+ "better now\n"
+ "\n"
@@ -1324,7 +1324,7 @@
+ c
+ "/comment/"
+ ps2List.get(3).id
- + " \n"
+ + " :\n"
+ "PS2, Line 2: cntent\n"
+ "typo: content\n"
+ "\n"
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 61b5e55..9cd002e 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -825,7 +825,8 @@
ImmutableList.of(", ", ":\"", ",", "!@#$%^\0&*):\" \n: \r\"#$@,. :");
for (String strangeTag : strangeTags) {
Change c = newChange();
- CurrentUser otherUserAsOwner = userFactory.runAs(null, changeOwner.getAccountId(), otherUser);
+ CurrentUser otherUserAsOwner =
+ userFactory.runAs(/* remotePeer= */ null, changeOwner.getAccountId(), otherUser);
ChangeUpdate update = newUpdate(c, otherUserAsOwner);
update.putApproval(LabelId.CODE_REVIEW, (short) 2);
update.setTag(strangeTag);
diff --git a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
index b53de89..25f2f98 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -354,7 +354,8 @@
@Test
public void realUser() throws Exception {
Change c = newChange();
- CurrentUser ownerAsOtherUser = userFactory.runAs(null, otherUserId, changeOwner);
+ CurrentUser ownerAsOtherUser =
+ userFactory.runAs(/* remotePeer= */ null, otherUserId, changeOwner);
ChangeUpdate update = newUpdate(c, ownerAsOtherUser);
update.setChangeMessage("Message on behalf of other user");
update.commit();
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 5e6803e..527e78e 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -399,7 +399,9 @@
IdentifiedUser impersonatedChangeOwner =
this.userFactory.runAs(
- null, changeOwner.getAccountId(), requireNonNull(otherUser).getRealUser());
+ /* remotePeer= */ null,
+ changeOwner.getAccountId(),
+ requireNonNull(otherUser).getRealUser());
ChangeUpdate impersonatedChangeMessageUpdate = newUpdate(c, impersonatedChangeOwner);
impersonatedChangeMessageUpdate.setChangeMessage("Other comment on behalf of");
impersonatedChangeMessageUpdate.commit();
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts
index ce9ce2d..e3b3ad4 100644
--- a/polygerrit-ui/app/api/diff.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -201,7 +201,7 @@
syntax_highlighting?: boolean;
tab_size: number;
font_size: number;
- // TODO: Missing documentation
+ // Hides the FILE and LOST diff rows. Default is TRUE.
show_file_comment_button?: boolean;
line_wrapping?: boolean;
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
index 8d6d89a..3599224 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
@@ -25,7 +25,7 @@
RepoState,
SubmitType,
} from '../../../constants/constants';
-import {hasOwnProperty} from '../../../utils/common-util';
+import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
import {firePageError, fireTitleChange} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
import {WebLinkInfo} from '../../../types/diff';
@@ -36,8 +36,9 @@
import {sharedStyles} from '../../../styles/shared-styles';
import {BindValueChangeEvent} from '../../../types/events';
import {deepClone} from '../../../utils/deep-util';
-import {LitElement, PropertyValues, css, html} from 'lit';
+import {LitElement, PropertyValues, css, html, nothing} from 'lit';
import {customElement, property, state} from 'lit/decorators.js';
+import {when} from 'lit/directives/when.js';
import {subscribe} from '../../lit/subscription-controller';
import {createSearchUrl} from '../../../models/views/search';
import {userModelToken} from '../../../models/user/user-model';
@@ -150,16 +151,6 @@
color: var(--deemphasized-text-color);
content: ' *';
}
- .loading,
- .hide {
- display: none;
- }
- #loading.loading {
- display: block;
- }
- #loading:not(.loading) {
- display: none;
- }
#options .repositorySettings {
display: none;
}
@@ -187,49 +178,48 @@
>
</div>
</div>
- <div id="loading" class=${this.loading ? 'loading' : ''}>
- Loading...
- </div>
- <div id="loadedContent" class=${this.loading ? 'loading' : ''}>
- ${this.renderDownloadCommands()}
- <h2
- id="configurations"
- class="heading-2 ${configChanged ? 'edited' : ''}"
- >
- Configurations
- </h2>
- <div id="form">
- <fieldset>
- ${this.renderDescription()} ${this.renderRepoOptions()}
- ${this.renderPluginConfig()}
- <gr-button
- ?disabled=${this.readOnly || !configChanged}
- @click=${this.handleSaveRepoConfig}
- >Save changes</gr-button
- >
- </fieldset>
- <gr-endpoint-decorator name="repo-config">
- <gr-endpoint-param
- name="repoName"
- .value=${this.repo}
- ></gr-endpoint-param>
- <gr-endpoint-param
- name="readOnly"
- .value=${this.readOnly}
- ></gr-endpoint-param>
- </gr-endpoint-decorator>
- </div>
- </div>
+ ${when(
+ this.loading || !this.repoConfig,
+ () => html`<div id="loading">Loading...</div>`,
+ () => html`<div id="loadedContent">
+ ${this.renderDownloadCommands()}
+ <h2
+ id="configurations"
+ class="heading-2 ${configChanged ? 'edited' : ''}"
+ >
+ Configurations
+ </h2>
+ <div id="form">
+ <fieldset>
+ ${this.renderDescription()} ${this.renderRepoOptions()}
+ ${this.renderPluginConfig()}
+ <gr-button
+ ?disabled=${this.readOnly || !configChanged}
+ @click=${this.handleSaveRepoConfig}
+ >Save changes</gr-button
+ >
+ </fieldset>
+ <gr-endpoint-decorator name="repo-config">
+ <gr-endpoint-param
+ name="repoName"
+ .value=${this.repo}
+ ></gr-endpoint-param>
+ <gr-endpoint-param
+ name="readOnly"
+ .value=${this.readOnly}
+ ></gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </div>
+ </div>`
+ )}
</div>
`;
}
private renderDownloadCommands() {
+ if (!this.schemes.length) return nothing;
return html`
- <div
- id="downloadContent"
- class=${!this.schemes || !this.schemes.length ? 'hide' : ''}
- >
+ <div id="downloadContent">
<h2 id="download" class="heading-2">Download</h2>
<fieldset>
<gr-download-commands
@@ -252,6 +242,7 @@
}
private renderDescription() {
+ assertIsDefined(this.repoConfig, 'repoConfig');
return html`
<h3 id="Description" class="heading-3">Description</h3>
<fieldset>
@@ -263,7 +254,7 @@
rows="4"
monospace
?disabled=${this.readOnly}
- .text=${this.repoConfig?.description ?? ''}
+ .text=${this.repoConfig.description ?? ''}
@text-changed=${this.handleDescriptionTextChanged}
></gr-textarea>
</fieldset>
@@ -725,8 +716,9 @@
private renderPluginConfig() {
const pluginData = this.computePluginData();
+ if (!pluginData.length) return nothing;
return html` <div
- class="pluginConfig ${!pluginData || !pluginData.length ? 'hide' : ''}"
+ class="pluginConfig"
@plugin-config-changed=${this.handlePluginConfigChanged}
>
<h3 class="heading-3">Plugins</h3>
@@ -762,6 +754,12 @@
// private but used in test
async loadRepo() {
if (!this.repo) return Promise.resolve();
+ this.repoConfig = undefined;
+ this.originalConfig = undefined;
+ this.loading = true;
+ this.weblinks = [];
+ this.schemesObj = undefined;
+ this.readOnly = true;
const promises = [];
@@ -1121,6 +1119,7 @@
private handleDescriptionTextChanged(e: BindValueChangeEvent) {
if (!this.repoConfig || this.loading) return;
+ if (this.repoConfig.description === e.detail.value) return;
this.repoConfig = {
...this.repoConfig,
description: e.detail.value,
@@ -1130,6 +1129,7 @@
private handleStateSelectBindValueChanged(e: BindValueChangeEvent) {
if (!this.repoConfig || this.loading) return;
+ if (this.repoConfig.state === e.detail.value) return;
this.repoConfig = {
...this.repoConfig,
state: e.detail.value as RepoState,
@@ -1139,6 +1139,7 @@
private handleSubmitTypeSelectBindValueChanged(e: BindValueChangeEvent) {
if (!this.repoConfig || this.loading) return;
+ if (this.repoConfig.submit_type === e.detail.value) return;
this.repoConfig = {
...this.repoConfig,
submit_type: e.detail.value as SubmitType,
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
index c013c9e..4deb99a 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.ts
@@ -157,14 +157,17 @@
element = await fixture(html`<gr-repo></gr-repo>`);
});
- test('render', () => {
+ test('render', async () => {
+ element.repo = REPO as RepoName;
+ await element.loadRepo();
+ await element.updateComplete;
// prettier and shadowDom assert do not agree about span.title wrapping
assert.shadowDom.equal(
element,
/* prettier-ignore */ /* HTML */ `
<div class="gr-form-styles main read-only">
<div class="info">
- <h1 class="heading-1" id="Title"></h1>
+ <h1 class="heading-1" id="Title">test-repo</h1>
<hr />
<div>
<a href="">
@@ -178,7 +181,7 @@
Browse
</gr-button>
</a>
- <a href="">
+ <a href="/q/project:test-repo">
<gr-button
aria-disabled="false"
link=""
@@ -190,15 +193,7 @@
</a>
</div>
</div>
- <div class="loading" id="loading">Loading...</div>
- <div class="loading" id="loadedContent">
- <div class="hide" id="downloadContent">
- <h2 class="heading-2" id="download">Download</h2>
- <fieldset>
- <gr-download-commands id="downloadCommands">
- </gr-download-commands>
- </fieldset>
- </div>
+ <div id="loadedContent">
<h2 class="heading-2" id="configurations">Configurations</h2>
<div id="form">
<fieldset>
@@ -266,7 +261,7 @@
</span>
</section>
<section
- class="repositorySettings"
+ class="repositorySettings showConfig"
id="enableSignedPushSettings"
>
<span class="title"> Enable signed push </span>
@@ -277,7 +272,7 @@
</span>
</section>
<section
- class="repositorySettings"
+ class="repositorySettings showConfig"
id="requireSignedPushSettings"
>
<span class="title"> Require signed push </span>
@@ -379,9 +374,6 @@
</span>
</section>
</fieldset>
- <div class="hide pluginConfig">
- <h3 class="heading-3">Plugins</h3>
- </div>
<gr-button
aria-disabled="true"
disabled=""
@@ -398,7 +390,51 @@
</div>
</div>
</div>
- `
+ `,
+ {ignoreTags: ['option']}
+ );
+ });
+
+ test('render loading', async () => {
+ element.repo = REPO as RepoName;
+ element.loading = true;
+ await element.updateComplete;
+ // prettier and shadowDom assert do not agree about span.title wrapping
+ assert.shadowDom.equal(
+ element,
+ /* prettier-ignore */ /* HTML */ `
+ <div class="gr-form-styles main read-only">
+ <div class="info">
+ <h1 class="heading-1" id="Title">test-repo</h1>
+ <hr />
+ <div>
+ <a href="">
+ <gr-button
+ aria-disabled="true"
+ disabled=""
+ link=""
+ role="button"
+ tabindex="-1"
+ >
+ Browse
+ </gr-button>
+ </a>
+ <a href="/q/project:test-repo">
+ <gr-button
+ aria-disabled="false"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ View Changes
+ </gr-button>
+ </a>
+ </div>
+ </div>
+ <div id="loading">Loading...</div>
+ </div>
+ `,
+ {ignoreTags: ['option']}
);
});
@@ -451,55 +487,22 @@
assert.isTrue(requestUpdateStub.called);
});
- test('loading displays before repo config is loaded', () => {
- assert.isTrue(
- queryAndAssert<HTMLDivElement>(element, '#loading').classList.contains(
- 'loading'
- )
- );
- assert.isFalse(
- getComputedStyle(queryAndAssert<HTMLDivElement>(element, '#loading'))
- .display === 'none'
- );
- assert.isTrue(
- queryAndAssert<HTMLDivElement>(
- element,
- '#loadedContent'
- ).classList.contains('loading')
- );
- assert.isTrue(
- getComputedStyle(
- queryAndAssert<HTMLDivElement>(element, '#loadedContent')
- ).display === 'none'
- );
- });
-
- test('download commands visibility', async () => {
- element.loading = false;
- await element.updateComplete;
- assert.isTrue(
- queryAndAssert<HTMLDivElement>(
- element,
- '#downloadContent'
- ).classList.contains('hide')
- );
- assert.isTrue(
- getComputedStyle(
- queryAndAssert<HTMLDivElement>(element, '#downloadContent')
- ).display === 'none'
- );
+ test('render download commands', async () => {
+ element.repo = REPO as RepoName;
+ await element.loadRepo();
element.schemesObj = SCHEMES;
await element.updateComplete;
- assert.isFalse(
- queryAndAssert<HTMLDivElement>(
- element,
- '#downloadContent'
- ).classList.contains('hide')
- );
- assert.isFalse(
- getComputedStyle(
- queryAndAssert<HTMLDivElement>(element, '#downloadContent')
- ).display === 'none'
+ const content = queryAndAssert<HTMLDivElement>(element, '#downloadContent');
+ assert.dom.equal(
+ content,
+ /* HTML */ `
+ <div id="downloadContent">
+ <h2 class="heading-2" id="download">Download</h2>
+ <fieldset>
+ <gr-download-commands id="downloadCommands"></gr-download-commands>
+ </fieldset>
+ </div>
+ `
);
});
@@ -715,9 +718,9 @@
Promise.resolve(new Response())
);
- const button = queryAll<GrButton>(element, 'gr-button')[2];
-
await element.loadRepo();
+
+ const button = queryAll<GrButton>(element, 'gr-button')[2];
assert.isTrue(button.hasAttribute('disabled'));
assert.isFalse(
queryAndAssert<HTMLHeadingElement>(
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index c0ed3b3..d4627e2 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1256,7 +1256,14 @@
flatten
down-arrow
class="showCopyLinkDialogButton"
- @click=${() => this.copyLinksDropdown?.toggleDropdown()}
+ @click=${(e: MouseEvent) => {
+ // We don't want to handle clicks on the star or the <a> link.
+ // Calling `stopPropagation()` from the click handler of <a> is not an
+ // option, because then the click does not reach the top-level page.js
+ // click handler and would result is a full page reload.
+ if ((e.target as HTMLElement)?.nodeName !== 'GR-BUTTON') return;
+ this.copyLinksDropdown?.toggleDropdown();
+ }}
><gr-change-star
id="changeStar"
.change=${this.change}
@@ -1267,10 +1274,7 @@
<a
class="changeNumber"
aria-label=${`Change ${this.change?._number}`}
- @click=${(e: MouseEvent) => {
- fireReload(this, true);
- e.stopPropagation();
- }}
+ href=${ifDefined(this.computeChangeUrl(true))}
>${this.change?._number}</a
>
</gr-button>
@@ -2075,9 +2079,7 @@
// Private but used in tests.
viewStateChanged() {
- // viewState is set by gr-router in handleChangeRoute method and is never
- // set to undefined
- assertIsDefined(this.viewState, 'viewState');
+ if (!this.viewState) return;
if (this.isChangeObsolete()) {
// Tell the app element that we are not going to handle the new change
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
index 0a21da4..efc6efe 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result.ts
@@ -8,11 +8,21 @@
import {LitElement, css, html, PropertyValues, nothing} from 'lit';
import {customElement, property, state} from 'lit/decorators.js';
import {RunResult} from '../../models/checks/checks-model';
-import {createFixAction, iconFor} from '../../models/checks/checks-util';
+import {
+ createFixAction,
+ createPleaseFixComment,
+ iconFor,
+} from '../../models/checks/checks-util';
import {modifierPressed} from '../../utils/dom-util';
import './gr-checks-results';
import './gr-hovercard-run';
import {fontStyles} from '../../styles/gr-font-styles';
+import {Action} from '../../api/checks';
+import {assertIsDefined} from '../../utils/common-util';
+import {resolve} from '../../models/dependency';
+import {commentsModelToken} from '../../models/comments/comments-model';
+import {subscribe} from '../lit/subscription-controller';
+import {changeModelToken} from '../../models/change/change-model';
@customElement('gr-diff-check-result')
export class GrDiffCheckResult extends LitElement {
@@ -32,6 +42,13 @@
@state()
isExpandable = false;
+ @state()
+ isOwner = false;
+
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
+ private readonly getCommentsModel = resolve(this, commentsModelToken);
+
static override get styles() {
return [
fontStyles,
@@ -114,6 +131,15 @@
];
}
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getChangeModel().isOwner$,
+ x => (this.isOwner = x)
+ );
+ }
+
override render() {
if (!this.result) return;
const cat = this.result.category.toLowerCase();
@@ -182,14 +208,39 @@
private renderActions() {
if (!this.isExpanded) return nothing;
- return html`<div class="actions">${this.renderFixButton()}</div>`;
+ return html`<div class="actions">
+ ${this.renderPleaseFixButton()}${this.renderShowFixButton()}
+ </div>`;
}
- private renderFixButton() {
+ private renderPleaseFixButton() {
+ if (this.isOwner) return nothing;
+ const action: Action = {
+ name: 'Please Fix',
+ callback: () => {
+ assertIsDefined(this.result, 'result');
+ this.getCommentsModel().saveDraft(createPleaseFixComment(this.result));
+ return undefined;
+ },
+ };
+ return html`
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ .action=${action}
+ ></gr-checks-action>
+ `;
+ }
+
+ private renderShowFixButton() {
const action = createFixAction(this, this.result);
if (!action) return nothing;
return html`
- <gr-checks-action context="diff-fix" .action=${action}></gr-checks-action>
+ <gr-checks-action
+ id="show-fix"
+ context="diff-fix"
+ .action=${action}
+ ></gr-checks-action>
`;
}
diff --git a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
index 3892c9a..0377e0e 100644
--- a/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
+++ b/polygerrit-ui/app/elements/checks/gr-diff-check-result_test.ts
@@ -7,6 +7,7 @@
import {fakeRun1} from '../../models/checks/checks-fakes';
import {RunResult} from '../../models/checks/checks-model';
import '../../test/common-test-setup';
+import {queryAndAssert} from '../../utils/common-util';
import './gr-diff-check-result';
import {GrDiffCheckResult} from './gr-diff-check-result';
@@ -50,4 +51,30 @@
`
);
});
+
+ test('renders expanded', async () => {
+ element.result = {...fakeRun1, ...fakeRun1.results?.[2]} as RunResult;
+ element.isExpanded = true;
+ await element.updateComplete;
+
+ const details = queryAndAssert(element, 'div.details');
+ assert.dom.equal(
+ details,
+ /* HTML */ `
+ <div class="details">
+ <gr-result-expanded hidecodepointers=""></gr-result-expanded>
+ <div class="actions">
+ <gr-checks-action
+ id="please-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ <gr-checks-action
+ id="show-fix"
+ context="diff-fix"
+ ></gr-checks-action>
+ </div>
+ </div>
+ `
+ );
+ });
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index e9da5b6..c74993f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -732,7 +732,7 @@
changedProperties.has('focusLineNum') ||
changedProperties.has('leftSide')
) {
- this.initLineOfInterestAndCursor();
+ this.initCursor();
}
if (
changedProperties.has('change') ||
@@ -774,6 +774,7 @@
.change=${this.change}
.patchRange=${this.patchRange}
.file=${file}
+ .lineOfInterest=${this.getLineOfInterest()}
.path=${this.path}
.projectName=${this.change?.project}
@is-blame-loaded-changed=${this.onIsBlameLoadedChanged}
@@ -1350,13 +1351,6 @@
return {path: fileList[idx]};
}
- // Private but used in tests.
- initLineOfInterestAndCursor() {
- if (!this.diffHost) return;
- this.diffHost.lineOfInterest = this.getLineOfInterest();
- this.initCursor();
- }
-
private updateUrlToDiffUrl(lineNum?: number, leftSide?: boolean) {
if (!this.change) return;
if (!this.patchNum) return;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index 9bbe4b3..889e9dd 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -1315,7 +1315,7 @@
test('hash is determined from viewState', async () => {
assertIsDefined(element.diffHost);
sinon.stub(element.diffHost, 'reload');
- const initLineStub = sinon.stub(element, 'initLineOfInterestAndCursor');
+ const initLineStub = sinon.stub(element, 'initCursor');
element.focusLineNum = 123;
@@ -1819,7 +1819,7 @@
test('File change should trigger setUrl once', async () => {
element.files = getFilesFromFileList(['file1', 'file2', 'file3']);
- sinon.stub(element, 'initLineOfInterestAndCursor');
+ sinon.stub(element, 'initCursor');
// Load file1
viewModel.setState({
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
index 28e83ae..b952a3d 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-section.ts
@@ -76,6 +76,9 @@
const pairs = this.getLinePairs();
const responsiveMode = getResponsiveMode(this.diffPrefs, this.renderPrefs);
+ const hideFileCommentButton =
+ this.diffPrefs?.show_file_comment_button === false ||
+ this.renderPrefs?.show_file_comment_button === false;
const body = html`
<tbody class=${diffClasses(...extras)}>
${this.renderContextControls()} ${this.renderMoveControls()}
@@ -92,6 +95,7 @@
.tabSize=${this.diffPrefs?.tab_size ?? 2}
.unifiedDiff=${this.isUnifiedDiff()}
.responsiveMode=${responsiveMode}
+ .hideFileCommentButton=${hideFileCommentButton}
>
</gr-diff-row>
`;
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index 6a5933c..ba43eb4 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -14,12 +14,13 @@
Replacement,
RunStatus,
} from '../../api/checks';
-import {PatchSetNumber} from '../../api/rest-api';
+import {PatchSetNumber, RevisionPatchSetNum} from '../../api/rest-api';
+import {CommentSide} from '../../constants/constants';
import {FixSuggestionInfo, FixReplacementInfo} from '../../types/common';
import {OpenFixPreviewEventDetail} from '../../types/events';
import {isDefined} from '../../types/types';
-import {PROVIDED_FIX_ID} from '../../utils/comment-util';
-import {assert, assertNever} from '../../utils/common-util';
+import {PROVIDED_FIX_ID, UnsavedInfo} from '../../utils/comment-util';
+import {assert, assertIsDefined, assertNever} from '../../utils/common-util';
import {fire} from '../../utils/event-util';
import {CheckResult, CheckRun, RunResult} from './checks-model';
@@ -86,6 +87,27 @@
}
}
+function pleaseFixMessage(result: RunResult) {
+ return `Please fix this ${result.category} reported by ${result.checkName}: ${result.summary}
+
+${result.message}`;
+}
+
+export function createPleaseFixComment(result: RunResult): UnsavedInfo {
+ const pointer = result.codePointers?.[0];
+ assertIsDefined(pointer, 'codePointer');
+ return {
+ __unsaved: true,
+ path: pointer.path,
+ patch_set: result.patchset as RevisionPatchSetNum,
+ side: CommentSide.REVISION,
+ line: pointer.range.end_line ?? pointer.range.start_line,
+ range: pointer.range,
+ message: pleaseFixMessage(result),
+ unresolved: true,
+ };
+}
+
export function createFixAction(
target: EventTarget,
result?: RunResult
diff --git a/polygerrit-ui/app/utils/attention-set-util.ts b/polygerrit-ui/app/utils/attention-set-util.ts
index 77834bd..4404e59 100644
--- a/polygerrit-ui/app/utils/attention-set-util.ts
+++ b/polygerrit-ui/app/utils/attention-set-util.ts
@@ -3,7 +3,12 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {AccountInfo, ChangeInfo, ServerInfo} from '../types/common';
+import {
+ AccountInfo,
+ ChangeInfo,
+ DetailedLabelInfo,
+ ServerInfo,
+} from '../types/common';
import {ParsedChangeInfo} from '../types/types';
import {
getAccountTemplate,
@@ -13,6 +18,7 @@
} from './account-util';
import {CommentThread, isMentionedThread, isUnresolved} from './comment-util';
import {hasOwnProperty} from './common-util';
+import {getCodeReviewLabel} from './label-util';
export function canHaveAttention(account?: AccountInfo): boolean {
return !!account?._account_id && !isServiceUser(account);
@@ -101,9 +107,10 @@
/**
* Sort order:
* 1. The user themselves
- * 2. Human users in the attention set.
- * 3. Other human users.
- * 4. Service users.
+ * 2. Users in the attention set first.
+ * 3. Human users first.
+ * 4. Users that have voted first in this order of vote values:
+ * -2, -1, +2, +1, 0 or no vote.
*/
export function sortReviewers(
r1: AccountInfo,
@@ -117,7 +124,22 @@
}
const a1 = hasAttention(r1, change) ? 1 : 0;
const a2 = hasAttention(r2, change) ? 1 : 0;
- const s1 = isServiceUser(r1) ? -2 : 0;
- const s2 = isServiceUser(r2) ? -2 : 0;
- return a2 - a1 + s2 - s1;
+ if (a2 - a1 !== 0) return a2 - a1;
+
+ const s1 = isServiceUser(r1) ? -1 : 0;
+ const s2 = isServiceUser(r2) ? -1 : 0;
+ if (s2 - s1 !== 0) return s2 - s1;
+
+ const crLabel = getCodeReviewLabel(change?.labels ?? {}) as DetailedLabelInfo;
+ let v1 =
+ crLabel?.all?.find(vote => vote._account_id === r1._account_id)?.value ?? 0;
+ let v2 =
+ crLabel?.all?.find(vote => vote._account_id === r2._account_id)?.value ?? 0;
+ // We want negative votes getting a higher score than positive votes, so
+ // we choose 10 as a random number that is higher than all positive votes that
+ // are in use, and then add the absolute value of the vote to that.
+ // So -2 becomes 12.
+ if (v1 < 0) v1 = 10 - v1;
+ if (v2 < 0) v2 = 10 - v2;
+ return v2 - v1;
}
diff --git a/polygerrit-ui/app/utils/attention-set-util_test.ts b/polygerrit-ui/app/utils/attention-set-util_test.ts
index 8092a6e..5bd1924 100644
--- a/polygerrit-ui/app/utils/attention-set-util_test.ts
+++ b/polygerrit-ui/app/utils/attention-set-util_test.ts
@@ -6,9 +6,11 @@
import '../test/common-test-setup';
import {
createAccountDetailWithIdNameAndEmail,
+ createAccountWithId,
createChange,
createComment,
createCommentThread,
+ createParsedChange,
createServerInfo,
} from '../test/test-data-generators';
import {
@@ -22,9 +24,10 @@
getMentionedReason,
getReason,
hasAttention,
+ sortReviewers,
} from './attention-set-util';
import {DefaultDisplayNameConfig} from '../api/rest-api';
-import {AccountsVisibility} from '../constants/constants';
+import {AccountsVisibility, AccountTag} from '../constants/constants';
import {assert} from '@open-wc/testing';
const KERMIT: AccountInfo = {
@@ -101,6 +104,45 @@
assert.equal(getReason(config, OTHER_ACCOUNT, change), 'Added by kermit');
});
+ test('sortReviewers', () => {
+ const a1 = createAccountWithId(1);
+ a1.tags = [AccountTag.SERVICE_USER];
+ const a2 = createAccountWithId(2);
+ a2.tags = [AccountTag.SERVICE_USER];
+ const a3 = createAccountWithId(3);
+ const a4 = createAccountWithId(4);
+ const a5 = createAccountWithId(5);
+ const a6 = createAccountWithId(6);
+ const a7 = createAccountWithId(7);
+
+ const reviewers = [a1, a2, a3, a4, a5, a6, a7];
+ const change = {
+ ...createParsedChange(),
+ attention_set: {'6': {account: a6}},
+ labels: {
+ 'Code-Review': {
+ all: [
+ {...a2, value: 1},
+ {...a4, value: 1},
+ {...a5, value: -1},
+ ],
+ },
+ },
+ };
+ assert.sameOrderedMembers(
+ reviewers.sort((r1, r2) => sortReviewers(r1, r2, change, a7)),
+ [
+ a7, // self
+ a6, // is in the attention set
+ a5, // human user, has voted -1
+ a4, // human user, has voted +1
+ a3, // human user, has not voted
+ a2, // service user, has voted
+ a1, // service user, has not voted
+ ]
+ );
+ });
+
test('getMentionReason', () => {
let comment = {
...createComment(),
diff --git a/resources/com/google/gerrit/server/mail/Comment.soy b/resources/com/google/gerrit/server/mail/Comment.soy
index 98ab4b2..4b621b5 100644
--- a/resources/com/google/gerrit/server/mail/Comment.soy
+++ b/resources/com/google/gerrit/server/mail/Comment.soy
@@ -77,13 +77,8 @@
{for $line, $index in $comment.lines}
{if $index == 0}
{if $comment.startLine != 0}
- {$comment.link}
+ {$comment.link}{sp}:{\n}
{/if}
-
- // Insert a space before the newline so that Gmail does not mistakenly
- // link the following line with the file link. See issue 9201.
- {sp}{\n}
-
{$comment.linePrefix}
{else}
{$comment.linePrefixEmpty}
