Merge "Use optional chaining in gr-change-metadata"
diff --git a/Documentation/config-accounts.txt b/Documentation/config-accounts.txt
index b4a5cef..c6d9fb4 100644
--- a/Documentation/config-accounts.txt
+++ b/Documentation/config-accounts.txt
@@ -343,6 +343,12 @@
The `accountId` field is mandatory. The `email` and `password` fields
are optional.
+Note that git will automatically nest these notes at varying levels. If
+refs/meta/external-ids:7c/2a55657d911109dbc930836e7a770fb946e8ef is not
+found then check
+refs/meta/external-ids:7c/2a/55657d911109dbc930836e7a770fb946e8ef and
+so on.
+
The external IDs are maintained by Gerrit. This means users are not
allowed to manually edit their external IDs. Only users with the
link:access-control.html#capability_accessDatabase[Access Database]
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index b2e1589..1db27d5 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -2729,8 +2729,8 @@
[source, java]
----
import java.util.Optional;
-import com.google.gerrit.common.data.SubmitRecord;
-import com.google.gerrit.common.data.SubmitRecord.Status;
+import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.entities.SubmitRecord.Status;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.SubmitRule;
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index d34ccb4..6889de3 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -4196,7 +4196,9 @@
repository.<name>.defaultSubmitType] is set to a different value.
|`branches` |optional|
A list of branches that should be initially created. +
-For the branch names the `refs/heads/` prefix can be omitted.
+For the branch names the `refs/heads/` prefix can be omitted. +
+The first entry of the list will be the default branch (ie. the target +
+of the `HEAD` symbolic ref).
|`owners` |optional|
A list of groups that should be assigned as project owner. +
Each group in the list must be specified as
diff --git a/java/com/google/gerrit/acceptance/AbstractDynamicOptionsTest.java b/java/com/google/gerrit/acceptance/AbstractDynamicOptionsTest.java
new file mode 100644
index 0000000..a4ed80a
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/AbstractDynamicOptionsTest.java
@@ -0,0 +1,117 @@
+// 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.
+
+package com.google.gerrit.acceptance;
+
+import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
+
+import com.google.common.collect.Lists;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.json.OutputFormat;
+import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.sshd.CommandMetaData;
+import com.google.gerrit.sshd.CommandModule;
+import com.google.gerrit.sshd.SshCommand;
+import com.google.gson.reflect.TypeToken;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import java.io.BufferedWriter;
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
+import java.util.Collections;
+import java.util.List;
+
+public class AbstractDynamicOptionsTest extends AbstractDaemonTest {
+ protected static final String LS_SAMPLES = "ls-samples";
+
+ protected interface Bean {
+ void setSamples(List<String> samples);
+ }
+
+ protected static class ListSamples implements Bean, DynamicOptions.BeanReceiver {
+ protected List<String> samples = Collections.emptyList();
+
+ @Override
+ public void setSamples(List<String> samples) {
+ this.samples = samples;
+ }
+
+ public void display(OutputStream displayOutputStream) throws Exception {
+ PrintWriter stdout =
+ new PrintWriter(new BufferedWriter(new OutputStreamWriter(displayOutputStream, "UTF-8")));
+ try {
+ OutputFormat.JSON
+ .newGson()
+ .toJson(samples, new TypeToken<List<String>>() {}.getType(), stdout);
+ stdout.print('\n');
+ } finally {
+ stdout.flush();
+ }
+ }
+
+ @Override
+ public void setDynamicBean(String plugin, DynamicOptions.DynamicBean dynamicBean) {}
+ }
+
+ @CommandMetaData(name = LS_SAMPLES, runsAt = MASTER_OR_SLAVE)
+ protected static class ListSamplesCommand extends SshCommand {
+ @Inject private ListSamples impl;
+
+ @Override
+ protected void run() throws Exception {
+ impl.display(out);
+ }
+
+ @Override
+ protected void parseCommandLine(DynamicOptions pluginOptions) throws UnloggedFailure {
+ parseCommandLine(impl, pluginOptions);
+ }
+ }
+
+ public static class PluginOneSshModule extends CommandModule {
+ @Override
+ public void configure() {
+ command(LS_SAMPLES).to(ListSamplesCommand.class);
+ }
+ }
+
+ protected static class ListSamplesOptions implements DynamicOptions.BeanParseListener {
+ @Override
+ public void onBeanParseStart(String plugin, Object bean) {
+ ((Bean) bean).setSamples(Lists.newArrayList("sample1", "sample2"));
+ }
+
+ @Override
+ public void onBeanParseEnd(String plugin, Object bean) {}
+ }
+
+ protected static class PluginTwoModule extends AbstractModule {
+ @Override
+ public void configure() {
+ bind(DynamicOptions.DynamicBean.class)
+ .annotatedWith(
+ Exports.named("com.google.gerrit.acceptance.AbstractDynamicOptionsTest.ListSamples"))
+ .to(ListSamplesOptionsClassNameProvider.class);
+ }
+ }
+
+ protected static class ListSamplesOptionsClassNameProvider
+ implements DynamicOptions.ClassNameProvider {
+ @Override
+ public String getClassName() {
+ return "com.google.gerrit.acceptance.AbstractDynamicOptionsTest$ListSamplesOptions";
+ }
+ }
+}
diff --git a/java/com/google/gerrit/entities/CoreDownloadSchemes.java b/java/com/google/gerrit/entities/CoreDownloadSchemes.java
index 37c10f1..9bcd365 100644
--- a/java/com/google/gerrit/entities/CoreDownloadSchemes.java
+++ b/java/com/google/gerrit/entities/CoreDownloadSchemes.java
@@ -21,6 +21,7 @@
public static final String HTTP = "http";
public static final String SSH = "ssh";
public static final String REPO_DOWNLOAD = "repo";
+ public static final String REPO = "repo";
private CoreDownloadSchemes() {}
}
diff --git a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
index 681d0bd..4cb52b7 100644
--- a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
+++ b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
@@ -27,7 +27,6 @@
/** Preferred method to download a change. */
public enum DownloadCommand {
- REPO_DOWNLOAD,
PULL,
CHECKOUT,
CHERRY_PICK,
diff --git a/java/com/google/gerrit/httpd/restapi/ParameterParser.java b/java/com/google/gerrit/httpd/restapi/ParameterParser.java
index 95d99f0..326cab8 100644
--- a/java/com/google/gerrit/httpd/restapi/ParameterParser.java
+++ b/java/com/google/gerrit/httpd/restapi/ParameterParser.java
@@ -161,6 +161,8 @@
HttpServletResponse res)
throws IOException {
CmdLineParser clp = parserFactory.create(param);
+ pluginOptions.setBean(param);
+ pluginOptions.startLifecycleListeners();
pluginOptions.parseDynamicBeans(clp);
pluginOptions.setDynamicBeans();
pluginOptions.onBeanParseStart();
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 4d55b36..0e525ce 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -507,7 +507,7 @@
}
try (DynamicOptions pluginOptions =
- new DynamicOptions(viewData.view, globals.injector, globals.dynamicBeans)) {
+ new DynamicOptions(globals.injector, globals.dynamicBeans)) {
if (!globals
.paramParser
.get()
diff --git a/java/com/google/gerrit/server/DynamicOptions.java b/java/com/google/gerrit/server/DynamicOptions.java
index 1d36ff0..db0aa70 100644
--- a/java/com/google/gerrit/server/DynamicOptions.java
+++ b/java/com/google/gerrit/server/DynamicOptions.java
@@ -193,6 +193,7 @@
protected Object bean;
protected Map<String, DynamicBean> beansByPlugin;
protected Injector injector;
+ protected DynamicMap<DynamicBean> dynamicBeans;
protected LifecycleManager lifecycleManager;
/**
@@ -200,7 +201,9 @@
* this class so the following methods can be called if desired:
*
* <pre>
- * DynamicOptions pluginOptions = new DynamicOptions(bean, injector, dynamicBeans);
+ * DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans);
+ * pluginOptions.setBean(bean);
+ * pluginOptions.startLifecycleListeners();
* pluginOptions.parseDynamicBeans(clp);
* pluginOptions.setDynamicBeans();
* pluginOptions.onBeanParseStart();
@@ -210,11 +213,15 @@
* pluginOptions.onBeanParseEnd();
* </pre>
*/
- public DynamicOptions(Object bean, Injector injector, DynamicMap<DynamicBean> dynamicBeans) {
- this.bean = bean;
+ public DynamicOptions(Injector injector, DynamicMap<DynamicBean> dynamicBeans) {
this.injector = injector;
+ this.dynamicBeans = dynamicBeans;
lifecycleManager = new LifecycleManager();
beansByPlugin = new HashMap<>();
+ }
+
+ public void setBean(Object bean) {
+ this.bean = bean;
Class<?> beanClass =
(bean instanceof BeanReceiver)
? ((BeanReceiver) bean).getExportedBeanReceiver()
@@ -226,7 +233,6 @@
beansByPlugin.put(plugin, getDynamicBean(bean, provider.get()));
}
}
- startLifecycleListeners();
}
@SuppressWarnings("unchecked")
diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index 0992bcd..d349dda 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -177,6 +177,8 @@
private final Provider<GetPureRevert> getPureRevertProvider;
private final StarredChangesUtil stars;
private final DynamicOptionParser dynamicOptionParser;
+ private final Injector injector;
+ private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
@Inject
ChangeApiImpl(
@@ -230,7 +232,9 @@
Provider<GetPureRevert> getPureRevertProvider,
StarredChangesUtil stars,
DynamicOptionParser dynamicOptionParser,
- @Assisted ChangeResource change) {
+ @Assisted ChangeResource change,
+ Injector injector,
+ DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
this.changeApi = changeApi;
this.revert = revert;
this.revertSubmission = revertSubmission;
@@ -282,6 +286,8 @@
this.stars = stars;
this.dynamicOptionParser = dynamicOptionParser;
this.change = change;
+ this.injector = injector;
+ this.dynamicBeans = dynamicBeans;
}
@Override
@@ -500,10 +506,10 @@
public ChangeInfo get(
EnumSet<ListChangesOption> options, ImmutableListMultimap<String, String> pluginOptions)
throws RestApiException {
- try {
+ try (DynamicOptions dynamicOptions = new DynamicOptions(injector, dynamicBeans)) {
GetChange getChange = getChangeProvider.get();
options.forEach(getChange::addOption);
- dynamicOptionParser.parseDynamicOptions(getChange, pluginOptions);
+ dynamicOptionParser.parseDynamicOptions(getChange, pluginOptions, dynamicOptions);
return getChange.apply(change).value();
} catch (Exception e) {
throw asRestApiException("Cannot retrieve change", e);
@@ -759,8 +765,6 @@
@Singleton
static class DynamicOptionParser {
private final CmdLineParser.Factory cmdLineParserFactory;
- private final Injector injector;
- private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
@Inject
DynamicOptionParser(
@@ -768,14 +772,14 @@
Injector injector,
DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
this.cmdLineParserFactory = cmdLineParserFactory;
- this.injector = injector;
- this.dynamicBeans = dynamicBeans;
}
- void parseDynamicOptions(Object bean, ListMultimap<String, String> pluginOptions)
+ void parseDynamicOptions(
+ Object bean, ListMultimap<String, String> pluginOptions, DynamicOptions dynamicOptions)
throws BadRequestException {
CmdLineParser clp = cmdLineParserFactory.create(bean);
- DynamicOptions dynamicOptions = new DynamicOptions(bean, injector, dynamicBeans);
+ dynamicOptions.setBean(bean);
+ dynamicOptions.startLifecycleListeners();
dynamicOptions.parseDynamicBeans(clp);
dynamicOptions.setDynamicBeans();
dynamicOptions.onBeanParseStart();
diff --git a/java/com/google/gerrit/server/api/changes/ChangesImpl.java b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
index d6ef61c..0596524 100644
--- a/java/com/google/gerrit/server/api/changes/ChangesImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
@@ -26,15 +26,18 @@
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
+import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.api.changes.ChangeApiImpl.DynamicOptionParser;
import com.google.gerrit.server.restapi.change.ChangesCollection;
import com.google.gerrit.server.restapi.change.CreateChange;
import com.google.gerrit.server.restapi.change.QueryChanges;
import com.google.inject.Inject;
+import com.google.inject.Injector;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.List;
@@ -46,6 +49,8 @@
private final CreateChange createChange;
private final DynamicOptionParser dynamicOptionParser;
private final Provider<QueryChanges> queryProvider;
+ private final Injector injector;
+ private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
@Inject
ChangesImpl(
@@ -53,12 +58,16 @@
ChangeApiImpl.Factory api,
CreateChange createChange,
DynamicOptionParser dynamicOptionParser,
- Provider<QueryChanges> queryProvider) {
+ Provider<QueryChanges> queryProvider,
+ Injector injector,
+ DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
this.changes = changes;
this.api = api;
this.createChange = createChange;
this.dynamicOptionParser = dynamicOptionParser;
this.queryProvider = queryProvider;
+ this.injector = injector;
+ this.dynamicBeans = dynamicBeans;
}
@Override
@@ -123,34 +132,36 @@
}
private List<ChangeInfo> get(QueryRequest q) throws RestApiException {
- QueryChanges qc = queryProvider.get();
- if (q.getQuery() != null) {
- qc.addQuery(q.getQuery());
- }
- qc.setLimit(q.getLimit());
- qc.setStart(q.getStart());
- qc.setNoLimit(q.getNoLimit());
- for (ListChangesOption option : q.getOptions()) {
- qc.addOption(option);
- }
- dynamicOptionParser.parseDynamicOptions(qc, q.getPluginOptions());
-
- try {
- List<?> result = qc.apply(TopLevelResource.INSTANCE).value();
- if (result.isEmpty()) {
- return ImmutableList.of();
+ try (DynamicOptions dynamicOptions = new DynamicOptions(injector, dynamicBeans)) {
+ QueryChanges qc = queryProvider.get();
+ if (q.getQuery() != null) {
+ qc.addQuery(q.getQuery());
}
+ qc.setLimit(q.getLimit());
+ qc.setStart(q.getStart());
+ qc.setNoLimit(q.getNoLimit());
+ for (ListChangesOption option : q.getOptions()) {
+ qc.addOption(option);
+ }
+ dynamicOptionParser.parseDynamicOptions(qc, q.getPluginOptions(), dynamicOptions);
- // Check type safety of result; the extension API should be safer than the
- // REST API in this case, since it's intended to be used in Java.
- Object first = requireNonNull(result.iterator().next());
- checkState(first instanceof ChangeInfo);
- @SuppressWarnings("unchecked")
- List<ChangeInfo> infos = (List<ChangeInfo>) result;
+ try {
+ List<?> result = qc.apply(TopLevelResource.INSTANCE).value();
+ if (result.isEmpty()) {
+ return ImmutableList.of();
+ }
- return ImmutableList.copyOf(infos);
- } catch (Exception e) {
- throw asRestApiException("Cannot query changes", e);
+ // Check type safety of result; the extension API should be safer than the
+ // REST API in this case, since it's intended to be used in Java.
+ Object first = requireNonNull(result.iterator().next());
+ checkState(first instanceof ChangeInfo);
+ @SuppressWarnings("unchecked")
+ List<ChangeInfo> infos = (List<ChangeInfo>) result;
+
+ return ImmutableList.copyOf(infos);
+ } catch (Exception e) {
+ throw asRestApiException("Cannot query changes", e);
+ }
}
}
}
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index 07cb04f..bf00d27 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -40,6 +40,7 @@
import com.google.gerrit.server.mail.send.DeleteReviewerSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RemoveReviewerControl;
@@ -132,9 +133,15 @@
for (LabelType lt : labelTypes.getLabelTypes()) {
newApprovals.put(lt.getName(), (short) 0);
}
-
+ String ccOrReviewer =
+ approvalsUtil
+ .getReviewers(ctx.getNotes())
+ .byState(ReviewerStateInternal.CC)
+ .contains(reviewerId)
+ ? "cc"
+ : "reviewer";
StringBuilder msg = new StringBuilder();
- msg.append("Removed reviewer " + reviewer.account().fullName());
+ msg.append(String.format("Removed %s %s", ccOrReviewer, reviewer.account().fullName()));
StringBuilder removedVotesMsg = new StringBuilder();
removedVotesMsg.append(" with the following votes:\n\n");
boolean votesRemoved = false;
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 1650421..220e683 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -121,12 +121,18 @@
// Single Timestamp overhead.
private static final int T = O + 8;
+ /**
+ * {@inheritDoc}
+ *
+ * <p>Take all columns and all collection sizes into account, but use estimated average element
+ * sizes rather than iterating over collections. Numbers are largely hand-wavy based on
+ * http://stackoverflow.com/questions/258120/what-is-the-memory-consumption-of-an-object-in-java
+ *
+ * <p>Should be kept up to date with {@link ChangeNotesState}. Please, keep weights listed in
+ * the same order as fields.
+ */
@Override
public int weigh(Key key, ChangeNotesState state) {
- // Take all columns and all collection sizes into account, but use
- // estimated average element sizes rather than iterating over collections.
- // Numbers are largely hand-wavy based on
- // http://stackoverflow.com/questions/258120/what-is-the-memory-consumption-of-an-object-in-java
return P
+ O
+ 20 // metaId
@@ -138,6 +144,7 @@
+ K // owner
+ P
+ str(state.columns().branch())
+ + P // status
+ P
+ patchSetId() // currentPatchSetId
+ P
@@ -148,9 +155,16 @@
+ str(state.columns().originalSubject())
+ P
+ str(state.columns().submissionId())
- + P // status
+ + 1 // isPrivate
+ + 1 // workInProgress
+ + 1 // reviewStarted
+ + P
+ + K // revertOf
+ + P
+ + patchSetId() // cherryPickOf
+ P
+ set(state.hashtags(), str(10))
+ + str(state.serverId()) // serverId
+ P
+ list(state.patchSets(), patchSet())
+ P
@@ -177,9 +191,6 @@
+ list(state.changeMessages(), changeMessage())
+ P
+ map(state.publishedComments().asMap(), comment())
- + 1 // isPrivate
- + 1 // workInProgress
- + 1 // reviewStarted
+ I; // updateCount
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index fa32686..27cfb70 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -81,6 +81,9 @@
* <p>One instance is the output of a single {@link ChangeNotesParser}, and contains types required
* to support public methods on {@link ChangeNotes}. It is intended to be cached in-process.
*
+ * <p>When new fields are added to the {@link ChangeNotesState}, {@link
+ * ChangeNotesCache.Weigher#weigh} should be updated.
+ *
* <p>Note that {@link ChangeNotes} contains more than just a single {@code ChangeNoteState}, such
* as per-draft information, so that class is not cached directly.
*/
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java
index eceb970..27c6793 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -60,9 +60,9 @@
* <p>{@code PermissionBackend} is a singleton for the server, acting as a factory for lightweight
* request instances. Implementation classes may cache supporting data inside of {@link WithUser},
* {@link ForProject}, {@link ForRef}, and {@link ForChange} instances, in addition to storing
- * within {@link CurrentUser} using a {@link com.google.gerrit.server.CurrentUser.PropertyKey}.
- * {@link GlobalPermission} caching for {@link WithUser} may best cached inside {@link CurrentUser}
- * as {@link WithUser} instances are frequently created.
+ * within {@link CurrentUser} using a {@link com.google.gerrit.server.PropertyMap.Key}. {@link
+ * GlobalPermission} caching for {@link WithUser} may best cached inside {@link CurrentUser} as
+ * {@link WithUser} instances are frequently created.
*
* <p>Example use:
*
diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java
index 0dbae0a..fe03770 100644
--- a/java/com/google/gerrit/sshd/BaseCommand.java
+++ b/java/com/google/gerrit/sshd/BaseCommand.java
@@ -235,6 +235,8 @@
protected void parseCommandLine(Object options, DynamicOptions pluginOptions)
throws UnloggedFailure {
final CmdLineParser clp = newCmdLineParser(options);
+ pluginOptions.setBean(options);
+ pluginOptions.startLifecycleListeners();
pluginOptions.parseDynamicBeans(clp);
pluginOptions.setDynamicBeans();
pluginOptions.onBeanParseStart();
@@ -468,8 +470,7 @@
try {
if (thunk instanceof ProjectCommandRunnable) {
- try (DynamicOptions pluginOptions =
- new DynamicOptions(BaseCommand.this, injector, dynamicBeans)) {
+ try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
((ProjectCommandRunnable) thunk).executeParseCommand(pluginOptions);
projectName = ((ProjectCommandRunnable) thunk).getProjectName();
thunk.run();
diff --git a/java/com/google/gerrit/sshd/DispatchCommand.java b/java/com/google/gerrit/sshd/DispatchCommand.java
index a45cd31..54171a3 100644
--- a/java/com/google/gerrit/sshd/DispatchCommand.java
+++ b/java/com/google/gerrit/sshd/DispatchCommand.java
@@ -72,8 +72,7 @@
@Override
public void start(ChannelSession channel, Environment env) throws IOException {
- try (DynamicOptions pluginOptions =
- new DynamicOptions(DispatchCommand.this, injector, dynamicBeans)) {
+ try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
parseCommandLine(pluginOptions);
if (Strings.isNullOrEmpty(commandName)) {
StringWriter msg = new StringWriter();
diff --git a/java/com/google/gerrit/sshd/SshCommand.java b/java/com/google/gerrit/sshd/SshCommand.java
index 3ef7061..c94b25c 100644
--- a/java/com/google/gerrit/sshd/SshCommand.java
+++ b/java/com/google/gerrit/sshd/SshCommand.java
@@ -50,8 +50,7 @@
public void start(ChannelSession channel, Environment env) throws IOException {
startThread(
() -> {
- try (DynamicOptions pluginOptions =
- new DynamicOptions(SshCommand.this, injector, dynamicBeans)) {
+ try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
parseCommandLine(pluginOptions);
stdout = toPrintWriter(out);
stderr = toPrintWriter(err);
diff --git a/java/com/google/gerrit/sshd/SuExec.java b/java/com/google/gerrit/sshd/SuExec.java
index bf785bb..3c6e8c2 100644
--- a/java/com/google/gerrit/sshd/SuExec.java
+++ b/java/com/google/gerrit/sshd/SuExec.java
@@ -93,7 +93,7 @@
@Override
public void start(ChannelSession channel, Environment env) throws IOException {
- try (DynamicOptions pluginOptions = new DynamicOptions(SuExec.this, injector, dynamicBeans)) {
+ try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
checkCanRunAs();
parseCommandLine(pluginOptions);
diff --git a/java/com/google/gerrit/sshd/commands/StreamEvents.java b/java/com/google/gerrit/sshd/commands/StreamEvents.java
index 188cc83..c47d24c 100644
--- a/java/com/google/gerrit/sshd/commands/StreamEvents.java
+++ b/java/com/google/gerrit/sshd/commands/StreamEvents.java
@@ -108,8 +108,7 @@
@Override
public void start(ChannelSession channel, Environment env) throws IOException {
- try (DynamicOptions pluginOptions =
- new DynamicOptions(StreamEvents.this, injector, dynamicBeans)) {
+ try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
try {
parseCommandLine(pluginOptions);
} catch (UnloggedFailure e) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index f59fba0..b7f1ef0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -2258,6 +2258,10 @@
assertThat(message.body()).contains("Removed reviewer " + user.fullName() + ".");
assertThat(message.body()).doesNotContain("with the following votes");
+ // Make sure the change message for removing a reviewer is correct.
+ assertThat(Iterables.getLast(gApi.changes().id(changeId).messages()).message)
+ .contains("Removed reviewer " + user.fullName());
+
// Make sure the reviewer can still be added again.
gApi.changes().id(changeId).addReviewer(user.id().toString());
c = gApi.changes().id(changeId).get();
@@ -2273,6 +2277,31 @@
}
@Test
+ public void removeCC() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ // Add a cc
+ AddReviewerInput addReviewerInput = new AddReviewerInput();
+ addReviewerInput.state = CC;
+ addReviewerInput.reviewer = user.id().toString();
+ gApi.changes().id(changeId).addReviewer(addReviewerInput);
+
+ // Remove a cc
+ sender.clear();
+ gApi.changes().id(changeId).reviewer(user.id().toString()).remove();
+ assertThat(gApi.changes().id(changeId).get().reviewers).isEmpty();
+
+ // Make sure the email for removing a cc is correct.
+ assertThat(sender.getMessages()).hasSize(1);
+ Message message = sender.getMessages().get(0);
+ assertThat(message.body()).contains("Removed cc " + user.fullName() + ".");
+
+ // Make sure the change message for removing a reviewer is correct.
+ assertThat(Iterables.getLast(gApi.changes().id(changeId).messages()).message)
+ .contains("Removed cc " + user.fullName());
+ }
+
+ @Test
public void removeReviewer() throws Exception {
testRemoveReviewer(true);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index e4bb73a..0af116a 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -113,7 +113,6 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import java.util.Optional;
import java.util.stream.Stream;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -705,36 +704,6 @@
}
@Test
- public void renamingGroupChangesProjectConfigs() throws Exception {
- String name = name("Name1");
- GroupInfo group = gApi.groups().create(name).get();
-
- // Use group in a permission
- projectOperations
- .project(project)
- .forUpdate()
- .add(allow(Permission.READ).ref(RefNames.REFS_CONFIG).group(AccountGroup.uuid(group.id)))
- .update();
- Optional<String> beforeRename =
- projectCache.get(project).get().getLocalGroups().stream()
- .filter(g -> g.getUUID().get().equals(group.id))
- .map(GroupReference::getName)
- .findAny();
- // Groups created with ProjectOperations always have their UUID as local name
- assertThat(beforeRename).hasValue(group.id);
-
- String newName = name("Name2");
- gApi.groups().id(name).name(newName);
-
- Optional<String> afterRename =
- projectCache.get(project).get().getLocalGroups().stream()
- .filter(g -> g.getUUID().get().equals(group.id))
- .map(GroupReference::getName)
- .findAny();
- assertThat(afterRename).hasValue(newName);
- }
-
- @Test
public void groupDescription() throws Exception {
String name = name("group");
gApi.groups().create(name);
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
index e99a6f5..d5fc1c1 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.project;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.server.project.ProjectState.INHERITED_FROM_GLOBAL;
@@ -39,7 +40,9 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.GroupReference;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
@@ -55,6 +58,7 @@
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.ProjectInfo;
import com.google.gerrit.extensions.events.ChangeIndexedListener;
import com.google.gerrit.extensions.events.ProjectIndexedListener;
@@ -63,6 +67,7 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.config.ProjectConfigEntry;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.inject.AbstractModule;
@@ -70,6 +75,7 @@
import com.google.inject.Module;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
@@ -939,6 +945,42 @@
projectOperations.project(allProjects).getHead(RefNames.REFS_CONFIG).name()));
}
+ @Test
+ public void renamingGroupGetsPersisted() throws Exception {
+ String name = name("Name1");
+ GroupInfo group = gApi.groups().create(name).get();
+
+ // Use group in a permission
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.READ).ref(RefNames.REFS_CONFIG).group(AccountGroup.uuid(group.id)))
+ .update();
+ Optional<String> beforeRename =
+ projectCache.get(project).get().getLocalGroups().stream()
+ .filter(g -> g.getUUID().get().equals(group.id))
+ .map(GroupReference::getName)
+ .findAny();
+ // Groups created with ProjectOperations always have their UUID as local name
+ assertThat(beforeRename).hasValue(group.id);
+
+ // Rename the group directly on the project config
+ String newName = name("Name2");
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig config = projectConfigFactory.read(md);
+ config.renameGroup(AccountGroup.uuid(group.id), newName);
+ config.commit(md);
+ projectCache.evict(config.getProject());
+ }
+
+ Optional<String> afterRename =
+ projectCache.get(project).get().getLocalGroups().stream()
+ .filter(g -> g.getUUID().get().equals(group.id))
+ .map(GroupReference::getName)
+ .findAny();
+ assertThat(afterRename).hasValue(newName);
+ }
+
private CommentLinkInfo commentLinkInfo(String name, String match, String link) {
CommentLinkInfo info = new CommentLinkInfo();
info.name = name;
diff --git a/javatests/com/google/gerrit/acceptance/ssh/DynamicOptionsIT.java b/javatests/com/google/gerrit/acceptance/ssh/DynamicOptionsIT.java
new file mode 100644
index 0000000..c0f2b36
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/ssh/DynamicOptionsIT.java
@@ -0,0 +1,52 @@
+// 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.
+
+package com.google.gerrit.acceptance.ssh;
+
+import static com.google.gerrit.server.query.change.OutputStreamQuery.GSON;
+import static junit.framework.TestCase.assertEquals;
+
+import com.google.common.collect.Lists;
+import com.google.gerrit.acceptance.AbstractDynamicOptionsTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.UseSsh;
+import com.google.gson.reflect.TypeToken;
+import com.google.inject.Module;
+import java.io.IOException;
+import java.util.List;
+import org.junit.Test;
+
+@NoHttpd
+@UseSsh
+public class DynamicOptionsIT extends AbstractDynamicOptionsTest {
+
+ @Override
+ public Module createSshModule() {
+ return new AbstractDynamicOptionsTest.PluginOneSshModule();
+ }
+
+ @Test
+ public void testDynamicPluginOptions() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin("my-plugin", AbstractDynamicOptionsTest.PluginTwoModule.class)) {
+ List<String> samples = getSamplesList(adminSshSession.exec("ls-samples"));
+ adminSshSession.assertSuccess();
+ assertEquals(Lists.newArrayList("sample1", "sample2"), samples);
+ }
+ }
+
+ protected List<String> getSamplesList(String sshOutput) throws IOException {
+ return GSON.fromJson(sshOutput, new TypeToken<List<String>>() {}.getType());
+ }
+}
diff --git a/plugins/download-commands b/plugins/download-commands
index 87e3930..5bd359c 160000
--- a/plugins/download-commands
+++ b/plugins/download-commands
@@ -1 +1 @@
-Subproject commit 87e3930cea7c06aea454998abdddf6515a9f103b
+Subproject commit 5bd359c08e10b93d2c08762f75cde01a14e45fc6
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 ce3a811..0cf9357 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
@@ -219,7 +219,9 @@
)
);
- const checkForNewUser = !project && user === 'self';
+ // Checking `this.account` to make sure that the user is logged in.
+ // Otherwise sending a query for 'owner:self' will result in an error.
+ const checkForNewUser = !project && !!this.account && user === 'self';
return dashboardPromise
.then(res => {
if (res && res.title) {
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 44f203d..f788e74 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
@@ -21,6 +21,7 @@
import {GerritNav, GerritView} from '../../core/gr-navigation/gr-navigation.js';
import {changeIsOpen} from '../../../utils/change-util.js';
import {ChangeStatus} from '../../../constants/constants.js';
+import {createAccountWithId} from '../../../test/test-data-generators.js';
const basicFixture = fixtureFromElement('gr-dashboard-view');
@@ -201,9 +202,23 @@
user: 'self',
};
return paramsChangedPromise.then(() => {
- assert.isTrue(
- getChangesStub.calledWith(undefined,
- ['1', '2', 'owner:self limit:1']));
+ assert.isTrue(getChangesStub.calledWith(undefined, ['1', '2']));
+ });
+ });
+
+ test('viewing dashboard when logged in includes owner:self query', () => {
+ element.account = createAccountWithId(1);
+ element.params = {
+ view: GerritNav.View.DASHBOARD,
+ sections: [
+ {query: '1'},
+ {query: '2', selfOnly: true},
+ ],
+ user: 'self',
+ };
+ return paramsChangedPromise.then(() => {
+ assert.isTrue(getChangesStub.calledWith(undefined,
+ ['1', '2', 'owner:self limit:1']));
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 8989440..20fbc32 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -42,6 +42,7 @@
import {
fetchChangeUpdates,
patchNumEquals,
+ CURRENT,
} from '../../../utils/patch-set-util';
import {
changeIsOpen,
@@ -114,6 +115,7 @@
UIActionInfo,
} from '../../shared/gr-js-api-interface/gr-change-actions-js-api';
import {fireAlert} from '../../../utils/event-util';
+import {CODE_REVIEW} from '../../../utils/label-util';
const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.';
const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.';
@@ -947,6 +949,16 @@
return null;
}
}
+ // Allow the user to use quick approve to vote the max score on code review
+ // even if it is already granted.
+ if (
+ !result &&
+ this.change.labels[CODE_REVIEW] &&
+ this._getLabelStatus(this.change.labels[CODE_REVIEW]) === LabelStatus.OK
+ ) {
+ result = CODE_REVIEW;
+ }
+
if (result) {
const score = this.change.permitted_labels[result].slice(-1)[0];
const labelInfo = this.change.labels[result];
@@ -1580,7 +1592,7 @@
if (!labels) {
return Promise.resolve(undefined);
}
- return this.$.restAPI.saveChangeReview(newChangeId, 'current', {labels});
+ return this.$.restAPI.saveChangeReview(newChangeId, CURRENT, {labels});
}
_handleResponse(action: UIActionInfo, response?: Response) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
index ae49a57..1098760 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
@@ -1717,6 +1717,31 @@
.querySelector('gr-button[data-action-key=\'review\']');
assert.equal(approveButton.getAttribute('data-label'), 'bar+2');
});
+
+ test('added when can approve an already-approved code review label',
+ () => {
+ element.change = {
+ current_revision: 'abc1234',
+ labels: {
+ 'Code-Review': {
+ approved: {},
+ values: {
+ ' 0': '',
+ '+1': '',
+ '+2': '',
+ },
+ },
+ },
+ permitted_labels: {
+ 'Code-Review': [' 0', '+1', '+2'],
+ },
+ };
+ flush();
+ const approveButton =
+ element.shadowRoot
+ .querySelector('gr-button[data-action-key=\'review\']');
+ assert.isNotNull(approveButton);
+ });
});
test('adds download revision action', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 108f9ed..7759b7b 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -1558,7 +1558,7 @@
'changeComments, patchRange and diffPrefs must be set'
);
}
- diffElem.comments = this.changeComments.getCommentsBySideForFile(
+ diffElem.threads = this.changeComments.getThreadsBySideForFile(
file,
this.patchRange,
this.projectConfig
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
index 4f3d7ce6..e4e4056 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
@@ -336,7 +336,7 @@
const el = this._createToastAlert();
el.show(text, actionText, actionCallback);
this._alertElement = el;
- this.fire('iron-announce', {text}, {bubbles: true});
+ this.fire('iron-announce', {text: `Alert: ${text}`}, {bubbles: true});
this.reporting.reportInteraction('show-alert', {text});
}
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index b01a110..ef5be9fd 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -23,6 +23,7 @@
getParentIndex,
isMergeParent,
patchNumEquals,
+ CURRENT,
} from '../../../utils/patch-set-util';
import {customElement, property} from '@polymer/decorators';
import {
@@ -38,7 +39,7 @@
RevisionId,
} from '../../../types/common';
import {hasOwnProperty} from '../../../utils/common-util';
-import {CommentSide} from '../../../constants/constants';
+import {CommentSide, Side} from '../../../constants/constants';
import {RestApiService} from '../../../services/services/gr-rest-api/gr-rest-api';
import {
Comment,
@@ -313,6 +314,30 @@
return allDrafts;
}
+ _addCommentSide(comments: TwoSidesComments) {
+ const allComments = [];
+ for (const side of [Side.LEFT, Side.RIGHT]) {
+ // This is needed by the threading.
+ for (const comment of comments[side]) {
+ comment.__commentSide = side;
+ }
+ allComments.push(...comments[side]);
+ }
+ return allComments;
+ }
+
+ getThreadsBySideForPath(
+ path: string,
+ patchRange: PatchRange,
+ projectConfig?: ConfigInfo
+ ): CommentThread[] {
+ return createCommentThreads(
+ this._addCommentSide(
+ this.getCommentsBySideForPath(path, patchRange, projectConfig)
+ )
+ );
+ }
+
/**
* Get the comments (with drafts and robot comments) for a path and
* patch-range. Returns an object with left and right properties mapping to
@@ -371,6 +396,18 @@
};
}
+ getThreadsBySideForFile(
+ file: PatchSetFile,
+ patchRange: PatchRange,
+ projectConfig?: ConfigInfo
+ ): CommentThread[] {
+ return createCommentThreads(
+ this._addCommentSide(
+ this.getCommentsBySideForFile(file, patchRange, projectConfig)
+ )
+ );
+ }
+
/**
* Get the comments (with drafts and robot comments) for a file and
* patch-range. Returns an object with left and right properties mapping to
@@ -576,7 +613,7 @@
}
getPortedComments(changeNum: NumericChangeId, revision?: RevisionId) {
- if (!revision) revision = 'current';
+ if (!revision) revision = CURRENT;
return Promise.all([
this.$.restAPI.getPortedComments(changeNum, revision),
this.$.restAPI.getPortedDrafts(changeNum, revision),
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
index 1ae302a..ecedb28 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.ts
@@ -105,6 +105,8 @@
row.classList.add('diff-row', 'side-by-side');
row.setAttribute('left-type', leftLine.type);
row.setAttribute('right-type', rightLine.type);
+ // TabIndex makes screen reader read a row when navigating with j/k
+ row.tabIndex = -1;
row.appendChild(this._createBlameCell(leftLine.beforeNumber));
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.ts
index 04ac472..2011a59 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.ts
@@ -104,6 +104,8 @@
_createRow(line: GrDiffLine) {
const row = this._createElement('tr', line.type);
row.classList.add('diff-row', 'unified');
+ // TabIndex makes screen reader read a row when navigating with j/k
+ row.tabIndex = -1;
row.appendChild(this._createBlameCell(line.beforeNumber));
let lineNumberEl = this._createLineEl(
line,
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index aa769c6..4b4f429 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -31,8 +31,7 @@
isMergeParent,
isNumber,
} from '../../../utils/patch-set-util';
-import {CommentThread, createCommentThreads} from '../../../utils/comment-util';
-import {TwoSidesComments} from '../gr-comment-api/gr-comment-api';
+import {CommentThread} from '../../../utils/comment-util';
import {customElement, observe, property} from '@polymer/decorators';
import {
CommitRange,
@@ -192,8 +191,8 @@
@property({type: Boolean})
noRenderOnPrefsChange = false;
- @property({type: Object, observer: '_commentsChanged'})
- comments?: TwoSidesComments;
+ @property({type: Object, observer: '_threadsChanged'})
+ threads?: CommentThread[];
@property({type: Boolean})
lineWrapping = false;
@@ -674,21 +673,12 @@
return isImageDiff(diff);
}
- _commentsChanged(newComments: TwoSidesComments) {
- const allComments = [];
- for (const side of [Side.LEFT, Side.RIGHT]) {
- // This is needed by the threading.
- for (const comment of newComments[side]) {
- comment.__commentSide = side;
- }
- allComments.push(...newComments[side]);
- }
+ _threadsChanged(threads: CommentThread[]) {
// Currently, the only way this is ever changed here is when the initial
- // comments are loaded, so it's okay performance wise to clear the threads
+ // threads are loaded, so it's okay performance wise to clear the threads
// and recreate them. If this changes in future, we might want to reuse
// some DOM nodes here.
this._clearThreads();
- const threads = createCommentThreads(allComments);
for (const thread of threads) {
const threadEl = this._createThreadElement(thread);
this._attachThreadElement(threadEl);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index c169bf4..5a6113c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -48,6 +48,7 @@
computeLatestPatchNum,
patchNumEquals,
PatchSet,
+ CURRENT,
} from '../../../utils/patch-set-util';
import {
addUnmodifiedFiles,
@@ -65,11 +66,7 @@
GrDropdownList,
} from '../../shared/gr-dropdown-list/gr-dropdown-list';
import {GrOverlay} from '../../shared/gr-overlay/gr-overlay';
-import {
- ChangeComments,
- GrCommentApi,
- TwoSidesComments,
-} from '../gr-comment-api/gr-comment-api';
+import {ChangeComments, GrCommentApi} from '../gr-comment-api/gr-comment-api';
import {GrDiffModeSelector} from '../gr-diff-mode-selector/gr-diff-mode-selector';
import {
ChangeInfo,
@@ -240,9 +237,6 @@
@property({type: Object})
_commentMap?: CommentMap;
- @property({type: Object})
- _commentsForDiff?: TwoSidesComments;
-
@property({
type: Object,
computed: '_computeCommentSkips(_commentMap, _fileList, _path)',
@@ -1013,12 +1007,6 @@
}
this._commentMap = this._getPaths(this._patchRange);
-
- this._commentsForDiff = this._getCommentsForPath(
- this._path,
- this._patchRange,
- this._projectConfig
- );
}
_isFileUnchanged(diff: DiffInfo) {
@@ -1060,7 +1048,7 @@
const portedCommentsPromise = this.$.commentAPI.getPortedComments(
value.changeNum,
- value.patchNum || 'current'
+ value.patchNum || CURRENT
);
const promises: Promise<unknown>[] = [];
@@ -1087,7 +1075,13 @@
this._loading = false;
this._initPatchRange();
this._initCommitRange();
- this.$.diffHost.comments = this._commentsForDiff;
+ if (this._changeComments && this._path && this._patchRange) {
+ this.$.diffHost.threads = this._changeComments.getThreadsBySideForPath(
+ this._path,
+ this._patchRange,
+ this._projectConfig
+ );
+ }
portedCommentsPromise.then(() => {
this.reporting.timeEnd(PORTING_COMMENTS_DIFF_LATENCY_LABEL);
});
@@ -1575,13 +1569,11 @@
const file = files[path];
if (file && file.old_path) {
- this._commentsForDiff = this._changeComments.getCommentsBySideForFile(
+ this.$.diffHost.threads = this._changeComments.getThreadsBySideForFile(
{path, basePath: file.old_path},
patchRange,
projectConfig
);
-
- this.$.diffHost.comments = this._commentsForDiff;
}
}
@@ -1590,22 +1582,6 @@
return this._changeComments.getPaths(patchRange);
}
- _getCommentsForPath(
- path?: string,
- patchRange?: PatchRange,
- projectConfig?: ConfigInfo
- ) {
- if (!path) return undefined;
- if (!patchRange) return undefined;
- if (!this._changeComments) return undefined;
-
- return this._changeComments.getCommentsBySideForPath(
- path,
- patchRange,
- projectConfig
- );
- }
-
_getDiffDrafts() {
if (!this._changeNum) throw new Error('Missing this._changeNum');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index 7ef75ed..8e1e5c1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -151,8 +151,10 @@
computeCommentThreadCount: () => {},
computeUnresolvedNum: () => {},
getPaths: () => {},
- getCommentsBySideForPath: () => {},
+ getThreadsBySideForPath: () => {},
+ getThreadsBySideForFile: () => {},
findCommentById: _testOnly_findCommentById,
+
}));
await element._loadComments();
await flush();
@@ -200,11 +202,6 @@
commentLink: true,
commentId: 'c1',
};
- sinon.stub(element.$.diffHost, '_commentsChanged');
- sinon.stub(element, '_getCommentsForPath').returns({
- left: [{id: 'c1', __commentSide: 'left', line: 10}],
- right: [{id: 'c2', __commentSide: 'right', line: 11}],
- });
element._change = {
...createChange(),
revisions: createRevisions(11),
@@ -263,7 +260,6 @@
commentLink: true,
commentId: 'c1',
};
- sinon.stub(element.$.diffHost, '_commentsChanged');
element._change = {
...createChange(),
revisions: createRevisions(11),
@@ -293,7 +289,6 @@
commentLink: true,
commentId: 'c3',
};
- sinon.stub(element.$.diffHost, '_commentsChanged');
element._change = {
...createChange(),
revisions: createRevisions(11),
@@ -1455,7 +1450,6 @@
await flush();
});
test('empty', () => {
- sinon.stub(element, '_getCommentsForPath');
sinon.stub(element, '_getPaths').returns(new Map());
element._initPatchRange();
assert.equal(Object.keys(element._commentMap).length, 0);
@@ -1467,7 +1461,6 @@
'path/to/file/one.cpp': [{patch_set: 3, message: 'lorem'}],
'path-to/file/two.py': [{patch_set: 5, message: 'ipsum'}],
});
- sinon.stub(element, '_getCommentsForPath').returns({meta: {}});
element._changeNum = '42';
element._patchRange = {
basePatchNum: 3,
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index 2915acd..617034b 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -492,6 +492,9 @@
registrationOverlay.refit();
});
}
+ // To fix bug announce read after each new view, we reset announce with
+ // empty space
+ this.fire('iron-announce', {text: ' '}, {bubbles: true});
}
_handleShortcutTriggered(event: ShortcutTriggeredEvent) {
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
index da2881e..0b4e577 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
@@ -46,6 +46,7 @@
} from '../../../utils/attention-set-util';
import {ReviewerState} from '../../../constants/constants';
import {isRemovableReviewer} from '../../../utils/change-util';
+import {CURRENT} from '../../../utils/patch-set-util';
export interface GrHovercardAccount {
$: {
@@ -186,7 +187,7 @@
];
this.$.restAPI
- .saveChangeReview(this.change._number, 'current', reviewInput)
+ .saveChangeReview(this.change._number, CURRENT, reviewInput)
.then(response => {
if (!response || !response.ok) {
throw new Error(
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 d75c186..b86fcff 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
@@ -22,6 +22,7 @@
import {ListChangesOption} from '../../../utils/change-util.js';
import {appContext} from '../../../services/app-context.js';
import {createChange} from '../../../test/test-data-generators.js';
+import {CURRENT} from '../../../utils/patch-set-util.js';
const basicFixture = fixtureFromElement('gr-rest-api-interface');
@@ -1350,7 +1351,7 @@
sinon.stub(element._restApiHelper, 'fetchJSON').returns(Promise.resolve({
ok: false}));
- element.getPortedComments(change._number, 'current');
+ element.getPortedComments(change._number, CURRENT);
assert.isFalse(dispatchStub.called);
});
@@ -1361,7 +1362,7 @@
const getChangeURLAndFetchStub = sinon.stub(element,
'_getChangeURLAndFetch');
- element.getPortedDrafts(change._number, 'current');
+ element.getPortedDrafts(change._number, CURRENT);
assert.isFalse(getChangeURLAndFetchStub.called);
});
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 8974af8..d063168 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -45,6 +45,8 @@
PARENT: 'PARENT',
};
+export const CURRENT = 'current';
+
export interface PatchSet {
num: PatchSetNum;
desc: string | undefined;
