Merge "Revert "Ensure that quoted messages show up in the reply comment""
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index ca72f8b..33c5bbd 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -533,6 +533,24 @@
Certain operations in Gerrit can be validated by plugins by
implementing the corresponding link:config-validation.html[listeners].
+[[taskListeners]]
+== WorkQueue.TaskListeners
+
+It is possible for plugins to listen to
+`com.google.gerrit.server.git.WorkQueue$Task`s directly before they run, and
+directly after they complete. This may be used to delay task executions based
+on custom criteria by blocking, likely on a lock or semaphore, inside
+onStart(), and a lock/semaphore release in onStop(). Plugins may listen to
+tasks by implementing a `com.google.gerrit.server.git.WorkQueue$TaskListener`
+and registering the new listener like this:
+
+[source,java]
+----
+bind(TaskListener.class)
+ .annotatedWith(Exports.named("MyListener"))
+ .to(MyListener.class);
+----
+
[[change-message-modifier]]
== Change Message Modifier
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index efc746a..685f9d9 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -3804,6 +3804,7 @@
|`enabled` |optional|Whether the commentlink is enabled, as documented
in link:config-gerrit.html#commentlink.name.enabled[
commentlink.name.enabled]. If not set the commentlink is enabled.
+|==================================================
[[commentlink-input]]
=== CommentLinkInput
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 1177734..c5d319f 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -32,6 +32,7 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static com.google.gerrit.server.project.testing.TestLabels.label;
import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toList;
@@ -89,6 +90,7 @@
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ProjectWatchInfo;
+import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -98,6 +100,7 @@
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.GerritPersonIdent;
@@ -1100,6 +1103,42 @@
gApi.changes().id(id).current().review(ReviewInput.recommend());
}
+ protected void assertThatAccountIsNotVisible(TestAccount... testAccounts) {
+ for (TestAccount testAccount : testAccounts) {
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(testAccount.id().get()).get());
+ }
+ }
+
+ protected void assertReviewers(String changeId, TestAccount... expectedReviewers)
+ throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).containsKey(ReviewerState.REVIEWER);
+ List<Integer> reviewers =
+ reviewerMap.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList());
+ assertThat(reviewers)
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedReviewers).map(a -> a.id().get()).collect(toList()));
+ }
+
+ protected void assertCcs(String changeId, TestAccount... expectedCcs) throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).containsKey(ReviewerState.CC);
+ List<Integer> ccs =
+ reviewerMap.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList());
+ assertThat(ccs)
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedCcs).map(a -> a.id().get()).collect(toList()));
+ }
+
+ protected void assertNoCcs(String changeId) throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).doesNotContainKey(ReviewerState.CC);
+ }
+
protected void assertSubmittedTogether(String chId, String... expected) throws Exception {
assertSubmittedTogether(chId, ImmutableSet.of(), expected);
}
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index 11999ab..3f51872 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -14,6 +14,7 @@
package com.google.gerrit.extensions.api.changes;
+import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
import com.google.gerrit.extensions.client.Comment;
@@ -154,7 +155,11 @@
}
public ReviewInput reviewer(String reviewer) {
- return reviewer(reviewer, REVIEWER, false);
+ return reviewer(reviewer, REVIEWER, /* confirmed= */ false);
+ }
+
+ public ReviewInput cc(String cc) {
+ return reviewer(cc, CC, /* confirmed= */ false);
}
public ReviewInput reviewer(String reviewer, ReviewerState state, boolean confirmed) {
diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java
index c4e185d..762d988 100644
--- a/java/com/google/gerrit/pgm/Reindex.java
+++ b/java/com/google/gerrit/pgm/Reindex.java
@@ -36,6 +36,7 @@
import com.google.gerrit.server.cache.CacheInfo;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
import com.google.gerrit.server.index.IndexModule;
import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
import com.google.gerrit.server.index.options.AutoFlush;
@@ -175,6 +176,8 @@
}
boolean replica = ReplicaUtil.isReplica(globalConfig);
List<Module> modules = new ArrayList<>();
+ modules.add(new WorkQueueModule());
+
Module indexModule;
IndexType indexType = IndexModule.getIndexType(dbInjector);
if (indexType.isLucene()) {
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 8824d56..65eb332 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -559,6 +559,11 @@
return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::allVisible);
}
+ public Result resolveIncludeInactiveIgnoreVisibility(String input)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::allVisible);
+ }
+
public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::isActive);
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index edaca70..46f307d 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -275,14 +275,29 @@
Iterables.transform(ccs, Account.Id::toString));
}
+ public ChangeInserter setReviewersAndCcsIgnoreVisibility(
+ Iterable<Account.Id> reviewers, Iterable<Account.Id> ccs) {
+ return setReviewersAndCcsAsStrings(
+ Iterables.transform(reviewers, Account.Id::toString),
+ Iterables.transform(ccs, Account.Id::toString),
+ /* skipVisibilityCheck= */ true);
+ }
+
public ChangeInserter setReviewersAndCcsAsStrings(
Iterable<String> reviewers, Iterable<String> ccs) {
+ return setReviewersAndCcsAsStrings(reviewers, ccs, /* skipVisibilityCheck= */ false);
+ }
+
+ private ChangeInserter setReviewersAndCcsAsStrings(
+ Iterable<String> reviewers, Iterable<String> ccs, boolean skipVisibilityCheck) {
reviewerInputs =
Streams.concat(
Streams.stream(reviewers)
.distinct()
- .map(id -> newReviewerInput(id, ReviewerState.REVIEWER)),
- Streams.stream(ccs).distinct().map(id -> newReviewerInput(id, ReviewerState.CC)))
+ .map(id -> newReviewerInput(id, ReviewerState.REVIEWER, skipVisibilityCheck)),
+ Streams.stream(ccs)
+ .distinct()
+ .map(id -> newReviewerInput(id, ReviewerState.CC, skipVisibilityCheck)))
.collect(toImmutableList());
return this;
}
@@ -595,7 +610,8 @@
}
}
- private static InternalReviewerInput newReviewerInput(String reviewer, ReviewerState state) {
+ private static InternalReviewerInput newReviewerInput(
+ String reviewer, ReviewerState state, boolean skipVisibilityCheck) {
// Disable individual emails when adding reviewers, as all reviewers will receive the single
// bulk new change email.
InternalReviewerInput input =
@@ -606,7 +622,9 @@
// certain commit footers: putting a nonexistent user in a footer should not cause an error. In
// theory we could provide finer control to do this for some reviewers and not others, but it's
// not worth complicating the ChangeInserter interface further at this time.
- input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE;
+ input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE_EXCEPT_NOT_FOUND;
+
+ input.skipVisibilityCheck = skipVisibilityCheck;
return input;
}
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index 9580565..f3ad4f7 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -94,9 +94,21 @@
public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
public static final int DEFAULT_MAX_REVIEWERS = 20;
+ /**
+ * Controls which failures should be ignored.
+ *
+ * <p>If a failure is ignored the operation succeeds, but the reviewer is not added. If not
+ * ignored a failure means that the operation fails.
+ */
public enum FailureBehavior {
+ // All failures cause the operation to fail.
FAIL,
- IGNORE;
+
+ // Only not found failures cause the operation to fail, all other failures are ignored.
+ IGNORE_EXCEPT_NOT_FOUND,
+
+ // All failures are ignored.
+ IGNORE_ALL;
}
private enum FailureType {
@@ -113,6 +125,9 @@
* resolving to an account/group/email.
*/
public FailureBehavior otherFailureBehavior = FailureBehavior.FAIL;
+
+ /** Whether the visibility check for the reviewer account should be skipped. */
+ public boolean skipVisibilityCheck = false;
}
public static InternalReviewerInput newReviewerInput(
@@ -143,7 +158,7 @@
in.reviewer = accountId.toString();
in.state = CC;
in.notify = notify;
- in.otherFailureBehavior = FailureBehavior.IGNORE;
+ in.otherFailureBehavior = FailureBehavior.IGNORE_ALL;
return Optional.of(in);
}
@@ -262,7 +277,13 @@
IdentifiedUser reviewerUser;
boolean exactMatchFound = false;
try {
- reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
+ if (input instanceof InternalReviewerInput
+ && ((InternalReviewerInput) input).skipVisibilityCheck) {
+ reviewerUser =
+ accountResolver.resolveIncludeInactiveIgnoreVisibility(input.reviewer).asUniqueUser();
+ } else {
+ reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
+ }
if (input.reviewer.equalsIgnoreCase(reviewerUser.getName())
|| input.reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
exactMatchFound = true;
@@ -577,7 +598,9 @@
(input instanceof InternalReviewerInput)
? ((InternalReviewerInput) input).otherFailureBehavior
: FailureBehavior.FAIL;
- return failureType == FailureType.OTHER && behavior == FailureBehavior.IGNORE;
+ return behavior == FailureBehavior.IGNORE_ALL
+ || (failureType == FailureType.OTHER
+ && behavior == FailureBehavior.IGNORE_EXCEPT_NOT_FOUND);
}
}
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index fa46bf4..f90daff 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -286,7 +286,7 @@
reviewers.remove(user.getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.getAccountId());
- ins.setReviewersAndCcs(reviewers, ccs);
+ ins.setReviewersAndCcsIgnoreVisibility(reviewers, ccs);
ins.setRevertOf(notes.getChangeId());
ins.setWorkInProgress(input.workInProgress);
diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java
index 3032bfe..715cb30 100644
--- a/java/com/google/gerrit/server/git/WorkQueue.java
+++ b/java/com/google/gerrit/server/git/WorkQueue.java
@@ -20,6 +20,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.events.LifecycleListener;
+import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
@@ -27,6 +28,7 @@
import com.google.gerrit.server.config.ScheduleConfig.Schedule;
import com.google.gerrit.server.logging.LoggingContext;
import com.google.gerrit.server.logging.LoggingContextAwareRunnable;
+import com.google.gerrit.server.plugincontext.PluginMapContext;
import com.google.gerrit.server.util.IdGenerator;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -58,6 +60,30 @@
public class WorkQueue {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ /**
+ * To register a TaskListener, which will be called directly before Tasks run, and directly after
+ * they complete, bind the TaskListener like this:
+ *
+ * <p><code>
+ * bind(TaskListener.class)
+ * .annotatedWith(Exports.named("MyListener"))
+ * .to(MyListener.class);
+ * </code>
+ */
+ public interface TaskListener {
+ public static class NoOp implements TaskListener {
+ @Override
+ public void onStart(Task<?> task) {}
+
+ @Override
+ public void onStop(Task<?> task) {}
+ }
+
+ void onStart(Task<?> task);
+
+ void onStop(Task<?> task);
+ }
+
public static class Lifecycle implements LifecycleListener {
private final WorkQueue workQueue;
@@ -78,6 +104,7 @@
public static class WorkQueueModule extends LifecycleModule {
@Override
protected void configure() {
+ DynamicMap.mapOf(binder(), WorkQueue.TaskListener.class);
bind(WorkQueue.class);
listener().to(Lifecycle.class);
}
@@ -87,18 +114,32 @@
private final IdGenerator idGenerator;
private final MetricMaker metrics;
private final CopyOnWriteArrayList<Executor> queues;
+ private final PluginMapContext<TaskListener> listeners;
@Inject
- WorkQueue(IdGenerator idGenerator, @GerritServerConfig Config cfg, MetricMaker metrics) {
- this(idGenerator, Math.max(cfg.getInt("execution", "defaultThreadPoolSize", 2), 2), metrics);
+ WorkQueue(
+ IdGenerator idGenerator,
+ @GerritServerConfig Config cfg,
+ MetricMaker metrics,
+ PluginMapContext<TaskListener> listeners) {
+ this(
+ idGenerator,
+ Math.max(cfg.getInt("execution", "defaultThreadPoolSize", 2), 2),
+ metrics,
+ listeners);
}
/** Constructor to allow binding the WorkQueue more explicitly in a vhost setup. */
- public WorkQueue(IdGenerator idGenerator, int defaultThreadPoolSize, MetricMaker metrics) {
+ public WorkQueue(
+ IdGenerator idGenerator,
+ int defaultThreadPoolSize,
+ MetricMaker metrics,
+ PluginMapContext<TaskListener> listeners) {
this.idGenerator = idGenerator;
this.metrics = metrics;
this.queues = new CopyOnWriteArrayList<>();
this.defaultQueue = createQueue(defaultThreadPoolSize, "WorkQueue", true);
+ this.listeners = listeners;
}
/** Get the default work queue, for miscellaneous tasks. */
@@ -438,6 +479,14 @@
Collection<Task<?>> getTasks() {
return all.values();
}
+
+ public void onStart(Task<?> task) {
+ listeners.runEach(extension -> extension.getProvider().get().onStart(task));
+ }
+
+ public void onStop(Task<?> task) {
+ listeners.runEach(extension -> extension.getProvider().get().onStop(task));
+ }
}
private static void logUncaughtException(Thread t, Throwable e) {
@@ -608,10 +657,12 @@
if (running.compareAndSet(false, true)) {
String oldThreadName = Thread.currentThread().getName();
try {
+ executor.onStart(this);
Thread.currentThread().setName(oldThreadName + "[" + task.toString() + "]");
task.run();
} finally {
Thread.currentThread().setName(oldThreadName);
+ executor.onStop(this);
if (isPeriodic()) {
running.set(false);
} else {
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 644f82e..ae7c3a10 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -405,7 +405,7 @@
// Ignore failures for reasons like the reviewer being inactive or being unable to see the
// change. See discussion in ChangeInserter.
- input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE;
+ input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE_EXCEPT_NOT_FOUND;
return input;
}
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 00c48dc..d0113e5 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -527,7 +527,7 @@
reviewers.remove(user.get().getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.get().getAccountId());
- ins.setReviewersAndCcs(reviewers, ccs);
+ ins.setReviewersAndCcsIgnoreVisibility(reviewers, ccs);
}
// If there is a base, and the base is not merged, the groups will be overridden by the base's
// groups.
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index b00cadb..781965e 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -79,6 +79,7 @@
import com.google.gerrit.server.git.PerThreadRequestScope;
import com.google.gerrit.server.git.SearchingChangeCacheImpl.SearchingChangeCacheImplModule;
import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
import com.google.gerrit.server.group.testing.TestGroupBackend;
import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
import com.google.gerrit.server.index.account.AllAccountsIndexer;
@@ -195,6 +196,7 @@
install(new AuditModule());
install(new SubscriptionGraphModule());
install(new SuperprojectUpdateSubmissionListenerModule());
+ install(new WorkQueueModule());
bindScope(RequestScoped.class, PerThreadRequestScope.REQUEST);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index 90ca047..93ac038 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -424,6 +425,46 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void revertWithNonVisibleUsers() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount reverter = user;
+ TestAccount changeOwner = admin; // must be admin, since admin cloned testRepo
+ TestAccount reviewer = accountCreator.user2();
+ TestAccount cc =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the reverter can neither see the changeOwner, the reviewer nor the cc.
+ requestScopeOperations.setApiUser(reverter.id());
+ assertThatAccountIsNotVisible(changeOwner, reviewer, cc);
+
+ // Create the change.
+ requestScopeOperations.setApiUser(changeOwner.id());
+ PushOneCommit.Result r = createChange();
+
+ // Add reviewer and cc.
+ ReviewInput reviewerInput = ReviewInput.approve();
+ reviewerInput.reviewer(reviewer.email());
+ reviewerInput.cc(cc.email());
+ gApi.changes().id(r.getChangeId()).current().review(reviewerInput);
+
+ // Approve and submit the change.
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // Revert the change.
+ requestScopeOperations.setApiUser(reverter.id());
+ String revertChangeId = gApi.changes().id(r.getChangeId()).revert().get().id;
+
+ // Revert doesn't check the reviewer/CC visibility. Since the reverter can see the reverted
+ // change, they can also see its reviewers/CCs. This means preserving them on the revert change
+ // doesn't expose their account existence and it's OK to keep them even if their accounts are
+ // not visible to the reverter.
+ assertReviewers(revertChangeId, changeOwner, reviewer);
+ assertCcs(revertChangeId, cc);
+ }
+
+ @Test
@TestProjectInput(createEmptyCommit = false)
public void revertInitialCommit() throws Exception {
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 2c80333..8d64cf7 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1009,6 +1009,84 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void cherryPickWithNonVisibleUsers() throws Exception {
+ // Create a target branch for the cherry-pick.
+ createBranch(BranchNameKey.create(project, "stable"));
+
+ // Define readable names for the users we use in this test.
+ TestAccount cherryPicker = user;
+ TestAccount changeOwner = admin;
+ TestAccount reviewer = accountCreator.user2();
+ TestAccount cc =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+ TestAccount authorCommitter =
+ accountCreator.create("user4", "user4@example.com", "User4", /* displayName= */ null);
+
+ // Check that the cherry-picker can neither see the changeOwner, the reviewer, the cc nor the
+ // authorCommitter.
+ requestScopeOperations.setApiUser(cherryPicker.id());
+ assertThatAccountIsNotVisible(changeOwner, reviewer, cc, authorCommitter);
+
+ // Create the change with authorCommitter as the author and the committer.
+ requestScopeOperations.setApiUser(changeOwner.id());
+ PushOneCommit push = pushFactory.create(authorCommitter.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ // Check that authorCommitter was set as the author and committer.
+ ChangeInfo changeInfo = gApi.changes().id(r.getChangeId()).get();
+ CommitInfo commit = changeInfo.revisions.get(changeInfo.currentRevision).commit;
+ assertThat(commit.author.email).isEqualTo(authorCommitter.email());
+ assertThat(commit.committer.email).isEqualTo(authorCommitter.email());
+
+ // Pushing a commit with a forged author/committer adds the author/committer as a CC.
+ assertCcs(r.getChangeId(), authorCommitter);
+
+ // Remove the author/committer as a CC because because otherwise there are two signals for CCing
+ // authorCommitter on the cherry-pick change: once because they are author and committer and
+ // once because they are a CC. For authorCommitter we only want to test the first signal here
+ // (the second signal is covered by adding an explicit CC below).
+ gApi.changes().id(r.getChangeId()).reviewer(authorCommitter.email()).remove();
+ assertNoCcs(r.getChangeId());
+
+ // Add reviewer and cc.
+ ReviewInput reviewerInput = ReviewInput.approve();
+ reviewerInput.reviewer(reviewer.email());
+ reviewerInput.cc(cc.email());
+ gApi.changes().id(r.getChangeId()).current().review(reviewerInput);
+
+ // Approve and submit the change.
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // Cherry-pick the change.
+ requestScopeOperations.setApiUser(cherryPicker.id());
+ CherryPickInput cherryPickInput = new CherryPickInput();
+ cherryPickInput.message = "Cherry-pick to stable branch";
+ cherryPickInput.destination = "stable";
+ cherryPickInput.keepReviewers = true;
+ String cherryPickChangeId =
+ gApi.changes().id(r.getChangeId()).current().cherryPick(cherryPickInput).get().id;
+
+ // Cherry-pick doesn't check the visibility of explicit reviewers/CCs. Since the cherry-picker
+ // can see the cherry-picked change, they can also see its reviewers/CCs. This means preserving
+ // them on the cherry-pick change doesn't expose their account existence and it's OK to keep
+ // them even if their accounts are not visible to the cherry-picker.
+ // In contrast to this for implicit CCs that are added for the author/committer the account
+ // visibility is checked, but if their accounts are not visible the CC is silently dropped (so
+ // that the cherry-pick request can still succeed). Since in this case authorCommitter is not
+ // visible, we expect that CCing them is being dropped and hence authorCommitter is not returned
+ // as a CC here. The reason that the visibility for author/committer must be checked is that
+ // author/committer may not match a Gerrit account (if they are forged). This means by seeing
+ // the author/committer on the cherry-picked change, it's not possible to deduce that these
+ // Gerrit accounts exists, but if they would be added as a CC on the cherry-pick change even if
+ // they are not visible the account existence would be exposed.
+ assertReviewers(cherryPickChangeId, changeOwner, reviewer);
+ assertCcs(cherryPickChangeId, cc);
+ }
+
+ @Test
public void cherryPickToMergedChangeRevision() throws Exception {
createBranch(BranchNameKey.create(project, "foo"));
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index cd1d911..3b94a50 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -128,6 +128,7 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;
import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
@@ -1154,7 +1155,7 @@
.add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
.message(PushOneCommit.SUBJECT)
.create();
- // Push commit as "Admnistrator".
+ // Push commit as "Administrator".
pushHead(testRepo, "refs/for/master");
String changeId = GitUtil.getChangeId(testRepo, c).get();
@@ -1168,6 +1169,71 @@
}
@Test
+ public void pushForMasterWithNonExistingForgedAuthorAndCommitter() throws Exception {
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ commitBuilder()
+ .author(new PersonIdent("author", "author@example.com"))
+ .committer(new PersonIdent("committer", "committer@example.com"))
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+ // Push commit as "Administrator".
+ pushHead(testRepo, "refs/for/master");
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void pushForMasterWithNonVisibleForgedAuthorAndCommitter() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount uploader = user; // cannot use admin since admin can see all users
+ TestAccount author = accountCreator.user2();
+ TestAccount committer =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the uploader can neither see the author nor the committer.
+ requestScopeOperations.setApiUser(uploader.id());
+ assertThatAccountIsNotVisible(author, committer);
+
+ // Allow the uploader to forge author and committer.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.FORGE_AUTHOR).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(allow(Permission.FORGE_COMMITTER).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // Clone the repo as uploader so that the push is done by the uplaoder.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, uploader);
+
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ testRepo
+ .branch("HEAD")
+ .commit()
+ .insertChangeId()
+ .author(author.newIdent())
+ .committer(committer.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+
+ PushResult r = pushHead(testRepo, "refs/for/master");
+ RemoteRefUpdate refUpdate = r.getRemoteUpdate("refs/for/master");
+ assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(uploader.email());
+
+ // author and committer have not been CCed because their accounts are not visible
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
public void pushNewPatchSetForMasterWithForgedAuthorAndCommitter() throws Exception {
TestAccount user2 = accountCreator.user2();
// First patch set has author and committer matching change owner.
@@ -1192,6 +1258,74 @@
.containsExactly(user.getNameEmail(), user2.getNameEmail());
}
+ @Test
+ public void pushNewPatchSetForMasterWithNonExistingForgedAuthorAndCommitter() throws Exception {
+ // First patch set has author and committer matching change owner.
+ PushOneCommit.Result r = pushTo("refs/for/master");
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER)).isEmpty();
+
+ amendBuilder()
+ .author(new PersonIdent("author", "author@example.com"))
+ .committer(new PersonIdent("committer", "committer@example.com"))
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT + "2")
+ .create();
+ pushHead(testRepo, "refs/for/master");
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void pushNewPatchSetForMasterWithNonVisibleForgedAuthorAndCommitter() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount uploader = user; // cannot use admin since admin can see all users
+ TestAccount author = accountCreator.user2();
+ TestAccount committer =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the uploader can neither see the author nor the committer.
+ requestScopeOperations.setApiUser(uploader.id());
+ assertThatAccountIsNotVisible(author, committer);
+
+ // Allow the uploader to forge author and committer.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.FORGE_AUTHOR).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(allow(Permission.FORGE_COMMITTER).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // Clone the repo as uploader so that the push is done by the uplaoder.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, uploader);
+
+ // First patch set has author and committer matching uploader.
+ PushOneCommit push = pushFactory.create(uploader.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(uploader.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER)).isEmpty();
+
+ testRepo
+ .amendRef("HEAD")
+ .author(author.newIdent())
+ .committer(committer.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT + "2")
+ .create();
+
+ PushResult r2 = pushHead(testRepo, "refs/for/master");
+ RemoteRefUpdate refUpdate = r2.getRemoteUpdate("refs/for/master");
+ assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(uploader.email());
+
+ // author and committer have not been CCed because their accounts are not visible
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.CC)).isEmpty();
+ }
+
/**
* There was a bug that allowed a user with Forge Committer Identity access right to upload a
* commit and put *votes on behalf of another user* on it. This test checks that this is not
diff --git a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
new file mode 100644
index 0000000..b3094ac
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
@@ -0,0 +1,255 @@
+// 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.acceptance.server.util;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.git.WorkQueue.Task;
+import com.google.gerrit.server.git.WorkQueue.TaskListener;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.junit.Before;
+import org.junit.Test;
+
+public class TaskListenerIT extends AbstractDaemonTest {
+ /**
+ * Use a LatchedMethod in a method to allow another thread to await the method's call. Once
+ * called, the Latch.call() method will block until another thread calls its LatchedMethods's
+ * complete() method.
+ */
+ private static class LatchedMethod {
+ private static final int AWAIT_TIMEOUT = 20;
+ private static final TimeUnit AWAIT_TIMEUNIT = TimeUnit.MILLISECONDS;
+
+ /** API class meant be used by the class whose method is being latched */
+ private class Latch {
+ /** Ensure that the latched method calls this on entry */
+ public void call() {
+ called.countDown();
+ await(complete);
+ }
+ }
+
+ public Latch latch = new Latch();
+
+ private CountDownLatch called = new CountDownLatch(1);
+ private CountDownLatch complete = new CountDownLatch(1);
+
+ /** Assert that the Latch's call() method has not yet been called */
+ public void assertUncalled() {
+ assertThat(called.getCount()).isEqualTo(1);
+ }
+
+ /**
+ * Assert that a timeout does not occur while awaiting Latch's call() method to be called. Fails
+ * if the waiting time elapses before Latch's call() method is called, otherwise passes.
+ */
+ public void assertAwait() {
+ assertThat(await(called)).isEqualTo(true);
+ }
+
+ /** Unblock the Latch's call() method so that it can complete */
+ public void complete() {
+ complete.countDown();
+ }
+
+ private static boolean await(CountDownLatch latch) {
+ try {
+ return latch.await(AWAIT_TIMEOUT, AWAIT_TIMEUNIT);
+ } catch (InterruptedException e) {
+ return false;
+ }
+ }
+ }
+
+ private static class LatchedRunnable implements Runnable {
+ public LatchedMethod run = new LatchedMethod();
+
+ @Override
+ public void run() {
+ run.latch.call();
+ }
+ }
+
+ private static class ForwardingListener implements TaskListener {
+ public volatile TaskListener delegate;
+ public volatile Task task;
+
+ public void resetDelegate(TaskListener listener) {
+ delegate = listener;
+ task = null;
+ }
+
+ @Override
+ public void onStart(Task<?> task) {
+ if (delegate != null) {
+ if (this.task == null || this.task == task) {
+ this.task = task;
+ delegate.onStart(task);
+ }
+ }
+ }
+
+ @Override
+ public void onStop(Task<?> task) {
+ if (delegate != null) {
+ if (this.task == task) {
+ delegate.onStop(task);
+ }
+ }
+ }
+ }
+
+ private static class LatchedListener implements TaskListener {
+ public LatchedMethod onStart = new LatchedMethod();
+ public LatchedMethod onStop = new LatchedMethod();
+
+ @Override
+ public void onStart(Task<?> task) {
+ onStart.latch.call();
+ }
+
+ @Override
+ public void onStop(Task<?> task) {
+ onStop.latch.call();
+ }
+ }
+
+ private static ForwardingListener forwarder;
+
+ @Inject private WorkQueue workQueue;
+ private ScheduledExecutorService executor;
+
+ private LatchedListener listener = new LatchedListener();
+ private LatchedRunnable runnable = new LatchedRunnable();
+
+ @Override
+ public Module createModule() {
+ return new AbstractModule() {
+ @Override
+ public void configure() {
+ // Forwarder.delegate is empty on start to protect test listener from non test tasks
+ // (such as the "Log File Compressor") interference
+ forwarder = new ForwardingListener(); // Only gets bound once for all tests
+ bind(TaskListener.class).annotatedWith(Exports.named("listener")).toInstance(forwarder);
+ }
+ };
+ }
+
+ @Before
+ public void setupExecutorAndForwarder() throws InterruptedException {
+ executor = workQueue.createQueue(1, "TaskListeners");
+
+ // "Log File Compressor"s are likely running and will interfere with tests
+ while (0 != workQueue.getTasks().size()) {
+ for (Task<?> t : workQueue.getTasks()) {
+ t.cancel(true);
+ }
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+
+ forwarder.resetDelegate(listener);
+
+ assertQueueSize(0);
+ assertThat(forwarder.task).isEqualTo(null);
+ listener.onStart.assertUncalled();
+ runnable.run.assertUncalled();
+ listener.onStop.assertUncalled();
+ }
+
+ @Test
+ public void onStartThenRunThenOnStopAreCalled() throws Exception {
+ int size = assertQueueBlockedOnExecution(runnable);
+
+ // onStartThenRunThenOnStopAreCalled -> onStart...Called
+ listener.onStart.assertAwait();
+ assertQueueSize(size);
+ runnable.run.assertUncalled();
+ listener.onStop.assertUncalled();
+
+ listener.onStart.complete();
+ // onStartThenRunThenOnStopAreCalled -> ...ThenRun...Called
+ runnable.run.assertAwait();
+ listener.onStop.assertUncalled();
+
+ runnable.run.complete();
+ // onStartThenRunThenOnStopAreCalled -> ...ThenOnStop...Called
+ listener.onStop.assertAwait();
+ assertQueueSize(size);
+
+ listener.onStop.complete();
+ assertAwaitQueueSize(--size);
+ }
+
+ @Test
+ public void firstBlocksSecond() throws Exception {
+ int size = assertQueueBlockedOnExecution(runnable);
+
+ // firstBlocksSecond -> first...
+ listener.onStart.assertAwait();
+ assertQueueSize(size);
+
+ LatchedRunnable runnable2 = new LatchedRunnable();
+ size = assertQueueBlockedOnExecution(runnable2);
+
+ // firstBlocksSecond -> ...BlocksSecond
+ runnable2.run.assertUncalled();
+ assertQueueSize(size); // waiting on first
+
+ listener.onStart.complete();
+ runnable.run.assertAwait();
+ assertQueueSize(size); // waiting on first
+ runnable2.run.assertUncalled();
+
+ runnable.run.complete();
+ listener.onStop.assertAwait();
+ assertQueueSize(size); // waiting on first
+ runnable2.run.assertUncalled();
+
+ listener.onStop.complete();
+ runnable2.run.assertAwait();
+ assertQueueSize(--size);
+
+ runnable2.run.complete();
+ assertAwaitQueueSize(--size);
+ }
+
+ private int assertQueueBlockedOnExecution(Runnable runnable) {
+ int expectedSize = workQueue.getTasks().size() + 1;
+ executor.execute(runnable);
+ assertQueueSize(expectedSize);
+ return expectedSize;
+ }
+
+ private void assertQueueSize(int size) {
+ assertThat(workQueue.getTasks().size()).isEqualTo(size);
+ }
+
+ /** Fails if the waiting time elapses before the count is reached, otherwise passes */
+ private void assertAwaitQueueSize(int size) throws InterruptedException {
+ long i = 0;
+ do {
+ TimeUnit.NANOSECONDS.sleep(10);
+ assertThat(i++).isLessThan(100);
+ } while (size != workQueue.getTasks().size());
+ }
+}
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 f2977dc..8293daa 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
@@ -2252,8 +2252,6 @@
if (tab === Tab.CHECKS) {
const state: ChecksTabState = {};
detail.tabState = {checksTab: state};
- if (this.viewState?.filter) state.filter = this.viewState.filter;
- if (this.viewState?.attempt) state.attempt = this.viewState.attempt;
}
this.setActiveTab(
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index a4e026b..11fe0e2 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -28,8 +28,8 @@
import {pluralize} from '../../../utils/string-util';
import {
changeIsOpen,
+ getChangeNumber,
getRevisionKey,
- isChangeInfo,
} from '../../../utils/change-util';
import {DEFALT_NUM_CHANGES_WHEN_COLLAPSED} from './gr-related-collapse';
import {createChangeUrl} from '../../../models/views/change';
@@ -639,29 +639,12 @@
a?: ChangeInfo | RelatedChangeAndCommitInfo,
b?: ChangeInfo | ParsedChangeInfo | RelatedChangeAndCommitInfo
) {
- const aNum = this._getChangeNumber(a);
- const bNum = this._getChangeNumber(b);
+ if (!a || !b) return false;
+ const aNum = getChangeNumber(a);
+ const bNum = getChangeNumber(b);
return aNum === bNum;
}
- /**
- * Get the change number from either a ChangeInfo (such as those included in
- * SubmittedTogetherInfo responses) or get the change number from a
- * RelatedChangeAndCommitInfo (such as those included in a
- * RelatedChangesInfo response).
- */
- _getChangeNumber(
- change?: ChangeInfo | ParsedChangeInfo | RelatedChangeAndCommitInfo
- ) {
- // Default to 0 if change property is not defined.
- if (!change) return 0;
-
- if (isChangeInfo(change)) {
- return change._number;
- }
- return change._change_number;
- }
-
/*
* A list of commit ids connected to change to understand if other change
* is direct or indirect ancestor / descendant.
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
index 09cdf5f..dcc6039 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
@@ -36,6 +36,7 @@
SubmittedTogetherInfo,
} from '../../../types/common';
import {ParsedChangeInfo} from '../../../types/types';
+import {getChangeNumber} from '../../../utils/change-util';
import {GrEndpointDecorator} from '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import './gr-related-changes-list';
@@ -359,8 +360,8 @@
change_id: '456' as ChangeId,
_number: 1 as NumericChangeId,
};
- assert.equal(element._getChangeNumber(change1), 0);
- assert.equal(element._getChangeNumber(change2), 1);
+ assert.equal(getChangeNumber(change1), 0);
+ assert.equal(getChangeNumber(change2), 1);
});
suite('get conflicts tests', () => {
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 26fa0cb..a4e26fe 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -1246,13 +1246,16 @@
}
private onPatchsetSelected(e: CustomEvent<{value: string}>) {
- const patchset = Number(e.detail.value);
- assert(!isNaN(patchset), 'selected patchset must be a number');
- this.getChecksModel().setPatchset(patchset as PatchSetNumber);
+ let patchset: number | undefined = Number(e.detail.value);
+ assert(Number.isInteger(patchset), `patchset must be integer: ${patchset}`);
+ if (patchset === this.latestPatchsetNumber) patchset = undefined;
+ this.getChecksModel().updateStateSetPatchset(
+ patchset as PatchSetNumber | undefined
+ );
}
private goToLatestPatchset() {
- this.getChecksModel().setPatchset(undefined);
+ this.getChecksModel().updateStateSetPatchset(undefined);
}
private createAttemptDropdownItems() {
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
index c72bccb..82893bc 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
@@ -3,7 +3,7 @@
* Copyright 2020 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import {LitElement, css, html, PropertyValues} from 'lit';
+import {LitElement, css, html} from 'lit';
import {customElement, property, query, state} from 'lit/decorators.js';
import {
CheckResult,
@@ -22,7 +22,6 @@
import {Interaction} from '../../constants/reporting';
import {resolve} from '../../models/dependency';
import {GrChecksRuns} from './gr-checks-runs';
-import {LATEST_ATTEMPT} from '../../models/checks/checks-util';
/**
* The "Checks" tab on the Gerrit change page. Gets its data from plugins that
@@ -147,18 +146,6 @@
`;
}
- protected override updated(changedProperties: PropertyValues) {
- super.updated(changedProperties);
- if (changedProperties.has('tabState')) this.tabStateUpdated();
- }
-
- private tabStateUpdated() {
- if (!this.tabState?.checksTab) return;
- const {attempt, filter} = this.tabState.checksTab;
- this.getChecksModel().updateStateSetAttempt(attempt ?? LATEST_ATTEMPT);
- this.getChecksModel().updateStateSetRunFilter(filter ?? '');
- }
-
handleRunSelected(e: RunSelectedEvent) {
this.reporting.reportInteraction(Interaction.CHECKS_RUN_SELECTED, {
checkName: e.detail.checkName,
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 762f6b7..a35ca40 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -21,6 +21,7 @@
RepoName,
UrlEncodedCommentId,
PARENT,
+ PatchSetNumber,
} from '../../../types/common';
import {AppElement, AppElementParams} from '../../gr-app-types';
import {LocationChangeEventDetail} from '../../../types/events';
@@ -59,7 +60,11 @@
GroupViewState,
} from '../../../models/views/group';
import {DiffViewModel, DiffViewState} from '../../../models/views/diff';
-import {ChangeViewModel, ChangeViewState} from '../../../models/views/change';
+import {
+ ChangeViewModel,
+ ChangeViewState,
+ createChangeUrl,
+} from '../../../models/views/change';
import {EditViewModel, EditViewState} from '../../../models/views/edit';
import {
DashboardViewModel,
@@ -80,6 +85,7 @@
import {PluginViewModel, PluginViewState} from '../../../models/views/plugin';
import {SearchViewModel, SearchViewState} from '../../../models/views/search';
import {DashboardSection} from '../../../utils/dashboard-util';
+import {Subscription} from 'rxjs';
const RoutePattern = {
ROOT: '/',
@@ -279,6 +285,10 @@
// and for first navigation in app after loaded from server (true).
_isInitialLoad = true;
+ private subscriptions: Subscription[] = [];
+
+ private view?: GerritView;
+
constructor(
private readonly reporting: ReportingService,
private readonly routerModel: RouterModel,
@@ -295,9 +305,36 @@
private readonly repoViewModel: RepoViewModel,
private readonly searchViewModel: SearchViewModel,
private readonly settingsViewModel: SettingsViewModel
- ) {}
+ ) {
+ this.subscriptions = [
+ // TODO: Do the same for other view models.
+ // We want to make sure that the current view model state is always
+ // reflected back into the URL bar.
+ this.changeViewModel.state$.subscribe(state => {
+ if (!state) return;
+ // Note that router model view must be updated before view model state.
+ // So this check is slightly fragile, but should work.
+ if (this.view !== GerritView.CHANGE) return;
+ const browserUrl = window.location.toString();
+ const stateUrl = new URL(createChangeUrl(state), browserUrl).toString();
+ if (browserUrl !== stateUrl) {
+ page.replace(
+ stateUrl,
+ null,
+ /* init: */ false,
+ /* dispatch: */ false
+ );
+ }
+ }),
+ this.routerModel.routerView$.subscribe(view => (this.view = view)),
+ ];
+ }
- finalize(): void {}
+ finalize(): void {
+ for (const subscription of this.subscriptions) {
+ subscription.unsubscribe();
+ }
+ }
start() {
if (!this._app) {
@@ -940,6 +977,7 @@
view: GerritView.DASHBOARD,
user: ctx.params[0],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.dashboardViewModel.setState(state);
}
@@ -976,6 +1014,7 @@
sections,
title,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.dashboardViewModel.setState(state);
return Promise.resolve();
@@ -988,6 +1027,7 @@
project,
dashboard: decodeURIComponent(ctx.params[1]) as DashboardId,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.dashboardViewModel.setState(state);
this.reporting.setRepoName(project);
@@ -1010,6 +1050,7 @@
view: GerritView.GROUP,
groupId: ctx.params[0] as GroupId,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.groupViewModel.setState(state);
}
@@ -1020,6 +1061,7 @@
detail: GroupDetailView.LOG,
groupId: ctx.params[0] as GroupId,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.groupViewModel.setState(state);
}
@@ -1030,6 +1072,7 @@
detail: GroupDetailView.MEMBERS,
groupId: ctx.params[0] as GroupId,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.groupViewModel.setState(state);
}
@@ -1042,6 +1085,7 @@
filter: null,
openCreateModal: ctx.hash === 'create',
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1053,6 +1097,7 @@
offset: ctx.params['offset'],
filter: ctx.params['filter'],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1063,6 +1108,7 @@
adminView: AdminChildView.GROUPS,
filter: ctx.params['filter'] || null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1086,6 +1132,7 @@
detail: RepoDetailView.COMMANDS,
repo,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
this.reporting.setRepoName(repo);
@@ -1098,6 +1145,7 @@
detail: RepoDetailView.GENERAL,
repo,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
this.reporting.setRepoName(repo);
@@ -1110,6 +1158,7 @@
detail: RepoDetailView.ACCESS,
repo,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
this.reporting.setRepoName(repo);
@@ -1122,6 +1171,7 @@
detail: RepoDetailView.DASHBOARDS,
repo,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
this.reporting.setRepoName(repo);
@@ -1135,6 +1185,7 @@
offset: ctx.params[2] || 0,
filter: null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
}
@@ -1147,6 +1198,7 @@
offset: ctx.params['offset'],
filter: ctx.params['filter'],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
}
@@ -1158,6 +1210,7 @@
repo: ctx.params['repo'] as RepoName,
filter: ctx.params['filter'] || null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
}
@@ -1170,6 +1223,7 @@
offset: ctx.params[2] || 0,
filter: null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
}
@@ -1182,6 +1236,7 @@
offset: ctx.params['offset'],
filter: ctx.params['filter'],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
}
@@ -1193,6 +1248,7 @@
repo: ctx.params['repo'] as RepoName,
filter: ctx.params['filter'] || null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.repoViewModel.setState(state);
}
@@ -1205,6 +1261,7 @@
filter: null,
openCreateModal: ctx.hash === 'create',
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1216,6 +1273,7 @@
offset: ctx.params['offset'],
filter: ctx.params['filter'],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1226,6 +1284,7 @@
adminView: AdminChildView.REPOS,
filter: ctx.params['filter'] || null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1253,6 +1312,7 @@
offset: ctx.params[1] || 0,
filter: null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1264,6 +1324,7 @@
offset: ctx.params['offset'],
filter: ctx.params['filter'],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1274,6 +1335,7 @@
adminView: AdminChildView.PLUGINS,
filter: ctx.params['filter'] || null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1283,6 +1345,7 @@
view: GerritView.ADMIN,
adminView: AdminChildView.PLUGINS,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.adminViewModel.setState(state);
}
@@ -1293,6 +1356,7 @@
query: ctx.params[0],
offset: ctx.params[2],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.searchViewModel.setState(state);
}
@@ -1305,6 +1369,7 @@
view: GerritView.SEARCH,
query: ctx.params[0],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.searchViewModel.setState(state);
}
@@ -1329,26 +1394,15 @@
};
const queryMap = new URLSearchParams(ctx.querystring);
- if (queryMap.has('forceReload')) {
- state.forceReload = true;
- history.replaceState(
- null,
- '',
- location.href.replace(/[?&]forceReload=true/, '')
- );
- }
-
- if (queryMap.has('openReplyDialog')) {
- state.openReplyDialog = true;
- history.replaceState(
- null,
- '',
- location.href.replace(/[?&]openReplyDialog=true/, '')
- );
- }
+ if (queryMap.has('forceReload')) state.forceReload = true;
+ if (queryMap.has('openReplyDialog')) state.openReplyDialog = true;
const tab = queryMap.get('tab');
if (tab) state.tab = tab;
+ const checksPatchset = Number(queryMap.get('checksPatchset'));
+ if (Number.isInteger(checksPatchset) && checksPatchset > 0) {
+ state.checksPatchset = checksPatchset as PatchSetNumber;
+ }
const filter = queryMap.get('filter');
if (filter) state.filter = filter;
const attempt = stringToAttemptChoice(queryMap.get('attempt'));
@@ -1358,6 +1412,7 @@
this.reporting.setRepoName(state.project);
this.reporting.setChangeId(changeNum);
this.normalizePatchRangeParams(state);
+ // Note that router model view must be updated before view models.
this.setState(state);
this.changeViewModel.setState(state);
}
@@ -1374,6 +1429,7 @@
this.reporting.setRepoName(state.project ?? '');
this.reporting.setChangeId(changeNum);
this.normalizePatchRangeParams(state);
+ // Note that router model view must be updated before view models.
this.setState(state);
this.diffViewModel.setState(state);
}
@@ -1390,6 +1446,7 @@
this.reporting.setRepoName(state.project);
this.reporting.setChangeId(changeNum);
this.normalizePatchRangeParams(state);
+ // Note that router model view must be updated before view models.
this.setState(state);
this.changeViewModel.setState(state);
}
@@ -1413,6 +1470,7 @@
this.reporting.setRepoName(state.project ?? '');
this.reporting.setChangeId(changeNum);
this.normalizePatchRangeParams(state);
+ // Note that router model view must be updated before view models.
this.setState(state);
this.diffViewModel.setState(state);
}
@@ -1452,6 +1510,7 @@
view: GerritView.EDIT,
};
this.normalizePatchRangeParams(state);
+ // Note that router model view must be updated before view models.
this.setState(state);
this.editViewModel.setState(state);
this.reporting.setRepoName(project);
@@ -1480,6 +1539,7 @@
);
}
this.normalizePatchRangeParams(state);
+ // Note that router model view must be updated before view models.
this.setState(state);
this.changeViewModel.setState(state);
this.reporting.setRepoName(project);
@@ -1494,6 +1554,7 @@
const state: AgreementViewState = {
view: GerritView.AGREEMENTS,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.agreementViewModel.setState(state);
}
@@ -1507,12 +1568,14 @@
view: GerritView.SETTINGS,
emailToken: token,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.settingsViewModel.setState(state);
}
handleSettingsRoute(_: PageContext) {
const state: SettingsViewState = {view: GerritView.SETTINGS};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.settingsViewModel.setState(state);
}
@@ -1558,6 +1621,7 @@
plugin: ctx.params[0],
screen: ctx.params[1],
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.pluginViewModel.setState(state);
}
@@ -1567,6 +1631,7 @@
view: GerritView.DOCUMENTATION_SEARCH,
filter: ctx.params['filter'] || null,
};
+ // Note that router model view must be updated before view models.
this.setState(state);
this.documentationViewModel.setState(state);
}
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 ad2daf5..721349f 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -508,14 +508,22 @@
if (isUnsaved(this.comment) && !this.editing) return;
const classes = {container: true, draft: isDraftOrUnsaved(this.comment)};
return html`
- <div id="container" class=${classMap(classes)}>
- ${this.renderHeader()}
- <div class="body">
- ${this.renderRobotAuthor()} ${this.renderEditingTextarea()}
- ${this.renderCommentMessage()} ${this.renderHumanActions()}
- ${this.renderRobotActions()} ${this.renderSuggestEditActions()}
+ <gr-endpoint-decorator name="comment">
+ <gr-endpoint-param name="comment" .value=${this.comment}>
+ </gr-endpoint-param>
+ <gr-endpoint-param name="editing" .value=${this.editing}>
+ </gr-endpoint-param>
+ <div id="container" class=${classMap(classes)}>
+ ${this.renderHeader()}
+ <div class="body">
+ ${this.renderRobotAuthor()} ${this.renderEditingTextarea()}
+ ${this.renderCommentMessage()}
+ <gr-endpoint-slot name="above-actions"></gr-endpoint-slot>
+ ${this.renderHumanActions()} ${this.renderRobotActions()}
+ ${this.renderSuggestEditActions()}
+ </div>
</div>
- </div>
+ </gr-endpoint-decorator>
${this.renderConfirmDialog()}
`;
}
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 5686c6b..1b48658 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
@@ -92,26 +92,32 @@
assert.shadowDom.equal(
initiallyCollapsedElement,
/* HTML */ `
- <div class="container" id="container">
- <div class="header" id="header">
- <div class="headerLeft">
- <gr-account-label deselected=""></gr-account-label>
+ <gr-endpoint-decorator name="comment">
+ <gr-endpoint-param name="comment"></gr-endpoint-param>
+ <gr-endpoint-param name="editing"></gr-endpoint-param>
+ <div class="container" id="container">
+ <div class="header" id="header">
+ <div class="headerLeft">
+ <gr-account-label deselected=""></gr-account-label>
+ </div>
+ <div class="headerMiddle">
+ <span class="collapsedContent">
+ This is the test comment message.
+ </span>
+ </div>
+ <span class="patchset-text">Patchset 1</span>
+ <div class="show-hide" tabindex="0">
+ <label aria-label="Expand" class="show-hide">
+ <input checked="" class="show-hide" type="checkbox" />
+ <gr-icon id="icon" icon="expand_more"></gr-icon>
+ </label>
+ </div>
</div>
- <div class="headerMiddle">
- <span class="collapsedContent">
- This is the test comment message.
- </span>
- </div>
- <span class="patchset-text">Patchset 1</span>
- <div class="show-hide" tabindex="0">
- <label aria-label="Expand" class="show-hide">
- <input checked="" class="show-hide" type="checkbox" />
- <gr-icon id="icon" icon="expand_more"></gr-icon>
- </label>
+ <div class="body">
+ <gr-endpoint-slot name="above-actions"></gr-endpoint-slot>
</div>
</div>
- <div class="body"></div>
- </div>
+ </gr-endpoint-decorator>
`
);
});
@@ -122,28 +128,33 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div class="container" id="container">
- <div class="header" id="header">
- <div class="headerLeft">
- <gr-account-label deselected=""></gr-account-label>
+ <gr-endpoint-decorator name="comment">
+ <gr-endpoint-param name="comment"></gr-endpoint-param>
+ <gr-endpoint-param name="editing"></gr-endpoint-param>
+ <div class="container" id="container">
+ <div class="header" id="header">
+ <div class="headerLeft">
+ <gr-account-label deselected=""></gr-account-label>
+ </div>
+ <div class="headerMiddle"></div>
+ <span class="patchset-text">Patchset 1</span>
+ <span class="separator"></span>
+ <span class="date" tabindex="0">
+ <gr-date-formatter withtooltip=""></gr-date-formatter>
+ </span>
+ <div class="show-hide" tabindex="0">
+ <label aria-label="Collapse" class="show-hide">
+ <input class="show-hide" type="checkbox" />
+ <gr-icon id="icon" icon="expand_less"></gr-icon>
+ </label>
+ </div>
</div>
- <div class="headerMiddle"></div>
- <span class="patchset-text">Patchset 1</span>
- <span class="separator"></span>
- <span class="date" tabindex="0">
- <gr-date-formatter withtooltip=""></gr-date-formatter>
- </span>
- <div class="show-hide" tabindex="0">
- <label aria-label="Collapse" class="show-hide">
- <input class="show-hide" type="checkbox" />
- <gr-icon id="icon" icon="expand_less"></gr-icon>
- </label>
+ <div class="body">
+ <gr-formatted-text class="message"></gr-formatted-text>
+ <gr-endpoint-slot name="above-actions"></gr-endpoint-slot>
</div>
</div>
- <div class="body">
- <gr-formatted-text class="message"></gr-formatted-text>
- </div>
- </div>
+ </gr-endpoint-decorator>
`
);
});
@@ -155,60 +166,65 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div class="container" id="container">
- <div class="header" id="header">
- <div class="headerLeft">
- <span class="robotName">robot-id-123</span>
+ <gr-endpoint-decorator name="comment">
+ <gr-endpoint-param name="comment"></gr-endpoint-param>
+ <gr-endpoint-param name="editing"></gr-endpoint-param>
+ <div class="container" id="container">
+ <div class="header" id="header">
+ <div class="headerLeft">
+ <span class="robotName">robot-id-123</span>
+ </div>
+ <div class="headerMiddle"></div>
+ <span class="patchset-text">Patchset 1</span>
+ <span class="separator"></span>
+ <span class="date" tabindex="0">
+ <gr-date-formatter withtooltip=""></gr-date-formatter>
+ </span>
+ <div class="show-hide" tabindex="0">
+ <label aria-label="Collapse" class="show-hide">
+ <input class="show-hide" type="checkbox" />
+ <gr-icon id="icon" icon="expand_less"></gr-icon>
+ </label>
+ </div>
</div>
- <div class="headerMiddle"></div>
- <span class="patchset-text">Patchset 1</span>
- <span class="separator"></span>
- <span class="date" tabindex="0">
- <gr-date-formatter withtooltip=""></gr-date-formatter>
- </span>
- <div class="show-hide" tabindex="0">
- <label aria-label="Collapse" class="show-hide">
- <input class="show-hide" type="checkbox" />
- <gr-icon id="icon" icon="expand_less"></gr-icon>
- </label>
+ <div class="body">
+ <div class="robotId"></div>
+ <gr-formatted-text class="message"></gr-formatted-text>
+ <gr-endpoint-slot name="above-actions"></gr-endpoint-slot>
+ <div class="robotActions">
+ <gr-icon
+ icon="link"
+ class="copy link-icon"
+ role="button"
+ tabindex="0"
+ title="Copy link to this comment"
+ ></gr-icon>
+ <gr-endpoint-decorator name="robot-comment-controls">
+ <gr-endpoint-param name="comment"></gr-endpoint-param>
+ </gr-endpoint-decorator>
+ <gr-button
+ aria-disabled="false"
+ class="action show-fix"
+ link=""
+ role="button"
+ secondary=""
+ tabindex="0"
+ >
+ Show Fix
+ </gr-button>
+ <gr-button
+ aria-disabled="false"
+ class="action fix"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ Please Fix
+ </gr-button>
+ </div>
</div>
</div>
- <div class="body">
- <div class="robotId"></div>
- <gr-formatted-text class="message"></gr-formatted-text>
- <div class="robotActions">
- <gr-icon
- icon="link"
- class="copy link-icon"
- role="button"
- tabindex="0"
- title="Copy link to this comment"
- ></gr-icon>
- <gr-endpoint-decorator name="robot-comment-controls">
- <gr-endpoint-param name="comment"></gr-endpoint-param>
- </gr-endpoint-decorator>
- <gr-button
- aria-disabled="false"
- class="action show-fix"
- link=""
- role="button"
- secondary=""
- tabindex="0"
- >
- Show Fix
- </gr-button>
- <gr-button
- aria-disabled="false"
- class="action fix"
- link=""
- role="button"
- tabindex="0"
- >
- Please Fix
- </gr-button>
- </div>
- </div>
- </div>
+ </gr-endpoint-decorator>
`
);
});
@@ -242,64 +258,69 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div class="container draft" id="container">
- <div class="header" id="header">
- <div class="headerLeft">
- <gr-tooltip-content
- class="draftTooltip"
- has-tooltip=""
- max-width="20em"
- title="This draft is only visible to you. To publish drafts, click the 'Reply' or 'Start review' button at the top of the change or press the 'a' key."
- >
- <gr-icon filled icon="rate_review"></gr-icon>
- <span class="draftLabel">Draft</span>
- </gr-tooltip-content>
- </div>
- <div class="headerMiddle"></div>
- <span class="patchset-text">Patchset 1</span>
- <span class="separator"></span>
- <span class="date" tabindex="0">
- <gr-date-formatter withtooltip=""></gr-date-formatter>
- </span>
- <div class="show-hide" tabindex="0">
- <label aria-label="Collapse" class="show-hide">
- <input class="show-hide" type="checkbox" />
- <gr-icon id="icon" icon="expand_less"></gr-icon>
- </label>
- </div>
- </div>
- <div class="body">
- <gr-formatted-text class="message"></gr-formatted-text>
- <div class="actions">
- <div class="action resolve">
- <label>
- <input checked="" id="resolvedCheckbox" type="checkbox" />
- Resolved
+ <gr-endpoint-decorator name="comment">
+ <gr-endpoint-param name="comment"></gr-endpoint-param>
+ <gr-endpoint-param name="editing"></gr-endpoint-param>
+ <div class="container draft" id="container">
+ <div class="header" id="header">
+ <div class="headerLeft">
+ <gr-tooltip-content
+ class="draftTooltip"
+ has-tooltip=""
+ max-width="20em"
+ title="This draft is only visible to you. To publish drafts, click the 'Reply' or 'Start review' button at the top of the change or press the 'a' key."
+ >
+ <gr-icon filled icon="rate_review"></gr-icon>
+ <span class="draftLabel">Draft</span>
+ </gr-tooltip-content>
+ </div>
+ <div class="headerMiddle"></div>
+ <span class="patchset-text">Patchset 1</span>
+ <span class="separator"></span>
+ <span class="date" tabindex="0">
+ <gr-date-formatter withtooltip=""></gr-date-formatter>
+ </span>
+ <div class="show-hide" tabindex="0">
+ <label aria-label="Collapse" class="show-hide">
+ <input class="show-hide" type="checkbox" />
+ <gr-icon id="icon" icon="expand_less"></gr-icon>
</label>
</div>
- <div class="rightActions">
- <gr-button
- aria-disabled="false"
- class="action discard"
- link=""
- role="button"
- tabindex="0"
- >
- Discard
- </gr-button>
- <gr-button
- aria-disabled="false"
- class="action edit"
- link=""
- role="button"
- tabindex="0"
- >
- Edit
- </gr-button>
+ </div>
+ <div class="body">
+ <gr-formatted-text class="message"></gr-formatted-text>
+ <gr-endpoint-slot name="above-actions"></gr-endpoint-slot>
+ <div class="actions">
+ <div class="action resolve">
+ <label>
+ <input checked="" id="resolvedCheckbox" type="checkbox" />
+ Resolved
+ </label>
+ </div>
+ <div class="rightActions">
+ <gr-button
+ aria-disabled="false"
+ class="action discard"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ Discard
+ </gr-button>
+ <gr-button
+ aria-disabled="false"
+ class="action edit"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ Edit
+ </gr-button>
+ </div>
</div>
</div>
</div>
- </div>
+ </gr-endpoint-decorator>
`
);
});
@@ -312,72 +333,77 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div class="container draft" id="container">
- <div class="header" id="header">
- <div class="headerLeft">
- <gr-tooltip-content
- class="draftTooltip"
- has-tooltip=""
- max-width="20em"
- title="This draft is only visible to you. To publish drafts, click the 'Reply' or 'Start review' button at the top of the change or press the 'a' key."
- >
- <gr-icon filled icon="rate_review"></gr-icon>
- <span class="draftLabel">Draft</span>
- </gr-tooltip-content>
- </div>
- <div class="headerMiddle"></div>
- <span class="patchset-text">Patchset 1</span>
- <span class="separator"></span>
- <span class="date" tabindex="0">
- <gr-date-formatter withtooltip=""></gr-date-formatter>
- </span>
- <div class="show-hide" tabindex="0">
- <label aria-label="Collapse" class="show-hide">
- <input class="show-hide" type="checkbox" />
- <gr-icon id="icon" icon="expand_less"></gr-icon>
- </label>
- </div>
- </div>
- <div class="body">
- <gr-textarea
- autocomplete="on"
- class="code editMessage"
- code=""
- id="editTextarea"
- rows="4"
- text="This is the test comment message."
- >
- </gr-textarea>
- <div class="actions">
- <div class="action resolve">
- <label>
- <input checked="" id="resolvedCheckbox" type="checkbox" />
- Resolved
+ <gr-endpoint-decorator name="comment">
+ <gr-endpoint-param name="comment"></gr-endpoint-param>
+ <gr-endpoint-param name="editing"></gr-endpoint-param>
+ <div class="container draft" id="container">
+ <div class="header" id="header">
+ <div class="headerLeft">
+ <gr-tooltip-content
+ class="draftTooltip"
+ has-tooltip=""
+ max-width="20em"
+ title="This draft is only visible to you. To publish drafts, click the 'Reply' or 'Start review' button at the top of the change or press the 'a' key."
+ >
+ <gr-icon filled icon="rate_review"></gr-icon>
+ <span class="draftLabel">Draft</span>
+ </gr-tooltip-content>
+ </div>
+ <div class="headerMiddle"></div>
+ <span class="patchset-text">Patchset 1</span>
+ <span class="separator"></span>
+ <span class="date" tabindex="0">
+ <gr-date-formatter withtooltip=""></gr-date-formatter>
+ </span>
+ <div class="show-hide" tabindex="0">
+ <label aria-label="Collapse" class="show-hide">
+ <input class="show-hide" type="checkbox" />
+ <gr-icon id="icon" icon="expand_less"></gr-icon>
</label>
</div>
- <div class="rightActions">
- <gr-button
- aria-disabled="false"
- class="action cancel"
- link=""
- role="button"
- tabindex="0"
- >
- Cancel
- </gr-button>
- <gr-button
- aria-disabled="false"
- class="action save"
- link=""
- role="button"
- tabindex="0"
- >
- Save
- </gr-button>
+ </div>
+ <div class="body">
+ <gr-textarea
+ autocomplete="on"
+ class="code editMessage"
+ code=""
+ id="editTextarea"
+ rows="4"
+ text="This is the test comment message."
+ >
+ </gr-textarea>
+ <gr-endpoint-slot name="above-actions"></gr-endpoint-slot>
+ <div class="actions">
+ <div class="action resolve">
+ <label>
+ <input checked="" id="resolvedCheckbox" type="checkbox" />
+ Resolved
+ </label>
+ </div>
+ <div class="rightActions">
+ <gr-button
+ aria-disabled="false"
+ class="action cancel"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ Cancel
+ </gr-button>
+ <gr-button
+ aria-disabled="false"
+ class="action save"
+ link=""
+ role="button"
+ tabindex="0"
+ >
+ Save
+ </gr-button>
+ </div>
</div>
</div>
</div>
- </div>
+ </gr-endpoint-decorator>
`
);
});
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
index da90b17..6ab0ec4 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
@@ -154,7 +154,17 @@
// for this.
// 4. Rewrite plain text ("text") to apply linking and other config-based
// rewrites. Text within code blocks is not passed here.
+ // 5. Open links in a new tab by rendering with target="_blank" attribute.
function customRenderer(renderer: {[type: string]: Function}) {
+ renderer['link'] = (href: string, title: string, text: string) =>
+ /* HTML */
+ `<a
+ href="${href}"
+ target="_blank"
+ ${title ? `title="${title}"` : ''}
+ rel="noopener"
+ >${text}</a
+ >`;
renderer['image'] = (href: string, _title: string, text: string) =>
``;
renderer['codespan'] = (text: string) =>
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
index b405e6b..6391347 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
@@ -332,7 +332,13 @@
<div slot="markdown-html">
<p>
@
- <a href="mailto:someone@google.com"> someone@google.com </a>
+ <a
+ href="mailto:someone@google.com"
+ rel="noopener"
+ target="_blank"
+ >
+ someone@google.com
+ </a>
</p>
</div>
</marked-element>
@@ -383,7 +389,13 @@
<div slot="markdown-html">
<p>
<code>@</code>
- <a href="mailto:someone@google.com"> someone@google.com </a>
+ <a
+ href="mailto:someone@google.com"
+ rel="noopener"
+ target="_blank"
+ >
+ someone@google.com
+ </a>
</p>
</div>
</marked-element>
@@ -401,7 +413,9 @@
<marked-element>
<div slot="markdown-html">
<p>
- <a href="https://www.google.com">myLink</a>
+ <a href="https://www.google.com" rel="noopener" target="_blank"
+ >myLink</a
+ >
</p>
</div>
</marked-element>
@@ -482,7 +496,9 @@
<p>block quote ${escapedDiv}</p>
</blockquote>
<p>
- <a href="http://google.com">inline link ${escapedDiv}</a>
+ <a href="http://google.com" rel="noopener" target="_blank"
+ >inline link ${escapedDiv}</a
+ >
</p>
</div>
</marked-element>
diff --git a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
index 1d3802a..f706712 100644
--- a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
+++ b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model.ts
@@ -22,8 +22,11 @@
ReviewInput,
ReviewerInput,
AttentionSetInput,
+ RelatedChangeAndCommitInfo,
} from '../../types/common';
import {getUserId} from '../../utils/account-util';
+import {getChangeNumber} from '../../utils/change-util';
+import {deepEqual} from '../../utils/deep-util';
export const bulkActionsModelToken =
define<BulkActionsModel>('bulk-actions-model');
@@ -136,12 +139,12 @@
return Promise.resolve(new Response());
}
return this.restApiService.executeChangeAction(
- change._number,
+ getChangeNumber(change),
change.actions!.abandon!.method,
'/abandon',
undefined,
{message: reason ?? ''},
- () => errFn && errFn(change._number)
+ () => errFn && errFn(getChangeNumber(change))
);
});
}
@@ -152,7 +155,7 @@
const change = current.allChanges.get(changeNum)!;
if (!change) throw new Error('invalid change id');
return this.restApiService.saveChangeReview(
- change._number,
+ getChangeNumber(change),
'current',
reviewInput,
() => {
@@ -196,7 +199,7 @@
add_to_attention_set: attentionSetUpdates,
};
return this.restApiService.saveChangeReview(
- change._number,
+ getChangeNumber(change),
'current',
reviewInput
);
@@ -233,13 +236,13 @@
);
}
- async sync(changes: ChangeInfo[]) {
- const basicChanges = new Map(changes.map(c => [c._number, c]));
+ async sync(changes: (ChangeInfo | RelatedChangeAndCommitInfo)[]) {
+ const basicChanges = new Map(changes.map(c => [getChangeNumber(c), c]));
let currentState = this.getState();
const selectedChangeNums = currentState.selectedChangeNums.filter(
changeNum => basicChanges.has(changeNum)
);
- const selectableChangeNums = changes.map(c => c._number);
+ const selectableChangeNums = changes.map(c => getChangeNumber(c));
this.updateState({
loadingState: LoadingState.LOADING,
selectedChangeNums,
@@ -252,11 +255,13 @@
}
const changeDetails =
await this.restApiService.getDetailedChangesWithActions(
- changes.map(c => c._number)
+ changes.map(c => getChangeNumber(c))
);
currentState = this.getState();
// Return early if sync has been called again since starting the load.
- if (selectableChangeNums !== currentState.selectableChangeNums) return;
+ if (!deepEqual(selectableChangeNums, currentState.selectableChangeNums)) {
+ return;
+ }
const allDetailedChanges: Map<NumericChangeId, ChangeInfo> = new Map();
for (const detailedChange of changeDetails ?? []) {
allDetailedChanges.set(detailedChange._number, detailedChange);
diff --git a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts
index 2192246..e2f75f7 100644
--- a/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts
+++ b/polygerrit-ui/app/models/bulk-actions/bulk-actions-model_test.ts
@@ -500,7 +500,7 @@
const getChangesStub = stubRestApi(
'getDetailedChangesWithActions'
).callsFake(() => promise);
- bulkActionsModel.sync([c1, c2]);
+ bulkActionsModel.sync([c1]);
assert.strictEqual(getChangesStub.callCount, 1);
await waitUntilObserved(
bulkActionsModel.loadingState$,
@@ -534,7 +534,6 @@
// Resolve the old promise.
responsePromise1.resolve([
{...createChange(), _number: 1, subject: 'Subject 1-old'},
- {...createChange(), _number: 2, subject: 'Subject 2-old'},
] as ChangeInfo[]);
await waitEventLoop();
const model2 = bulkActionsModel.getState();
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts
index 8e058aa..2b92529 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -62,6 +62,7 @@
ChecksUpdate,
PluginsModel,
} from '../plugins/plugins-model';
+import {ChangeViewModel} from '../views/change';
/**
* The checks model maintains the state of checks for two patchsets: the latest
@@ -137,20 +138,6 @@
}
interface ChecksState {
- /**
- * This is the patchset number selected by the user. The *latest* patchset
- * can be picked up from the change model.
- */
- patchsetNumberSelected?: PatchSetNumber;
- /**
- * This is the attempt number selected by the user. If this is `undefined`
- * (default), then for each run the latest attempt is displayed.
- */
- attemptNumberSelected: AttemptChoice;
- /**
- * Current filter set by the user in the runs panel or via URL.
- */
- runFilterRegexp: string;
/** Checks data for the latest patchset. */
pluginStateLatest: {
[name: string]: ChecksProviderState;
@@ -210,24 +197,30 @@
private subscriptions: Subscription[] = [];
+ // TODO: Maybe consider migrating usages to `changeViewModel` directly.
public checksSelectedPatchsetNumber$ = select(
- this.state$,
- state => state.patchsetNumberSelected
+ this.changeViewModel.checksPatchset$,
+ ps => ps
);
public checksSelectedAttemptNumber$ = select(
- this.state$,
- state => state.attemptNumberSelected
+ this.changeViewModel.attempt$,
+ attempt => attempt ?? LATEST_ATTEMPT
);
- public runFilterRegexp$ = select(this.state$, state => state.runFilterRegexp);
+ public runFilterRegexp$ = select(
+ this.changeViewModel.filter$,
+ filter => filter ?? ''
+ );
public checksLatest$ = select(this.state$, state => state.pluginStateLatest);
- public checksSelected$ = select(this.state$, state =>
- state.patchsetNumberSelected
- ? state.pluginStateSelected
- : state.pluginStateLatest
+ public checksSelected$ = select(
+ combineLatest([this.state$, this.changeViewModel.checksPatchset$]),
+ ([state, ps]) => {
+ const checksPs = ps ? ChecksPatchset.SELECTED : ChecksPatchset.LATEST;
+ return this.getPluginState(state, checksPs);
+ }
);
public aPluginHasRegistered$ = select(
@@ -379,14 +372,12 @@
constructor(
readonly routerModel: RouterModel,
+ readonly changeViewModel: ChangeViewModel,
readonly changeModel: ChangeModel,
readonly reporting: ReportingService,
readonly pluginsModel: PluginsModel
) {
super({
- patchsetNumberSelected: undefined,
- attemptNumberSelected: LATEST_ATTEMPT,
- runFilterRegexp: '',
pluginStateLatest: {},
pluginStateSelected: {},
});
@@ -405,19 +396,6 @@
this.checkToPluginMap$.subscribe(map => {
this.checkToPluginMap = map;
}),
- combineLatest([
- this.routerModel.routerPatchNum$,
- this.changeModel.latestPatchNum$,
- ]).subscribe(([routerPs, latestPs]) => {
- this.latestPatchNum = latestPs;
- if (latestPs === undefined) {
- this.setPatchset(undefined);
- } else if (typeof routerPs === 'number') {
- this.setPatchset(routerPs);
- } else {
- this.setPatchset(latestPs);
- }
- }),
this.firstLoadCompleted$
.pipe(
filter(completed => !!completed),
@@ -655,20 +633,18 @@
this.setState(nextState);
}
- updateStateSetPatchset(patchsetNumberSelected?: PatchSetNumber) {
- this.updateState({patchsetNumberSelected});
+ updateStateSetPatchset(num?: PatchSetNumber) {
+ this.changeViewModel.updateState({
+ checksPatchset: num === this.latestPatchNum ? undefined : num,
+ });
}
updateStateSetAttempt(attemptNumberSelected: AttemptChoice) {
- this.updateState({attemptNumberSelected});
+ this.changeViewModel.updateState({attempt: attemptNumberSelected});
}
updateStateSetRunFilter(runFilterRegexp: string) {
- this.updateState({runFilterRegexp});
- }
-
- setPatchset(num?: PatchSetNumber) {
- this.updateStateSetPatchset(num === this.latestPatchNum ? undefined : num);
+ this.changeViewModel.updateState({filter: runFilterRegexp});
}
reload(pluginName: string) {
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 88fbebc..83ed464 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -21,6 +21,7 @@
import {changeModelToken} from '../change/change-model';
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
+import {changeViewModelToken} from '../views/change';
const PLUGIN_NAME = 'test-plugin';
@@ -63,6 +64,7 @@
setup(() => {
model = new ChecksModel(
getAppContext().routerModel,
+ testResolver(changeViewModelToken),
testResolver(changeModelToken),
getAppContext().reportingService,
getAppContext().pluginsModel
diff --git a/polygerrit-ui/app/models/model.ts b/polygerrit-ui/app/models/model.ts
index 7cd8b2a..2e90a5e 100644
--- a/polygerrit-ui/app/models/model.ts
+++ b/polygerrit-ui/app/models/model.ts
@@ -5,6 +5,7 @@
*/
import {BehaviorSubject, Observable} from 'rxjs';
import {Finalizable} from '../services/registry';
+import {deepEqual} from '../utils/deep-util';
/**
* A Model stores a value <T> and controls changes to that value via `subject$`
@@ -35,6 +36,7 @@
}
setState(state: T) {
+ if (deepEqual(state, this.getState())) return;
this.subject$.next(state);
}
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index b8451a0..826ec66 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -9,9 +9,11 @@
RevisionPatchSetNum,
BasePatchSetNum,
ChangeInfo,
+ PatchSetNumber,
} from '../../api/rest-api';
import {GerritView} from '../../services/router/router-model';
import {UrlEncodedCommentId} from '../../types/common';
+import {select} from '../../utils/observable-util';
import {
encodeURL,
getBaseUrl,
@@ -24,22 +26,34 @@
export interface ChangeViewState extends ViewState {
view: GerritView.CHANGE;
+
changeNum: NumericChangeId;
project: RepoName;
edit?: boolean;
patchNum?: RevisionPatchSetNum;
basePatchNum?: BasePatchSetNum;
commentId?: UrlEncodedCommentId;
- forceReload?: boolean;
- openReplyDialog?: boolean;
tab?: string;
+
+ /** Checks related view state */
+
+ /** selected patchset for check runs (undefined=latest) */
+ checksPatchset?: PatchSetNumber;
/** regular expression for filtering check runs */
filter?: string;
- /** selected attempt for selected check runs */
+ /** selected attempt for check runs (undefined=latest) */
attempt?: AttemptChoice;
+ /** State properties that trigger one-time actions */
+
+ /** for scrolling a Change Log message into view in gr-change-view */
messageHash?: string;
+ /** for logging where the user came from */
usp?: string;
+ /** triggers all change related data to be reloaded */
+ forceReload?: boolean;
+ /** triggers opening the reply dialog */
+ openReplyDialog?: boolean;
}
/**
@@ -84,6 +98,15 @@
}
let suffix = `${range}`;
const queries = [];
+ if (state.checksPatchset && state.checksPatchset > 0) {
+ queries.push(`checksPatchset=${state.checksPatchset}`);
+ }
+ if (state.attempt) {
+ if (state.attempt !== 'latest') queries.push(`attempt=${state.attempt}`);
+ }
+ if (state.filter) {
+ queries.push(`filter=${state.filter}`);
+ }
if (state.forceReload) {
queries.push('forceReload=true');
}
@@ -117,7 +140,27 @@
define<ChangeViewModel>('change-view-model');
export class ChangeViewModel extends Model<ChangeViewState | undefined> {
+ public readonly tab$ = select(this.state$, state => state?.tab);
+
+ public readonly checksPatchset$ = select(
+ this.state$,
+ state => state?.checksPatchset
+ );
+
+ public readonly attempt$ = select(this.state$, state => state?.attempt);
+
+ public readonly filter$ = select(this.state$, state => state?.filter);
+
constructor() {
super(undefined);
+ this.state$.subscribe(s => {
+ if (s?.usp || s?.forceReload || s?.openReplyDialog) {
+ this.updateState({
+ usp: undefined,
+ forceReload: undefined,
+ openReplyDialog: undefined,
+ });
+ }
+ });
}
}
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 9f7a0c0..2e1b817 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -186,6 +186,7 @@
const checksModel = new ChecksModel(
appContext.routerModel,
+ changeViewModel,
changeModel,
appContext.reportingService,
appContext.pluginsModel
diff --git a/polygerrit-ui/app/services/router/router-model.ts b/polygerrit-ui/app/services/router/router-model.ts
index 0699554..99b9870 100644
--- a/polygerrit-ui/app/services/router/router-model.ts
+++ b/polygerrit-ui/app/services/router/router-model.ts
@@ -29,6 +29,7 @@
}
export interface RouterState {
+ // Note that this router model view must be updated before view model state.
view?: GerritView;
changeNum?: NumericChangeId;
patchNum?: RevisionPatchSetNum;
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index e34773e..2cdd4a5 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -207,6 +207,7 @@
const checksModelCreator = () =>
new ChecksModel(
appContext.routerModel,
+ resolver(changeViewModelToken),
resolver(changeModelToken),
appContext.reportingService,
appContext.pluginsModel
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index 23acff4..597bb6f 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -8,7 +8,6 @@
import {FetchRequest} from './types';
import {LineNumberEventDetail, MovedLinkClickedEventDetail} from '../api/diff';
import {Category, RunStatus} from '../api/checks';
-import {AttemptChoice} from '../models/checks/checks-util';
export enum EventType {
BIND_VALUE_CHANGED = 'bind-value-changed',
@@ -231,10 +230,6 @@
export interface ChecksTabState {
statusOrCategory?: RunStatus | Category;
checkName?: string;
- /** regular expression for filtering runs */
- filter?: string;
- /** selected attempt for selected runs */
- attempt?: AttemptChoice;
}
export type SwitchTabEvent = CustomEvent<SwitchTabEventDetail>;
diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts
index ad5d48a..07b63d6 100644
--- a/polygerrit-ui/app/utils/change-util.ts
+++ b/polygerrit-ui/app/utils/change-util.ts
@@ -137,6 +137,20 @@
) {
return change?.status === ChangeStatus.ABANDONED;
}
+/**
+ * Get the change number from either a ChangeInfo (such as those included in
+ * SubmittedTogetherInfo responses) or get the change number from a
+ * RelatedChangeAndCommitInfo (such as those included in a
+ * RelatedChangesInfo response).
+ */
+export function getChangeNumber(
+ change: ChangeInfo | ParsedChangeInfo | RelatedChangeAndCommitInfo
+): NumericChangeId {
+ if (isChangeInfo(change)) {
+ return change._number;
+ }
+ return change._change_number!;
+}
export function changeStatuses(
change: ChangeInfo,
diff --git a/polygerrit-ui/app/utils/page-wrapper-utils.ts b/polygerrit-ui/app/utils/page-wrapper-utils.ts
index 7096423..78e78ed 100644
--- a/polygerrit-ui/app/utils/page-wrapper-utils.ts
+++ b/polygerrit-ui/app/utils/page-wrapper-utils.ts
@@ -14,6 +14,7 @@
(pageCallback: PageCallback): void;
show(url: string): void;
redirect(url: string): void;
+ replace(path: string, state: null, init: boolean, dispatch: boolean): void;
base(url: string): void;
start(): void;
exit(pattern: string | RegExp, ...pageCallback: PageCallback[]): void;
