Merge "New change summary - metrics"
diff --git a/java/com/google/gerrit/entities/CachedProjectConfig.java b/java/com/google/gerrit/entities/CachedProjectConfig.java
index 0b755b7..2a94bc8 100644
--- a/java/com/google/gerrit/entities/CachedProjectConfig.java
+++ b/java/com/google/gerrit/entities/CachedProjectConfig.java
@@ -14,19 +14,17 @@
package com.google.gerrit.entities;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.eclipse.jgit.annotations.Nullable;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
/**
@@ -37,6 +35,8 @@
*/
@AutoValue
public abstract class CachedProjectConfig {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
public abstract Project getProject();
public abstract ImmutableMap<AccountGroup.UUID, GroupReference> getGroups();
@@ -126,34 +126,10 @@
public abstract ImmutableMap<String, String> getPluginConfigs();
- /**
- * Returns the {@link Config} that got parsed from the specified {@code fileName} on {@code
- * refs/meta/config}. The returned instance is a defensive copy of the cached value.
- *
- * @param fileName the name of the file. Must end in {@code .config}.
- * @return an {@link Optional} of the {@link Config}. {@link Optional#empty()} if the file was not
- * found or could not be parsed. {@link com.google.gerrit.server.project.ProjectConfig} will
- * surface validation errors in case of a parsing issue.
- */
- public Optional<Config> getProjectLevelConfig(String fileName) {
- checkState(fileName.endsWith(".config"), "file name must end in .config");
- if (getProjectLevelConfigs().containsKey(fileName)) {
- Config config = new Config();
- try {
- config.fromText(getProjectLevelConfigs().get(fileName));
- } catch (ConfigInvalidException e) {
- // This is OK to propagate as IllegalStateException because it's a programmer error.
- // The config was converted to a String using Config#toText. So #fromText must not
- // throw a ConfigInvalidException
- throw new IllegalStateException("invalid config for " + fileName, e);
- }
- return Optional.of(config);
- }
- return Optional.empty();
- }
-
public abstract ImmutableMap<String, String> getProjectLevelConfigs();
+ public abstract ImmutableMap<String, ImmutableConfig> getParsedProjectLevelConfigs();
+
public static Builder builder() {
return new AutoValue_CachedProjectConfig.Builder();
}
@@ -235,8 +211,15 @@
abstract ImmutableMap.Builder<String, String> projectLevelConfigsBuilder();
+ abstract ImmutableMap.Builder<String, ImmutableConfig> parsedProjectLevelConfigsBuilder();
+
public Builder addProjectLevelConfig(String configFileName, String config) {
projectLevelConfigsBuilder().put(configFileName, config);
+ try {
+ parsedProjectLevelConfigsBuilder().put(configFileName, ImmutableConfig.parse(config));
+ } catch (ConfigInvalidException e) {
+ logger.atInfo().withCause(e).log("Config for " + configFileName + " not parsable");
+ }
return this;
}
diff --git a/java/com/google/gerrit/entities/ImmutableConfig.java b/java/com/google/gerrit/entities/ImmutableConfig.java
new file mode 100644
index 0000000..a5efc14
--- /dev/null
+++ b/java/com/google/gerrit/entities/ImmutableConfig.java
@@ -0,0 +1,91 @@
+// Copyright (C) 2021 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.entities;
+
+import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+
+/**
+ * Immutable parsed representation of a {@link org.eclipse.jgit.lib.Config} that can be cached.
+ * Supports only a limited set of operations.
+ */
+public class ImmutableConfig {
+ public static final ImmutableConfig EMPTY = new ImmutableConfig("", new Config());
+
+ private final String stringCfg;
+ private final Config cfg;
+
+ private ImmutableConfig(String stringCfg, Config cfg) {
+ this.stringCfg = stringCfg;
+ this.cfg = cfg;
+ }
+
+ public static ImmutableConfig parse(String stringCfg) throws ConfigInvalidException {
+ Config cfg = new Config();
+ cfg.fromText(stringCfg);
+ return new ImmutableConfig(stringCfg, cfg);
+ }
+
+ /** Returns a mutable copy of this config. */
+ public Config mutableCopy() {
+ Config cfg = new Config();
+ try {
+ cfg.fromText(this.cfg.toText());
+ } catch (ConfigInvalidException e) {
+ // Can't happen as we used JGit to format that config.
+ throw new IllegalStateException(e);
+ }
+ return cfg;
+ }
+
+ /** @see Config#getSections() */
+ public Set<String> getSections() {
+ return cfg.getSections();
+ }
+
+ /** @see Config#getNames(String) */
+ public Set<String> getNames(String section) {
+ return cfg.getNames(section);
+ }
+
+ /** @see Config#getNames(String, String) */
+ public Set<String> getNames(String section, String subsection) {
+ return cfg.getNames(section, subsection);
+ }
+
+ /** @see Config#getStringList(String, String, String) */
+ public String[] getStringList(String section, String subsection, String name) {
+ return cfg.getStringList(section, subsection, name);
+ }
+
+ /** @see Config#getSubsections(String) */
+ public Set<String> getSubsections(String section) {
+ return cfg.getSubsections(section);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof ImmutableConfig)) {
+ return false;
+ }
+ return ((ImmutableConfig) o).stringCfg.equals(stringCfg);
+ }
+
+ @Override
+ public int hashCode() {
+ return stringCfg.hashCode();
+ }
+}
diff --git a/java/com/google/gerrit/server/ApprovalInference.java b/java/com/google/gerrit/server/ApprovalInference.java
index 572ae7a..d77427a 100644
--- a/java/com/google/gerrit/server/ApprovalInference.java
+++ b/java/com/google/gerrit/server/ApprovalInference.java
@@ -25,6 +25,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.LabelTypes;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
@@ -109,10 +110,10 @@
PatchSetApproval psa,
PatchSet.Id psId,
ChangeKind kind,
- PatchList patchList) {
+ LabelType type,
+ @Nullable PatchList patchList) {
int n = psa.key().patchSetId().get();
checkArgument(n != psId.get());
- LabelType type = project.getLabelTypes().byLabel(psa.labelId());
if (type == null) {
logger.atFine().log(
@@ -367,12 +368,18 @@
logger.atFine().log(
"change kind for patch set %d of change %d against prior patch set %s is %s",
ps.id().get(), ps.id().changeId().get(), priorPatchSet.getValue().id().changeId(), kind);
- PatchList patchList = getPatchList(project, ps, priorPatchSet);
+ PatchList patchList = null;
+ LabelTypes labelTypes = project.getLabelTypes();
for (PatchSetApproval psa : priorApprovals) {
if (resultByUser.contains(psa.label(), psa.accountId())) {
continue;
}
- if (!canCopy(project, psa, ps.id(), kind, patchList)) {
+ LabelType type = labelTypes.byLabel(psa.labelId());
+ // Only compute patchList if there is a relevant label, since this is expensive.
+ if (patchList == null && type != null && type.isCopyAllScoresIfListOfFilesDidNotChange()) {
+ patchList = getPatchList(project, ps, priorPatchSet);
+ }
+ if (!canCopy(project, psa, ps.id(), kind, type, patchList)) {
continue;
}
resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(ps.id()));
diff --git a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
index d507531..6e640f3 100644
--- a/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
+++ b/java/com/google/gerrit/server/git/validators/CommentCumulativeSizeValidator.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.validators.CommentValidationContext;
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
+import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.Inject;
@@ -34,6 +35,8 @@
* issues. Note that autogenerated change messages are not subject to validation.
*/
public class CommentCumulativeSizeValidator implements CommentValidator {
+ public static final int DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT = 3 << 20;
+
private final int maxCumulativeSize;
private final ChangeNotes.Factory notesFactory;
@@ -41,7 +44,9 @@
CommentCumulativeSizeValidator(
@GerritServerConfig Config serverConfig, ChangeNotes.Factory notesFactory) {
this.notesFactory = notesFactory;
- maxCumulativeSize = serverConfig.getInt("change", "cumulativeCommentSizeLimit", 3 << 20);
+ maxCumulativeSize =
+ serverConfig.getInt(
+ "change", "cumulativeCommentSizeLimit", DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT);
}
@Override
@@ -55,7 +60,13 @@
notes.getRobotComments().values().stream())
.mapToInt(Comment::getApproximateSize)
.sum()
- + notes.getChangeMessages().stream().mapToInt(cm -> cm.getMessage().length()).sum();
+ + notes.getChangeMessages().stream()
+ // Auto-generated change messages are not counted for the limit. This method is not
+ // called when those change messages are created, but we should also skip them when
+ // counting the size for unrelated messages.
+ .filter(cm -> !ChangeMessagesUtil.isAutogenerated(cm.getTag()))
+ .mapToInt(cm -> cm.getMessage().length())
+ .sum();
int newCumulativeSize =
comments.stream().mapToInt(CommentForValidation::getApproximateSize).sum();
ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder();
diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
index 63e0b7a..18d532b 100644
--- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
+++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
@@ -30,7 +30,9 @@
import com.google.gerrit.prettify.common.SparseFileContent;
import com.google.gerrit.prettify.common.SparseFileContent.Accessor;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.git.validators.CommentCumulativeSizeValidator;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -41,6 +43,7 @@
import java.util.List;
import java.util.stream.Collectors;
import org.eclipse.jgit.diff.Edit;
+import org.eclipse.jgit.lib.Config;
/**
* This class is used on submit to compute the diff between the latest approved patch-set, and the
@@ -58,15 +61,22 @@
private final ProjectCache projectCache;
private final PatchScriptFactory.Factory patchScriptFactoryFactory;
private final PatchListCache patchListCache;
+ private final int maxCumulativeSize;
@Inject
SubmitWithStickyApprovalDiff(
ProjectCache projectCache,
PatchScriptFactory.Factory patchScriptFactoryFactory,
- PatchListCache patchListCache) {
+ PatchListCache patchListCache,
+ @GerritServerConfig Config serverConfig) {
this.projectCache = projectCache;
this.patchScriptFactoryFactory = patchScriptFactoryFactory;
this.patchListCache = patchListCache;
+ maxCumulativeSize =
+ serverConfig.getInt(
+ "change",
+ "cumulativeCommentSizeLimit",
+ CommentCumulativeSizeValidator.DEFAULT_CUMULATIVE_COMMENT_SIZE_LIMIT);
}
public String apply(ChangeNotes notes, CurrentUser currentUser)
@@ -117,6 +127,16 @@
getDiffForFile(
notes, currentPatchset.id(), latestApprovedPatchsetId, patchListEntry, currentUser));
}
+ if (diff.length() > maxCumulativeSize) {
+ // The diff length is not counted as part of the limit (for technical reasons, since we'd
+ // have to call CommentCumulativeSizeValidator), but it's best not to post an extra large
+ // change message here.
+ return String.format(
+ "\n\n%d is the latest approved patch-set.\nThe change was submitted "
+ + "with many unreviewed changes (the diff is too large to show). Please review the "
+ + "diff.",
+ latestApprovedPatchsetId.get());
+ }
return diff.toString();
}
diff --git a/java/com/google/gerrit/server/project/ProjectLevelConfig.java b/java/com/google/gerrit/server/project/ProjectLevelConfig.java
index 4825233..555cf4c 100644
--- a/java/com/google/gerrit/server/project/ProjectLevelConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectLevelConfig.java
@@ -16,14 +16,15 @@
import static java.util.stream.Collectors.toList;
-import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
+import com.google.gerrit.entities.ImmutableConfig;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.meta.VersionedMetaData;
import java.io.IOException;
import java.util.Arrays;
import java.util.Set;
import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
import org.eclipse.jgit.annotations.Nullable;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
@@ -73,19 +74,17 @@
private final String fileName;
private final ProjectState project;
- private Config cfg;
+ private final ImmutableConfig immutableConfig;
- public ProjectLevelConfig(String fileName, ProjectState project, Config cfg) {
+ public ProjectLevelConfig(
+ String fileName, ProjectState project, @Nullable ImmutableConfig immutableConfig) {
this.fileName = fileName;
this.project = project;
- this.cfg = cfg;
+ this.immutableConfig = immutableConfig == null ? ImmutableConfig.EMPTY : immutableConfig;
}
public Config get() {
- if (cfg == null) {
- cfg = new Config();
- }
- return cfg;
+ return immutableConfig.mutableCopy();
}
public Config getWithInheritance() {
@@ -105,58 +104,54 @@
* @return a combined config.
*/
public Config getWithInheritance(boolean merge) {
- Config cfgWithInheritance = new Config();
- try {
- cfgWithInheritance.fromText(get().toText());
- } catch (ConfigInvalidException e) {
- // cannot happen
- }
- ProjectState parent = Iterables.getFirst(project.parents(), null);
- if (parent != null) {
- Config parentCfg = parent.getConfig(fileName).getWithInheritance();
- for (String section : parentCfg.getSections()) {
- Set<String> allNames = get().getNames(section);
- for (String name : parentCfg.getNames(section)) {
- String[] parentValues = parentCfg.getStringList(section, null, name);
- if (!allNames.contains(name)) {
- cfgWithInheritance.setStringList(section, null, name, Arrays.asList(parentValues));
- } else if (merge) {
- cfgWithInheritance.setStringList(
- section,
- null,
- name,
- Stream.concat(
- Arrays.stream(cfg.getStringList(section, null, name)),
- Arrays.stream(parentValues))
- .sorted()
- .distinct()
- .collect(toList()));
- }
- }
+ Config cfg = new Config();
+ // Traverse from All-Projects down to the current project
+ StreamSupport.stream(project.treeInOrder().spliterator(), false)
+ .forEach(
+ parent -> {
+ ImmutableConfig levelCfg = parent.getConfig(fileName).immutableConfig;
+ for (String section : levelCfg.getSections()) {
+ Set<String> allNames = cfg.getNames(section);
+ for (String name : levelCfg.getNames(section)) {
+ String[] levelValues = levelCfg.getStringList(section, null, name);
+ if (allNames.contains(name) && merge) {
+ cfg.setStringList(
+ section,
+ null,
+ name,
+ Stream.concat(
+ Arrays.stream(cfg.getStringList(section, null, name)),
+ Arrays.stream(levelValues))
+ .sorted()
+ .distinct()
+ .collect(toList()));
+ } else {
+ cfg.setStringList(section, null, name, Arrays.asList(levelValues));
+ }
+ }
- for (String subsection : parentCfg.getSubsections(section)) {
- allNames = get().getNames(section, subsection);
- for (String name : parentCfg.getNames(section, subsection)) {
- String[] parentValues = parentCfg.getStringList(section, subsection, name);
- if (!allNames.contains(name)) {
- cfgWithInheritance.setStringList(
- section, subsection, name, Arrays.asList(parentValues));
- } else if (merge) {
- cfgWithInheritance.setStringList(
- section,
- subsection,
- name,
- Streams.concat(
- Arrays.stream(cfg.getStringList(section, subsection, name)),
- Arrays.stream(parentValues))
- .sorted()
- .distinct()
- .collect(toList()));
- }
- }
- }
- }
- }
- return cfgWithInheritance;
+ for (String subsection : levelCfg.getSubsections(section)) {
+ allNames = cfg.getNames(section, subsection);
+ for (String name : levelCfg.getNames(section, subsection)) {
+ String[] levelValues = levelCfg.getStringList(section, subsection, name);
+ if (allNames.contains(name) && merge) {
+ cfg.setStringList(
+ section,
+ subsection,
+ name,
+ Streams.concat(
+ Arrays.stream(cfg.getStringList(section, subsection, name)),
+ Arrays.stream(levelValues))
+ .sorted()
+ .distinct()
+ .collect(toList()));
+ } else {
+ cfg.setStringList(section, subsection, name, Arrays.asList(levelValues));
+ }
+ }
+ }
+ }
+ });
+ return cfg;
}
}
diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java
index 8c024ef..249eb35 100644
--- a/java/com/google/gerrit/server/project/ProjectState.java
+++ b/java/com/google/gerrit/server/project/ProjectState.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.PermissionRule.Action.ALLOW;
import static java.util.Comparator.comparing;
@@ -176,8 +177,9 @@
}
public ProjectLevelConfig getConfig(String fileName) {
- Optional<Config> rawConfig = cachedConfig.getProjectLevelConfig(fileName);
- return new ProjectLevelConfig(fileName, this, rawConfig.orElse(new Config()));
+ checkState(fileName.endsWith(".config"), "file name must end in .config. is: " + fileName);
+ return new ProjectLevelConfig(
+ fileName, this, cachedConfig.getParsedProjectLevelConfigs().get(fileName));
}
public long getMaxObjectSizeLimit() {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
index eba2634..5ca7310 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
@@ -21,8 +21,10 @@
import static com.google.gerrit.server.project.testing.TestLabels.value;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.Change;
@@ -30,6 +32,7 @@
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.server.patch.filediff.Edit;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.inject.Inject;
@@ -144,6 +147,56 @@
}
@Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "1k")
+ public void autoGeneratedPostSubmitDiffIsNotPartOfTheCommentSizeLimit() throws Exception {
+ Change.Id changeId =
+ changeOperations.newChange().project(project).file("file").content("content").create();
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
+ String content = new String(new char[800]).replace("\0", "a");
+ changeOperations.change(changeId).newPatchset().file("file").content(content).create();
+
+ // Post a submit diff that is almost the cumulativeCommentSizeLimit
+ gApi.changes().id(changeId.get()).current().submit();
+ assertThat(Iterables.getLast(gApi.changes().id(changeId.get()).messages()).message)
+ .doesNotContain("many unreviewed changes");
+
+ // unrelated comment and change message posting works fine, since the post submit diff is not
+ // counted towards the cumulativeCommentSizeLimit for unrelated follow-up comments.
+ // 800 + 400 + 400 > 1k, but 400 + 400 < 1k, hence these comments are accepted (the original
+ // 800 is not counted).
+ String message = new String(new char[400]).replace("\0", "a");
+ ReviewInput reviewInput = new ReviewInput().message(message);
+ CommentInput commentInput = new CommentInput();
+ commentInput.line = 1;
+ commentInput.message = message;
+ commentInput.path = "file";
+ reviewInput.comments = ImmutableMap.of("file", ImmutableList.of(commentInput));
+
+ gApi.changes().id(changeId.get()).current().review(reviewInput);
+ }
+
+ @Test
+ @GerritConfig(name = "change.cumulativeCommentSizeLimit", value = "1k")
+ public void postSubmitDiffCannotBeTooBig() throws Exception {
+ Change.Id changeId =
+ changeOperations.newChange().project(project).file("file").content("content").create();
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
+
+ String content = new String(new char[1100]).replace("\0", "a");
+
+ changeOperations.change(changeId).newPatchset().file("file").content(content).create();
+
+ // Post submit diff is over the cumulativeCommentSizeLimit, so we shorten the message.
+ gApi.changes().id(changeId.get()).current().submit();
+ assertThat(Iterables.getLast(gApi.changes().id(changeId.get()).messages()).message)
+ .isEqualTo(
+ "Change has been successfully merged\n\n1 is the latest approved patch-set.\nThe "
+ + "change was submitted "
+ + "with many unreviewed changes (the diff is too large to show). Please review the "
+ + "diff.");
+ }
+
+ @Test
public void diffChangeMessageOnSubmitWithStickyVote_addedFile() throws Exception {
Change.Id changeId = changeOperations.newChange().project(project).create();
gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
diff --git a/plugins/replication b/plugins/replication
index a379adc..14766e7 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit a379adcd2f4f1a41731818e74b2a214de3fcf5d8
+Subproject commit 14766e75f91886ab48951035d59a78c8c3f07471
diff --git a/polygerrit-ui/app/api/checks.ts b/polygerrit-ui/app/api/checks.ts
index 799d1f7..4a5ef7e 100644
--- a/polygerrit-ui/app/api/checks.ts
+++ b/polygerrit-ui/app/api/checks.ts
@@ -247,7 +247,7 @@
checkName: string | undefined,
/** Identical to 'name' property of Action entity. */
actionName: string
-) => Promise<ActionResult>;
+) => Promise<ActionResult> | undefined;
export interface ActionResult {
/** An empty errorMessage means success. */
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
index 816c8ef..a34bd63 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
@@ -380,6 +380,19 @@
e.detail.change._number,
e.detail.starred
);
+ // When a change is updated the same change may appear elsewhere in the
+ // dashboard (but is not the same object), so we must update other
+ // occurrences of the same change.
+ this._results?.forEach((dashboardChange, dashboardIndex) =>
+ dashboardChange.results.forEach((change, changeIndex) => {
+ if (change.id === e.detail.change.id) {
+ this.set(
+ `_results.${dashboardIndex}.results.${changeIndex}.starred`,
+ e.detail.starred
+ );
+ }
+ })
+ );
}
_handleToggleReviewed(e: CustomEvent<ChangeListToggleReviewedDetail>) {
@@ -387,6 +400,19 @@
e.detail.change._number,
e.detail.reviewed
);
+ // When a change is updated the same change may appear elsewhere in the
+ // dashboard (but is not the same object), so we must update other
+ // occurrences of the same change.
+ this._results?.forEach((dashboardChange, dashboardIndex) =>
+ dashboardChange.results.forEach((change, changeIndex) => {
+ if (change.id === e.detail.change.id) {
+ this.set(
+ `_results.${dashboardIndex}.results.${changeIndex}.reviewed`,
+ e.detail.reviewed
+ );
+ }
+ })
+ );
}
/**
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js
index 47f885b..a5de72b 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js
@@ -332,6 +332,56 @@
});
});
+ test('toggling star will update change everywhere', () => {
+ // It is important that the same change is represented by multiple objects
+ // and all are updated.
+ const change = {id: '5', starred: false};
+ const sameChange = {id: '5', starred: false};
+ const differentChange = {id: '4', starred: false};
+ element._results = [
+ {query: 'has:draft', results: [change]},
+ {query: 'is:open', results: [sameChange, differentChange]},
+ ];
+
+ element._handleToggleStar(
+ new CustomEvent('toggle-star', {
+ detail: {
+ change,
+ starred: true,
+ },
+ })
+ );
+
+ assert.isTrue(change.starred);
+ assert.isTrue(sameChange.starred);
+ assert.isFalse(differentChange.starred);
+ });
+
+ test('toggling reviewed will update change everywhere', () => {
+ // It is important that the same change is represented by multiple objects
+ // and all are updated.
+ const change = {id: '5', reviewed: false};
+ const sameChange = {id: '5', reviewed: false};
+ const differentChange = {id: '4', reviewed: false};
+ element._results = [
+ {query: 'has:draft', results: [change]},
+ {query: 'is:open', results: [sameChange, differentChange]},
+ ];
+
+ element._handleToggleReviewed(
+ new CustomEvent('toggle-reviewed', {
+ detail: {
+ change,
+ reviewed: true,
+ },
+ })
+ );
+
+ assert.isTrue(change.reviewed);
+ assert.isTrue(sameChange.reviewed);
+ assert.isFalse(differentChange.reviewed);
+ });
+
test('_showNewUserHelp', () => {
element._loading = false;
element._showNewUserHelp = false;
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts
index b93040b..8d03e32 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts
@@ -305,7 +305,7 @@
</gr-button>
</template>
<template is="dom-if" if="[[message._revision_number]]">
- <span class="patchset">[[message._revision_number]]</span>
+ <span class="patchset">[[message._revision_number]] |</span>
</template>
<template is="dom-if" if="[[!message.id]]">
<span class="date">
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
index ad4f2ae..e5386f4 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-tab.ts
@@ -28,6 +28,7 @@
allActions$,
allResults$,
allRuns$,
+ someProvidersAreLoading$,
} from '../../services/checks/checks-model';
import './gr-checks-runs';
import './gr-checks-results';
@@ -41,6 +42,10 @@
import {checkRequiredProperty} from '../../utils/common-util';
import {RunSelectedEvent} from './gr-checks-runs';
import {ChecksTabState} from '../../types/events';
+import {fireAlert} from '../../utils/event-util';
+import {appContext} from '../../services/app-context';
+import {from, timer} from 'rxjs';
+import {takeUntil} from 'rxjs/operators';
/**
* The "Checks" tab on the Gerrit change page. Gets its data from plugins that
@@ -64,9 +69,14 @@
@property()
changeNum: NumericChangeId | undefined = undefined;
+ @property()
+ someProvidersAreLoading = false;
+
@internalProperty()
selectedRuns: string[] = [];
+ private readonly checksService = appContext.checksService;
+
constructor() {
super();
this.subscribe('runs', allRuns$);
@@ -74,6 +84,7 @@
this.subscribe('results', allResults$);
this.subscribe('currentPatchNum', currentPatchNum$);
this.subscribe('changeNum', changeNum$);
+ this.subscribe('someProvidersAreLoading', someProvidersAreLoading$);
this.addEventListener('action-triggered', (e: ActionTriggeredEvent) =>
this.handleActionTriggered(e.detail.action, e.detail.run)
@@ -131,6 +142,7 @@
},
]}"
></gr-dropdown-list>
+ <span ?hidden="${!this.someProvidersAreLoading}">Loading...</span>
</div>
<div class="right">
${this.actions.map(this.renderAction)}
@@ -170,10 +182,7 @@
handleActionTriggered(action: Action, run?: CheckRun) {
if (!this.changeNum) return;
if (!this.currentPatchNum) return;
- // TODO(brohlfs): The callback is supposed to be returning a promise.
- // A toast should be displayed until the promise completes. And then the
- // data should be updated.
- action.callback(
+ const promise = action.callback(
this.changeNum,
this.currentPatchNum as number,
run?.attempt,
@@ -181,6 +190,25 @@
run?.checkName,
action.name
);
+ // Plugins *should* return a promise, but you never know ...
+ if (promise?.then) {
+ const prefix = `Triggering action '${action.name}'`;
+ fireAlert(this, `${prefix} ...`);
+ from(promise)
+ // If the action takes longer than 5 seconds, then most likely the
+ // user is either not interested or the result not relevant anymore.
+ .pipe(takeUntil(timer(5000)))
+ .subscribe(result => {
+ if (result.errorMessage) {
+ fireAlert(this, `${prefix} failed with ${result.errorMessage}.`);
+ } else {
+ fireAlert(this, `${prefix} successful.`);
+ this.checksService.reloadForCheck(run?.checkName);
+ }
+ });
+ } else {
+ fireAlert(this, `Action '${action.name}' triggered.`);
+ }
}
handleRunSelected(e: RunSelectedEvent) {
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index 28aca73..d8dc688 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -87,6 +87,18 @@
})
);
+export const checkToPluginMap$ = checksState$.pipe(
+ map(state => {
+ const map = new Map<string, string>();
+ for (const [pluginName, providerState] of Object.entries(state)) {
+ for (const run of providerState.runs) {
+ map.set(run.checkName, pluginName);
+ }
+ }
+ return map;
+ })
+);
+
export const allResults$ = checksState$.pipe(
map(state => {
return Object.values(state)
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index b11eada..d2a4775 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -32,6 +32,7 @@
import {change$, currentPatchNum$} from '../change/change-model';
import {
updateStateSetLoading,
+ checkToPluginMap$,
updateStateSetProvider,
updateStateSetResults,
} from './checks-model';
@@ -51,6 +52,12 @@
private changeAndPatchNum$ = change$.pipe(withLatestFrom(currentPatchNum$));
+ private checkToPluginMap = new Map<string, string>();
+
+ constructor() {
+ checkToPluginMap$.subscribe(x => (this.checkToPluginMap = x));
+ }
+
reload(pluginName: string) {
this.reloadSubjects[pluginName].next();
}
@@ -59,6 +66,12 @@
Object.keys(this.providers).forEach(key => this.reload(key));
}
+ reloadForCheck(checkName?: string) {
+ if (!checkName) return;
+ const plugin = this.checkToPluginMap.get(checkName);
+ if (plugin) this.reload(plugin);
+ }
+
register(
pluginName: string,
provider: ChecksProvider,
