opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15'

Browse Source 
* stable-2.15:
  Fix star icon not highlighted in change list on UI
  Fix the "red" change size on elasticsearch site UI
  Restore not strict label votes per default
  SshCommandsIT: Include `logging ls` and `logging set` commands
  Add tests to make sure ssh commands can be executed
  Align ElasticIndexModule with LuceneIndexModule
  Revert "Remove unneeded nested MultiVersionModule class"
  Remove unneeded nested MultiVersionModule class
  Only bind index {start/activate} if needed
  Allow percent encoding in patch set titles.
  InitIndex: Allow to configure index.maxLimit for Elasticsearch
  Don't curse over files with up/down keys
  Add reset button to my menu in settings
  Add a check for name in Firefox/Safari
  Make sure plugins are not double counted
  Link account chips to owner search rather than user dashboard
  Add a link to group page in groups section of settings

Change-Id: Ifad7c2402614afe247e812bdd12de72dc3c299fe
This commit is contained in:
David Pursehouse
2018-04-12 10:13:39 +09:00
parent 4f51e4d1b5 c2fde0199d
commit 0a5f646bd8
12 changed files with 138 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
Documentation/config-gerrit.txt
View File

@@ -1245,6 +1245,14 @@ Zero or negative values allow robot comments of unlimited size.
+
The default limit is 1024kB.
[[change.strictLabels]]change.strictLabels::
+
Reject invalid label votes: invalid labels or invalid values. This
configuration option is provided for backwards compaitbility and may
be removed in future gerrit versions.
+
Default is false.
[[change.disablePrivateChanges]]change.disablePrivateChanges::
+
If set to true, users are not allowed to create private changes.
@@ -2741,9 +2749,10 @@ result lists). Set to 0 for no limit.
+
When `index.type` is set to `ELASTICSEARCH`, this value should not exceed
the `index.max_result_window` value configured on the Elasticsearch
server.
server. If a value is not configured during site initialization, defaults to
10000, which is the default value of `index.max_result_window` in Elasticsearch.
+
Defaults to no limit.
When `index.type` is set to `LUCENE`, defaults to no limit.
[[index.maxPages]]index.maxPages::
+

20
java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
View File

@@ -69,6 +69,7 @@ import io.searchbox.core.search.sort.Sort.Sorting;
import java.io.IOException;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.apache.commons.codec.binary.Base64;
import org.eclipse.jgit.lib.Config;
@@ -221,9 +222,24 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
// Changed lines.
int added = addedElement.getAsInt();
int deleted = deletedElement.getAsInt();
if (added != 0 && deleted != 0) {
cd.setChangedLines(added, deleted);
cd.setChangedLines(added, deleted);
}
// Star.
JsonElement starredElement = source.get(ChangeField.STAR.getName());
if (starredElement != null) {
ListMultimap<Account.Id, String> stars = MultimapBuilder.hashKeys().arrayListValues().build();
JsonArray starBy = starredElement.getAsJsonArray();
if (starBy.size() > 0) {
for (int i = 0; i < starBy.size(); i++) {
String[] indexableFields = starBy.get(i).getAsString().split(":");
Optional<Account.Id> id = Account.Id.tryParse(indexableFields[0]);
if (id.isPresent()) {
stars.put(id.get(), indexableFields[1]);
}
}
}
cd.setStars(stars);
}
// Mergeable.

2
java/com/google/gerrit/elasticsearch/ElasticIndexModule.java
View File

@@ -96,7 +96,6 @@ public class ElasticIndexModule extends AbstractModule {
} else {
install(new SingleVersionModule(singleVersions));
}
bind(VersionManager.class).to(ElasticVersionManager.class);
}
@SuppressWarnings("unused")
@@ -113,6 +112,7 @@ public class ElasticIndexModule extends AbstractModule {
private class MultiVersionModule extends LifecycleModule {
@Override
public void configure() {
bind(VersionManager.class).to(ElasticVersionManager.class);
listener().to(ElasticVersionManager.class);
if (onlineUpgrade) {
listener().to(OnlineUpgrader.class);

2
java/com/google/gerrit/httpd/init/WebAppInitializer.java
View File

@@ -413,7 +413,7 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi
false,
sysInjector.getInstance(DownloadConfig.class),
sysInjector.getInstance(LfsPluginAuthCommand.Module.class)));
modules.add(new IndexCommandsModule());
modules.add(new IndexCommandsModule(sysInjector));
return sysInjector.createChildInjector(modules);
}

2
java/com/google/gerrit/lucene/LuceneIndexModule.java
View File

@@ -106,7 +106,6 @@ public class LuceneIndexModule extends AbstractModule {
} else {
install(new SingleVersionModule(singleVersions));
}
bind(VersionManager.class).to(LuceneVersionManager.class);
}
@SuppressWarnings("unused")
@@ -125,6 +124,7 @@ public class LuceneIndexModule extends AbstractModule {
private class MultiVersionModule extends LifecycleModule {
@Override
public void configure() {
bind(VersionManager.class).to(LuceneVersionManager.class);
listener().to(LuceneVersionManager.class);
if (onlineUpgrade) {
listener().to(OnlineUpgrader.class);

2
java/com/google/gerrit/pgm/Daemon.java
View File

@@ -545,7 +545,7 @@ public class Daemon extends SiteProgram {
sysInjector.getInstance(DownloadConfig.class),
sysInjector.getInstance(LfsPluginAuthCommand.Module.class)));
if (!slave) {
modules.add(new IndexCommandsModule());
modules.add(new IndexCommandsModule(sysInjector));
}
return sysInjector.createChildInjector(modules);
}

1
java/com/google/gerrit/pgm/init/InitIndex.java
View File

@@ -70,6 +70,7 @@ class InitIndex implements InitStep {
"Transport protocol", "protocol", "http", Sets.newHashSet("http", "https"));
defaultServer.string("Hostname", "hostname", "localhost");
defaultServer.string("Port", "port", "9200");
index.string("Result window size", "maxLimit", "10000");
}
if ((site.isNew || isEmptySite()) && type == IndexType.LUCENE) {

26
java/com/google/gerrit/server/restapi/change/PostReview.java
View File

@@ -172,6 +172,7 @@ public class PostReview
private final Config gerritConfig;
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
private final boolean strictLabels;
@Inject
PostReview(
@@ -211,6 +212,7 @@ public class PostReview
this.gerritConfig = gerritConfig;
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@Override
@@ -450,8 +452,12 @@ public class PostReview
Map.Entry<String, Short> ent = itr.next();
LabelType type = labelTypes.byLabel(ent.getKey());
if (type == null) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
if (strictLabels) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
}
itr.remove();
continue;
}
if (!caller.isInternalUser()) {
@@ -490,8 +496,12 @@ public class PostReview
Map.Entry<String, Short> ent = itr.next();
LabelType lt = labelTypes.byLabel(ent.getKey());
if (lt == null) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
if (strictLabels) {
throw new BadRequestException(
String.format("label \"%s\" is not a configured label", ent.getKey()));
}
itr.remove();
continue;
}
if (ent.getValue() == null || ent.getValue() == 0) {
@@ -501,8 +511,12 @@ public class PostReview
}
if (lt.getValue(ent.getValue()) == null) {
throw new BadRequestException(
String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
if (strictLabels) {
throw new BadRequestException(
String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue()));
}
itr.remove();
continue;
}
short val = ent.getValue();

15
java/com/google/gerrit/sshd/commands/IndexCommandsModule.java
View File

@@ -14,20 +14,31 @@
package com.google.gerrit.sshd.commands;
import com.google.gerrit.server.index.VersionManager;
import com.google.gerrit.sshd.CommandModule;
import com.google.gerrit.sshd.CommandName;
import com.google.gerrit.sshd.Commands;
import com.google.gerrit.sshd.DispatchCommandProvider;
import com.google.inject.Injector;
import com.google.inject.Key;
public class IndexCommandsModule extends CommandModule {
private final Injector injector;
public IndexCommandsModule(Injector injector) {
this.injector = injector;
}
@Override
protected void configure() {
CommandName gerrit = Commands.named("gerrit");
CommandName index = Commands.named(gerrit, "index");
command(index).toProvider(new DispatchCommandProvider(index));
command(index, IndexActivateCommand.class);
command(index, IndexStartCommand.class);
if (injector.getExistingBinding(Key.get(VersionManager.class)) != null) {
command(index, IndexActivateCommand.class);
command(index, IndexStartCommand.class);
}
command(index, IndexChangesCommand.class);
command(index, IndexProjectCommand.class);
}

52
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -3105,6 +3105,58 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(approval.permittedVotingRange).isNull();
}
@Test
public void nonStrictLabelWithInvalidLabelPerDefault() throws Exception {
String changeId = createChange().getChangeId();
// Add a review with invalid labels.
ReviewInput input = ReviewInput.approve().label("Code-Style", 1);
gApi.changes().id(changeId).current().review(input);
Map<String, Short> votes = gApi.changes().id(changeId).current().reviewer(admin.email).votes();
assertThat(votes.keySet()).containsExactly("Code-Review");
assertThat(votes.values()).containsExactly((short) 2);
}
@Test
public void nonStrictLabelWithInvalidValuePerDefault() throws Exception {
String changeId = createChange().getChangeId();
// Add a review with invalid label values.
ReviewInput input = new ReviewInput().label("Code-Review", 3);
gApi.changes().id(changeId).current().review(input);
Map<String, Short> votes = gApi.changes().id(changeId).current().reviewer(admin.email).votes();
if (!notesMigration.readChanges()) {
assertThat(votes.keySet()).containsExactly("Code-Review");
assertThat(votes.values()).containsExactly((short) 0);
} else {
assertThat(votes).isEmpty();
}
}
@Test
@GerritConfig(name = "change.strictLabels", value = "true")
public void strictLabelWithInvalidLabel() throws Exception {
String changeId = createChange().getChangeId();
ReviewInput in = new ReviewInput().label("Code-Style", 1);
exception.expect(BadRequestException.class);
exception.expectMessage("label \"Code-Style\" is not a configured label");
gApi.changes().id(changeId).current().review(in);
}
@Test
@GerritConfig(name = "change.strictLabels", value = "true")
public void strictLabelWithInvalidValue() throws Exception {
String changeId = createChange().getChangeId();
ReviewInput in = new ReviewInput().label("Code-Review", 3);
exception.expect(BadRequestException.class);
exception.expectMessage("label \"Code-Review\": 3 is not a valid value");
gApi.changes().id(changeId).current().review(in);
}
@Test
public void unresolvedCommentsBlocked() throws Exception {
modifySubmitRules(

21
javatests/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
View File

@@ -139,18 +139,29 @@ public class ImpersonationIT extends AbstractDaemonTest {
}
@Test
@GerritConfig(name = "change.strictLabels", value = "true")
public void voteOnBehalfOfInvalidLabel() throws Exception {
allowCodeReviewOnBehalfOf();
PushOneCommit.Result r = createChange();
RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
ReviewInput in = new ReviewInput();
String changeId = createChange().getChangeId();
ReviewInput in = new ReviewInput().label("Not-A-Label", 5);
in.onBehalfOf = user.id.toString();
in.label("Not-A-Label", 5);
exception.expect(BadRequestException.class);
exception.expectMessage("label \"Not-A-Label\" is not a configured label");
revision.review(in);
gApi.changes().id(changeId).current().review(in);
}
@Test
public void voteOnBehalfOfInvalidLabelIgnoredWithoutStrictLabels() throws Exception {
allowCodeReviewOnBehalfOf();
String changeId = createChange().getChangeId();
ReviewInput in = new ReviewInput().label("Code-Review", 1).label("Not-A-Label", 5);
in.onBehalfOf = user.id.toString();
gApi.changes().id(changeId).current().review(in);
assertThat(gApi.changes().id(changeId).get().labels).doesNotContainKey("Not-A-Label");
}
@Test

4
javatests/com/google/gerrit/acceptance/ssh/SshCommandsIT.java
View File

@@ -95,7 +95,9 @@ public class SshCommandsIT extends AbstractDaemonTest {
}
}),
"index",
ImmutableList.of("activate", "changes", "project", "start"),
ImmutableList.of("changes", "project"), // "activate" and "start" are not included
"logging",
ImmutableList.of("ls", "set"),
"plugin",
ImmutableList.of("add", "enable", "install", "ls", "reload", "remove", "rm"),
"test-submit",