Merge "Remove IncludedInResolver.includedInAny"
diff --git a/Documentation/dev-crafting-changes.txt b/Documentation/dev-crafting-changes.txt
index 15bf785..ac0780d 100644
--- a/Documentation/dev-crafting-changes.txt
+++ b/Documentation/dev-crafting-changes.txt
@@ -26,7 +26,9 @@
* Improvements of existing features should also generally go into
`master`. But we understand that if you cannot run `master`, it
might take a while until you could benefit from it. In that case,
- start on the newest `stable-*` branch that you can run.
+ implement the feature on master and, if you really need it on an
+ earlier `stable-*` branch, cherry-pick the change and build
+ Gerrit on your own environent.
* Bug-fixes should generally at least cover the oldest affected and
still supported version. If you're affected and run an even older
version, you're welcome to upload to that older version, even if
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index a66d3b5..992d459 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -2366,38 +2366,15 @@
If neither resource `Documentation/index.html` or
`Documentation/index.md` exists in the plugin JAR, Gerrit will
-automatically generate an index page for the plugin's documentation
-tree by scanning every `*.md` and `*.html` file in the Documentation/
-directory.
+automatically generate an index page.
-For any discovered Markdown (`*.md`) file, Gerrit will parse the
-header of the file and extract the first level one title. This
-title text will be used as display text for a link to the HTML
-version of the page.
+The generated index page contains 3 sections:
-For any discovered HTML (`*.html`) file, Gerrit will use the name
-of the file, minus the `*.html` extension, as the link text. Any
-hyphens in the file name will be replaced with spaces.
-
-If a discovered file is named `about.md` or `about.html`, its
-content will be inserted in an 'About' section at the top of the
-auto-generated index page. If both `about.md` and `about.html`
-exist, only the first discovered file will be used.
-
-If a discovered file name beings with `cmd-` it will be clustered
-into a 'Commands' section of the generated index page.
-
-If a discovered file name beings with `servlet-` it will be clustered
-into a 'Servlets' section of the generated index page.
-
-If a discovered file name beings with `rest-api-` it will be clustered
-into a 'REST APIs' section of the generated index page.
-
-All other files are clustered under a 'Documentation' section.
-
+1. Manifest section
++
Some optional information from the manifest is extracted and
displayed as part of the index page, if present in the manifest:
-
++
[width="40%",options="header"]
|===================================================
|Field | Source Attribute
@@ -2408,6 +2385,49 @@
|API Version | Gerrit-ApiVersion
|===================================================
+2. About section
++
+If an `about.md` or `about.html` file exists, its content will be inserted in an
+'About' section.
++
+If both `about.md` and `about.html` exist, only the first discovered file will
+be used.
+
+3. TOC section
++
+If a `toc.md` or `toc.html` file exists, its content will be inserted in a
+'Documentation' section.
++
+`toc.md` or `toc.html` is a manually maintained index of the documentation pages
+that exist in the plugin. Having a manually maintained index has the advantage
+that you can group the documentation pages by topic and sort them by importance.
++
+If both `toc.md` and `toc.html` exist, only the first discovered file will
+be used.
++
+If no `toc` file is present the TOC section is automatically generated by
+scanning every `\*.md` and `*.html` file in the `Documentation/` directory.
++
+For any discovered Markdown (`*.md`) file, Gerrit will parse the
+header of the file and extract the first level one title. This
+title text will be used as display text for a link to the HTML
+version of the page.
++
+For any discovered HTML (`\*.html`) file, Gerrit will use the name
+of the file, minus the `*.html` extension, as the link text. Any
+hyphens in the file name will be replaced with spaces.
++
+If a discovered file name beings with `cmd-` it will be clustered
+into a 'Commands' section of the generated index page.
++
+If a discovered file name beings with `servlet-` it will be clustered
+into a 'Servlets' section of the generated index page.
++
+If a discovered file name beings with `rest-api-` it will be clustered
+into a 'REST APIs' section of the generated index page.
++
+All other files are clustered under a 'Documentation' section.
+
[[deployment]]
== Deployment
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index 0670968..a04ff35 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -622,6 +622,20 @@
point, which could be slow and create lots of unintended new changes.
To create multiple new changes, run push multiple times.
+[[ignore-attention-set]]
+=== Ignore automatic attention set rules
+
+Normally, we add users to the attention set based on several rules such as adding
+reviewers, replying, and many others. The full rule list is in
+link:user-attention-set.html[Attention Set].
+
+--ignore-automatic-attention-set-rules (also known as -ias and
+-ignore-attention-set) can be used to keep the attention set as it were before
+the push.
+
+----
+ git push ssh://john.doe@git.example.com:29418/kernel/common my-merged-commit:refs/for/master%ias
+----
== repo upload
diff --git a/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java b/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java
index c37d30b..ee28df9 100644
--- a/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java
+++ b/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java
@@ -17,11 +17,15 @@
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.inject.Inject;
+import com.google.inject.ProvisionException;
import com.google.inject.Singleton;
/**
* Stores the updated refs whenever they are updated, so that we can export this information in the
* response headers.
+ *
+ * <p>This is only working for HTTP requests. {@link WebSession} is not bound outside of HTTP
+ * requests.
*/
@Singleton
public class GitReferenceUpdatedTracker implements GitReferenceUpdatedListener {
@@ -35,7 +39,15 @@
@Override
public void onGitReferenceUpdated(GitReferenceUpdatedListener.Event event) {
- WebSession currentSession = webSession.get();
+ WebSession currentSession = null;
+ try {
+ currentSession = webSession.get();
+ } catch (ProvisionException ex) {
+ // We couldn't bind the current session properly. This is expected to happen at any point we
+ // perform ref updates without an HTTP request (git push for example).
+ // If we can't get a WebSession, we don't need to track the updated references.
+ return;
+ }
if (currentSession != null) {
currentSession.addRefUpdatedEvents(event);
}
diff --git a/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java
index 43eb3a0..ef37fc5 100644
--- a/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java
+++ b/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java
@@ -358,6 +358,33 @@
return cachedResource != null && cachedResource.isUnchanged(lastUpdateTime);
}
+ private void appendPageAsSection(
+ PluginContentScanner scanner, PluginEntry pluginEntry, String sectionTitle, StringBuilder md)
+ throws IOException {
+ InputStreamReader isr = new InputStreamReader(scanner.getInputStream(pluginEntry), UTF_8);
+ StringBuilder content = new StringBuilder();
+ try (BufferedReader reader = new BufferedReader(isr)) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ line = StringUtils.stripEnd(line, null);
+ if (line.isEmpty()) {
+ content.append("\n");
+ } else {
+ content.append(line).append("\n");
+ }
+ }
+ }
+
+ // Only append the section if there was anything in it
+ if (content.toString().trim().length() > 0) {
+ md.append("## ");
+ md.append(sectionTitle);
+ md.append(" ##\n");
+ md.append("\n").append(content);
+ md.append("\n");
+ }
+ }
+
private void appendEntriesSection(
PluginContentScanner scanner,
List<PluginEntry> entries,
@@ -400,6 +427,7 @@
List<PluginEntry> restApis = new ArrayList<>();
List<PluginEntry> docs = new ArrayList<>();
PluginEntry about = null;
+ PluginEntry toc = null;
Predicate<PluginEntry> filter =
entry -> {
@@ -437,6 +465,14 @@
"Plugin %s: Multiple 'about' documents found; using %s",
pluginName, about.getName().substring(prefix.length()));
}
+ } else if (name.startsWith("toc.")) {
+ if (toc == null) {
+ toc = entry;
+ } else {
+ logger.atWarning().log(
+ "Plugin %s: Multiple 'toc' documents found; using %s",
+ pluginName, toc.getName().substring(prefix.length()));
+ }
} else {
docs.add(entry);
}
@@ -451,31 +487,17 @@
appendPluginInfoTable(md, scanner.getManifest().getMainAttributes());
if (about != null) {
- InputStreamReader isr = new InputStreamReader(scanner.getInputStream(about), UTF_8);
- StringBuilder aboutContent = new StringBuilder();
- try (BufferedReader reader = new BufferedReader(isr)) {
- String line;
- while ((line = reader.readLine()) != null) {
- line = StringUtils.stripEnd(line, null);
- if (line.isEmpty()) {
- aboutContent.append("\n");
- } else {
- aboutContent.append(line).append("\n");
- }
- }
- }
-
- // Only append the About section if there was anything in it
- if (aboutContent.toString().trim().length() > 0) {
- md.append("## About ##\n");
- md.append("\n").append(aboutContent);
- }
+ appendPageAsSection(scanner, about, "About", md);
}
- appendEntriesSection(scanner, docs, "Documentation", md, prefix, 0);
- appendEntriesSection(scanner, servlets, "Servlets", md, prefix, "servlet-".length());
- appendEntriesSection(scanner, restApis, "REST APIs", md, prefix, "rest-api-".length());
- appendEntriesSection(scanner, cmds, "Commands", md, prefix, "cmd-".length());
+ if (toc != null) {
+ appendPageAsSection(scanner, toc, "Documentaion", md);
+ } else {
+ appendEntriesSection(scanner, docs, "Documentation", md, prefix, 0);
+ appendEntriesSection(scanner, servlets, "Servlets", md, prefix, "servlet-".length());
+ appendEntriesSection(scanner, restApis, "REST APIs", md, prefix, "rest-api-".length());
+ appendEntriesSection(scanner, cmds, "Commands", md, prefix, "cmd-".length());
+ }
sendMarkdownAsHtml(md.toString(), pluginName, cacheKey, res, lastModifiedTime);
}
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
index 7a53600..13b8b12 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
@@ -20,6 +20,7 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.CacheStats;
import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.BloomFilter;
@@ -44,7 +45,10 @@
import java.sql.Timestamp;
import java.time.Duration;
import java.time.Instant;
+import java.util.ArrayList;
import java.util.Calendar;
+import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
@@ -137,6 +141,23 @@
}
@Override
+ public ImmutableMap<K, V> getAll(Iterable<? extends K> keys) throws ExecutionException {
+ if (mem instanceof LoadingCache) {
+ ImmutableMap.Builder<K, V> result = ImmutableMap.builder();
+ LoadingCache<K, ValueHolder<V>> asLoadingCache = (LoadingCache<K, ValueHolder<V>>) mem;
+ ImmutableMap<K, ValueHolder<V>> values = asLoadingCache.getAll(keys);
+ for (Map.Entry<K, ValueHolder<V>> entry : values.entrySet()) {
+ result.put(entry.getKey(), entry.getValue().value);
+ if (store.needsRefresh(entry.getValue().created)) {
+ asLoadingCache.refresh(entry.getKey());
+ }
+ }
+ return result.build();
+ }
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
public V get(K key, Callable<? extends V> valueLoader) throws ExecutionException {
return mem.get(
key,
@@ -265,6 +286,40 @@
}
@Override
+ public Map<K, ValueHolder<V>> loadAll(Iterable<? extends K> keys) throws Exception {
+ try (TraceTimer timer = TraceContext.newTimer("Loading multiple values from cache")) {
+ List<K> notInMemory = new ArrayList<>();
+ Map<K, ValueHolder<V>> result = new HashMap<>();
+ for (K key : keys) {
+ if (!store.mightContain(key)) {
+ notInMemory.add(key);
+ continue;
+ }
+ ValueHolder<V> h = store.getIfPresent(key);
+ if (h != null) {
+ result.put(key, h);
+ } else {
+ notInMemory.add(key);
+ }
+ }
+ try {
+ Map<K, V> remaining = loader.loadAll(notInMemory);
+ Instant instant = Instant.ofEpochMilli(TimeUtil.nowMs());
+ storeInDatabase(remaining, instant);
+ remaining
+ .entrySet()
+ .forEach(e -> result.put(e.getKey(), new ValueHolder<>(e.getValue(), instant)));
+ } catch (UnsupportedLoadingOperationException e) {
+ // Fallback to the default load() if loadAll() is not implemented
+ for (K k : notInMemory) {
+ result.put(k, load(k)); // No need to storeInDatabase here; load(k) does that.
+ }
+ }
+ return result;
+ }
+ }
+
+ @Override
public ListenableFuture<ValueHolder<V>> reload(K key, ValueHolder<V> oldValue)
throws Exception {
ListenableFuture<V> reloadedValue = loader.reload(key, oldValue.value);
@@ -285,6 +340,15 @@
return Futures.transform(reloadedValue, v -> new ValueHolder<>(v, TimeUtil.now()), executor);
}
+
+ private void storeInDatabase(Map<K, V> entries, Instant instant) {
+ executor.execute(
+ () -> {
+ for (Map.Entry<K, V> entry : entries.entrySet()) {
+ store.put(entry.getKey(), new ValueHolder<>(entry.getValue(), instant));
+ }
+ });
+ }
}
static class SqlStore<K, V> {
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonModule.java b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
index de116bb..f90bd14 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonModule.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
@@ -14,31 +14,12 @@
package com.google.gerrit.server.change;
-import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.AbstractModule;
-import org.eclipse.jgit.lib.Config;
public class FileInfoJsonModule extends AbstractModule {
- /** Use the new diff cache implementation {@link FileInfoJsonNewImpl}. */
- private final boolean useNewDiffCache;
-
- /** Used to dark launch the new diff cache with the list files endpoint. */
- private final boolean runNewDiffCacheAsync;
-
- public FileInfoJsonModule(@GerritServerConfig Config cfg) {
- this.useNewDiffCache =
- cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false);
- this.runNewDiffCacheAsync =
- cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_listFiles", false);
- }
@Override
public void configure() {
- if (runNewDiffCacheAsync) {
- bind(FileInfoJson.class).to(FileInfoJsonComparingImpl.class);
- return;
- }
- bind(FileInfoJson.class)
- .to(useNewDiffCache ? FileInfoJsonNewImpl.class : FileInfoJsonOldImpl.class);
+ bind(FileInfoJson.class).to(FileInfoJsonComparingImpl.class);
}
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 5a74c78..076ba46 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -271,7 +271,7 @@
install(new IgnoreSelfApprovalRule.Module());
install(new ReceiveCommitsModule());
install(new SshAddressesModule());
- install(new FileInfoJsonModule(cfg));
+ install(new FileInfoJsonModule());
install(ThreadLocalRequestContext.module());
install(new ApprovalModule());
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index ec2ed4f..6d6c19d2 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -111,6 +111,7 @@
import com.google.gerrit.server.RequestListener;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.approval.ApprovalsUtil;
+import com.google.gerrit.server.change.AttentionSetUnchangedOp;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.SetHashtagsOp;
@@ -1662,6 +1663,12 @@
@Option(name = "--create-cod-token", usage = "create a token for consistency-on-demand")
private boolean createCodToken;
+ @Option(
+ name = "--ignore-automatic-attention-set-rules",
+ aliases = {"-ias", "-ignore-attention-set"},
+ usage = "do not change the attention set on this push")
+ boolean ignoreAttentionSet;
+
MagicBranchInput(
IdentifiedUser user, ProjectState projectState, ReceiveCommand cmd, LabelTypes labelTypes) {
this.user = user;
@@ -2652,6 +2659,9 @@
if (!Strings.isNullOrEmpty(magicBranch.topic)) {
bu.addOp(changeId, setTopicFactory.create(magicBranch.topic));
}
+ if (magicBranch.ignoreAttentionSet) {
+ bu.addOp(changeId, new AttentionSetUnchangedOp());
+ }
bu.addOp(
changeId,
new BatchUpdateOp() {
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index b55e91b..f00b48eb 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -305,6 +305,9 @@
change.setWorkInProgress(true);
update.setWorkInProgress(true);
}
+ if (magicBranch.ignoreAttentionSet) {
+ update.ignoreFurtherAttentionSetUpdates();
+ }
}
newPatchSet =
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index f56e933..93e9c3f 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -56,6 +56,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.AttentionSetUpdate;
+import com.google.gerrit.entities.AttentionSetUpdate.Operation;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.HumanComment;
@@ -762,9 +763,7 @@
}
}
- if (plannedAttentionSetUpdates != null) {
- updateAttentionSet(msg);
- }
+ updateAttentionSet(msg);
CommitBuilder cb = new CommitBuilder();
cb.setMessage(msg.toString());
@@ -842,7 +841,7 @@
*/
private void updateAttentionSet(StringBuilder msg) {
if (plannedAttentionSetUpdates == null) {
- return;
+ plannedAttentionSetUpdates = new HashMap<>();
}
Set<Account.Id> currentUsersInAttentionSet =
AttentionSetUtil.additionsOnly(getNotes().getAttentionSet()).stream()
@@ -864,6 +863,8 @@
.map(r -> r.getKey())
.collect(ImmutableSet.toImmutableSet()));
+ removeInactiveUsersFromAttentionSet(currentReviewers);
+
for (AttentionSetUpdate attentionSetUpdate : plannedAttentionSetUpdates.values()) {
if (attentionSetUpdate.operation() == AttentionSetUpdate.Operation.ADD
&& currentUsersInAttentionSet.contains(attentionSetUpdate.account())) {
@@ -901,6 +902,38 @@
}
}
+ private void removeInactiveUsersFromAttentionSet(Set<Account.Id> currentReviewers) {
+ Set<Account.Id> inActiveUsersInTheAttentionSet =
+ // get the current attention set.
+ getNotes().getAttentionSet().stream()
+ .filter(a -> a.operation().equals(Operation.ADD))
+ .map(a -> a.account())
+ // remove users that are currently being removed from the attention set.
+ .filter(
+ a ->
+ plannedAttentionSetUpdates.getOrDefault(a, /*defaultValue= */ null) == null
+ || plannedAttentionSetUpdates.get(a).operation().equals(Operation.REMOVE))
+ // remove users that are still active on the change.
+ .filter(a -> !isActiveOnChange(currentReviewers, a))
+ .collect(ImmutableSet.toImmutableSet());
+
+ // We override the flag, as we never want such users in the attention set.
+ ignoreFurtherAttentionSetUpdates = false;
+
+ addToPlannedAttentionSetUpdates(
+ inActiveUsersInTheAttentionSet.stream()
+ .map(
+ a ->
+ AttentionSetUpdate.createForWrite(
+ a,
+ Operation.REMOVE,
+ /* reason= */ "Only change owner, uploader, reviewers, and cc can "
+ + "be in the attention set"))
+ .collect(ImmutableSet.toImmutableSet()));
+
+ ignoreFurtherAttentionSetUpdates = true;
+ }
+
/**
* Returns whether {@code accountId} is active on a change based on the {@code currentReviewers}.
* Activity is defined as being a part of the reviewers, an uploader, or an owner of a change.
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
index 9e74eec..7f7c1ad 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
@@ -216,7 +216,7 @@
for (ChangeData cd : result) {
for (Account.Id reviewer : cd.reviewers().all()) {
- if (Strings.isNullOrEmpty(query) || accountMatchesQuery(reviewer, query)) {
+ if (accountMatchesQuery(reviewer, query)) {
suggestions
.computeIfAbsent(reviewer, (ignored) -> new MutableDouble(0))
.add(baseWeight);
@@ -234,7 +234,8 @@
private boolean accountMatchesQuery(Account.Id id, String query) {
Optional<Account> account = accountCache.get(id).map(AccountState::account);
if (account.isPresent() && account.get().isActive()) {
- if ((account.get().fullName() != null && account.get().fullName().startsWith(query))
+ if (Strings.isNullOrEmpty(query)
+ || (account.get().fullName() != null && account.get().fullName().startsWith(query))
|| (account.get().preferredEmail() != null
&& account.get().preferredEmail().startsWith(query))) {
return true;
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 7ca763a1..cd5032a 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -250,7 +250,7 @@
install(new RestApiModule());
install(new OAuthRestModule());
install(new DefaultProjectNameLockManager.Module());
- install(new FileInfoJsonModule(cfg));
+ install(new FileInfoJsonModule());
bind(ProjectOperations.class).to(ProjectOperationsImpl.class);
}
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 45d1b76..5cf0403 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -66,6 +66,7 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Address;
+import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
@@ -97,6 +98,7 @@
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.events.TopicEditedListener;
+import com.google.gerrit.extensions.restapi.testing.AttentionSetUpdateSubject;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.events.CommitReceivedEvent;
@@ -2831,6 +2833,37 @@
r.assertErrorStatus("\"--skip-validation\" option is only supported for direct push");
}
+ @Test
+ public void pushWithReviewerAddsToAttentionSet() throws Exception {
+ String pushSpec = "refs/for/master%r=" + user.email();
+ PushOneCommit.Result r = pushTo(pushSpec);
+ r.assertOkStatus();
+
+ AttentionSetUpdate attentionSet = Iterables.getOnlyElement(r.getChange().attentionSet());
+ AttentionSetUpdateSubject.assertThat(attentionSet).hasAccountIdThat().isEqualTo(user.id());
+ AttentionSetUpdateSubject.assertThat(attentionSet)
+ .hasOperationThat()
+ .isEqualTo(AttentionSetUpdate.Operation.ADD);
+ AttentionSetUpdateSubject.assertThat(attentionSet)
+ .hasReasonThat()
+ .isEqualTo("Reviewer was added");
+ }
+
+ @Test
+ public void pushWithReviewerAndIgnoreAttentionSetDoesNotAddToAttentionSet() throws Exception {
+ // Create a change
+ String pushSpec = "refs/for/master%r=" + user.email() + ",-ignore-attention-set";
+ PushOneCommit.Result r = pushTo(pushSpec);
+ r.assertOkStatus();
+ assertThat(r.getChange().attentionSet()).isEmpty();
+
+ // push a new patch-set with another reviewer
+ pushSpec = "refs/for/master%r=" + accountCreator.user2().email() + ",-ignore-attention-set";
+ r = pushTo(pushSpec);
+ r.assertOkStatus();
+ assertThat(r.getChange().attentionSet()).isEmpty();
+ }
+
private DraftInput newDraft(String path, int line, String message) {
DraftInput d = new DraftInput();
d.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index d480eb1..800ee42 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -1758,6 +1758,30 @@
}
@Test
+ public void usersNotPartOfTheChangeAreNeverInTheAttentionSet() throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).addReviewer(user.email());
+
+ AttentionSetUpdate attentionSetUpdate =
+ Iterables.getOnlyElement(getAttentionSetUpdates(r.getChange().getId()));
+ assertThat(attentionSetUpdate).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(attentionSetUpdate).hasOperationThat().isEqualTo(Operation.ADD);
+
+ ReviewInput reviewInput = ReviewInput.create();
+ reviewInput.reviewer(user.email(), ReviewerState.REMOVED, /* confirmed= */ true);
+ reviewInput.ignoreAutomaticAttentionSetRules = true;
+ change(r).current().review(reviewInput);
+
+ // user removed from the attention set although we ignored automatic attention set rules.
+ attentionSetUpdate = Iterables.getOnlyElement(getAttentionSetUpdates(r.getChange().getId()));
+ assertThat(attentionSetUpdate).hasAccountIdThat().isEqualTo(user.id());
+ assertThat(attentionSetUpdate).hasOperationThat().isEqualTo(Operation.REMOVE);
+ assertThat(attentionSetUpdate)
+ .hasReasonThat()
+ .isEqualTo("Only change owner, uploader, reviewers, and cc can be in the attention set");
+ }
+
+ @Test
@GerritConfig(name = "accounts.visibility", value = "NONE")
public void canModifyAttentionSetForInvisibleUsersOnVisibleChanges() throws Exception {
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index ed6254a..3850e13 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -447,6 +447,8 @@
gApi.accounts().id(foo2.username()).setActive(false);
assertThat(gApi.accounts().id(foo2.id().get()).getActive()).isFalse();
assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(foo1), ImmutableList.of());
+ assertReviewers(
+ suggestReviewers(changeId, /*query=*/ ""), ImmutableList.of(foo1), ImmutableList.of());
}
@Test
diff --git a/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java b/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
index 3ade4d0..14af43b 100644
--- a/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
+++ b/javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java
@@ -26,6 +26,7 @@
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.gerrit.server.cache.h2.H2CacheImpl.SqlStore;
@@ -34,6 +35,10 @@
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.TypeLiteral;
import java.time.Duration;
+import java.time.Instant;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.Nullable;
@@ -105,6 +110,62 @@
}
@Test
+ public void getAll_WithLoadingCache_LoaderNotImplementingLoadAll() throws ExecutionException {
+ Cache<String, ValueHolder<String>> mem =
+ CacheBuilder.newBuilder()
+ .build(
+ new CacheLoader<String, ValueHolder<String>>() {
+ @Override
+ public ValueHolder<String> load(String s) throws Exception {
+ return new ValueHolder<>(s + "_loaded", Instant.now());
+ }
+ });
+
+ H2CacheImpl<String, String> impl =
+ newH2CacheImpl(newStore(nextDbId(), DEFAULT_VERSION, null, null), mem);
+
+ assertThat(impl.getAll(Arrays.asList("S1", "S2")))
+ .containsExactlyEntriesIn(ImmutableMap.of("S1", "S1_loaded", "S2", "S2_loaded"));
+
+ // Make sure the values were cached
+ assertWithMessage("in-memory value").that(impl.getIfPresent("S1")).isEqualTo("S1_loaded");
+ assertWithMessage("in-memory value").that(impl.getIfPresent("S2")).isEqualTo("S2_loaded");
+ }
+
+ @Test
+ public void getAll_WithLoadingCache_LoaderImplementingLoadAll() throws ExecutionException {
+ Cache<String, ValueHolder<String>> mem =
+ CacheBuilder.newBuilder()
+ .build(
+ new CacheLoader<String, ValueHolder<String>>() {
+ @Override
+ public ValueHolder<String> load(String s) throws Exception {
+ return new ValueHolder<>(s + "_loaded", Instant.now());
+ }
+
+ @Override
+ public Map<String, ValueHolder<String>> loadAll(Iterable<? extends String> keys)
+ throws Exception {
+ Map<String, ValueHolder<String>> result = new HashMap<>();
+ for (String k : keys) {
+ result.put(k, load(k));
+ }
+ return result;
+ }
+ });
+
+ H2CacheImpl<String, String> impl =
+ newH2CacheImpl(newStore(nextDbId(), DEFAULT_VERSION, null, null), mem);
+
+ assertThat(impl.getAll(Arrays.asList("S1", "S2")))
+ .containsExactlyEntriesIn(ImmutableMap.of("S1", "S1_loaded", "S2", "S2_loaded"));
+
+ // Make sure the values were cached
+ assertWithMessage("in-memory value").that(impl.getIfPresent("S1")).isEqualTo("S1_loaded");
+ assertWithMessage("in-memory value").that(impl.getIfPresent("S2")).isEqualTo("S2_loaded");
+ }
+
+ @Test
public void stringSerializer() {
String input = "foo";
byte[] serialized = StringCacheSerializer.INSTANCE.serialize(input);
diff --git a/plugins/commit-message-length-validator b/plugins/commit-message-length-validator
index 556e427..c38e0a9 160000
--- a/plugins/commit-message-length-validator
+++ b/plugins/commit-message-length-validator
@@ -1 +1 @@
-Subproject commit 556e427fd737744ce8a6a37b89fd427ae59bc8ea
+Subproject commit c38e0a9d36767092b20558b28eff7f546c6d754c
diff --git a/plugins/download-commands b/plugins/download-commands
index 774e915..c99bc84 160000
--- a/plugins/download-commands
+++ b/plugins/download-commands
@@ -1 +1 @@
-Subproject commit 774e9159128a72a76a0b226033b038c8f24fd88b
+Subproject commit c99bc8457910ec19315c1384e20267288b019592
diff --git a/polygerrit-ui/app/constants/reporting.ts b/polygerrit-ui/app/constants/reporting.ts
index d22026a..a09c1c3 100644
--- a/polygerrit-ui/app/constants/reporting.ts
+++ b/polygerrit-ui/app/constants/reporting.ts
@@ -30,6 +30,8 @@
PLUGIN_API = 'plugin-api',
REACHABLE_CODE = 'reachable code',
METHOD_USED = 'method used',
+ CHECKS_API_NOT_LOGGED_IN = 'checks-api not-logged-in',
+ CHECKS_API_ERROR = 'checks-api error',
}
export enum Timing {
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 c8a04ee..4304734 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
@@ -75,7 +75,12 @@
hasEditPatchsetLoaded,
PatchSet,
} from '../../../utils/patch-set-util';
-import {changeStatuses, isOwner, isReviewer} from '../../../utils/change-util';
+import {
+ changeStatuses,
+ isCc,
+ isOwner,
+ isReviewer,
+} from '../../../utils/change-util';
import {EventType as PluginEventType} from '../../../api/plugin';
import {customElement, observe, property} from '@polymer/decorators';
import {GrApplyFixDialog} from '../../diff/gr-apply-fix-dialog/gr-apply-fix-dialog';
@@ -2140,6 +2145,7 @@
this.reporting.changeDisplayed({
isOwner: isOwner(this._change, this._account),
isReviewer: isReviewer(this._change, this._account),
+ isCc: isCc(this._change, this._account),
});
}
});
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 939b23c..e8d11fe 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -1325,7 +1325,7 @@
_computePatchSetWarning(patchNum?: PatchSetNum, labelsChanged?: boolean) {
let str = `Patch ${patchNum} is not latest.`;
if (labelsChanged) {
- str += ' Voting will have no effect.';
+ str += ' Voting may have no effect.';
}
return str;
}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 2f62df9..2ea6dd0 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -613,12 +613,6 @@
}
}
-const SHOW_ALL_THRESHOLDS: Map<Category, number> = new Map();
-SHOW_ALL_THRESHOLDS.set(Category.ERROR, 20);
-SHOW_ALL_THRESHOLDS.set(Category.WARNING, 10);
-SHOW_ALL_THRESHOLDS.set(Category.INFO, 5);
-SHOW_ALL_THRESHOLDS.set(Category.SUCCESS, 5);
-
const CATEGORY_TOOLTIPS: Map<Category, string> = new Map();
CATEGORY_TOOLTIPS.set(Category.ERROR, 'Must be fixed and is blocking submit');
CATEGORY_TOOLTIPS.set(
@@ -1173,9 +1167,8 @@
const expandedClass = expanded ? 'expanded' : 'collapsed';
const icon = expanded ? 'gr-icons:expand-less' : 'gr-icons:expand-more';
const isShowAll = this.isShowAll.get(category) ?? false;
- const threshold = SHOW_ALL_THRESHOLDS.get(category) ?? 5;
const resultCount = filtered.length;
- const resultLimit = isShowAll ? 1000 : isScrollTarget ? 25 : threshold;
+ const resultLimit = isShowAll ? 1000 : 20;
const showAllButton = this.renderShowAllButton(
category,
isShowAll,
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.js b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.js
deleted file mode 100644
index 4cf15b4..0000000
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.js
+++ /dev/null
@@ -1,574 +0,0 @@
-/**
- * @license
- * Copyright (C) 2016 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.
- */
-
-import '../../../test/common-test-setup-karma.js';
-import './gr-error-manager.js';
-import {_testOnly_initGerritPluginApi} from '../../shared/gr-js-api-interface/gr-gerrit.js';
-import {constructServerErrorMsg, __testOnly_ErrorType} from './gr-error-manager.js';
-import {stubRestApi} from '../../../test/test-utils.js';
-import {appContext} from '../../../services/app-context.js';
-import {createPreferences} from '../../../test/test-data-generators.js';
-
-const basicFixture = fixtureFromElement('gr-error-manager');
-
-_testOnly_initGerritPluginApi();
-
-suite('gr-error-manager tests', () => {
- let element;
-
- suite('when authed', () => {
- let toastSpy;
- let openOverlaySpy;
- let fetchStub;
- let getLoggedInStub;
-
- setup(() => {
- fetchStub = sinon.stub(window, 'fetch')
- .returns(Promise.resolve({ok: true, status: 204}));
- getLoggedInStub = stubRestApi('getLoggedIn')
- .callsFake(() => appContext.authService.authCheck());
- stubRestApi('getPreferences').returns(Promise.resolve(
- createPreferences()));
- element = basicFixture.instantiate();
- element._authService.clearCache();
- toastSpy = sinon.spy(element, '_createToastAlert');
- openOverlaySpy = sinon.spy(element.$.noInteractionOverlay, 'open');
- });
-
- teardown(() => {
- toastSpy.getCalls().forEach(call => {
- call.returnValue.remove();
- });
- });
-
- test('does not show auth error on 403 by default', done => {
- const showAuthErrorStub = sinon.stub(element, '_showAuthErrorAlert');
- const responseText = Promise.resolve('server says no.');
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail:
- {response: {status: 403, text() { return responseText; }}},
- composed: true, bubbles: true,
- }));
- flush(() => {
- assert.isFalse(showAuthErrorStub.calledOnce);
- done();
- });
- });
-
- test('show auth required for 403 with auth error and not authed before',
- done => {
- const showAuthErrorStub = sinon.stub(
- element, '_showAuthErrorAlert'
- );
- const responseText = Promise.resolve('Authentication required\n');
- getLoggedInStub.returns(Promise.resolve(true));
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail:
- {response: {status: 403, text() { return responseText; }}},
- composed: true, bubbles: true,
- }));
- flush(() => {
- assert.isTrue(showAuthErrorStub.calledOnce);
- done();
- });
- });
-
- test('recheck auth for 403 with auth error if authed before', async () => {
- // Set status to AUTHED.
- appContext.authService.authCheck();
- const responseText = Promise.resolve('Authentication required\n');
- getLoggedInStub.returns(Promise.resolve(true));
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail:
- {response: {status: 403, text() { return responseText; }}},
- composed: true, bubbles: true,
- }));
- await flush();
- assert.isTrue(getLoggedInStub.calledOnce);
- });
-
- test('show logged in error', () => {
- const spy = sinon.spy(element, '_showAuthErrorAlert');
- element.dispatchEvent(
- new CustomEvent('show-auth-required', {
- composed: true, bubbles: true,
- }));
- assert.isTrue(spy.calledWithExactly(
- 'Log in is required to perform that action.', 'Log in.'));
- });
-
- test('show normal Error', done => {
- const showErrorSpy = sinon.spy(element, '_showErrorDialog');
- const textSpy = sinon.spy(() => Promise.resolve('ZOMG'));
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail: {response: {status: 500, text: textSpy}},
- composed: true, bubbles: true,
- }));
-
- assert.isTrue(textSpy.called);
- flush(() => {
- assert.isTrue(showErrorSpy.calledOnce);
- assert.isTrue(showErrorSpy.lastCall.calledWithExactly(
- 'Error 500: ZOMG'));
- done();
- });
- });
-
- test('constructServerErrorMsg', () => {
- const errorText = 'change conflicts';
- const status = 409;
- const statusText = 'Conflict';
- const url = '/my/test/url';
-
- assert.equal(constructServerErrorMsg({status}),
- 'Error 409');
- assert.equal(constructServerErrorMsg({status, url}),
- 'Error 409: \nEndpoint: /my/test/url');
- assert.equal(constructServerErrorMsg({status, statusText, url}),
- 'Error 409 (Conflict): \nEndpoint: /my/test/url');
- assert.equal(constructServerErrorMsg({
- status,
- statusText,
- errorText,
- url,
- }), 'Error 409 (Conflict): change conflicts' +
- '\nEndpoint: /my/test/url');
- assert.equal(constructServerErrorMsg({
- status,
- statusText,
- errorText,
- url,
- trace: 'xxxxx',
- }), 'Error 409 (Conflict): change conflicts' +
- '\nEndpoint: /my/test/url\nTrace Id: xxxxx');
- });
-
- test('extract trace id from headers if exists', done => {
- const textSpy = sinon.spy(
- () => Promise.resolve('500')
- );
- const headers = new Headers();
- headers.set('X-Gerrit-Trace', 'xxxx');
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail: {
- response: {
- headers,
- status: 500,
- text: textSpy,
- },
- },
- composed: true, bubbles: true,
- }));
- flush(() => {
- assert.equal(
- element.$.errorDialog.text,
- 'Error 500: 500\nTrace Id: xxxx'
- );
- done();
- });
- });
-
- test('suppress TOO_MANY_FILES error', done => {
- const showAlertStub = sinon.stub(element, '_showAlert');
- const textSpy = sinon.spy(
- () => Promise.resolve('too many files to find conflicts')
- );
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail: {response: {status: 500, text: textSpy}},
- composed: true, bubbles: true,
- }));
-
- assert.isTrue(textSpy.called);
- flush(() => {
- assert.isFalse(showAlertStub.called);
- done();
- });
- });
-
- test('show network error', done => {
- const showAlertStub = sinon.stub(element, '_showAlert');
- element.dispatchEvent(
- new CustomEvent('network-error', {
- detail: {error: new Error('ZOMG')},
- composed: true, bubbles: true,
- }));
- flush(() => {
- assert.isTrue(showAlertStub.calledOnce);
- assert.isTrue(showAlertStub.lastCall.calledWithExactly(
- 'Server unavailable'));
- done();
- });
- });
-
- test('_canOverride alerts', () => {
- assert.isFalse(element._canOverride(undefined,
- __testOnly_ErrorType.AUTH));
- assert.isFalse(element._canOverride(undefined,
- __testOnly_ErrorType.NETWORK));
- assert.isTrue(element._canOverride(undefined,
- __testOnly_ErrorType.GENERIC));
- assert.isTrue(element._canOverride(undefined, undefined));
-
- assert.isTrue(element._canOverride(__testOnly_ErrorType.NETWORK,
- undefined));
- assert.isTrue(element._canOverride(__testOnly_ErrorType.AUTH,
- undefined));
- assert.isFalse(element._canOverride(__testOnly_ErrorType.NETWORK,
- __testOnly_ErrorType.AUTH));
-
- assert.isTrue(element._canOverride(__testOnly_ErrorType.AUTH,
- __testOnly_ErrorType.NETWORK));
- });
-
- test('show auth refresh toast', async () => {
- // Set status to AUTHED.
- appContext.authService.authCheck();
- const refreshStub = stubRestApi(
- 'getAccount').callsFake(
- () => Promise.resolve({}));
- const windowOpen = sinon.stub(window, 'open');
- const responseText = Promise.resolve('Authentication required\n');
- // fake failed auth
- fetchStub.returns(Promise.resolve({status: 403}));
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail:
- {response: {status: 403, text() { return responseText; }}},
- composed: true, bubbles: true,
- }));
- assert.equal(fetchStub.callCount, 1);
- await flush();
-
- // here needs two flush as there are two chanined
- // promises on server-error handler and flush only flushes one
- assert.equal(fetchStub.callCount, 2);
- await flush();
- // Sometime overlay opens with delay, waiting while open is complete
- await openOverlaySpy.lastCall.returnValue;
- // auth-error fired
- assert.isTrue(toastSpy.called);
-
- // toast
- let toast = toastSpy.lastCall.returnValue;
- assert.isOk(toast);
- assert.include(
- toast.root.textContent, 'Credentials expired.');
- assert.include(
- toast.root.textContent, 'Refresh credentials');
-
- // noInteractionOverlay
- const noInteractionOverlay = element.$.noInteractionOverlay;
- assert.isOk(noInteractionOverlay);
- sinon.spy(noInteractionOverlay, 'close');
- assert.equal(
- noInteractionOverlay.backdropElement.getAttribute('opened'),
- '');
- assert.isFalse(windowOpen.called);
- MockInteractions.tap(toast.shadowRoot
- .querySelector('gr-button.action'));
- assert.isTrue(windowOpen.called);
-
- // @see Issue 5822: noopener breaks closeAfterLogin
- assert.equal(windowOpen.lastCall.args[2].indexOf('noopener=yes'),
- -1);
-
- const hideToastSpy = sinon.spy(toast, 'hide');
-
- // now fake authed
- fetchStub.returns(Promise.resolve({status: 204}));
- element.handleWindowFocus();
- element.checkLoggedInTask.flush();
- await flush();
- assert.isTrue(refreshStub.called);
- assert.isTrue(hideToastSpy.called);
-
- // toast update
- assert.notStrictEqual(toastSpy.lastCall.returnValue, toast);
- toast = toastSpy.lastCall.returnValue;
- assert.isOk(toast);
- assert.include(
- toast.root.textContent, 'Credentials refreshed');
-
- // close overlay
- assert.isTrue(noInteractionOverlay.close.called);
- });
-
- test('auth toast should dismiss existing toast', async () => {
- // Set status to AUTHED.
- appContext.authService.authCheck();
- const responseText = Promise.resolve('Authentication required\n');
-
- // fake an alert
- element.dispatchEvent(
- new CustomEvent('show-alert', {
- detail: {message: 'test reload', action: 'reload'},
- composed: true, bubbles: true,
- }));
- let toast = toastSpy.lastCall.returnValue;
- assert.isOk(toast);
- assert.include(
- toast.root.textContent, 'test reload');
-
- // fake auth
- fetchStub.returns(Promise.resolve({status: 403}));
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail:
- {response: {status: 403, text() { return responseText; }}},
- composed: true, bubbles: true,
- }));
- assert.equal(fetchStub.callCount, 1);
- await flush();
- // here needs two flush as there are two chained
- // promises on server-error handler and flush only flushes one
- assert.equal(fetchStub.callCount, 2);
- await flush();
- // Sometime overlay opens with delay, waiting while open is complete
- await openOverlaySpy.lastCall.returnValue;
- // toast
- toast = toastSpy.lastCall.returnValue;
- assert.include(
- toast.root.textContent, 'Credentials expired.');
- assert.include(
- toast.root.textContent, 'Refresh credentials');
- });
-
- test('regular toast should dismiss regular toast', () => {
- // Set status to AUTHED.
- appContext.authService.authCheck();
-
- // fake an alert
- element.dispatchEvent(
- new CustomEvent('show-alert', {
- detail: {message: 'test reload', action: 'reload'},
- composed: true, bubbles: true,
- }));
- let toast = toastSpy.lastCall.returnValue;
- assert.isOk(toast);
- assert.include(
- toast.root.textContent, 'test reload');
-
- // new alert
- element.dispatchEvent(
- new CustomEvent('show-alert', {
- detail: {message: 'second-test', action: 'reload'},
- composed: true, bubbles: true,
- }));
-
- toast = toastSpy.lastCall.returnValue;
- assert.include(toast.root.textContent, 'second-test');
- });
-
- test('regular toast should not dismiss auth toast', done => {
- // Set status to AUTHED.
- appContext.authService.authCheck();
- const responseText = Promise.resolve('Authentication required\n');
-
- // fake auth
- fetchStub.returns(Promise.resolve({status: 403}));
- element.dispatchEvent(
- new CustomEvent('server-error', {
- detail:
- {response: {status: 403, text() { return responseText; }}},
- composed: true, bubbles: true,
- }));
- assert.equal(fetchStub.callCount, 1);
- flush(() => {
- // here needs two flush as there are two chained
- // promises on server-error handler and flush only flushes one
- assert.equal(fetchStub.callCount, 2);
- flush(() => {
- let toast = toastSpy.lastCall.returnValue;
- assert.include(
- toast.root.textContent, 'Credentials expired.');
- assert.include(
- toast.root.textContent, 'Refresh credentials');
-
- // fake an alert
- element.dispatchEvent(
- new CustomEvent('show-alert', {
- detail: {
- message: 'test-alert', action: 'reload',
- },
- composed: true, bubbles: true,
- }));
- flush(() => {
- toast = toastSpy.lastCall.returnValue;
- assert.isOk(toast);
- assert.include(
- toast.root.textContent, 'Credentials expired.');
- done();
- });
- });
- });
- });
-
- test('show alert', () => {
- const alertObj = {message: 'foo'};
- sinon.stub(element, '_showAlert');
- element.dispatchEvent(
- new CustomEvent('show-alert', {
- detail: alertObj,
- composed: true, bubbles: true,
- }));
- assert.isTrue(element._showAlert.calledOnce);
- assert.equal(element._showAlert.lastCall.args[0], 'foo');
- assert.isNotOk(element._showAlert.lastCall.args[1]);
- assert.isNotOk(element._showAlert.lastCall.args[2]);
- });
-
- test('checks stale credentials on visibility change', () => {
- const refreshStub = sinon.stub(element,
- '_checkSignedIn');
- sinon.stub(Date, 'now').returns(999999);
- element._lastCredentialCheck = 0;
- element.handleVisibilityChange();
-
- // Since there is no known account, it should not test credentials.
- assert.isFalse(refreshStub.called);
- assert.equal(element._lastCredentialCheck, 0);
-
- element.knownAccountId = 123;
- element.handleVisibilityChange();
-
- // Should test credentials, since there is a known account.
- assert.isTrue(refreshStub.called);
- assert.equal(element._lastCredentialCheck, 999999);
- });
-
- test('refreshes with same credentials', done => {
- const accountPromise = Promise.resolve({_account_id: 1234});
- stubRestApi('getAccount')
- .returns(accountPromise);
- const requestCheckStub = sinon.stub(element, '_requestCheckLoggedIn');
- const handleRefreshStub = sinon.stub(element,
- 'handleCredentialRefreshed');
- const reloadStub = sinon.stub(element, '_reloadPage');
-
- element.knownAccountId = 1234;
- element._refreshingCredentials = true;
- element._checkSignedIn();
-
- flush(() => {
- assert.isFalse(requestCheckStub.called);
- assert.isTrue(handleRefreshStub.called);
- assert.isFalse(reloadStub.called);
- done();
- });
- });
-
- test('_showAlert hides existing alerts', () => {
- element._alertElement = element._createToastAlert();
- const hideStub = sinon.stub(element, 'hideAlert');
- element._showAlert();
- assert.isTrue(hideStub.calledOnce);
- });
-
- test('show-error', () => {
- const openStub = sinon.stub(element.$.errorOverlay, 'open');
- const closeStub = sinon.stub(element.$.errorOverlay, 'close');
- const reportStub = sinon.stub(
- element.reporting,
- 'reportErrorDialog'
- );
-
- const message = 'test message';
- element.dispatchEvent(
- new CustomEvent('show-error', {
- detail: {message},
- composed: true, bubbles: true,
- }));
- flush();
-
- assert.isTrue(openStub.called);
- assert.isTrue(reportStub.called);
- assert.equal(element.$.errorDialog.text, message);
-
- element.$.errorDialog.dispatchEvent(
- new CustomEvent('dismiss', {
- composed: true, bubbles: true,
- }));
- flush();
-
- assert.isTrue(closeStub.called);
- });
-
- test('reloads when refreshed credentials differ', done => {
- const accountPromise = Promise.resolve({_account_id: 1234});
- stubRestApi('getAccount')
- .returns(accountPromise);
- const requestCheckStub = sinon.stub(
- element,
- '_requestCheckLoggedIn');
- const handleRefreshStub = sinon.stub(element,
- 'handleCredentialRefreshed');
- const reloadStub = sinon.stub(element, '_reloadPage');
-
- element.knownAccountId = 4321; // Different from 1234
- element._refreshingCredentials = true;
- element._checkSignedIn();
-
- flush(() => {
- assert.isFalse(requestCheckStub.called);
- assert.isFalse(handleRefreshStub.called);
- assert.isTrue(reloadStub.called);
- done();
- });
- });
- });
-
- suite('when not authed', () => {
- let toastSpy;
- setup(() => {
- stubRestApi('getLoggedIn').returns(Promise.resolve(false));
- element = basicFixture.instantiate();
- toastSpy = sinon.spy(element, '_createToastAlert');
- });
-
- teardown(() => {
- toastSpy.getCalls().forEach(call => {
- call.returnValue.remove();
- });
- });
-
- test('refresh loop continues on credential fail', done => {
- const requestCheckStub = sinon.stub(
- element,
- '_requestCheckLoggedIn');
- const handleRefreshStub = sinon.stub(element,
- 'handleCredentialRefreshed');
- const reloadStub = sinon.stub(element, '_reloadPage');
-
- element._refreshingCredentials = true;
- element._checkSignedIn();
-
- flush(() => {
- assert.isTrue(requestCheckStub.called);
- assert.isFalse(handleRefreshStub.called);
- assert.isFalse(reloadStub.called);
- done();
- });
- });
- });
-});
-
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.ts b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.ts
new file mode 100644
index 0000000..aff5a85
--- /dev/null
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager_test.ts
@@ -0,0 +1,675 @@
+/**
+ * @license
+ * Copyright (C) 2016 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.
+ */
+
+import '../../../test/common-test-setup-karma';
+import './gr-error-manager';
+import {
+ constructServerErrorMsg,
+ GrErrorManager,
+ __testOnly_ErrorType,
+} from './gr-error-manager';
+import {stubAuth, stubReporting, stubRestApi} from '../../../test/test-utils';
+import {appContext} from '../../../services/app-context';
+import {
+ createAccountDetailWithId,
+ createPreferences,
+} from '../../../test/test-data-generators';
+import {tap} from '@polymer/iron-test-helpers/mock-interactions';
+import {AccountId} from '../../../types/common';
+
+const basicFixture = fixtureFromElement('gr-error-manager');
+
+suite('gr-error-manager tests', () => {
+ let element: GrErrorManager;
+
+ suite('when authed', () => {
+ let toastSpy: sinon.SinonSpy;
+ let fetchStub: sinon.SinonStub;
+ let getLoggedInStub: sinon.SinonStub;
+
+ setup(() => {
+ fetchStub = stubAuth('fetch').returns(
+ Promise.resolve({...new Response(), ok: true, status: 204})
+ );
+ getLoggedInStub = stubRestApi('getLoggedIn').callsFake(() =>
+ appContext.authService.authCheck()
+ );
+ stubRestApi('getPreferences').returns(
+ Promise.resolve(createPreferences())
+ );
+ element = basicFixture.instantiate();
+ appContext.authService.clearCache();
+ toastSpy = sinon.spy(element, '_createToastAlert');
+ });
+
+ teardown(() => {
+ toastSpy.getCalls().forEach(call => {
+ call.returnValue.remove();
+ });
+ });
+
+ test('does not show auth error on 403 by default', done => {
+ const showAuthErrorStub = sinon.stub(element, '_showAuthErrorAlert');
+ const responseText = Promise.resolve('server says no.');
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ status: 403,
+ text() {
+ return responseText;
+ },
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush(() => {
+ assert.isFalse(showAuthErrorStub.calledOnce);
+ done();
+ });
+ });
+
+ test('show auth required for 403 with auth error and not authed before', done => {
+ const showAuthErrorStub = sinon.stub(element, '_showAuthErrorAlert');
+ const responseText = Promise.resolve('Authentication required\n');
+ getLoggedInStub.returns(Promise.resolve(true));
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ status: 403,
+ text() {
+ return responseText;
+ },
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush(() => {
+ assert.isTrue(showAuthErrorStub.calledOnce);
+ done();
+ });
+ });
+
+ test('recheck auth for 403 with auth error if authed before', async () => {
+ // Set status to AUTHED.
+ appContext.authService.authCheck();
+ const responseText = Promise.resolve('Authentication required\n');
+ getLoggedInStub.returns(Promise.resolve(true));
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ status: 403,
+ text() {
+ return responseText;
+ },
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ await flush();
+ assert.isTrue(getLoggedInStub.calledOnce);
+ });
+
+ test('show logged in error', () => {
+ const spy = sinon.spy(element, '_showAuthErrorAlert');
+ element.dispatchEvent(
+ new CustomEvent('show-auth-required', {
+ composed: true,
+ bubbles: true,
+ })
+ );
+ assert.isTrue(
+ spy.calledWithExactly(
+ 'Log in is required to perform that action.',
+ 'Log in.'
+ )
+ );
+ });
+
+ test('show normal Error', done => {
+ const showErrorSpy = sinon.spy(element, '_showErrorDialog');
+ const textSpy = sinon.spy(() => Promise.resolve('ZOMG'));
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {response: {status: 500, text: textSpy}},
+ composed: true,
+ bubbles: true,
+ })
+ );
+
+ assert.isTrue(textSpy.called);
+ flush(() => {
+ assert.isTrue(showErrorSpy.calledOnce);
+ assert.isTrue(
+ showErrorSpy.lastCall.calledWithExactly('Error 500: ZOMG')
+ );
+ done();
+ });
+ });
+
+ test('constructServerErrorMsg', () => {
+ const errorText = 'change conflicts';
+ const status = 409;
+ const statusText = 'Conflict';
+ const url = '/my/test/url';
+
+ assert.equal(constructServerErrorMsg({status}), 'Error 409');
+ assert.equal(
+ constructServerErrorMsg({status, url}),
+ 'Error 409: \nEndpoint: /my/test/url'
+ );
+ assert.equal(
+ constructServerErrorMsg({status, statusText, url}),
+ 'Error 409 (Conflict): \nEndpoint: /my/test/url'
+ );
+ assert.equal(
+ constructServerErrorMsg({
+ status,
+ statusText,
+ errorText,
+ url,
+ }),
+ 'Error 409 (Conflict): change conflicts' + '\nEndpoint: /my/test/url'
+ );
+ assert.equal(
+ constructServerErrorMsg({
+ status,
+ statusText,
+ errorText,
+ url,
+ trace: 'xxxxx',
+ }),
+ 'Error 409 (Conflict): change conflicts' +
+ '\nEndpoint: /my/test/url\nTrace Id: xxxxx'
+ );
+ });
+
+ test('extract trace id from headers if exists', done => {
+ const textSpy = sinon.spy(() => Promise.resolve('500'));
+ const headers = new Headers();
+ headers.set('X-Gerrit-Trace', 'xxxx');
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ headers,
+ status: 500,
+ text: textSpy,
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush(() => {
+ assert.equal(
+ element.$.errorDialog.text,
+ 'Error 500: 500\nTrace Id: xxxx'
+ );
+ done();
+ });
+ });
+
+ test('suppress TOO_MANY_FILES error', done => {
+ const showAlertStub = sinon.stub(element, '_showAlert');
+ const textSpy = sinon.spy(() =>
+ Promise.resolve('too many files to find conflicts')
+ );
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {response: {status: 500, text: textSpy}},
+ composed: true,
+ bubbles: true,
+ })
+ );
+
+ assert.isTrue(textSpy.called);
+ flush(() => {
+ assert.isFalse(showAlertStub.called);
+ done();
+ });
+ });
+
+ test('show network error', done => {
+ const showAlertStub = sinon.stub(element, '_showAlert');
+ element.dispatchEvent(
+ new CustomEvent('network-error', {
+ detail: {error: new Error('ZOMG')},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush(() => {
+ assert.isTrue(showAlertStub.calledOnce);
+ assert.isTrue(
+ showAlertStub.lastCall.calledWithExactly('Server unavailable')
+ );
+ done();
+ });
+ });
+
+ test('_canOverride alerts', () => {
+ assert.isFalse(
+ element._canOverride(undefined, __testOnly_ErrorType.AUTH)
+ );
+ assert.isFalse(
+ element._canOverride(undefined, __testOnly_ErrorType.NETWORK)
+ );
+ assert.isTrue(
+ element._canOverride(undefined, __testOnly_ErrorType.GENERIC)
+ );
+ assert.isTrue(element._canOverride(undefined, undefined));
+
+ assert.isTrue(
+ element._canOverride(__testOnly_ErrorType.NETWORK, undefined)
+ );
+ assert.isTrue(element._canOverride(__testOnly_ErrorType.AUTH, undefined));
+ assert.isFalse(
+ element._canOverride(
+ __testOnly_ErrorType.NETWORK,
+ __testOnly_ErrorType.AUTH
+ )
+ );
+
+ assert.isTrue(
+ element._canOverride(
+ __testOnly_ErrorType.AUTH,
+ __testOnly_ErrorType.NETWORK
+ )
+ );
+ });
+
+ test('show auth refresh toast', async () => {
+ const clock = sinon.useFakeTimers();
+
+ // Set status to AUTHED.
+ appContext.authService.authCheck();
+ const refreshStub = stubRestApi('getAccount').callsFake(() =>
+ Promise.resolve(createAccountDetailWithId())
+ );
+ const windowOpen = sinon.stub(window, 'open');
+ const responseText = Promise.resolve('Authentication required\n');
+ // fake failed auth
+ fetchStub.returns(Promise.resolve({...new Response(), status: 403}));
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ status: 403,
+ text() {
+ return responseText;
+ },
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ assert.equal(fetchStub.callCount, 1);
+ await flush();
+
+ // here needs two flush as there are two chanined
+ // promises on server-error handler and flush only flushes one
+ assert.equal(fetchStub.callCount, 2);
+ await flush();
+ // Sometime overlay opens with delay, waiting while open is complete
+ clock.tick(1000);
+ await flush();
+ // auth-error fired
+ assert.isTrue(toastSpy.called);
+
+ // toast
+ let toast = toastSpy.lastCall.returnValue;
+ assert.isOk(toast);
+ assert.include(toast.root.textContent, 'Credentials expired.');
+ assert.include(toast.root.textContent, 'Refresh credentials');
+
+ // noInteractionOverlay
+ const noInteractionOverlay = element.$.noInteractionOverlay;
+ assert.isOk(noInteractionOverlay);
+ const noInteractionOverlayCloseSpy = sinon.spy(
+ noInteractionOverlay,
+ 'close'
+ );
+ assert.equal(
+ noInteractionOverlay.backdropElement.getAttribute('opened'),
+ ''
+ );
+ assert.isFalse(windowOpen.called);
+ tap(toast.shadowRoot.querySelector('gr-button.action'));
+ assert.isTrue(windowOpen.called);
+
+ // @see Issue 5822: noopener breaks closeAfterLogin
+ assert.equal(windowOpen.lastCall.args[2]?.indexOf('noopener=yes'), -1);
+
+ const hideToastSpy = sinon.spy(toast, 'hide');
+
+ // now fake authed
+ fetchStub.returns(Promise.resolve({status: 204}));
+
+ clock.tick(1000);
+ element.knownAccountId = 5 as AccountId;
+ element._checkSignedIn();
+ await flush();
+
+ assert.isTrue(refreshStub.called);
+ assert.isTrue(hideToastSpy.called);
+
+ // toast update
+ assert.notStrictEqual(toastSpy.lastCall.returnValue, toast);
+ toast = toastSpy.lastCall.returnValue;
+ assert.isOk(toast);
+ assert.include(toast.root.textContent, 'Credentials refreshed');
+
+ // close overlay
+ assert.isTrue(noInteractionOverlayCloseSpy.called);
+ });
+
+ test('auth toast should dismiss existing toast', async () => {
+ const clock = sinon.useFakeTimers();
+ // Set status to AUTHED.
+ appContext.authService.authCheck();
+ const responseText = Promise.resolve('Authentication required\n');
+
+ // fake an alert
+ element.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: {message: 'test reload', action: 'reload'},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ let toast = toastSpy.lastCall.returnValue;
+ assert.isOk(toast);
+ assert.include(toast.root.textContent, 'test reload');
+
+ // fake auth
+ fetchStub.returns(Promise.resolve({status: 403}));
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ status: 403,
+ text() {
+ return responseText;
+ },
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ await flush();
+ await flush();
+ // here needs two flush as there are two chained
+ // promises on server-error handler and flush only flushes one
+ assert.equal(fetchStub.callCount, 2);
+ await flush();
+ // Sometime overlay opens with delay, waiting while open is complete
+ clock.tick(1000);
+ await flush();
+ // toast
+ toast = toastSpy.lastCall.returnValue;
+ assert.include(toast.root.textContent, 'Credentials expired.');
+ assert.include(toast.root.textContent, 'Refresh credentials');
+ });
+
+ test('regular toast should dismiss regular toast', () => {
+ // Set status to AUTHED.
+ appContext.authService.authCheck();
+
+ // fake an alert
+ element.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: {message: 'test reload', action: 'reload'},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ let toast = toastSpy.lastCall.returnValue;
+ assert.isOk(toast);
+ assert.include(toast.root.textContent, 'test reload');
+
+ // new alert
+ element.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: {message: 'second-test', action: 'reload'},
+ composed: true,
+ bubbles: true,
+ })
+ );
+
+ toast = toastSpy.lastCall.returnValue;
+ assert.include(toast.root.textContent, 'second-test');
+ });
+
+ test('regular toast should not dismiss auth toast', done => {
+ // Set status to AUTHED.
+ appContext.authService.authCheck();
+ const responseText = Promise.resolve('Authentication required\n');
+
+ // fake auth
+ fetchStub.returns(Promise.resolve({status: 403}));
+ element.dispatchEvent(
+ new CustomEvent('server-error', {
+ detail: {
+ response: {
+ status: 403,
+ text() {
+ return responseText;
+ },
+ },
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ assert.equal(fetchStub.callCount, 1);
+ flush(() => {
+ // here needs two flush as there are two chained
+ // promises on server-error handler and flush only flushes one
+ assert.equal(fetchStub.callCount, 2);
+ flush(() => {
+ let toast = toastSpy.lastCall.returnValue;
+ assert.include(toast.root.textContent, 'Credentials expired.');
+ assert.include(toast.root.textContent, 'Refresh credentials');
+
+ // fake an alert
+ element.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: {
+ message: 'test-alert',
+ action: 'reload',
+ },
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush(() => {
+ toast = toastSpy.lastCall.returnValue;
+ assert.isOk(toast);
+ assert.include(toast.root.textContent, 'Credentials expired.');
+ done();
+ });
+ });
+ });
+ });
+
+ test('show alert', () => {
+ const alertObj = {message: 'foo'};
+ const showAlertStub = sinon.stub(element, '_showAlert');
+ element.dispatchEvent(
+ new CustomEvent('show-alert', {
+ detail: alertObj,
+ composed: true,
+ bubbles: true,
+ })
+ );
+ assert.isTrue(showAlertStub.calledOnce);
+ assert.equal(showAlertStub.lastCall.args[0], 'foo');
+ assert.isNotOk(showAlertStub.lastCall.args[1]);
+ assert.isNotOk(showAlertStub.lastCall.args[2]);
+ });
+
+ test('checks stale credentials on visibility change', () => {
+ const refreshStub = sinon.stub(element, '_checkSignedIn');
+ sinon.stub(Date, 'now').returns(999999);
+ element._lastCredentialCheck = 0;
+
+ document.dispatchEvent(new CustomEvent('visibilitychange'));
+
+ // Since there is no known account, it should not test credentials.
+ assert.isFalse(refreshStub.called);
+ assert.equal(element._lastCredentialCheck, 0);
+
+ element.knownAccountId = 123 as AccountId;
+
+ document.dispatchEvent(new CustomEvent('visibilitychange'));
+
+ // Should test credentials, since there is a known account.
+ assert.isTrue(refreshStub.called);
+ assert.equal(element._lastCredentialCheck, 999999);
+ });
+
+ test('refreshes with same credentials', done => {
+ const accountPromise = Promise.resolve({
+ ...createAccountDetailWithId(1234),
+ });
+ stubRestApi('getAccount').returns(accountPromise);
+ const requestCheckStub = sinon.stub(element, '_requestCheckLoggedIn');
+ const handleRefreshStub = sinon.stub(
+ element,
+ 'handleCredentialRefreshed'
+ );
+ const reloadStub = sinon.stub(element, '_reloadPage');
+
+ element.knownAccountId = 1234 as AccountId;
+ element._refreshingCredentials = true;
+ element._checkSignedIn();
+
+ flush(() => {
+ assert.isFalse(requestCheckStub.called);
+ assert.isTrue(handleRefreshStub.called);
+ assert.isFalse(reloadStub.called);
+ done();
+ });
+ });
+
+ test('_showAlert hides existing alerts', () => {
+ element._alertElement = element._createToastAlert();
+ // const hideStub = sinon.stub(element, 'hideAlert');
+ // element._showAlert('');
+ // assert.isTrue(hideStub.calledOnce);
+ });
+
+ test('show-error', () => {
+ const openStub = sinon.stub(element.$.errorOverlay, 'open');
+ const closeStub = sinon.stub(element.$.errorOverlay, 'close');
+ const reportStub = stubReporting('reportErrorDialog');
+
+ const message = 'test message';
+ element.dispatchEvent(
+ new CustomEvent('show-error', {
+ detail: {message},
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush();
+
+ assert.isTrue(openStub.called);
+ assert.isTrue(reportStub.called);
+ assert.equal(element.$.errorDialog.text, message);
+
+ element.$.errorDialog.dispatchEvent(
+ new CustomEvent('dismiss', {
+ composed: true,
+ bubbles: true,
+ })
+ );
+ flush();
+
+ assert.isTrue(closeStub.called);
+ });
+
+ test('reloads when refreshed credentials differ', done => {
+ const accountPromise = Promise.resolve({
+ ...createAccountDetailWithId(1234),
+ });
+ stubRestApi('getAccount').returns(accountPromise);
+ const requestCheckStub = sinon.stub(element, '_requestCheckLoggedIn');
+ const handleRefreshStub = sinon.stub(
+ element,
+ 'handleCredentialRefreshed'
+ );
+ const reloadStub = sinon.stub(element, '_reloadPage');
+
+ element.knownAccountId = 4321 as AccountId; // Different from 1234
+ element._refreshingCredentials = true;
+ element._checkSignedIn();
+
+ flush(() => {
+ assert.isFalse(requestCheckStub.called);
+ assert.isFalse(handleRefreshStub.called);
+ assert.isTrue(reloadStub.called);
+ done();
+ });
+ });
+ });
+
+ suite('when not authed', () => {
+ let toastSpy: sinon.SinonSpy;
+ setup(() => {
+ stubRestApi('getLoggedIn').returns(Promise.resolve(false));
+ element = basicFixture.instantiate();
+ toastSpy = sinon.spy(element, '_createToastAlert');
+ });
+
+ teardown(() => {
+ toastSpy.getCalls().forEach(call => {
+ call.returnValue.remove();
+ });
+ });
+
+ test('refresh loop continues on credential fail', done => {
+ const requestCheckStub = sinon.stub(element, '_requestCheckLoggedIn');
+ const handleRefreshStub = sinon.stub(
+ element,
+ 'handleCredentialRefreshed'
+ );
+ const reloadStub = sinon.stub(element, '_reloadPage');
+
+ element._refreshingCredentials = true;
+ element._checkSignedIn();
+
+ flush(() => {
+ assert.isTrue(requestCheckStub.called);
+ assert.isFalse(handleRefreshStub.called);
+ assert.isFalse(reloadStub.called);
+ done();
+ });
+ });
+ });
+});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-image-viewer/gr-image-viewer.ts b/polygerrit-ui/app/elements/diff/gr-diff-image-viewer/gr-image-viewer.ts
index dad855c..466b1f4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-image-viewer/gr-image-viewer.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-image-viewer/gr-image-viewer.ts
@@ -252,6 +252,8 @@
display: flex;
align-items: center;
margin: var(--spacing-xl);
+ /* Start a stacking context to contain FAB below. */
+ z-index: 0;
}
#version-switcher paper-button {
flex-grow: 1;
@@ -264,7 +266,7 @@
}
#version-switcher paper-fab {
/* Round button overlaps Base and Revision buttons. */
- z-index: 10;
+ z-index: 1;
margin: 0 -12px;
/* Styled as an outlined button. */
color: var(--primary-button-background-color);
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
index 7cbcaef..1aee75a 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
@@ -16,7 +16,7 @@
*/
import '../../../test/common-test-setup-karma.js';
-import {addListenerForTest, mockPromise} from '../../../test/test-utils.js';
+import {addListenerForTest, mockPromise, stubAuth} from '../../../test/test-utils.js';
import {GrReviewerUpdatesParser} from './gr-reviewer-updates-parser.js';
import {ListChangesOption} from '../../../utils/change-util.js';
import {appContext} from '../../../services/app-context.js';
@@ -264,7 +264,7 @@
test('server error', () => {
const getResponseObjectStub = sinon.stub(element, 'getResponseObject');
- window.fetch.returns(Promise.resolve({ok: false}));
+ stubAuth('fetch').returns(Promise.resolve({ok: false}));
const serverErrorEventPromise = new Promise(resolve => {
addListenerForTest(document, 'server-error', resolve);
});
@@ -832,7 +832,7 @@
});
test('gerrit auth is used', () => {
- sinon.stub(appContext.authService, 'fetch').returns(Promise.resolve());
+ stubAuth('fetch').returns(Promise.resolve());
element._restApiHelper.fetchJSON({url: 'foo'});
assert(appContext.authService.fetch.called);
});
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.js
index 3a5a587..70bd369 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper_test.js
@@ -18,6 +18,7 @@
import '../../../../test/common-test-setup-karma.js';
import {SiteBasedCache, FetchPromisesCache, GrRestApiHelper} from './gr-rest-api-helper.js';
import {appContext} from '../../../../services/app-context.js';
+import {stubAuth} from '../../../../test/test-utils.js';
suite('gr-rest-api-helper tests', () => {
let helper;
@@ -25,6 +26,7 @@
let cache;
let fetchPromisesCache;
let originalCanonicalPath;
+ let authFetchStub;
setup(() => {
cache = new SiteBasedCache();
@@ -38,7 +40,7 @@
};
const testJSON = ')]}\'\n{"hello": "bonjour"}';
- sinon.stub(window, 'fetch').returns(Promise.resolve({
+ authFetchStub = stubAuth('fetch').returns(Promise.resolve({
ok: true,
text() {
return Promise.resolve(testJSON);
@@ -55,8 +57,6 @@
suite('fetchJSON()', () => {
test('Sets header to accept application/json', () => {
- const authFetchStub = sinon.stub(helper._auth, 'fetch')
- .returns(Promise.resolve());
helper.fetchJSON({url: '/dummy/url'});
assert.isTrue(authFetchStub.called);
assert.equal(authFetchStub.lastCall.args[1].headers.get('Accept'),
@@ -64,8 +64,6 @@
});
test('Use header option accept when provided', () => {
- const authFetchStub = sinon.stub(helper._auth, 'fetch')
- .returns(Promise.resolve());
const headers = new Headers();
headers.append('Accept', '*/*');
const fetchOptions = {headers};
@@ -142,7 +140,7 @@
test('request callbacks can be canceled', () => {
let cancelCalled = false;
- window.fetch.returns(Promise.resolve({
+ authFetchStub.returns(Promise.resolve({
body: {
cancel() { cancelCalled = true; },
},
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index d683514..bc05484 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -57,6 +57,7 @@
import {assertIsDefined} from '../../utils/common-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
import {routerPatchNum$} from '../router/router-model';
+import {Execution} from '../../constants/reporting';
export class ChecksService {
private readonly providers: {[name: string]: ChecksProvider} = {};
@@ -173,17 +174,24 @@
// the Observable has terminated and we won't recover from that. No
// further attempts to fetch results for this plugin will be made.
this.reporting.error(e, `checks-service crash for ${pluginName}`);
- return of(this.createErrorResponse(pluginName, `${e}`));
+ return of(this.createErrorResponse(pluginName, e));
})
)
.subscribe(response => {
switch (response.responseCode) {
case ResponseCode.ERROR:
assertIsDefined(response.errorMessage, 'errorMessage');
+ this.reporting.reportExecution(Execution.CHECKS_API_ERROR, {
+ plugin: pluginName,
+ message: response.errorMessage,
+ });
updateStateSetError(pluginName, response.errorMessage, patchset);
break;
case ResponseCode.NOT_LOGGED_IN:
assertIsDefined(response.loginCallback, 'loginCallback');
+ this.reporting.reportExecution(Execution.CHECKS_API_NOT_LOGGED_IN, {
+ plugin: pluginName,
+ });
updateStateSetNotLoggedIn(
pluginName,
response.loginCallback,
@@ -212,11 +220,13 @@
private createErrorResponse(
pluginName: string,
- message: string
+ message: object
): FetchResponse {
return {
responseCode: ResponseCode.ERROR,
- errorMessage: `Error message from plugin '${pluginName}': ${message}`,
+ errorMessage:
+ `Error message from plugin '${pluginName}':` +
+ ` ${JSON.stringify(message)}`,
};
}
@@ -234,7 +244,7 @@
return response;
});
return from(fetchPromise).pipe(
- catchError(e => of(this.createErrorResponse(pluginName, `${e}`)))
+ catchError(e => of(this.createErrorResponse(pluginName, e)))
);
}
}
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts b/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
index 6fadfde..08f2e25 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
@@ -26,7 +26,7 @@
Token,
} from './gr-auth';
-const MAX_AUTH_CHECK_WAIT_TIME_MS = 1000 * 30; // 30s
+export const MAX_AUTH_CHECK_WAIT_TIME_MS = 1000 * 30; // 30s
const MAX_GET_TOKEN_RETRIES = 2;
interface ValidToken extends Token {
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts b/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts
new file mode 100644
index 0000000..3dbb4c3
--- /dev/null
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts
@@ -0,0 +1,77 @@
+/**
+ * @license
+ * Copyright (C) 2020 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.
+ */
+
+import {EventEmitterService} from '../gr-event-interface/gr-event-interface';
+import {
+ AuthRequestInit,
+ AuthService,
+ AuthStatus,
+ DefaultAuthOptions,
+ GetTokenCallback,
+} from './gr-auth';
+import {Auth} from './gr-auth_impl';
+
+export class GrAuthMock implements AuthService {
+ baseUrl = '';
+
+ private _status = AuthStatus.UNDETERMINED;
+
+ public eventEmitter: EventEmitterService;
+
+ constructor(eventEmitter: EventEmitterService) {
+ this.eventEmitter = eventEmitter;
+ }
+
+ get isAuthed() {
+ return this._status === Auth.STATUS.AUTHED;
+ }
+
+ private _setStatus(status: AuthStatus) {
+ if (this._status === status) return;
+ if (this._status === AuthStatus.AUTHED) {
+ this.eventEmitter.emit('auth-error', {
+ message: Auth.CREDS_EXPIRED_MSG,
+ action: 'Refresh credentials',
+ });
+ }
+ this._status = status;
+ }
+
+ get status() {
+ return this._status;
+ }
+
+ authCheck() {
+ return this.fetch(`${this.baseUrl}/auth-check`).then(res => {
+ if (res.status === 204) {
+ this._setStatus(Auth.STATUS.AUTHED);
+ return true;
+ } else {
+ this._setStatus(Auth.STATUS.NOT_AUTHED);
+ return false;
+ }
+ });
+ }
+
+ clearCache() {}
+
+ setup(_getToken: GetTokenCallback, _defaultOptions: DefaultAuthOptions) {}
+
+ fetch(_url: string, _opt_options?: AuthRequestInit): Promise<Response> {
+ return Promise.resolve(new Response());
+ }
+}
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth_test.js b/polygerrit-ui/app/services/gr-auth/gr-auth_test.js
index 80938ad..ac93a7d 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth_test.js
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth_test.js
@@ -24,7 +24,7 @@
let auth;
setup(() => {
- auth = appContext.authService;
+ auth = new Auth(appContext.eventEmitter);
});
suite('Auth class methods', () => {
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index a3df94c..30989d6 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -266,7 +266,7 @@
return Promise.resolve(createServerInfo());
},
getDashboard(): Promise<DashboardInfo | undefined> {
- throw new Error('getDashboard() not implemented by RestApiMock.');
+ return Promise.resolve(undefined);
},
getDefaultPreferences(): Promise<PreferencesInfo | undefined> {
throw new Error('getDefaultPreferences() not implemented by RestApiMock.');
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index d74a9c1..483baa6 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -21,6 +21,7 @@
import {AppContext, appContext} from '../services/app-context';
import {grRestApiMock} from './mocks/gr-rest-api_mock';
import {grStorageMock} from '../services/storage/gr-storage_mock';
+import {GrAuthMock} from '../services/gr-auth/gr-auth_mock';
export function _testOnlyInitAppContext() {
initAppContext();
@@ -38,4 +39,5 @@
setMock('reportingService', grReportingMock);
setMock('restApiService', grRestApiMock);
setMock('storageService', grStorageMock);
+ setMock('authService', new GrAuthMock(appContext.eventEmitter));
}
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index 34f7fe4..4d8e8ae 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -25,6 +25,8 @@
import {RestApiService} from '../services/gr-rest-api/gr-rest-api';
import {SinonSpy} from 'sinon/pkg/sinon-esm';
import {StorageService} from '../services/storage/gr-storage';
+import {AuthService} from '../services/gr-auth/gr-auth';
+import {ReportingService} from '../services/gr-reporting/gr-reporting';
export interface MockPromise extends Promise<unknown> {
resolve: (value?: unknown) => void;
@@ -179,6 +181,14 @@
return sinon.spy(appContext.storageService, method);
}
+export function stubAuth<K extends keyof AuthService>(method: K) {
+ return sinon.stub(appContext.authService, method);
+}
+
+export function stubReporting<K extends keyof ReportingService>(method: K) {
+ return sinon.stub(appContext.reportingService, method);
+}
+
export type SinonSpyMember<F extends (...args: any) => any> = SinonSpy<
Parameters<F>,
ReturnType<F>
diff --git a/polygerrit-ui/app/utils/change-util.ts b/polygerrit-ui/app/utils/change-util.ts
index 380d06c..3dac8d3 100644
--- a/polygerrit-ui/app/utils/change-util.ts
+++ b/polygerrit-ui/app/utils/change-util.ts
@@ -187,11 +187,15 @@
account?: AccountInfo
): boolean {
if (!change || !account) return false;
+ if (isOwner(change, account)) return false;
const reviewers = change.reviewers.REVIEWER ?? [];
return reviewers.some(r => r._account_id === account._account_id);
}
-export function isCc(change?: ChangeInfo, account?: AccountInfo): boolean {
+export function isCc(
+ change?: ChangeInfo | ParsedChangeInfo,
+ account?: AccountInfo
+): boolean {
if (!change || !account) return false;
const ccs = change.reviewers.CC ?? [];
return ccs.some(r => r._account_id === account._account_id);
