Merge "Add debug log that shows up in trace when an operation is retried"
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index 1aa6cd7c..ff43520 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -374,6 +374,15 @@
link:access-control.html#reference[here], but must not contain `${username}` or
`${shardeduserid}`.
+[[label_ignoreSelfApproval]]
+=== `label.Label-Name.ignoreSelfApproval`
+
+If true, the label may be voted on by the uploader of the latest patch set,
+but their approval does not make a change submittable. Instead, a
+non-uploader who has the right to vote has to approve the change.
+
+Defaults to false.
+
[[label_example]]
=== Example
diff --git a/Documentation/rest-api.txt b/Documentation/rest-api.txt
index 97cca38..8f6a47b 100644
--- a/Documentation/rest-api.txt
+++ b/Documentation/rest-api.txt
@@ -210,6 +210,15 @@
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/suggest_reviewers?trace&q=J
----
+Alternatively request tracing can also be enabled by setting the
+`X-Gerrit-Trace` header:
+
+.Example Request
+----
+ GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/suggest_reviewers?q=J
+ X-Gerrit-Trace: issue/123
+----
+
Enabling tracing results in additional logs with debug information that
are written to the `error_log`. All logs that correspond to the traced
request are associated with the trace ID. The trace ID is returned with
diff --git a/Documentation/user-request-tracing.txt b/Documentation/user-request-tracing.txt
index ac09d3c..8430e97 100644
--- a/Documentation/user-request-tracing.txt
+++ b/Documentation/user-request-tracing.txt
@@ -11,10 +11,10 @@
request type:
* REST API: For REST calls tracing can be enabled by setting the
- `trace` request parameter, the trace ID is returned as
- `X-Gerrit-Trace` header. More information about this can be found in
- the link:rest-api.html#tracing[Request Tracing] section of the
- link:rest-api.html[REST API documentation].
+ `trace` request parameter or the `X-Gerrit-Trace` header, the trace
+ ID is returned as `X-Gerrit-Trace` header. More information about
+ this can be found in the link:rest-api.html#tracing[Request Tracing]
+ section of the link:rest-api.html[REST API documentation].
* SSH API: For SSH calls tracing can be enabled by setting the
`--trace` option. More information about this can be found in
the link:cmd-index.html#trace[Trace] section of the
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 1d87880..69d603f 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1487,12 +1487,7 @@
assertNotifyTo(expected.email, expected.fullName);
}
- protected void assertNotifyTo(
- com.google.gerrit.acceptance.testsuite.account.TestAccount expected) {
- assertNotifyTo(expected.preferredEmail().orElse(null), expected.fullname().orElse(null));
- }
-
- private void assertNotifyTo(String expectedEmail, String expectedFullname) {
+ protected void assertNotifyTo(String expectedEmail, String expectedFullname) {
Address expectedAddress = new Address(expectedFullname, expectedEmail);
assertThat(sender.getMessages()).hasSize(1);
Message m = sender.getMessages().get(0);
@@ -1506,11 +1501,6 @@
assertNotifyCc(expected.emailAddress);
}
- protected void assertNotifyCc(
- com.google.gerrit.acceptance.testsuite.account.TestAccount expected) {
- assertNotifyCc(expected.preferredEmail().orElse(null), expected.fullname().orElse(null));
- }
-
protected void assertNotifyCc(String expectedEmail, String expectedFullname) {
Address expectedAddress = new Address(expectedFullname, expectedEmail);
assertNotifyCc(expectedAddress);
@@ -1533,13 +1523,10 @@
assertThat(m.headers().get("Cc").isEmpty()).isTrue();
}
- protected void assertNotifyBcc(
- com.google.gerrit.acceptance.testsuite.account.TestAccount expected) {
+ protected void assertNotifyBcc(String expectedEmail, String expectedFullName) {
assertThat(sender.getMessages()).hasSize(1);
Message m = sender.getMessages().get(0);
- assertThat(m.rcpt())
- .containsExactly(
- new Address(expected.fullname().orElse(null), expected.preferredEmail().orElse(null)));
+ assertThat(m.rcpt()).containsExactly(new Address(expectedFullName, expectedEmail));
assertThat(m.headers().get("To").isEmpty()).isTrue();
assertThat(m.headers().get("Cc").isEmpty()).isTrue();
}
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index 6e5424c..582c7cb 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -24,6 +24,8 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.account.AccountOperationsImpl;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperationsImpl;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.lucene.LuceneIndexModule;
@@ -441,6 +443,7 @@
bindConstant().annotatedWith(SshEnabled.class).to(daemon.getEnableSshd());
bind(AccountCreator.class);
bind(AccountOperations.class).to(AccountOperationsImpl.class);
+ bind(GroupOperations.class).to(GroupOperationsImpl.class);
factory(PushOneCommit.Factory.class);
install(InProcessProtocol.module());
install(new NoSshModule());
diff --git a/java/com/google/gerrit/acceptance/HttpResponse.java b/java/com/google/gerrit/acceptance/HttpResponse.java
index 6c03793..3e98d71 100644
--- a/java/com/google/gerrit/acceptance/HttpResponse.java
+++ b/java/com/google/gerrit/acceptance/HttpResponse.java
@@ -14,13 +14,16 @@
package com.google.gerrit.acceptance;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.Reader;
import java.nio.ByteBuffer;
+import java.util.Arrays;
import org.apache.http.Header;
import org.eclipse.jgit.util.IO;
import org.eclipse.jgit.util.RawParseUtils;
@@ -61,6 +64,13 @@
return hdr != null ? hdr.getValue() : null;
}
+ public ImmutableList<String> getHeaders(String name) {
+ return Arrays.asList(response.getHeaders(name))
+ .stream()
+ .map(Header::getValue)
+ .collect(toImmutableList());
+ }
+
public boolean hasContent() {
Preconditions.checkNotNull(response, "Response is not initialized.");
return response.getEntity() != null;
diff --git a/java/com/google/gerrit/acceptance/testsuite/account/AccountOperations.java b/java/com/google/gerrit/acceptance/testsuite/account/AccountOperations.java
index 58a00d0..61b7599 100644
--- a/java/com/google/gerrit/acceptance/testsuite/account/AccountOperations.java
+++ b/java/com/google/gerrit/acceptance/testsuite/account/AccountOperations.java
@@ -42,7 +42,7 @@
* <p>Example:
*
* <pre>
- * TestAccount createdAccount = accountOperations
+ * Account.Id createdAccountId = accountOperations
* .newAccount()
* .username("janedoe")
* .preferredEmail("janedoe@example.com")
diff --git a/java/com/google/gerrit/acceptance/testsuite/account/AccountOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/account/AccountOperationsImpl.java
index 94b511b..ebbcfe4 100644
--- a/java/com/google/gerrit/acceptance/testsuite/account/AccountOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/account/AccountOperationsImpl.java
@@ -59,12 +59,12 @@
return TestAccountCreation.builder(this::createAccount);
}
- private TestAccount createAccount(TestAccountCreation accountCreation) throws Exception {
+ private Account.Id createAccount(TestAccountCreation accountCreation) throws Exception {
AccountsUpdate.AccountUpdater accountUpdater =
(account, updateBuilder) ->
fillBuilder(updateBuilder, accountCreation, account.getAccount().getId());
AccountState createdAccount = createAccount(accountUpdater);
- return toTestAccount(createdAccount);
+ return createdAccount.getAccount().getId();
}
private AccountState createAccount(AccountsUpdate.AccountUpdater accountUpdater)
@@ -85,17 +85,6 @@
accountCreation.active().ifPresent(builder::setActive);
}
- private static TestAccount toTestAccount(AccountState accountState) {
- Account createdAccount = accountState.getAccount();
- return TestAccount.builder()
- .accountId(createdAccount.getId())
- .preferredEmail(Optional.ofNullable(createdAccount.getPreferredEmail()))
- .fullname(Optional.ofNullable(createdAccount.getFullName()))
- .username(accountState.getUserName())
- .active(accountState.getAccount().isActive())
- .build();
- }
-
private static InternalAccountUpdate.Builder setPreferredEmail(
InternalAccountUpdate.Builder builder, Account.Id accountId, String preferredEmail) {
return builder
@@ -133,6 +122,17 @@
return toTestAccount(account);
}
+ private TestAccount toTestAccount(AccountState accountState) {
+ Account account = accountState.getAccount();
+ return TestAccount.builder()
+ .accountId(account.getId())
+ .preferredEmail(Optional.ofNullable(account.getPreferredEmail()))
+ .fullname(Optional.ofNullable(account.getFullName()))
+ .username(accountState.getUserName())
+ .active(accountState.getAccount().isActive())
+ .build();
+ }
+
@Override
public TestAccountUpdate.Builder forUpdate() {
return TestAccountUpdate.builder(this::updateAccount);
diff --git a/java/com/google/gerrit/acceptance/testsuite/account/TestAccountCreation.java b/java/com/google/gerrit/acceptance/testsuite/account/TestAccountCreation.java
index a82d180..ab32409 100644
--- a/java/com/google/gerrit/acceptance/testsuite/account/TestAccountCreation.java
+++ b/java/com/google/gerrit/acceptance/testsuite/account/TestAccountCreation.java
@@ -16,6 +16,7 @@
import com.google.auto.value.AutoValue;
import com.google.gerrit.acceptance.testsuite.ThrowingFunction;
+import com.google.gerrit.reviewdb.client.Account;
import java.util.Optional;
@AutoValue
@@ -32,9 +33,9 @@
public abstract Optional<Boolean> active();
- abstract ThrowingFunction<TestAccountCreation, TestAccount> accountCreator();
+ abstract ThrowingFunction<TestAccountCreation, Account.Id> accountCreator();
- public static Builder builder(ThrowingFunction<TestAccountCreation, TestAccount> accountCreator) {
+ public static Builder builder(ThrowingFunction<TestAccountCreation, Account.Id> accountCreator) {
return new AutoValue_TestAccountCreation.Builder()
.accountCreator(accountCreator)
.httpPassword("http-pass");
@@ -83,11 +84,11 @@
}
abstract Builder accountCreator(
- ThrowingFunction<TestAccountCreation, TestAccount> accountCreator);
+ ThrowingFunction<TestAccountCreation, Account.Id> accountCreator);
abstract TestAccountCreation autoBuild();
- public TestAccount create() throws Exception {
+ public Account.Id create() throws Exception {
TestAccountCreation accountUpdate = autoBuild();
return accountUpdate.accountCreator().apply(accountUpdate);
}
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/GroupOperations.java b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperations.java
new file mode 100644
index 0000000..f75ca2e
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperations.java
@@ -0,0 +1,97 @@
+// Copyright (C) 2018 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.testsuite.group;
+
+import com.google.gerrit.reviewdb.client.AccountGroup;
+
+/**
+ * An aggregation of operations on groups for test purposes.
+ *
+ * <p>To execute the operations, no Gerrit permissions are necessary.
+ *
+ * <p><strong>Note:</strong> This interface is not implemented using the REST or extension API.
+ * Hence, it cannot be used for testing those APIs.
+ */
+public interface GroupOperations {
+ /**
+ * Starts the fluent chain for querying or modifying a group. Please see the methods of {@link
+ * MoreGroupOperations} for details on possible operations.
+ *
+ * @return an aggregation of operations on a specific group
+ */
+ MoreGroupOperations group(AccountGroup.UUID groupUuid);
+
+ /**
+ * Starts the fluent chain to create a group. The returned builder can be used to specify the
+ * attributes of the new group. To create the group for real, {@link
+ * TestGroupCreation.Builder#create()} must be called.
+ *
+ * <p>Example:
+ *
+ * <pre>
+ * AccountGroup.UUID createdGroupUuid = groupOperations
+ * .newGroup()
+ * .name("verifiers")
+ * .description("All verifiers of this server")
+ * .create();
+ * </pre>
+ *
+ * <p><strong>Note:</strong> If another group with the provided name already exists, the creation
+ * of the group will fail.
+ *
+ * @return a builder to create the new group
+ */
+ TestGroupCreation.Builder newGroup();
+
+ /** An aggregation of methods on a specific group. */
+ interface MoreGroupOperations {
+
+ /**
+ * Checks whether the group exists.
+ *
+ * @return {@code true} if the group exists
+ */
+ boolean exists() throws Exception;
+
+ /**
+ * Retrieves the group.
+ *
+ * <p><strong>Note:</strong> This call will fail with an exception if the requested group
+ * doesn't exist. If you want to check for the existence of a group, use {@link #exists()}
+ * instead.
+ *
+ * @return the corresponding {@code TestGroup}
+ */
+ TestGroup get() throws Exception;
+
+ /**
+ * Starts the fluent chain to update a group. The returned builder can be used to specify how
+ * the attributes of the group should be modified. To update the group for real, {@link
+ * TestGroupUpdate.Builder#update()} must be called.
+ *
+ * <p>Example:
+ *
+ * <pre>
+ * groupOperations.forUpdate().description("Another description for this group").update();
+ * </pre>
+ *
+ * <p><strong>Note:</strong> The update will fail with an exception if the group to update
+ * doesn't exist. If you want to check for the existence of a group, use {@link #exists()}.
+ *
+ * @return a builder to update the group
+ */
+ TestGroupUpdate.Builder forUpdate();
+ }
+}
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java
new file mode 100644
index 0000000..f9769c5
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImpl.java
@@ -0,0 +1,159 @@
+// Copyright (C) 2018 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.testsuite.group;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.Sequences;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.GroupUUID;
+import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.group.db.Groups;
+import com.google.gerrit.server.group.db.GroupsUpdate;
+import com.google.gerrit.server.group.db.InternalGroupCreation;
+import com.google.gerrit.server.group.db.InternalGroupUpdate;
+import com.google.gwtorm.server.OrmDuplicateKeyException;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.PersonIdent;
+
+/**
+ * The implementation of {@code GroupOperations}.
+ *
+ * <p>There is only one implementation of {@code GroupOperations}. Nevertheless, we keep the
+ * separation between interface and implementation to enhance clarity.
+ */
+public class GroupOperationsImpl implements GroupOperations {
+ private final Groups groups;
+ private final GroupsUpdate groupsUpdate;
+ private final Sequences seq;
+ private final PersonIdent serverIdent;
+
+ @Inject
+ public GroupOperationsImpl(
+ Groups groups,
+ @ServerInitiated GroupsUpdate groupsUpdate,
+ Sequences seq,
+ @GerritPersonIdent PersonIdent serverIdent) {
+ this.groups = groups;
+ this.groupsUpdate = groupsUpdate;
+ this.seq = seq;
+ this.serverIdent = serverIdent;
+ }
+
+ @Override
+ public MoreGroupOperations group(AccountGroup.UUID groupUuid) {
+ return new MoreGroupOperationsImpl(groupUuid);
+ }
+
+ @Override
+ public TestGroupCreation.Builder newGroup() {
+ return TestGroupCreation.builder(this::createNewGroup);
+ }
+
+ private AccountGroup.UUID createNewGroup(TestGroupCreation groupCreation)
+ throws ConfigInvalidException, IOException, OrmException {
+ InternalGroupCreation internalGroupCreation = toInternalGroupCreation(groupCreation);
+ InternalGroupUpdate internalGroupUpdate = toInternalGroupUpdate(groupCreation);
+ InternalGroup internalGroup =
+ groupsUpdate.createGroup(internalGroupCreation, internalGroupUpdate);
+ return internalGroup.getGroupUUID();
+ }
+
+ private InternalGroupCreation toInternalGroupCreation(TestGroupCreation groupCreation)
+ throws OrmException {
+ AccountGroup.Id groupId = new AccountGroup.Id(seq.nextGroupId());
+ String groupName = groupCreation.name().orElse("group-with-id-" + groupId.get());
+ AccountGroup.UUID groupUuid = GroupUUID.make(groupName, serverIdent);
+ AccountGroup.NameKey nameKey = new AccountGroup.NameKey(groupName);
+ return InternalGroupCreation.builder()
+ .setId(groupId)
+ .setGroupUUID(groupUuid)
+ .setNameKey(nameKey)
+ .build();
+ }
+
+ private static InternalGroupUpdate toInternalGroupUpdate(TestGroupCreation groupCreation) {
+ InternalGroupUpdate.Builder builder = InternalGroupUpdate.builder();
+ groupCreation.description().ifPresent(builder::setDescription);
+ groupCreation.ownerGroupUuid().ifPresent(builder::setOwnerGroupUUID);
+ groupCreation.visibleToAll().ifPresent(builder::setVisibleToAll);
+ builder.setMemberModification(originalMembers -> groupCreation.members());
+ builder.setSubgroupModification(originalSubgroups -> groupCreation.subgroups());
+ return builder.build();
+ }
+
+ private class MoreGroupOperationsImpl implements MoreGroupOperations {
+ private final AccountGroup.UUID groupUuid;
+
+ MoreGroupOperationsImpl(AccountGroup.UUID groupUuid) {
+ this.groupUuid = groupUuid;
+ }
+
+ @Override
+ public boolean exists() throws Exception {
+ return groups.getGroup(groupUuid).isPresent();
+ }
+
+ @Override
+ public TestGroup get() throws Exception {
+ Optional<InternalGroup> group = groups.getGroup(groupUuid);
+ checkState(group.isPresent(), "Tried to get non-existing test group");
+ return toTestGroup(group.get());
+ }
+
+ private TestGroup toTestGroup(InternalGroup internalGroup) {
+ return TestGroup.builder()
+ .groupUuid(internalGroup.getGroupUUID())
+ .groupId(internalGroup.getId())
+ .nameKey(internalGroup.getNameKey())
+ .description(Optional.ofNullable(internalGroup.getDescription()))
+ .ownerGroupUuid(internalGroup.getOwnerGroupUUID())
+ .visibleToAll(internalGroup.isVisibleToAll())
+ .createdOn(internalGroup.getCreatedOn())
+ .members(internalGroup.getMembers())
+ .subgroups(internalGroup.getSubgroups())
+ .build();
+ }
+
+ @Override
+ public TestGroupUpdate.Builder forUpdate() {
+ return TestGroupUpdate.builder(this::updateGroup);
+ }
+
+ private void updateGroup(TestGroupUpdate groupUpdate)
+ throws OrmDuplicateKeyException, NoSuchGroupException, ConfigInvalidException, IOException {
+ InternalGroupUpdate internalGroupUpdate = toInternalGroupUpdate(groupUpdate);
+ groupsUpdate.updateGroup(groupUuid, internalGroupUpdate);
+ }
+
+ private InternalGroupUpdate toInternalGroupUpdate(TestGroupUpdate groupUpdate) {
+ InternalGroupUpdate.Builder builder = InternalGroupUpdate.builder();
+ groupUpdate.name().map(AccountGroup.NameKey::new).ifPresent(builder::setName);
+ groupUpdate.description().ifPresent(builder::setDescription);
+ groupUpdate.ownerGroupUuid().ifPresent(builder::setOwnerGroupUUID);
+ groupUpdate.visibleToAll().ifPresent(builder::setVisibleToAll);
+ builder.setMemberModification(groupUpdate.memberModification()::apply);
+ builder.setSubgroupModification(groupUpdate.subgroupModification()::apply);
+ return builder.build();
+ }
+ }
+}
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/TestGroup.java b/java/com/google/gerrit/acceptance/testsuite/group/TestGroup.java
new file mode 100644
index 0000000..b450304
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/TestGroup.java
@@ -0,0 +1,78 @@
+// Copyright (C) 2018 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.testsuite.group;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.sql.Timestamp;
+import java.util.Optional;
+
+@AutoValue
+public abstract class TestGroup {
+
+ public abstract AccountGroup.UUID groupUuid();
+
+ public abstract AccountGroup.Id groupId();
+
+ public String name() {
+ return nameKey().get();
+ }
+
+ public abstract AccountGroup.NameKey nameKey();
+
+ public abstract Optional<String> description();
+
+ public abstract AccountGroup.UUID ownerGroupUuid();
+
+ public abstract boolean visibleToAll();
+
+ public abstract Timestamp createdOn();
+
+ public abstract ImmutableSet<Account.Id> members();
+
+ public abstract ImmutableSet<AccountGroup.UUID> subgroups();
+
+ static Builder builder() {
+ return new AutoValue_TestGroup.Builder();
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+
+ public abstract Builder groupUuid(AccountGroup.UUID groupUuid);
+
+ public abstract Builder groupId(AccountGroup.Id id);
+
+ public abstract Builder nameKey(AccountGroup.NameKey name);
+
+ public abstract Builder description(String description);
+
+ public abstract Builder description(Optional<String> description);
+
+ public abstract Builder ownerGroupUuid(AccountGroup.UUID ownerGroupUuid);
+
+ public abstract Builder visibleToAll(boolean visibleToAll);
+
+ public abstract Builder createdOn(Timestamp createdOn);
+
+ public abstract Builder members(ImmutableSet<Account.Id> members);
+
+ public abstract Builder subgroups(ImmutableSet<AccountGroup.UUID> subgroups);
+
+ abstract TestGroup build();
+ }
+}
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/TestGroupCreation.java b/java/com/google/gerrit/acceptance/testsuite/group/TestGroupCreation.java
new file mode 100644
index 0000000..efed720
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/TestGroupCreation.java
@@ -0,0 +1,112 @@
+// Copyright (C) 2018 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.testsuite.group;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.gerrit.acceptance.testsuite.ThrowingFunction;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.util.Optional;
+import java.util.Set;
+
+@AutoValue
+public abstract class TestGroupCreation {
+
+ public abstract Optional<String> name();
+
+ public abstract Optional<String> description();
+
+ public abstract Optional<AccountGroup.UUID> ownerGroupUuid();
+
+ public abstract Optional<Boolean> visibleToAll();
+
+ public abstract ImmutableSet<Account.Id> members();
+
+ public abstract ImmutableSet<AccountGroup.UUID> subgroups();
+
+ abstract ThrowingFunction<TestGroupCreation, AccountGroup.UUID> groupCreator();
+
+ public static Builder builder(
+ ThrowingFunction<TestGroupCreation, AccountGroup.UUID> groupCreator) {
+ return new AutoValue_TestGroupCreation.Builder().groupCreator(groupCreator);
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder name(String name);
+
+ public abstract Builder description(String description);
+
+ public Builder clearDescription() {
+ return description("");
+ }
+
+ public abstract Builder ownerGroupUuid(AccountGroup.UUID ownerGroupUuid);
+
+ public abstract Builder visibleToAll(boolean visibleToAll);
+
+ public Builder clearMembers() {
+ return members(ImmutableSet.of());
+ }
+
+ public Builder members(Account.Id member1, Account.Id... otherMembers) {
+ return members(Sets.union(ImmutableSet.of(member1), ImmutableSet.copyOf(otherMembers)));
+ }
+
+ public abstract Builder members(Set<Account.Id> members);
+
+ abstract ImmutableSet.Builder<Account.Id> membersBuilder();
+
+ public Builder addMember(Account.Id member) {
+ membersBuilder().add(member);
+ return this;
+ }
+
+ public Builder clearSubgroups() {
+ return subgroups(ImmutableSet.of());
+ }
+
+ public Builder subgroups(AccountGroup.UUID subgroup1, AccountGroup.UUID... otherSubgroups) {
+ return subgroups(Sets.union(ImmutableSet.of(subgroup1), ImmutableSet.copyOf(otherSubgroups)));
+ }
+
+ public abstract Builder subgroups(Set<AccountGroup.UUID> subgroups);
+
+ abstract ImmutableSet.Builder<AccountGroup.UUID> subgroupsBuilder();
+
+ public Builder addSubgroup(AccountGroup.UUID subgroup) {
+ subgroupsBuilder().add(subgroup);
+ return this;
+ }
+
+ abstract Builder groupCreator(
+ ThrowingFunction<TestGroupCreation, AccountGroup.UUID> groupCreator);
+
+ abstract TestGroupCreation autoBuild();
+
+ /**
+ * Executes the group creation as specified.
+ *
+ * @return the UUID of the created group
+ */
+ public AccountGroup.UUID create() throws Exception {
+ TestGroupCreation groupCreation = autoBuild();
+ return groupCreation.groupCreator().apply(groupCreation);
+ }
+ }
+}
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/TestGroupUpdate.java b/java/com/google/gerrit/acceptance/testsuite/group/TestGroupUpdate.java
new file mode 100644
index 0000000..095a270
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/TestGroupUpdate.java
@@ -0,0 +1,134 @@
+// Copyright (C) 2018 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.testsuite.group;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.gerrit.acceptance.testsuite.ThrowingConsumer;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
+
+@AutoValue
+public abstract class TestGroupUpdate {
+
+ public abstract Optional<String> name();
+
+ public abstract Optional<String> description();
+
+ public abstract Optional<AccountGroup.UUID> ownerGroupUuid();
+
+ public abstract Optional<Boolean> visibleToAll();
+
+ public abstract Function<ImmutableSet<Account.Id>, Set<Account.Id>> memberModification();
+
+ public abstract Function<ImmutableSet<AccountGroup.UUID>, Set<AccountGroup.UUID>>
+ subgroupModification();
+
+ abstract ThrowingConsumer<TestGroupUpdate> groupUpdater();
+
+ public static Builder builder(ThrowingConsumer<TestGroupUpdate> groupUpdater) {
+ return new AutoValue_TestGroupUpdate.Builder()
+ .groupUpdater(groupUpdater)
+ .memberModification(in -> in)
+ .subgroupModification(in -> in);
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder name(String name);
+
+ public abstract Builder description(String description);
+
+ public Builder clearDescription() {
+ return description("");
+ }
+
+ public abstract Builder ownerGroupUuid(AccountGroup.UUID ownerGroupUUID);
+
+ public abstract Builder visibleToAll(boolean visibleToAll);
+
+ abstract Builder memberModification(
+ Function<ImmutableSet<Account.Id>, Set<Account.Id>> memberModification);
+
+ abstract Function<ImmutableSet<Account.Id>, Set<Account.Id>> memberModification();
+
+ public Builder clearMembers() {
+ return memberModification(originalMembers -> ImmutableSet.of());
+ }
+
+ public Builder addMember(Account.Id member) {
+ Function<ImmutableSet<Account.Id>, Set<Account.Id>> previousModification =
+ memberModification();
+ memberModification(
+ originalMembers ->
+ Sets.union(previousModification.apply(originalMembers), ImmutableSet.of(member)));
+ return this;
+ }
+
+ public Builder removeMember(Account.Id member) {
+ Function<ImmutableSet<Account.Id>, Set<Account.Id>> previousModification =
+ memberModification();
+ memberModification(
+ originalMembers ->
+ Sets.difference(
+ previousModification.apply(originalMembers), ImmutableSet.of(member)));
+ return this;
+ }
+
+ abstract Builder subgroupModification(
+ Function<ImmutableSet<AccountGroup.UUID>, Set<AccountGroup.UUID>> subgroupModification);
+
+ abstract Function<ImmutableSet<AccountGroup.UUID>, Set<AccountGroup.UUID>>
+ subgroupModification();
+
+ public Builder clearSubgroups() {
+ return subgroupModification(originalMembers -> ImmutableSet.of());
+ }
+
+ public Builder addSubgroup(AccountGroup.UUID subgroup) {
+ Function<ImmutableSet<AccountGroup.UUID>, Set<AccountGroup.UUID>> previousModification =
+ subgroupModification();
+ subgroupModification(
+ originalSubgroups ->
+ Sets.union(previousModification.apply(originalSubgroups), ImmutableSet.of(subgroup)));
+ return this;
+ }
+
+ public Builder removeSubgroup(AccountGroup.UUID subgroup) {
+ Function<ImmutableSet<AccountGroup.UUID>, Set<AccountGroup.UUID>> previousModification =
+ subgroupModification();
+ subgroupModification(
+ originalSubgroups ->
+ Sets.difference(
+ previousModification.apply(originalSubgroups), ImmutableSet.of(subgroup)));
+ return this;
+ }
+
+ abstract Builder groupUpdater(ThrowingConsumer<TestGroupUpdate> groupUpdater);
+
+ abstract TestGroupUpdate autoBuild();
+
+ /** Executes the group update as specified. */
+ public void update() throws Exception {
+ TestGroupUpdate groupUpdater = autoBuild();
+ groupUpdater.groupUpdater().accept(groupUpdater);
+ }
+ }
+}
diff --git a/java/com/google/gerrit/common/data/LabelType.java b/java/com/google/gerrit/common/data/LabelType.java
index ef10c22..ff7d25b 100644
--- a/java/com/google/gerrit/common/data/LabelType.java
+++ b/java/com/google/gerrit/common/data/LabelType.java
@@ -36,6 +36,7 @@
public static final boolean DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE = false;
public static final boolean DEF_COPY_MAX_SCORE = false;
public static final boolean DEF_COPY_MIN_SCORE = false;
+ public static final boolean DEF_IGNORE_SELF_APPROVAL = false;
public static LabelType withDefaultValues(String name) {
checkName(name);
@@ -103,6 +104,7 @@
protected boolean copyAllScoresIfNoCodeChange;
protected boolean copyAllScoresIfNoChange;
protected boolean allowPostSubmit;
+ protected boolean ignoreSelfApproval;
protected short defaultValue;
protected List<LabelValue> values;
@@ -141,6 +143,7 @@
setCopyMaxScore(DEF_COPY_MAX_SCORE);
setCopyMinScore(DEF_COPY_MIN_SCORE);
setAllowPostSubmit(DEF_ALLOW_POST_SUBMIT);
+ setIgnoreSelfApproval(DEF_IGNORE_SELF_APPROVAL);
byValue = new HashMap<>();
for (LabelValue v : values) {
@@ -188,6 +191,14 @@
this.allowPostSubmit = allowPostSubmit;
}
+ public boolean ignoreSelfApproval() {
+ return ignoreSelfApproval;
+ }
+
+ public void setIgnoreSelfApproval(boolean ignoreSelfApproval) {
+ this.ignoreSelfApproval = ignoreSelfApproval;
+ }
+
public void setRefPatterns(List<String> refPatterns) {
if (refPatterns != null) {
this.refPatterns =
diff --git a/java/com/google/gerrit/httpd/BUILD b/java/com/google/gerrit/httpd/BUILD
index 3e71098..fae7c6a 100644
--- a/java/com/google/gerrit/httpd/BUILD
+++ b/java/com/google/gerrit/httpd/BUILD
@@ -16,6 +16,7 @@
"//java/com/google/gerrit/server/audit",
"//java/com/google/gerrit/server/git/receive",
"//java/com/google/gerrit/server/ioutil",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/util/cli",
"//java/com/google/gerrit/util/http",
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 38fe0e2..10f2638 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -110,6 +110,7 @@
import com.google.gerrit.server.cache.PerThreadCache;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.LockFailureException;
+import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -1322,11 +1323,42 @@
}
private TraceContext enableTracing(HttpServletRequest req, HttpServletResponse res) {
- String traceValue = req.getParameter(ParameterParser.TRACE_PARAMETER);
- return TraceContext.newTrace(
- traceValue != null,
- traceValue,
- (tagName, traceId) -> res.setHeader(X_GERRIT_TRACE, traceId.toString()));
+ // There are 2 ways to enable tracing for REST calls:
+ // 1. by using the 'trace' or 'trace=<trace-id>' request parameter
+ // 2. by setting the 'X-Gerrit-Trace:' or 'X-Gerrit-Trace:<trace-id>' header
+ String traceValueFromHeader = req.getHeader(X_GERRIT_TRACE);
+ String traceValueFromRequestParam = req.getParameter(ParameterParser.TRACE_PARAMETER);
+ boolean doTrace = traceValueFromHeader != null || traceValueFromRequestParam != null;
+
+ // Check whether no trace ID, one trace ID or 2 different trace IDs have been specified.
+ String traceId1;
+ String traceId2;
+ if (!Strings.isNullOrEmpty(traceValueFromHeader)) {
+ traceId1 = traceValueFromHeader;
+ if (!Strings.isNullOrEmpty(traceValueFromRequestParam)
+ && !traceValueFromHeader.equals(traceValueFromRequestParam)) {
+ traceId2 = traceValueFromRequestParam;
+ } else {
+ traceId2 = null;
+ }
+ } else {
+ traceId1 = Strings.emptyToNull(traceValueFromRequestParam);
+ traceId2 = null;
+ }
+
+ // Use the first trace ID to start tracing. If this trace ID is null, a trace ID will be
+ // generated.
+ TraceContext traceContext =
+ TraceContext.newTrace(
+ doTrace,
+ traceId1,
+ (tagName, traceId) -> res.setHeader(X_GERRIT_TRACE, traceId.toString()));
+ // If a second trace ID was specified, add a tag for it as well.
+ if (traceId2 != null) {
+ traceContext.addTag(RequestId.Type.TRACE_ID, traceId2);
+ res.addHeader(X_GERRIT_TRACE, traceId2);
+ }
+ return traceContext;
}
private boolean isDelete(HttpServletRequest req) {
diff --git a/java/com/google/gerrit/lucene/BUILD b/java/com/google/gerrit/lucene/BUILD
index 6cb7751..9c6ba74 100644
--- a/java/com/google/gerrit/lucene/BUILD
+++ b/java/com/google/gerrit/lucene/BUILD
@@ -33,6 +33,7 @@
"//java/com/google/gerrit/lifecycle",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/logging",
"//lib:guava",
"//lib:gwtorm",
"//lib/flogger:api",
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index 8d77ed8..683a205 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -75,6 +75,7 @@
import com.google.gerrit.server.query.change.ChangeQueryProcessor;
import com.google.gerrit.server.restapi.group.GroupModule;
import com.google.gerrit.server.rules.DefaultSubmitRule;
+import com.google.gerrit.server.rules.IgnoreSelfApprovalRule;
import com.google.gerrit.server.rules.PrologModule;
import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.update.BatchUpdate;
@@ -188,6 +189,7 @@
factory(SubmitRuleEvaluator.Factory.class);
install(new PrologModule());
install(new DefaultSubmitRule.Module());
+ install(new IgnoreSelfApprovalRule.Module());
bind(ChangeJson.Factory.class).toProvider(Providers.<ChangeJson.Factory>of(null));
bind(EventUtil.class).toProvider(Providers.<EventUtil>of(null));
diff --git a/java/com/google/gerrit/server/BUILD b/java/com/google/gerrit/server/BUILD
index 96fcd39..e5bc480 100644
--- a/java/com/google/gerrit/server/BUILD
+++ b/java/com/google/gerrit/server/BUILD
@@ -42,6 +42,7 @@
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server/cache/serialize",
"//java/com/google/gerrit/server/ioutil",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/util/git",
"//java/com/google/gerrit/util/cli",
"//java/com/google/gerrit/util/ssl",
diff --git a/java/com/google/gerrit/server/cache/h2/BUILD b/java/com/google/gerrit/server/cache/h2/BUILD
index 2ce756b..f6418e3 100644
--- a/java/com/google/gerrit/server/cache/h2/BUILD
+++ b/java/com/google/gerrit/server/cache/h2/BUILD
@@ -9,6 +9,7 @@
"//java/com/google/gerrit/lifecycle",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/cache/serialize",
+ "//java/com/google/gerrit/server/logging",
"//lib:guava",
"//lib:h2",
"//lib/flogger:api",
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index e8c55e8..38c97f7 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -552,8 +552,6 @@
return;
}
- PermissionBackend.ForRef perm =
- permissionBackend.user(ctx.getUser()).project(ctx.getProject()).ref(refName);
try {
try (CommitReceivedEvent event =
new CommitReceivedEvent(
@@ -565,7 +563,7 @@
ctx.getIdentifiedUser())) {
commitValidatorsFactory
.forGerritCommits(
- perm,
+ permissionBackend.user(ctx.getUser()).project(ctx.getProject()),
new Branch.NameKey(ctx.getProject(), refName),
ctx.getIdentifiedUser(),
new NoSshInfo(),
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index e02f666..173d1da 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -1405,8 +1405,7 @@
out.commitWithFooters =
mergeUtilFactory
.create(projectCache.get(project))
- .createCommitMessageOnSubmit(
- commit, mergeTip, cd.notes(), userProvider.get(), in.getId());
+ .createCommitMessageOnSubmit(commit, mergeTip, cd.notes(), in.getId());
}
}
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index d71a93d..8bd6c17 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -323,9 +323,6 @@
return;
}
- PermissionBackend.ForRef perm =
- permissionBackend.user(ctx.getUser()).ref(origNotes.getChange().getDest());
-
String refName = getPatchSetId().toRefName();
try (CommitReceivedEvent event =
new CommitReceivedEvent(
@@ -340,7 +337,7 @@
ctx.getIdentifiedUser())) {
commitValidatorsFactory
.forGerritCommits(
- perm,
+ permissionBackend.user(ctx.getUser()).project(ctx.getProject()),
origNotes.getChange().getDest(),
ctx.getIdentifiedUser(),
new NoSshInfo(),
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 909ea3a..1f216f0 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -165,8 +165,7 @@
rw.parseBody(baseCommit);
newCommitMessage =
newMergeUtil()
- .createCommitMessageOnSubmit(
- original, baseCommit, notes, changeOwner, originalPatchSet.getId());
+ .createCommitMessageOnSubmit(original, baseCommit, notes, originalPatchSet.getId());
} else {
newCommitMessage = original.getFullMessage();
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index b6a257b..0761d2e 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -170,6 +170,7 @@
import com.google.gerrit.server.restapi.change.SuggestReviewers;
import com.google.gerrit.server.restapi.group.GroupModule;
import com.google.gerrit.server.rules.DefaultSubmitRule;
+import com.google.gerrit.server.rules.IgnoreSelfApprovalRule;
import com.google.gerrit.server.rules.PrologModule;
import com.google.gerrit.server.rules.RulesCache;
import com.google.gerrit.server.rules.SubmitRule;
@@ -244,6 +245,7 @@
install(new NoteDbModule(cfg));
install(new PrologModule());
install(new DefaultSubmitRule.Module());
+ install(new IgnoreSelfApprovalRule.Module());
install(new ReceiveCommitsModule());
install(new SshAddressesModule());
install(ThreadLocalRequestContext.module());
diff --git a/java/com/google/gerrit/server/git/MergeUtil.java b/java/com/google/gerrit/server/git/MergeUtil.java
index 686be19..c035269 100644
--- a/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/java/com/google/gerrit/server/git/MergeUtil.java
@@ -41,7 +41,6 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -315,12 +314,10 @@
*
* @param n
* @param notes
- * @param user
* @param psId
* @return new message
*/
- private String createDetailedCommitMessage(
- RevCommit n, ChangeNotes notes, CurrentUser user, PatchSet.Id psId) {
+ private String createDetailedCommitMessage(RevCommit n, ChangeNotes notes, PatchSet.Id psId) {
Change c = notes.getChange();
final List<FooterLine> footers = n.getFooterLines();
final StringBuilder msgbuf = new StringBuilder();
@@ -424,12 +421,7 @@
}
public String createCommitMessageOnSubmit(CodeReviewCommit n, RevCommit mergeTip) {
- return createCommitMessageOnSubmit(
- n,
- mergeTip,
- n.notes(),
- identifiedUserFactory.create(n.notes().getChange().getOwner()),
- n.getPatchsetId());
+ return createCommitMessageOnSubmit(n, mergeTip, n.notes(), n.getPatchsetId());
}
/**
@@ -442,14 +434,13 @@
* @param n
* @param mergeTip
* @param notes
- * @param user
* @param id
* @return new message
*/
public String createCommitMessageOnSubmit(
- RevCommit n, RevCommit mergeTip, ChangeNotes notes, CurrentUser user, Id id) {
+ RevCommit n, RevCommit mergeTip, ChangeNotes notes, Id id) {
return commitMessageGenerator.generate(
- n, mergeTip, notes.getChange().getDest(), createDetailedCommitMessage(n, notes, user, id));
+ n, mergeTip, notes.getChange().getDest(), createDetailedCommitMessage(n, notes, id));
}
private static boolean isCodeReview(LabelId id) {
diff --git a/java/com/google/gerrit/server/git/receive/BUILD b/java/com/google/gerrit/server/git/receive/BUILD
index fddb9d6..f1c604b 100644
--- a/java/com/google/gerrit/server/git/receive/BUILD
+++ b/java/com/google/gerrit/server/git/receive/BUILD
@@ -8,6 +8,7 @@
"//java/com/google/gerrit/extensions:api",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/util/cli",
"//lib:args4j",
"//lib:guava",
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index e9093cc..438438c 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -582,7 +582,7 @@
tracePushOption.isPresent(),
tracePushOption.orElse(null),
(tagName, traceId) -> addMessage(tagName + ": " + traceId))) {
- traceContext.addTag(RequestId.Type.RECEIVE_ID, RequestId.forProject(project.getNameKey()));
+ traceContext.addTag(RequestId.Type.RECEIVE_ID, new RequestId(project.getNameKey().get()));
// Log the push options here, rather than in parsePushOptions(), so that they are included
// into the trace if tracing is enabled.
@@ -1911,13 +1911,15 @@
}
try {
+ NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, receivePack.getRevWalk());
if (validCommit(
receivePack.getRevWalk().getObjectReader(),
changeEnt.getDest(),
cmd,
newCommit,
false,
- changeEnt)) {
+ changeEnt,
+ rejectCommits)) {
logger.atFine().log("Replacing change %s", changeEnt.getId());
requestReplace(cmd, true, changeEnt, newCommit);
}
@@ -1966,6 +1968,10 @@
}
}
+ private NoteMap loadRejectCommits() throws IOException {
+ return BanCommit.loadRejectCommitsMap(repo, receivePack.getRevWalk());
+ }
+
private List<CreateRequest> selectNewAndReplacedChangesFromMagicBranch(Task newProgress) {
logger.atFine().log("Finding new and replaced changes");
List<CreateRequest> newChanges = new ArrayList<>();
@@ -2079,7 +2085,8 @@
magicBranch.cmd,
c,
magicBranch.merged,
- null)) {
+ null,
+ loadRejectCommits())) {
// Not a change the user can propose? Abort as early as possible.
logger.atFine().log("Aborting early due to invalid commit");
return Collections.emptyList();
@@ -3043,6 +3050,8 @@
walk.reset();
walk.sort(RevSort.NONE);
try {
+ NoteMap rejectCommits = loadRejectCommits();
+
RevObject parsedObject = walk.parseAny(cmd.getNewId());
if (!(parsedObject instanceof RevCommit)) {
return;
@@ -3065,7 +3074,7 @@
continue;
}
- if (!validCommit(walk.getObjectReader(), branch, cmd, c, false, null)) {
+ if (!validCommit(walk.getObjectReader(), branch, cmd, c, false, null, rejectCommits)) {
break;
}
@@ -3102,9 +3111,9 @@
ReceiveCommand cmd,
RevCommit commit,
boolean isMerged,
- @Nullable Change change)
+ @Nullable Change change,
+ NoteMap rejectCommits)
throws IOException {
- PermissionBackend.ForRef perm = permissions.ref(branch.get());
ValidCommitKey key = new AutoValue_ReceiveCommits_ValidCommitKey(commit.copy(), branch);
if (validCommits.contains(key)) {
@@ -3113,18 +3122,21 @@
try (CommitReceivedEvent receiveEvent =
new CommitReceivedEvent(cmd, project, branch.get(), objectReader, commit, user)) {
- CommitValidators validators =
- isMerged
- ? commitValidatorsFactory.forMergedCommits(
- project.getNameKey(), perm, user.asIdentifiedUser())
- : commitValidatorsFactory.forReceiveCommits(
- perm,
- branch,
- user.asIdentifiedUser(),
- sshInfo,
- repo,
- receiveEvent.revWalk,
- change);
+ CommitValidators validators;
+ if (isMerged) {
+ validators =
+ commitValidatorsFactory.forMergedCommits(permissions, branch, user.asIdentifiedUser());
+ } else {
+ validators =
+ commitValidatorsFactory.forReceiveCommits(
+ permissions,
+ branch,
+ user.asIdentifiedUser(),
+ sshInfo,
+ rejectCommits,
+ receiveEvent.revWalk,
+ change);
+ }
for (CommitValidationMessage m : validators.validate(receiveEvent)) {
messages.add(
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 3e382fb..bcb910a 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -35,7 +35,6 @@
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Branch.NameKey;
import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -45,11 +44,9 @@
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.events.CommitReceivedEvent;
-import com.google.gerrit.server.git.BanCommit;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.ValidationError;
import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ProjectCache;
@@ -123,15 +120,15 @@
}
public CommitValidators forReceiveCommits(
- PermissionBackend.ForRef perm,
+ PermissionBackend.ForProject forProject,
Branch.NameKey branch,
IdentifiedUser user,
SshInfo sshInfo,
- Repository repo,
+ NoteMap rejectCommits,
RevWalk rw,
@Nullable Change change)
throws IOException {
- NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
+ PermissionBackend.ForRef perm = forProject.ref(branch.get());
ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
return new CommitValidators(
ImmutableList.of(
@@ -157,13 +154,14 @@
}
public CommitValidators forGerritCommits(
- ForRef perm,
+ PermissionBackend.ForProject forProject,
NameKey branch,
IdentifiedUser user,
SshInfo sshInfo,
RevWalk rw,
@Nullable Change change)
throws IOException {
+ PermissionBackend.ForRef perm = forProject.ref(branch.get());
ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
return new CommitValidators(
ImmutableList.of(
@@ -187,7 +185,7 @@
}
public CommitValidators forMergedCommits(
- Project.NameKey project, PermissionBackend.ForRef perm, IdentifiedUser user)
+ PermissionBackend.ForProject forProject, Branch.NameKey branch, IdentifiedUser user)
throws IOException {
// Generally only include validators that are based on permissions of the
// user creating a change for a merged commit; generally exclude
@@ -202,10 +200,11 @@
// discuss what to do about it.
// - Plugin validators may do things like require certain commit message
// formats, so we play it safe and exclude them.
+ PermissionBackend.ForRef perm = forProject.ref(branch.get());
return new CommitValidators(
ImmutableList.of(
new UploadMergesPermissionValidator(perm),
- new ProjectStateValidationListener(projectCache.checkedGet(project)),
+ new ProjectStateValidationListener(projectCache.checkedGet(branch.getParentKey())),
new AuthorUploaderValidator(user, perm, canonicalWebUrl),
new CommitterUploaderValidator(user, perm, canonicalWebUrl)));
}
diff --git a/java/com/google/gerrit/server/logging/BUILD b/java/com/google/gerrit/server/logging/BUILD
new file mode 100644
index 0000000..d3211f0
--- /dev/null
+++ b/java/com/google/gerrit/server/logging/BUILD
@@ -0,0 +1,13 @@
+java_library(
+ name = "logging",
+ srcs = glob(
+ ["**/*.java"],
+ ),
+ visibility = ["//visibility:public"],
+ deps = [
+ "//java/com/google/gerrit/common:annotations",
+ "//java/com/google/gerrit/common:server",
+ "//lib:guava",
+ "//lib/flogger:api",
+ ],
+)
diff --git a/java/com/google/gerrit/server/logging/RequestId.java b/java/com/google/gerrit/server/logging/RequestId.java
index 81619fb..b0a8ad9 100644
--- a/java/com/google/gerrit/server/logging/RequestId.java
+++ b/java/com/google/gerrit/server/logging/RequestId.java
@@ -19,8 +19,6 @@
import com.google.common.hash.Hashing;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.Project;
import java.net.InetAddress;
import java.net.UnknownHostException;
@@ -52,21 +50,13 @@
return LoggingContext.getInstance().getTagsAsMap().keySet().stream().anyMatch(Type::isId);
}
- public static RequestId forChange(Change c) {
- return new RequestId(c.getId().toString());
- }
-
- public static RequestId forProject(Project.NameKey p) {
- return new RequestId(p.toString());
- }
-
private final String str;
public RequestId() {
this(null);
}
- private RequestId(@Nullable String resourceId) {
+ public RequestId(@Nullable String resourceId) {
Hasher h = Hashing.murmur3_128().newHasher();
h.putLong(Thread.currentThread().getId()).putUnencodedChars(MACHINE_ID);
str =
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 5b79ab9..bccc415 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -92,6 +92,7 @@
public static final String KEY_DEFAULT_VALUE = "defaultValue";
public static final String KEY_COPY_MIN_SCORE = "copyMinScore";
public static final String KEY_ALLOW_POST_SUBMIT = "allowPostSubmit";
+ public static final String KEY_IGNORE_SELF_APPROVAL = "ignoreSelfApproval";
public static final String KEY_COPY_MAX_SCORE = "copyMaxScore";
public static final String KEY_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE =
"copyAllScoresOnMergeFirstParentUpdate";
@@ -883,6 +884,8 @@
}
label.setAllowPostSubmit(
rc.getBoolean(LABEL, name, KEY_ALLOW_POST_SUBMIT, LabelType.DEF_ALLOW_POST_SUBMIT));
+ label.setIgnoreSelfApproval(
+ rc.getBoolean(LABEL, name, KEY_IGNORE_SELF_APPROVAL, LabelType.DEF_IGNORE_SELF_APPROVAL));
label.setCopyMinScore(
rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, LabelType.DEF_COPY_MIN_SCORE));
label.setCopyMaxScore(
@@ -1322,6 +1325,13 @@
rc,
LABEL,
name,
+ KEY_IGNORE_SELF_APPROVAL,
+ label.ignoreSelfApproval(),
+ LabelType.DEF_IGNORE_SELF_APPROVAL);
+ setBooleanConfigKey(
+ rc,
+ LABEL,
+ name,
KEY_COPY_MIN_SCORE,
label.isCopyMinScore(),
LabelType.DEF_COPY_MIN_SCORE);
diff --git a/java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java b/java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java
new file mode 100644
index 0000000..b9ddbc6
--- /dev/null
+++ b/java/com/google/gerrit/server/rules/IgnoreSelfApprovalRule.java
@@ -0,0 +1,171 @@
+// Copyright (C) 2018 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.server.rules;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.data.LabelFunction;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRequirement;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+/**
+ * Rule to require an approval from a user that did not upload the current patch set or block
+ * submission.
+ */
+@Singleton
+public class IgnoreSelfApprovalRule implements SubmitRule {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final String E_UNABLE_TO_FETCH_UPLOADER = "Unable to fetch uploader";
+ private static final String E_UNABLE_TO_FETCH_LABELS =
+ "Unable to fetch labels and approvals for the change";
+
+ public static class Module extends FactoryModule {
+ @Override
+ public void configure() {
+ bind(SubmitRule.class)
+ .annotatedWith(Exports.named("IgnoreSelfApprovalRule"))
+ .to(IgnoreSelfApprovalRule.class);
+ }
+ }
+
+ @Inject
+ IgnoreSelfApprovalRule() {}
+
+ @Override
+ public Collection<SubmitRecord> evaluate(ChangeData cd, SubmitRuleOptions options) {
+ List<LabelType> labelTypes;
+ List<PatchSetApproval> approvals;
+ try {
+ labelTypes = cd.getLabelTypes().getLabelTypes();
+ approvals = cd.currentApprovals();
+ } catch (OrmException e) {
+ logger.atWarning().withCause(e).log(E_UNABLE_TO_FETCH_LABELS);
+ return singletonRuleError(E_UNABLE_TO_FETCH_LABELS);
+ }
+
+ boolean shouldIgnoreSelfApproval = labelTypes.stream().anyMatch(l -> l.ignoreSelfApproval());
+ if (!shouldIgnoreSelfApproval) {
+ // Shortcut to avoid further processing if no label should ignore uploader approvals
+ return ImmutableList.of();
+ }
+
+ Account.Id uploader;
+ try {
+ uploader = cd.currentPatchSet().getUploader();
+ } catch (OrmException e) {
+ logger.atWarning().withCause(e).log(E_UNABLE_TO_FETCH_UPLOADER);
+ return singletonRuleError(E_UNABLE_TO_FETCH_UPLOADER);
+ }
+
+ SubmitRecord submitRecord = new SubmitRecord();
+ submitRecord.status = SubmitRecord.Status.OK;
+ submitRecord.labels = new ArrayList<>(labelTypes.size());
+ submitRecord.requirements = new ArrayList<>();
+
+ for (LabelType t : labelTypes) {
+ if (!t.ignoreSelfApproval()) {
+ // The default rules are enough in this case.
+ continue;
+ }
+
+ LabelFunction labelFunction = t.getFunction();
+ if (labelFunction == null) {
+ continue;
+ }
+
+ Collection<PatchSetApproval> allApprovalsForLabel = filterApprovalsByLabel(approvals, t);
+ SubmitRecord.Label allApprovalsCheckResult = labelFunction.check(t, allApprovalsForLabel);
+ SubmitRecord.Label ignoreSelfApprovalCheckResult =
+ labelFunction.check(t, filterOutPositiveApprovalsOfUser(allApprovalsForLabel, uploader));
+
+ if (labelCheckPassed(allApprovalsCheckResult)
+ && !labelCheckPassed(ignoreSelfApprovalCheckResult)) {
+ // The label has a valid approval from the uploader and no other valid approval. Set the
+ // label
+ // to NOT_READY and indicate the need for non-uploader approval as requirement.
+ submitRecord.labels.add(ignoreSelfApprovalCheckResult);
+ submitRecord.status = SubmitRecord.Status.NOT_READY;
+ // Add an additional requirement to be more descriptive on why the label counts as not
+ // approved.
+ submitRecord.requirements.add(
+ SubmitRequirement.builder()
+ .setFallbackText("Approval from non-uploader required")
+ .setType("non_uploader_approval")
+ .build());
+ }
+ }
+
+ if (submitRecord.labels.isEmpty()) {
+ return ImmutableList.of();
+ }
+
+ return ImmutableList.of(submitRecord);
+ }
+
+ private static boolean labelCheckPassed(SubmitRecord.Label label) {
+ switch (label.status) {
+ case OK:
+ case MAY:
+ return true;
+
+ case NEED:
+ case REJECT:
+ case IMPOSSIBLE:
+ return false;
+ }
+ return false;
+ }
+
+ private static Collection<SubmitRecord> singletonRuleError(String reason) {
+ SubmitRecord submitRecord = new SubmitRecord();
+ submitRecord.errorMessage = reason;
+ submitRecord.status = SubmitRecord.Status.RULE_ERROR;
+ return ImmutableList.of(submitRecord);
+ }
+
+ @VisibleForTesting
+ static Collection<PatchSetApproval> filterOutPositiveApprovalsOfUser(
+ Collection<PatchSetApproval> approvals, Account.Id user) {
+ return approvals
+ .stream()
+ .filter(input -> input.getValue() < 0 || !input.getAccountId().equals(user))
+ .collect(toImmutableList());
+ }
+
+ @VisibleForTesting
+ static Collection<PatchSetApproval> filterApprovalsByLabel(
+ Collection<PatchSetApproval> approvals, LabelType t) {
+ return approvals
+ .stream()
+ .filter(input -> input.getLabelId().get().equals(t.getLabelId().get()))
+ .collect(toImmutableList());
+ }
+}
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 9a8895b..43d5f75 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -457,7 +457,7 @@
this.caller = caller;
this.ts = TimeUtil.nowTs();
this.db = db;
- this.submissionId = RequestId.forChange(change);
+ this.submissionId = new RequestId(change.getId().toString());
try (TraceContext traceContext =
TraceContext.open().addTag(RequestId.Type.SUBMISSION_ID, submissionId)) {
diff --git a/java/com/google/gerrit/sshd/BUILD b/java/com/google/gerrit/sshd/BUILD
index 4743b35..47b1d89 100644
--- a/java/com/google/gerrit/sshd/BUILD
+++ b/java/com/google/gerrit/sshd/BUILD
@@ -15,6 +15,7 @@
"//java/com/google/gerrit/server/cache/h2",
"//java/com/google/gerrit/server/git/receive",
"//java/com/google/gerrit/server/ioutil",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/schema",
"//java/com/google/gerrit/util/cli",
diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD
index cf65908..15ceb77 100644
--- a/java/com/google/gerrit/testing/BUILD
+++ b/java/com/google/gerrit/testing/BUILD
@@ -29,6 +29,7 @@
"//java/com/google/gerrit/server/audit",
"//java/com/google/gerrit/server/cache/h2",
"//java/com/google/gerrit/server/cache/mem",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/schema",
"//lib:guava",
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index b362b2b..7126354 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -526,9 +526,9 @@
@Test
public void validateAccountActivation() throws Exception {
- com.google.gerrit.acceptance.testsuite.account.TestAccount activatableAccount =
+ Account.Id activatableAccountId =
accountOperations.newAccount().inactive().preferredEmail("foo@activatable.com").create();
- com.google.gerrit.acceptance.testsuite.account.TestAccount deactivatableAccount =
+ Account.Id deactivatableAccountId =
accountOperations.newAccount().preferredEmail("foo@deactivatable.com").create();
RegistrationHandle registrationHandle =
accountActivationValidationListeners.add(
@@ -554,61 +554,56 @@
/* Test account that can be activated, but not deactivated */
// Deactivate account that is already inactive
try {
- gApi.accounts().id(activatableAccount.accountId().get()).setActive(false);
+ gApi.accounts().id(activatableAccountId.get()).setActive(false);
fail("Expected exception");
} catch (ResourceConflictException e) {
assertThat(e.getMessage()).isEqualTo("account not active");
}
- assertThat(accountOperations.account(activatableAccount.accountId()).get().active())
- .isFalse();
+ assertThat(accountOperations.account(activatableAccountId).get().active()).isFalse();
// Activate account that can be activated
- gApi.accounts().id(activatableAccount.accountId().get()).setActive(true);
- assertThat(accountOperations.account(activatableAccount.accountId()).get().active()).isTrue();
+ gApi.accounts().id(activatableAccountId.get()).setActive(true);
+ assertThat(accountOperations.account(activatableAccountId).get().active()).isTrue();
// Activate account that is already active
- gApi.accounts().id(activatableAccount.accountId().get()).setActive(true);
- assertThat(accountOperations.account(activatableAccount.accountId()).get().active()).isTrue();
+ gApi.accounts().id(activatableAccountId.get()).setActive(true);
+ assertThat(accountOperations.account(activatableAccountId).get().active()).isTrue();
// Try deactivating account that cannot be deactivated
try {
- gApi.accounts().id(activatableAccount.accountId().get()).setActive(false);
+ gApi.accounts().id(activatableAccountId.get()).setActive(false);
fail("Expected exception");
} catch (ResourceConflictException e) {
assertThat(e.getMessage()).isEqualTo("not allowed to deactive account");
}
- assertThat(accountOperations.account(activatableAccount.accountId()).get().active()).isTrue();
+ assertThat(accountOperations.account(activatableAccountId).get().active()).isTrue();
/* Test account that can be deactivated, but not activated */
// Activate account that is already inactive
- gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(true);
- assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active())
- .isTrue();
+ gApi.accounts().id(deactivatableAccountId.get()).setActive(true);
+ assertThat(accountOperations.account(deactivatableAccountId).get().active()).isTrue();
// Deactivate account that can be deactivated
- gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(false);
- assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active())
- .isFalse();
+ gApi.accounts().id(deactivatableAccountId.get()).setActive(false);
+ assertThat(accountOperations.account(deactivatableAccountId).get().active()).isFalse();
// Deactivate account that is already inactive
try {
- gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(false);
+ gApi.accounts().id(deactivatableAccountId.get()).setActive(false);
fail("Expected exception");
} catch (ResourceConflictException e) {
assertThat(e.getMessage()).isEqualTo("account not active");
}
- assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active())
- .isFalse();
+ assertThat(accountOperations.account(deactivatableAccountId).get().active()).isFalse();
// Try activating account that cannot be activated
try {
- gApi.accounts().id(deactivatableAccount.accountId().get()).setActive(true);
+ gApi.accounts().id(deactivatableAccountId.get()).setActive(true);
fail("Expected exception");
} catch (ResourceConflictException e) {
assertThat(e.getMessage()).isEqualTo("not allowed to active account");
}
- assertThat(accountOperations.account(deactivatableAccount.accountId()).get().active())
- .isFalse();
+ assertThat(accountOperations.account(deactivatableAccountId).get().active()).isFalse();
} finally {
registrationHandle.remove();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index fe55115..b3a5e2d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -68,7 +68,6 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
-import com.google.gerrit.acceptance.testsuite.account.TestAccount;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.LabelFunction;
@@ -1688,7 +1687,7 @@
// create a group named "ab" with one user: testUser
String email = "abcd@test.com";
String fullname = "abcd";
- TestAccount testUser =
+ Account.Id accountIdOfTestUser =
accountOperations
.newAccount()
.username("abcd")
@@ -1721,7 +1720,7 @@
Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
- assertThat(reviewers.iterator().next()._accountId).isEqualTo(testUser.accountId().get());
+ assertThat(reviewers.iterator().next()._accountId).isEqualTo(accountIdOfTestUser.get());
// Ensure ETag and lastUpdatedOn are updated.
rsrc = parseResource(r);
@@ -1748,7 +1747,7 @@
String myGroupUserEmail = "lee@test.com";
String myGroupUserFullname = "lee";
- TestAccount myGroupUser =
+ Account.Id accountIdOfGroupUser =
accountOperations
.newAccount()
.username("lee")
@@ -1785,7 +1784,7 @@
Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
- assertThat(reviewers.iterator().next()._accountId).isEqualTo(myGroupUser.accountId().get());
+ assertThat(reviewers.iterator().next()._accountId).isEqualTo(accountIdOfGroupUser.get());
// Ensure ETag and lastUpdatedOn are updated.
rsrc = parseResource(r);
@@ -2215,13 +2214,12 @@
// notify unrelated account as TO
String email = "user2@example.com";
- TestAccount user2 =
- accountOperations
- .newAccount()
- .username("user2")
- .preferredEmail(email)
- .fullname("User2")
- .create();
+ accountOperations
+ .newAccount()
+ .username("user2")
+ .preferredEmail(email)
+ .fullname("User2")
+ .create();
setApiUser(user);
recommend(r.getChangeId());
setApiUser(admin);
@@ -2229,7 +2227,7 @@
in.notifyDetails = new HashMap<>();
in.notifyDetails.put(RecipientType.TO, new NotifyInfo(ImmutableList.of(email)));
gApi.changes().id(r.getChangeId()).reviewer(user.getId().toString()).deleteVote(in);
- assertNotifyTo(user2);
+ assertNotifyTo(email, "User2");
// notify unrelated account as CC
setApiUser(user);
@@ -2239,7 +2237,7 @@
in.notifyDetails = new HashMap<>();
in.notifyDetails.put(RecipientType.CC, new NotifyInfo(ImmutableList.of(email)));
gApi.changes().id(r.getChangeId()).reviewer(user.getId().toString()).deleteVote(in);
- assertNotifyCc(user2);
+ assertNotifyCc(email, "User2");
// notify unrelated account as BCC
setApiUser(user);
@@ -2249,7 +2247,7 @@
in.notifyDetails = new HashMap<>();
in.notifyDetails.put(RecipientType.BCC, new NotifyInfo(ImmutableList.of(email)));
gApi.changes().id(r.getChangeId()).reviewer(user.getId().toString()).deleteVote(in);
- assertNotifyBcc(user2);
+ assertNotifyBcc(email, "User2");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 8f1bdbc..d6be960 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -183,24 +183,23 @@
@Test
public void cachedGroupsForMemberAreUpdatedOnMemberAdditionAndRemoval() throws Exception {
String username = name("user");
- com.google.gerrit.acceptance.testsuite.account.TestAccount account =
- accountOperations.newAccount().username(username).create();
+ Account.Id accountId = accountOperations.newAccount().username(username).create();
// Fill the cache for the observed account.
- groupIncludeCache.getGroupsWithMember(account.accountId());
+ groupIncludeCache.getGroupsWithMember(accountId);
String groupName = createGroup("users");
AccountGroup.UUID groupUuid = new AccountGroup.UUID(gApi.groups().id(groupName).get().id);
gApi.groups().id(groupName).addMembers(username);
Collection<AccountGroup.UUID> groupsWithMemberAfterAddition =
- groupIncludeCache.getGroupsWithMember(account.accountId());
+ groupIncludeCache.getGroupsWithMember(accountId);
assertThat(groupsWithMemberAfterAddition).contains(groupUuid);
gApi.groups().id(groupName).removeMembers(username);
Collection<AccountGroup.UUID> groupsWithMemberAfterRemoval =
- groupIncludeCache.getGroupsWithMember(account.accountId());
+ groupIncludeCache.getGroupsWithMember(accountId);
assertThat(groupsWithMemberAfterRemoval).doesNotContain(groupUuid);
}
@@ -411,19 +410,17 @@
@Test
public void cachedGroupsForMemberAreUpdatedOnGroupCreation() throws Exception {
- com.google.gerrit.acceptance.testsuite.account.TestAccount account =
- accountOperations.newAccount().create();
+ Account.Id accountId = accountOperations.newAccount().create();
// Fill the cache for the observed account.
- groupIncludeCache.getGroupsWithMember(account.accountId());
+ groupIncludeCache.getGroupsWithMember(accountId);
GroupInput groupInput = new GroupInput();
groupInput.name = name("Users");
- groupInput.members = ImmutableList.of(String.valueOf(account.accountId().get()));
+ groupInput.members = ImmutableList.of(String.valueOf(accountId.get()));
GroupInfo group = gApi.groups().create(groupInput).get();
- Collection<AccountGroup.UUID> groups =
- groupIncludeCache.getGroupsWithMember(account.accountId());
+ Collection<AccountGroup.UUID> groups = groupIncludeCache.getGroupsWithMember(accountId);
assertThat(groups).containsExactly(new AccountGroup.UUID(group.id));
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java b/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
index 2b1416a..e4194a3 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
@@ -20,16 +20,18 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.config.AccessCheckInfo;
import com.google.gerrit.extensions.api.config.AccessCheckInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.inject.Inject;
import java.util.List;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
@@ -39,35 +41,29 @@
public class CheckAccessIT extends AbstractDaemonTest {
+ @Inject private GroupOperations groupOperations;
+
private Project.NameKey normalProject;
private Project.NameKey secretProject;
private Project.NameKey secretRefProject;
private TestAccount privilegedUser;
- private InternalGroup privilegedGroup;
@Before
public void setUp() throws Exception {
normalProject = createProject("normal");
secretProject = createProject("secret");
secretRefProject = createProject("secretRef");
- privilegedGroup = group(createGroup("privilegedGroup"));
+ AccountGroup.UUID privilegedGroupUuid =
+ groupOperations.newGroup().name(name("privilegedGroup")).create();
privilegedUser = accountCreator.create("privilegedUser", "snowden@nsa.gov", "Ed Snowden");
- gApi.groups().id(privilegedGroup.getGroupUUID().get()).addMembers(privilegedUser.username);
+ groupOperations.group(privilegedGroupUuid).forUpdate().addMember(privilegedUser.id).update();
- assertThat(gApi.groups().id(privilegedGroup.getGroupUUID().get()).members().get(0).email)
- .contains("snowden");
-
- grant(secretProject, "refs/*", Permission.READ, false, privilegedGroup.getGroupUUID());
+ grant(secretProject, "refs/*", Permission.READ, false, privilegedGroupUuid);
block(secretProject, "refs/*", Permission.READ, SystemGroupBackend.REGISTERED_USERS);
deny(secretRefProject, "refs/*", Permission.READ, SystemGroupBackend.ANONYMOUS_USERS);
- grant(
- secretRefProject,
- "refs/heads/secret/*",
- Permission.READ,
- false,
- privilegedGroup.getGroupUUID());
+ grant(secretRefProject, "refs/heads/secret/*", Permission.READ, false, privilegedGroupUuid);
block(
secretRefProject,
"refs/heads/secret/*",
@@ -81,13 +77,8 @@
SystemGroupBackend.REGISTERED_USERS);
// Ref permission
- grant(
- normalProject,
- "refs/*",
- Permission.VIEW_PRIVATE_CHANGES,
- false,
- privilegedGroup.getGroupUUID());
- grant(normalProject, "refs/*", Permission.FORGE_SERVER, false, privilegedGroup.getGroupUUID());
+ grant(normalProject, "refs/*", Permission.VIEW_PRIVATE_CHANGES, false, privilegedGroupUuid);
+ grant(normalProject, "refs/*", Permission.FORGE_SERVER, false, privilegedGroupUuid);
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/rest/BUILD b/javatests/com/google/gerrit/acceptance/rest/BUILD
index b4940bc..b94a98d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/BUILD
+++ b/javatests/com/google/gerrit/acceptance/rest/BUILD
@@ -4,7 +4,10 @@
srcs = glob(["*IT.java"]),
group = "rest_bindings",
labels = ["rest"],
- deps = [":util"],
+ deps = [
+ ":util",
+ "//java/com/google/gerrit/server/logging",
+ ],
)
java_library(
diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
index b48d47e..4de54e3 100644
--- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java
@@ -18,6 +18,7 @@
import static org.apache.http.HttpStatus.SC_CREATED;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -36,6 +37,7 @@
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import java.util.List;
+import org.apache.http.message.BasicHeader;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -71,29 +73,90 @@
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
assertThat(projectCreationListener.traceId).isNull();
- assertThat(projectCreationListener.foundTraceId).isFalse();
assertThat(projectCreationListener.isLoggingForced).isFalse();
}
@Test
- public void restCallWithTrace() throws Exception {
+ public void restCallWithTraceRequestParam() throws Exception {
RestResponse response =
adminRestSession.put("/projects/new2?" + ParameterParser.TRACE_PARAMETER);
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull();
assertThat(projectCreationListener.traceId).isNotNull();
- assertThat(projectCreationListener.foundTraceId).isTrue();
assertThat(projectCreationListener.isLoggingForced).isTrue();
}
@Test
- public void restCallWithTraceAndProvidedTraceId() throws Exception {
+ public void restCallWithTraceRequestParamAndProvidedTraceId() throws Exception {
RestResponse response =
adminRestSession.put("/projects/new3?" + ParameterParser.TRACE_PARAMETER + "=issue/123");
assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
- assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull();
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
- assertThat(projectCreationListener.foundTraceId).isTrue();
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+ }
+
+ @Test
+ public void restCallWithTraceHeader() throws Exception {
+ RestResponse response =
+ adminRestSession.putWithHeader(
+ "/projects/new4", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, null));
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNotNull();
+ assertThat(projectCreationListener.traceId).isNotNull();
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+ }
+
+ @Test
+ public void restCallWithTraceHeaderAndProvidedTraceId() throws Exception {
+ RestResponse response =
+ adminRestSession.putWithHeader(
+ "/projects/new5", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/123"));
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+ assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+ }
+
+ @Test
+ public void restCallWithTraceRequestParamAndTraceHeader() throws Exception {
+ // trace ID only specified by trace header
+ RestResponse response =
+ adminRestSession.putWithHeader(
+ "/projects/new6?trace", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/123"));
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+ assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+
+ // trace ID only specified by trace request parameter
+ response =
+ adminRestSession.putWithHeader(
+ "/projects/new7?trace=issue/123", new BasicHeader(RestApiServlet.X_GERRIT_TRACE, null));
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+ assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+
+ // same trace ID specified by trace header and trace request parameter
+ response =
+ adminRestSession.putWithHeader(
+ "/projects/new8?trace=issue/123",
+ new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/123"));
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isEqualTo("issue/123");
+ assertThat(projectCreationListener.traceId).isEqualTo("issue/123");
+ assertThat(projectCreationListener.isLoggingForced).isTrue();
+
+ // different trace IDs specified by trace header and trace request parameter
+ response =
+ adminRestSession.putWithHeader(
+ "/projects/new9?trace=issue/123",
+ new BasicHeader(RestApiServlet.X_GERRIT_TRACE, "issue/456"));
+ assertThat(response.getStatusCode()).isEqualTo(SC_CREATED);
+ assertThat(response.getHeaders(RestApiServlet.X_GERRIT_TRACE))
+ .containsExactly("issue/123", "issue/456");
+ assertThat(projectCreationListener.traceIds).containsExactly("issue/123", "issue/456");
assertThat(projectCreationListener.isLoggingForced).isTrue();
}
@@ -103,7 +166,6 @@
PushOneCommit.Result r = push.to("refs/heads/master");
r.assertOkStatus();
assertThat(commitValidationListener.traceId).isNull();
- assertThat(commitValidationListener.foundTraceId).isFalse();
assertThat(commitValidationListener.isLoggingForced).isFalse();
}
@@ -114,7 +176,6 @@
PushOneCommit.Result r = push.to("refs/heads/master");
r.assertOkStatus();
assertThat(commitValidationListener.traceId).isNotNull();
- assertThat(commitValidationListener.foundTraceId).isTrue();
assertThat(commitValidationListener.isLoggingForced).isTrue();
}
@@ -125,7 +186,6 @@
PushOneCommit.Result r = push.to("refs/heads/master");
r.assertOkStatus();
assertThat(commitValidationListener.traceId).isEqualTo("issue/123");
- assertThat(commitValidationListener.foundTraceId).isTrue();
assertThat(commitValidationListener.isLoggingForced).isTrue();
}
@@ -135,7 +195,6 @@
PushOneCommit.Result r = push.to("refs/for/master");
r.assertOkStatus();
assertThat(commitValidationListener.traceId).isNull();
- assertThat(commitValidationListener.foundTraceId).isFalse();
assertThat(commitValidationListener.isLoggingForced).isFalse();
}
@@ -146,7 +205,6 @@
PushOneCommit.Result r = push.to("refs/for/master");
r.assertOkStatus();
assertThat(commitValidationListener.traceId).isNotNull();
- assertThat(commitValidationListener.foundTraceId).isTrue();
assertThat(commitValidationListener.isLoggingForced).isTrue();
}
@@ -157,28 +215,26 @@
PushOneCommit.Result r = push.to("refs/for/master");
r.assertOkStatus();
assertThat(commitValidationListener.traceId).isEqualTo("issue/123");
- assertThat(commitValidationListener.foundTraceId).isTrue();
assertThat(commitValidationListener.isLoggingForced).isTrue();
}
private static class TraceValidatingProjectCreationValidationListener
implements ProjectCreationValidationListener {
String traceId;
- Boolean foundTraceId;
+ ImmutableSet<String> traceIds;
Boolean isLoggingForced;
@Override
public void validateNewProject(CreateProjectArgs args) throws ValidationException {
this.traceId =
Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null);
- this.foundTraceId = traceId != null;
+ this.traceIds = LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID");
this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false);
}
}
private static class TraceValidatingCommitValidationListener implements CommitValidationListener {
String traceId;
- Boolean foundTraceId;
Boolean isLoggingForced;
@Override
@@ -186,7 +242,6 @@
throws CommitValidationException {
this.traceId =
Iterables.getFirst(LoggingContext.getInstance().getTagsAsMap().get("TRACE_ID"), null);
- this.foundTraceId = traceId != null;
this.isLoggingForced = LoggingContext.getInstance().shouldForceLogging(null, null, false);
return ImmutableList.of();
}
diff --git a/javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java b/javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java
new file mode 100644
index 0000000..d1b05e3
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/rules/IgnoreSelfApprovalRuleIT.java
@@ -0,0 +1,99 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.server.rules;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.common.data.SubmitRequirement;
+import com.google.gerrit.server.project.SubmitRuleOptions;
+import com.google.gerrit.server.rules.IgnoreSelfApprovalRule;
+import com.google.inject.Inject;
+import java.util.Collection;
+import java.util.Map;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.junit.Test;
+
+@NoHttpd
+public class IgnoreSelfApprovalRuleIT extends AbstractDaemonTest {
+ @Inject private IgnoreSelfApprovalRule rule;
+
+ @Test
+ public void blocksWhenUploaderIsOnlyApprover() throws Exception {
+ enableRule("Code-Review", true);
+
+ PushOneCommit.Result r = createChange();
+ approve(r.getChangeId());
+
+ Collection<SubmitRecord> submitRecords =
+ rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
+
+ assertThat(submitRecords).hasSize(1);
+ SubmitRecord result = submitRecords.iterator().next();
+ assertThat(result.status).isEqualTo(SubmitRecord.Status.NOT_READY);
+ assertThat(result.labels).isNotEmpty();
+ assertThat(result.requirements)
+ .containsExactly(
+ SubmitRequirement.builder()
+ .setFallbackText("Approval from non-uploader required")
+ .setType("non_uploader_approval")
+ .build());
+ }
+
+ @Test
+ public void allowsSubmissionWhenChangeHasNonUploaderApproval() throws Exception {
+ enableRule("Code-Review", true);
+
+ // Create change as user
+ TestRepository<InMemoryRepository> userTestRepo = cloneProject(project, user);
+ PushOneCommit push = pushFactory.create(db, user.getIdent(), userTestRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+
+ // Approve as admin
+ approve(r.getChangeId());
+
+ Collection<SubmitRecord> submitRecords =
+ rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
+ assertThat(submitRecords).isEmpty();
+ }
+
+ @Test
+ public void doesNothingByDefault() throws Exception {
+ enableRule("Code-Review", false);
+
+ PushOneCommit.Result r = createChange();
+ approve(r.getChangeId());
+
+ Collection<SubmitRecord> submitRecords =
+ rule.evaluate(r.getChange(), SubmitRuleOptions.defaults());
+ assertThat(submitRecords).isEmpty();
+ }
+
+ private void enableRule(String labelName, boolean newState) throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ Map<String, LabelType> localLabelSections = u.getConfig().getLabelSections();
+ if (localLabelSections.isEmpty()) {
+ localLabelSections.putAll(projectCache.getAllProjects().getConfig().getLabelSections());
+ }
+ localLabelSections.get(labelName).setIgnoreSelfApproval(newState);
+ u.save();
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/ssh/BUILD b/javatests/com/google/gerrit/acceptance/ssh/BUILD
index b195ecc..a01cd3e 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/BUILD
+++ b/javatests/com/google/gerrit/acceptance/ssh/BUILD
@@ -17,6 +17,7 @@
vm_args = ["-Xmx512m"],
deps = [
":util",
+ "//java/com/google/gerrit/server/logging",
"//lib/commons:compress",
],
)
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImplTest.java
new file mode 100644
index 0000000..954b0e6
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/testsuite/group/GroupOperationsImplTest.java
@@ -0,0 +1,656 @@
+// Copyright (C) 2018 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.testsuite.group;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.truth.Correspondence;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.extensions.api.groups.GroupInput;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.inject.Inject;
+import java.sql.Timestamp;
+import java.util.Objects;
+import java.util.Optional;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+public class GroupOperationsImplTest extends AbstractDaemonTest {
+
+ @Rule public ExpectedException expectedException = ExpectedException.none();
+
+ @Inject private AccountOperations accountOperations;
+
+ @Inject private GroupOperationsImpl groupOperations;
+
+ private int uniqueGroupNameIndex;
+
+ @Test
+ public void groupCanBeCreatedWithoutSpecifyingAnyParameters() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.id).isEqualTo(groupUuid.get());
+ assertThat(foundGroup.name).isNotEmpty();
+ }
+
+ @Test
+ public void twoGroupsWithoutAnyParametersDoNotClash() throws Exception {
+ AccountGroup.UUID groupUuid1 = groupOperations.newGroup().create();
+ AccountGroup.UUID groupUuid2 = groupOperations.newGroup().create();
+
+ TestGroup group1 = groupOperations.group(groupUuid1).get();
+ TestGroup group2 = groupOperations.group(groupUuid2).get();
+ assertThat(group1.groupUuid()).isNotEqualTo(group2.groupUuid());
+ }
+
+ @Test
+ public void groupCreatedByTestApiCanBeRetrievedViaOfficialApi() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().name("unique group created via test API").create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.id).isEqualTo(groupUuid.get());
+ assertThat(foundGroup.name).isEqualTo("unique group created via test API");
+ }
+
+ @Test
+ public void specifiedNameIsRespectedForGroupCreation() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().name("XYZ-123-this-name-must-be-unique").create();
+
+ GroupInfo group = getGroupFromServer(groupUuid);
+ assertThat(group.name).isEqualTo("XYZ-123-this-name-must-be-unique");
+ }
+
+ @Test
+ public void specifiedDescriptionIsRespectedForGroupCreation() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().description("All authenticated users").create();
+
+ GroupInfo group = getGroupFromServer(groupUuid);
+ assertThat(group.description).isEqualTo("All authenticated users");
+ }
+
+ @Test
+ public void requestingNoDescriptionIsPossibleForGroupCreation() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearDescription().create();
+
+ GroupInfo group = getGroupFromServer(groupUuid);
+ assertThat(group.description).isNull();
+ }
+
+ @Test
+ public void specifiedOwnerIsRespectedForGroupCreation() throws Exception {
+ AccountGroup.UUID ownerGroupUuid = groupOperations.newGroup().create();
+
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().ownerGroupUuid(ownerGroupUuid).create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.ownerId).isEqualTo(ownerGroupUuid.get());
+ }
+
+ @Test
+ public void specifiedVisibilityIsRespectedForGroupCreation() throws Exception {
+ AccountGroup.UUID group1Uuid = groupOperations.newGroup().visibleToAll(true).create();
+ AccountGroup.UUID group2Uuid = groupOperations.newGroup().visibleToAll(false).create();
+
+ GroupInfo foundGroup1 = getGroupFromServer(group1Uuid);
+ GroupInfo foundGroup2 = getGroupFromServer(group2Uuid);
+ assertThat(foundGroup1.options.visibleToAll).isTrue();
+ // False == null
+ assertThat(foundGroup2.options.visibleToAll).isNull();
+ }
+
+ @Test
+ public void specifiedMembersAreRespectedForGroupCreation() throws Exception {
+ Account.Id account1Id = accountOperations.newAccount().create();
+ Account.Id account2Id = accountOperations.newAccount().create();
+ Account.Id account3Id = accountOperations.newAccount().create();
+ Account.Id account4Id = accountOperations.newAccount().create();
+
+ AccountGroup.UUID groupUuid =
+ groupOperations
+ .newGroup()
+ .members(account1Id, account2Id)
+ .addMember(account3Id)
+ .addMember(account4Id)
+ .create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.members)
+ .comparingElementsUsing(getAccountToIdCorrespondence())
+ .containsExactly(account1Id, account2Id, account3Id, account4Id);
+ }
+
+ @Test
+ public void directlyAddingMembersIsPossibleForGroupCreation() throws Exception {
+ Account.Id account1Id = accountOperations.newAccount().create();
+ Account.Id account2Id = accountOperations.newAccount().create();
+
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().addMember(account1Id).addMember(account2Id).create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.members)
+ .comparingElementsUsing(getAccountToIdCorrespondence())
+ .containsExactly(account1Id, account2Id);
+ }
+
+ @Test
+ public void requestingNoMembersIsPossibleForGroupCreation() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearMembers().create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.members).isEmpty();
+ }
+
+ @Test
+ public void specifiedSubgroupsAreRespectedForGroupCreation() throws Exception {
+ AccountGroup.UUID group1Uuid = groupOperations.newGroup().create();
+ AccountGroup.UUID group2Uuid = groupOperations.newGroup().create();
+ AccountGroup.UUID group3Uuid = groupOperations.newGroup().create();
+ AccountGroup.UUID group4Uuid = groupOperations.newGroup().create();
+
+ AccountGroup.UUID groupUuid =
+ groupOperations
+ .newGroup()
+ .subgroups(group1Uuid, group2Uuid)
+ .addSubgroup(group3Uuid)
+ .addSubgroup(group4Uuid)
+ .create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.includes)
+ .comparingElementsUsing(getGroupToUuidCorrespondence())
+ .containsExactly(group1Uuid, group2Uuid, group3Uuid, group4Uuid);
+ }
+
+ @Test
+ public void directlyAddingSubgroupsIsPossibleForGroupCreation() throws Exception {
+ AccountGroup.UUID group1Uuid = groupOperations.newGroup().create();
+ AccountGroup.UUID group2Uuid = groupOperations.newGroup().create();
+
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().addSubgroup(group1Uuid).addSubgroup(group2Uuid).create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.includes)
+ .comparingElementsUsing(getGroupToUuidCorrespondence())
+ .containsExactly(group1Uuid, group2Uuid);
+ }
+
+ @Test
+ public void requestingNoSubgroupsIsPossibleForGroupCreation() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearSubgroups().create();
+
+ GroupInfo foundGroup = getGroupFromServer(groupUuid);
+ assertThat(foundGroup.includes).isEmpty();
+ }
+
+ @Test
+ public void existingGroupCanBeCheckedForExistence() throws Exception {
+ AccountGroup.UUID groupUuid = createGroupInServer(createArbitraryGroupInput());
+
+ boolean exists = groupOperations.group(groupUuid).exists();
+
+ assertThat(exists).isTrue();
+ }
+
+ @Test
+ public void notExistingGroupCanBeCheckedForExistence() throws Exception {
+ AccountGroup.UUID notExistingGroupUuid = new AccountGroup.UUID("not-existing-group");
+
+ boolean exists = groupOperations.group(notExistingGroupUuid).exists();
+
+ assertThat(exists).isFalse();
+ }
+
+ @Test
+ public void retrievingNotExistingGroupFails() throws Exception {
+ AccountGroup.UUID notExistingGroupUuid = new AccountGroup.UUID("not-existing-group");
+
+ expectedException.expect(IllegalStateException.class);
+ groupOperations.group(notExistingGroupUuid).get();
+ }
+
+ @Test
+ public void groupNotCreatedByTestApiCanBeRetrieved() throws Exception {
+ GroupInput input = createArbitraryGroupInput();
+ input.name = "unique group not created via test API";
+ AccountGroup.UUID groupUuid = createGroupInServer(input);
+
+ TestGroup foundGroup = groupOperations.group(groupUuid).get();
+
+ assertThat(foundGroup.groupUuid()).isEqualTo(groupUuid);
+ assertThat(foundGroup.name()).isEqualTo("unique group not created via test API");
+ }
+
+ @Test
+ public void uuidOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().create();
+
+ AccountGroup.UUID foundGroupUuid = groupOperations.group(groupUuid).get().groupUuid();
+
+ assertThat(foundGroupUuid).isEqualTo(groupUuid);
+ }
+
+ @Test
+ public void nameOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().name("ABC-789-this-name-must-be-unique").create();
+
+ String groupName = groupOperations.group(groupUuid).get().name();
+
+ assertThat(groupName).isEqualTo("ABC-789-this-name-must-be-unique");
+ }
+
+ @Test
+ public void nameKeyOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().name("ABC-789-this-name-must-be-unique").create();
+
+ AccountGroup.NameKey groupName = groupOperations.group(groupUuid).get().nameKey();
+
+ assertThat(groupName).isEqualTo(new AccountGroup.NameKey("ABC-789-this-name-must-be-unique"));
+ }
+
+ @Test
+ public void descriptionOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations
+ .newGroup()
+ .description("This is a very detailed description of this group.")
+ .create();
+
+ Optional<String> description = groupOperations.group(groupUuid).get().description();
+
+ assertThat(description).hasValue("This is a very detailed description of this group.");
+ }
+
+ @Test
+ public void emptyDescriptionOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearDescription().create();
+
+ Optional<String> description = groupOperations.group(groupUuid).get().description();
+
+ assertThat(description).isEmpty();
+ }
+
+ @Test
+ public void ownerGroupUuidOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID originalOwnerGroupUuid = new AccountGroup.UUID("owner group");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().ownerGroupUuid(originalOwnerGroupUuid).create();
+
+ AccountGroup.UUID ownerGroupUuid = groupOperations.group(groupUuid).get().ownerGroupUuid();
+
+ assertThat(ownerGroupUuid).isEqualTo(originalOwnerGroupUuid);
+ }
+
+ @Test
+ public void visibilityOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID visibleGroupUuid = groupOperations.newGroup().visibleToAll(true).create();
+ AccountGroup.UUID invisibleGroupUuid = groupOperations.newGroup().visibleToAll(false).create();
+
+ TestGroup visibleGroup = groupOperations.group(visibleGroupUuid).get();
+ TestGroup invisibleGroup = groupOperations.group(invisibleGroupUuid).get();
+
+ assertThat(visibleGroup.visibleToAll()).named("visibility of visible group").isTrue();
+ assertThat(invisibleGroup.visibleToAll()).named("visibility of invisible group").isFalse();
+ }
+
+ @Test
+ public void createdOnOfExistingGroupCanBeRetrieved() throws Exception {
+ GroupInfo group = gApi.groups().create(createArbitraryGroupInput()).detail();
+ AccountGroup.UUID groupUuid = new AccountGroup.UUID(group.id);
+
+ Timestamp createdOn = groupOperations.group(groupUuid).get().createdOn();
+
+ assertThat(createdOn).isEqualTo(group.createdOn);
+ }
+
+ @Test
+ public void membersOfExistingGroupCanBeRetrieved() throws Exception {
+ Account.Id memberId1 = new Account.Id(1000);
+ Account.Id memberId2 = new Account.Id(2000);
+ Account.Id memberId3 = new Account.Id(3000);
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().members(memberId1, memberId2, memberId3).create();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+
+ assertThat(members).containsExactly(memberId1, memberId2, memberId3);
+ }
+
+ @Test
+ public void emptyMembersOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearMembers().create();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+
+ assertThat(members).isEmpty();
+ }
+
+ @Test
+ public void subgroupsOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID subgroupUuid1 = new AccountGroup.UUID("subgroup 1");
+ AccountGroup.UUID subgroupUuid2 = new AccountGroup.UUID("subgroup 2");
+ AccountGroup.UUID subgroupUuid3 = new AccountGroup.UUID("subgroup 3");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().subgroups(subgroupUuid1, subgroupUuid2, subgroupUuid3).create();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+
+ assertThat(subgroups).containsExactly(subgroupUuid1, subgroupUuid2, subgroupUuid3);
+ }
+
+ @Test
+ public void emptySubgroupsOfExistingGroupCanBeRetrieved() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearSubgroups().create();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+
+ assertThat(subgroups).isEmpty();
+ }
+
+ @Test
+ public void updateWithoutAnyParametersIsANoop() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().create();
+ TestGroup originalGroup = groupOperations.group(groupUuid).get();
+
+ groupOperations.group(groupUuid).forUpdate().update();
+
+ TestGroup updatedGroup = groupOperations.group(groupUuid).get();
+ assertThat(updatedGroup).isEqualTo(originalGroup);
+ }
+
+ @Test
+ public void updateWritesToInternalGroupSystem() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().description("original description").create();
+
+ groupOperations.group(groupUuid).forUpdate().description("updated description").update();
+
+ String currentDescription = getGroupFromServer(groupUuid).description;
+ assertThat(currentDescription).isEqualTo("updated description");
+ }
+
+ @Test
+ public void nameCanBeUpdated() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().name("original name").create();
+
+ groupOperations.group(groupUuid).forUpdate().name("updated name").update();
+
+ String currentName = groupOperations.group(groupUuid).get().name();
+ assertThat(currentName).isEqualTo("updated name");
+ }
+
+ @Test
+ public void descriptionCanBeUpdated() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().description("original description").create();
+
+ groupOperations.group(groupUuid).forUpdate().description("updated description").update();
+
+ Optional<String> currentDescription = groupOperations.group(groupUuid).get().description();
+ assertThat(currentDescription).hasValue("updated description");
+ }
+
+ @Test
+ public void descriptionCanBeCleared() throws Exception {
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().description("original description").create();
+
+ groupOperations.group(groupUuid).forUpdate().clearDescription().update();
+
+ Optional<String> currentDescription = groupOperations.group(groupUuid).get().description();
+ assertThat(currentDescription).isEmpty();
+ }
+
+ @Test
+ public void ownerGroupUuidCanBeUpdated() throws Exception {
+ AccountGroup.UUID originalOwnerGroupUuid = new AccountGroup.UUID("original owner");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().ownerGroupUuid(originalOwnerGroupUuid).create();
+
+ AccountGroup.UUID updatedOwnerGroupUuid = new AccountGroup.UUID("updated owner");
+ groupOperations.group(groupUuid).forUpdate().ownerGroupUuid(updatedOwnerGroupUuid).update();
+
+ AccountGroup.UUID currentOwnerGroupUuid =
+ groupOperations.group(groupUuid).get().ownerGroupUuid();
+ assertThat(currentOwnerGroupUuid).isEqualTo(updatedOwnerGroupUuid);
+ }
+
+ @Test
+ public void visibilityCanBeUpdated() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().visibleToAll(true).create();
+
+ groupOperations.group(groupUuid).forUpdate().visibleToAll(false).update();
+
+ boolean visibleToAll = groupOperations.group(groupUuid).get().visibleToAll();
+ assertThat(visibleToAll).isFalse();
+ }
+
+ @Test
+ public void membersCanBeAdded() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearMembers().create();
+
+ Account.Id memberId1 = new Account.Id(1000);
+ Account.Id memberId2 = new Account.Id(2000);
+ groupOperations.group(groupUuid).forUpdate().addMember(memberId1).addMember(memberId2).update();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+ assertThat(members).containsExactly(memberId1, memberId2);
+ }
+
+ @Test
+ public void membersCanBeRemoved() throws Exception {
+ Account.Id memberId1 = new Account.Id(1000);
+ Account.Id memberId2 = new Account.Id(2000);
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().members(memberId1, memberId2).create();
+
+ groupOperations.group(groupUuid).forUpdate().removeMember(memberId2).update();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+ assertThat(members).containsExactly(memberId1);
+ }
+
+ @Test
+ public void memberAdditionAndRemovalCanBeMixed() throws Exception {
+ Account.Id memberId1 = new Account.Id(1000);
+ Account.Id memberId2 = new Account.Id(2000);
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().members(memberId1, memberId2).create();
+
+ Account.Id memberId3 = new Account.Id(3000);
+ groupOperations
+ .group(groupUuid)
+ .forUpdate()
+ .removeMember(memberId1)
+ .addMember(memberId3)
+ .update();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+ assertThat(members).containsExactly(memberId2, memberId3);
+ }
+
+ @Test
+ public void membersCanBeCleared() throws Exception {
+ Account.Id memberId1 = new Account.Id(1000);
+ Account.Id memberId2 = new Account.Id(2000);
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().members(memberId1, memberId2).create();
+
+ groupOperations.group(groupUuid).forUpdate().clearMembers().update();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+ assertThat(members).isEmpty();
+ }
+
+ @Test
+ public void furtherMembersCanBeAddedAfterClearingAll() throws Exception {
+ Account.Id memberId1 = new Account.Id(1000);
+ Account.Id memberId2 = new Account.Id(2000);
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().members(memberId1, memberId2).create();
+
+ Account.Id memberId3 = new Account.Id(3000);
+ groupOperations.group(groupUuid).forUpdate().clearMembers().addMember(memberId3).update();
+
+ ImmutableSet<Account.Id> members = groupOperations.group(groupUuid).get().members();
+ assertThat(members).containsExactly(memberId3);
+ }
+
+ @Test
+ public void subgroupsCanBeAdded() throws Exception {
+ AccountGroup.UUID groupUuid = groupOperations.newGroup().clearSubgroups().create();
+
+ AccountGroup.UUID subgroupUuid1 = new AccountGroup.UUID("subgroup 1");
+ AccountGroup.UUID subgroupUuid2 = new AccountGroup.UUID("subgroup 2");
+ groupOperations
+ .group(groupUuid)
+ .forUpdate()
+ .addSubgroup(subgroupUuid1)
+ .addSubgroup(subgroupUuid2)
+ .update();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+ assertThat(subgroups).containsExactly(subgroupUuid1, subgroupUuid2);
+ }
+
+ @Test
+ public void subgroupsCanBeRemoved() throws Exception {
+ AccountGroup.UUID subgroupUuid1 = new AccountGroup.UUID("subgroup 1");
+ AccountGroup.UUID subgroupUuid2 = new AccountGroup.UUID("subgroup 2");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().subgroups(subgroupUuid1, subgroupUuid2).create();
+
+ groupOperations.group(groupUuid).forUpdate().removeSubgroup(subgroupUuid2).update();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+ assertThat(subgroups).containsExactly(subgroupUuid1);
+ }
+
+ @Test
+ public void subgroupAdditionAndRemovalCanBeMixed() throws Exception {
+ AccountGroup.UUID subgroupUuid1 = new AccountGroup.UUID("subgroup 1");
+ AccountGroup.UUID subgroupUuid2 = new AccountGroup.UUID("subgroup 2");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().subgroups(subgroupUuid1, subgroupUuid2).create();
+
+ AccountGroup.UUID subgroupUuid3 = new AccountGroup.UUID("subgroup 3");
+ groupOperations
+ .group(groupUuid)
+ .forUpdate()
+ .removeSubgroup(subgroupUuid1)
+ .addSubgroup(subgroupUuid3)
+ .update();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+ assertThat(subgroups).containsExactly(subgroupUuid2, subgroupUuid3);
+ }
+
+ @Test
+ public void subgroupsCanBeCleared() throws Exception {
+ AccountGroup.UUID subgroupUuid1 = new AccountGroup.UUID("subgroup 1");
+ AccountGroup.UUID subgroupUuid2 = new AccountGroup.UUID("subgroup 2");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().subgroups(subgroupUuid1, subgroupUuid2).create();
+
+ groupOperations.group(groupUuid).forUpdate().clearSubgroups().update();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+ assertThat(subgroups).isEmpty();
+ }
+
+ @Test
+ public void furtherSubgroupsCanBeAddedAfterClearingAll() throws Exception {
+ AccountGroup.UUID subgroupUuid1 = new AccountGroup.UUID("subgroup 1");
+ AccountGroup.UUID subgroupUuid2 = new AccountGroup.UUID("subgroup 2");
+ AccountGroup.UUID groupUuid =
+ groupOperations.newGroup().subgroups(subgroupUuid1, subgroupUuid2).create();
+
+ AccountGroup.UUID subgroupUuid3 = new AccountGroup.UUID("subgroup 3");
+ groupOperations
+ .group(groupUuid)
+ .forUpdate()
+ .clearSubgroups()
+ .addSubgroup(subgroupUuid3)
+ .update();
+
+ ImmutableSet<AccountGroup.UUID> subgroups = groupOperations.group(groupUuid).get().subgroups();
+ assertThat(subgroups).containsExactly(subgroupUuid3);
+ }
+
+ private GroupInput createArbitraryGroupInput() {
+ GroupInput groupInput = new GroupInput();
+ groupInput.name = name("verifiers-" + uniqueGroupNameIndex++);
+ return groupInput;
+ }
+
+ private GroupInfo getGroupFromServer(AccountGroup.UUID groupUuid) throws RestApiException {
+ return gApi.groups().id(groupUuid.get()).detail();
+ }
+
+ private AccountGroup.UUID createGroupInServer(GroupInput input) throws RestApiException {
+ GroupInfo group = gApi.groups().create(input).detail();
+ return new AccountGroup.UUID(group.id);
+ }
+
+ private static Correspondence<AccountInfo, Account.Id> getAccountToIdCorrespondence() {
+ return new Correspondence<AccountInfo, Account.Id>() {
+ @Override
+ public boolean compare(AccountInfo actualAccount, Account.Id expectedId) {
+ Account.Id accountId =
+ Optional.ofNullable(actualAccount)
+ .map(account -> account._accountId)
+ .map(Account.Id::new)
+ .orElse(null);
+ return Objects.equals(accountId, expectedId);
+ }
+
+ @Override
+ public String toString() {
+ return "has ID";
+ }
+ };
+ }
+
+ private static Correspondence<GroupInfo, AccountGroup.UUID> getGroupToUuidCorrespondence() {
+ return new Correspondence<GroupInfo, AccountGroup.UUID>() {
+ @Override
+ public boolean compare(GroupInfo actualGroup, AccountGroup.UUID expectedUuid) {
+ AccountGroup.UUID groupUuid =
+ Optional.ofNullable(actualGroup)
+ .map(group -> group.id)
+ .map(AccountGroup.UUID::new)
+ .orElse(null);
+ return Objects.equals(groupUuid, expectedUuid);
+ }
+
+ @Override
+ public String toString() {
+ return "has UUID";
+ }
+ };
+ }
+}
diff --git a/javatests/com/google/gerrit/server/BUILD b/javatests/com/google/gerrit/server/BUILD
index 29e9a0b..7be1827 100644
--- a/javatests/com/google/gerrit/server/BUILD
+++ b/javatests/com/google/gerrit/server/BUILD
@@ -49,6 +49,7 @@
"//java/com/google/gerrit/server/cache/testing",
"//java/com/google/gerrit/server/group/testing",
"//java/com/google/gerrit/server/ioutil",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/project/testing:project-test-util",
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/schema",
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index c774fc5..8cd2753 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -426,7 +426,7 @@
@Test
public void approvalsPostSubmit() throws Exception {
Change c = newChange();
- RequestId submissionId = RequestId.forChange(c);
+ RequestId submissionId = submissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 1);
update.putApproval("Verified", (short) 1);
@@ -461,7 +461,7 @@
@Test
public void approvalsDuringSubmit() throws Exception {
Change c = newChange();
- RequestId submissionId = RequestId.forChange(c);
+ RequestId submissionId = submissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 1);
update.putApproval("Verified", (short) 1);
@@ -598,7 +598,7 @@
@Test
public void submitRecords() throws Exception {
Change c = newChange();
- RequestId submissionId = RequestId.forChange(c);
+ RequestId submissionId = submissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
@@ -640,7 +640,7 @@
@Test
public void latestSubmitRecordsOnly() throws Exception {
Change c = newChange();
- RequestId submissionId = RequestId.forChange(c);
+ RequestId submissionId = submissionId(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
update.merge(
@@ -941,7 +941,7 @@
// Finish off by merging the change.
update = newUpdate(c, changeOwner);
update.merge(
- RequestId.forChange(c),
+ submissionId(c),
ImmutableList.of(
submitRecord(
"NOT_READY",
@@ -3141,4 +3141,8 @@
update.commit();
return tr.parseBody(commit);
}
+
+ private RequestId submissionId(Change c) {
+ return new RequestId(c.getId().toString());
+ }
}
diff --git a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
index 43e2602..8daf67f 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitMessageOutputTest.java
@@ -151,7 +151,7 @@
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
- RequestId submissionId = RequestId.forChange(c);
+ RequestId submissionId = submissionId(c);
update.merge(
submissionId,
ImmutableList.of(
@@ -220,7 +220,7 @@
ChangeUpdate update = newUpdate(c, changeOwner);
update.setSubjectForCommit("Submit patch set 1");
- RequestId submissionId = RequestId.forChange(c);
+ RequestId submissionId = submissionId(c);
update.merge(
submissionId, ImmutableList.of(submitRecord("RULE_ERROR", "Problem with patch set:\n1")));
update.commit();
@@ -424,4 +424,8 @@
RevCommit commit = parseCommit(commitId);
assertThat(commit.getFullMessage()).isEqualTo(expected);
}
+
+ private RequestId submissionId(Change c) {
+ return new RequestId(c.getId().toString());
+ }
}
diff --git a/javatests/com/google/gerrit/server/rules/BUILD b/javatests/com/google/gerrit/server/rules/BUILD
index 42452df..8f4c90d 100644
--- a/javatests/com/google/gerrit/server/rules/BUILD
+++ b/javatests/com/google/gerrit/server/rules/BUILD
@@ -7,9 +7,11 @@
resources = ["//prologtests:gerrit_common_test"],
deps = [
"//java/com/google/gerrit/common:server",
+ "//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/project/testing:project-test-util",
"//java/com/google/gerrit/testing:gerrit-test-util",
+ "//lib:guava",
"//lib/guice",
"//lib/jgit/org.eclipse.jgit:jgit",
"//lib/prolog:runtime",
diff --git a/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java b/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java
new file mode 100644
index 0000000..27f4423
--- /dev/null
+++ b/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java
@@ -0,0 +1,96 @@
+// Copyright (C) 2018 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.server.rules;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.data.LabelType;
+import com.google.gerrit.common.data.LabelValue;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.LabelId;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Date;
+import java.util.List;
+import org.junit.Test;
+
+public class IgnoreSelfApprovalRuleTest {
+ private static final Change.Id CHANGE_ID = new Change.Id(100);
+ private static final PatchSet.Id PS_ID = new PatchSet.Id(CHANGE_ID, 1);
+ private static final LabelType VERIFIED = makeLabel("Verified");
+ private static final Account.Id USER1 = makeAccount(100001);
+
+ @Test
+ public void filtersByLabel() {
+ LabelType codeReview = makeLabel("Code-Review");
+ PatchSetApproval approvalVerified = makeApproval(VERIFIED.getLabelId(), USER1, 2);
+ PatchSetApproval approvalCr = makeApproval(codeReview.getLabelId(), USER1, 2);
+
+ Collection<PatchSetApproval> filteredApprovals =
+ IgnoreSelfApprovalRule.filterApprovalsByLabel(
+ ImmutableList.of(approvalVerified, approvalCr), VERIFIED);
+
+ assertThat(filteredApprovals).containsExactly(approvalVerified);
+ }
+
+ @Test
+ public void filtersVotesFromUser() {
+ PatchSetApproval approvalM2 = makeApproval(VERIFIED.getLabelId(), USER1, -2);
+ PatchSetApproval approvalM1 = makeApproval(VERIFIED.getLabelId(), USER1, -1);
+
+ ImmutableList<PatchSetApproval> approvals =
+ ImmutableList.of(
+ approvalM2,
+ approvalM1,
+ makeApproval(VERIFIED.getLabelId(), USER1, 0),
+ makeApproval(VERIFIED.getLabelId(), USER1, +1),
+ makeApproval(VERIFIED.getLabelId(), USER1, +2));
+
+ Collection<PatchSetApproval> filteredApprovals =
+ IgnoreSelfApprovalRule.filterOutPositiveApprovalsOfUser(approvals, USER1);
+
+ assertThat(filteredApprovals).containsExactly(approvalM1, approvalM2);
+ }
+
+ private static LabelType makeLabel(String labelName) {
+ List<LabelValue> values = new ArrayList<>();
+ // The label text is irrelevant here, only the numerical value is used
+ values.add(new LabelValue((short) -2, "-2"));
+ values.add(new LabelValue((short) -1, "-1"));
+ values.add(new LabelValue((short) 0, "No vote."));
+ values.add(new LabelValue((short) 1, "+1"));
+ values.add(new LabelValue((short) 2, "+2"));
+ return new LabelType(labelName, values);
+ }
+
+ private static PatchSetApproval makeApproval(LabelId labelId, Account.Id accountId, int value) {
+ PatchSetApproval.Key key = makeKey(PS_ID, accountId, labelId);
+ return new PatchSetApproval(key, (short) value, Date.from(Instant.now()));
+ }
+
+ private static PatchSetApproval.Key makeKey(
+ PatchSet.Id psId, Account.Id accountId, LabelId labelId) {
+ return new PatchSetApproval.Key(psId, accountId, labelId);
+ }
+
+ private static Account.Id makeAccount(int account) {
+ return new Account.Id(account);
+ }
+}
diff --git a/plugins/BUILD b/plugins/BUILD
index 8b9f4de..a7622b2 100644
--- a/plugins/BUILD
+++ b/plugins/BUILD
@@ -38,6 +38,7 @@
"//java/com/google/gerrit/metrics/dropwizard",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server/audit",
+ "//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/schema",
"//java/com/google/gerrit/util/http",
"//lib/commons:compress",
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.html b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
index 5608dec..00157ab 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.html
@@ -178,34 +178,20 @@
<gr-account-label
account="[[author]]"
hide-avatar></gr-account-label>
- <template is="dom-if" if="[[_successfulParse]]">
- <template is="dom-if" if="[[_parsedVotes.length]]">voted</template>
- <template is="dom-repeat" items="[[_parsedVotes]]" as="score">
- <span class$="score [[_computeScoreClass(score, labelExtremes)]]">
- [[score.label]] [[score.value]]
- </span>
- </template>
- [[_computeConversationalString(_parsedVotes, _parsedPatchNum, _parsedCommentCount)]]
+ <template is="dom-repeat" items="[[_getScores(message)]]" as="score">
+ <span class$="score [[_computeScoreClass(score, labelExtremes)]]">
+ [[score.label]] [[score.value]]
+ </span>
</template>
</div>
<template is="dom-if" if="[[message.message]]">
<div class="content">
- <template is="dom-if" if="[[_successfulParse]]">
- <div class="message hideOnOpen">[[_parsedChangeMessage]]</div>
- <gr-formatted-text
- no-trailing-margin
- class="message hideOnCollapsed"
- content="[[_parsedChangeMessage]]"
- config="[[_projectConfig.commentlinks]]"></gr-formatted-text>
- </template>
- <template is="dom-if" if="[[!_successfulParse]]">
- <div class="message hideOnOpen">[[message.message]]</div>
- <gr-formatted-text
- no-trailing-margin
- class="message hideOnCollapsed"
- content="[[message.message]]"
- config="[[_projectConfig.commentlinks]]"></gr-formatted-text>
- </template>
+ <div class="message hideOnOpen">[[message.message]]</div>
+ <gr-formatted-text
+ no-trailing-margin
+ class="message hideOnCollapsed"
+ content="[[message.message]]"
+ config="[[_projectConfig.commentlinks]]"></gr-formatted-text>
<div class="replyContainer" hidden$="[[!showReplyButton]]" hidden>
<gr-button link small on-tap="_handleReplyTap">Reply</gr-button>
</div>
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.js b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
index addd660..0590c73 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.js
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.js
@@ -17,8 +17,7 @@
(function() {
'use strict';
- const PATCH_SET_PREFIX_PATTERN = /^Patch Set (\d)+:[ ]?/;
- const COMMENTS_COUNT_PATTERN = /^\((\d+)( inline)? comments?\)$/;
+ const PATCH_SET_PREFIX_PATTERN = /^Patch Set \d+: /;
const LABEL_TITLE_SCORE_PATTERN = /^([A-Za-z0-9-]+)([+-]\d+)$/;
Polymer({
@@ -102,17 +101,10 @@
type: Boolean,
value: false,
},
-
- _parsedPatchNum: String,
- _parsedCommentCount: String,
- _parsedVotes: Array,
- _parsedChangeMessage: String,
- _successfulParse: Boolean,
},
observers: [
'_updateExpandedClass(message.expanded)',
- '_consumeMessage(message.message)',
],
ready() {
@@ -192,6 +184,19 @@
return event.type === 'REVIEWER_UPDATE';
},
+ _getScores(message) {
+ if (!message.message) { return []; }
+ const line = message.message.split('\n', 1)[0];
+ const patchSetPrefix = PATCH_SET_PREFIX_PATTERN;
+ if (!line.match(patchSetPrefix)) { return []; }
+ const scoresRaw = line.split(patchSetPrefix)[1];
+ if (!scoresRaw) { return []; }
+ return scoresRaw.split(' ')
+ .map(s => s.match(LABEL_TITLE_SCORE_PATTERN))
+ .filter(ms => ms && ms.length === 3)
+ .map(ms => ({label: ms[1], value: ms[2]}));
+ },
+
_computeScoreClass(score, labelExtremes) {
const classes = [];
if (score.value > 0) {
@@ -255,73 +260,5 @@
e.stopPropagation();
this.set('message.expanded', !this.message.expanded);
},
-
- /**
- * Attempts to consume a change message to create a shorter and more legible
- * format. If the function encounters unexpected characters at any point, it
- * sets the _successfulParse flag to false and terminates, causing the UI to
- * fall back to displaying the entirety of the change message.
- *
- * A successful parse results in a one-liner that reads:
- * `${AVATAR} voted ${VOTES} and left ${NUM} comment(s) on ${PATCHSET}`
- *
- * @param {string} text
- */
- _consumeMessage(text) {
- this._parsedPatchNum = '';
- this._parsedCommentCount = '';
- this._parsedChangeMessage = '';
- this._parsedVotes = [];
- if (!text) {
- // No message body means nothing to parse.
- this._successfulParse = false;
- return;
- }
- const lines = text.split('\n');
- const messageLines = lines.shift().split(PATCH_SET_PREFIX_PATTERN);
- if (!messageLines[1]) {
- // Message is in an unexpected format.
- this._successfulParse = false;
- return;
- }
- this._parsedPatchNum = messageLines[1];
- if (messageLines[2]) {
- // Content after the colon is always vote information. If it is in the
- // most up to date schema, parse it. Otherwise, cancel the parsing
- // completely.
- let match;
- for (const score of messageLines[2].split(' ')) {
- match = score.match(LABEL_TITLE_SCORE_PATTERN);
- if (!match || match.length !== 3) {
- this._successfulParse = false;
- return;
- }
- this._parsedVotes.push({label: match[1], value: match[2]});
- }
- }
- // Remove empty line.
- lines.shift();
- if (lines.length) {
- const commentMatch = lines[0].match(COMMENTS_COUNT_PATTERN);
- if (commentMatch) {
- this._parsedCommentCount = commentMatch[1];
- // Remove comment line and the following empty line.
- lines.splice(0, 2);
- }
- this._parsedChangeMessage = lines.join('\n');
- }
- this._successfulParse = true;
- },
-
- _computeConversationalString(votes, patchNum, commentCount) {
- let clause = ' on Patch Set ' + patchNum;
- if (commentCount) {
- let commentStr = ' comment';
- if (parseInt(commentCount, 10) > 1) { commentStr += 's'; }
- clause = ' left ' + commentCount + commentStr + clause;
- if (votes.length) { clause = ' and' + clause; }
- }
- return clause;
- },
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
index 6bb4618..870f366 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
@@ -169,9 +169,7 @@
author: {},
expanded: false,
message: 'Patch Set 1: Verified+1 Code-Review-2 Trybot-Ready+1',
- _revision_number: 1,
};
- element.comments = {};
element.labelExtremes = {
'Verified': {max: 1, min: -1},
'Code-Review': {max: 2, min: -2},
@@ -201,145 +199,5 @@
const scoreChips = Polymer.dom(element.root).querySelectorAll('.score');
assert.equal(scoreChips.length, 0);
});
-
- suite('_consumeMessage', () => {
- const assertConsumeFailed = str => {
- element._consumeMessage(str);
- assert.isFalse(element._successfulParse);
- };
-
- test('no message body', () => {
- assertConsumeFailed('');
- });
-
- test('known old schema', () => {
- const str = 'Patch Set 1: Looks good to me, approved; Verified';
- assertConsumeFailed(str);
- });
-
- test('known old schema 2', () => {
- const str = [
- 'Patch Set 2: Looks good to me',
- '',
- 'Patch set 2 compiles, and runs as expected.',
- ].join('\n');
- assertConsumeFailed(str);
- });
-
- test('known old schema 3', () => {
- const str = 'Patch Set 2: (1 inline comment)';
- assertConsumeFailed(str);
- });
-
- test('known old schema 4', () => {
- const str = [
- 'Patch Set 2: Looks good to me',
- '',
- '(1 inline comment)',
- ].join('\n');
- assertConsumeFailed(str);
- });
-
- test('just change message', () => {
- const str = [
- 'Patch Set 2:',
- '',
- 'I think you should reconsider this approach.',
- 'It really makes no sense.',
- ].join('\n');
-
- element._consumeMessage(str);
-
- assert.isTrue(element._successfulParse);
- assert.equal(element._parsedPatchNum, '2');
- assert.equal(element._parsedCommentCount, '');
- assert.deepEqual(element._parsedVotes, []);
- assert.equal(element._parsedChangeMessage, [
- 'I think you should reconsider this approach.',
- 'It really makes no sense.',
- ].join('\n'));
- });
-
- test('just votes', () => {
- element._consumeMessage('Patch Set 2: Code-Review-Label+1 Verified+1');
-
- assert.isTrue(element._successfulParse);
- assert.equal(element._parsedPatchNum, '2');
- assert.equal(element._parsedCommentCount, '');
- assert.deepEqual(element._parsedVotes, [
- {label: 'Code-Review-Label', value: '+1'},
- {label: 'Verified', value: '+1'},
- ]);
- assert.equal(element._parsedChangeMessage, '');
- });
-
- test('just comments', () => {
- const str = [
- 'Patch Set 2:',
- '',
- '(8 comments)',
- ].join('\n');
-
- element._consumeMessage(str);
-
- assert.isTrue(element._successfulParse);
- assert.equal(element._parsedPatchNum, '2');
- assert.equal(element._parsedCommentCount, '8');
- assert.deepEqual(element._parsedVotes, []);
- assert.equal(element._parsedChangeMessage, '');
- });
-
- test('vote with comments and change message', () => {
- const str = [
- 'Patch Set 2: Code-Review-Label+1 Verified+1',
- '',
- '(1 comment)',
- '',
- 'LGTM',
- '',
- 'Nice work, just one nit.',
- ].join('\n');
-
- element._consumeMessage(str);
-
- assert.isTrue(element._successfulParse);
- assert.equal(element._parsedPatchNum, '2');
- assert.equal(element._parsedCommentCount, '1');
- assert.deepEqual(element._parsedVotes, [
- {label: 'Code-Review-Label', value: '+1'},
- {label: 'Verified', value: '+1'},
- ]);
- assert.equal(element._parsedChangeMessage, [
- 'LGTM',
- '',
- 'Nice work, just one nit.',
- ].join('\n'));
- });
-
- test('vote with change message', () => {
- const str = [
- 'Patch Set 2: Code-Review-Label+2 Verified+1',
- '',
- 'LGTM',
- '',
- 'Nice work.',
- ].join('\n');
-
- element._consumeMessage(str);
-
- assert.isTrue(element._successfulParse);
- assert.equal(element._parsedPatchNum, '2');
- assert.equal(element._parsedCommentCount, '');
- assert.deepEqual(element._parsedVotes, [
- {label: 'Code-Review-Label', value: '+2'},
- {label: 'Verified', value: '+1'},
- ]);
- assert.equal(element._parsedChangeMessage, [
- 'LGTM',
- '',
- 'Nice work.',
- ].join('\n'));
- });
- });
});
</script>
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 84c1803..af10b8371 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1498,11 +1498,28 @@
*/
getRepos(filter, reposPerPage, opt_offset) {
const defaultFilter = 'state:active OR state:read-only';
+ const namePartDelimiters = /[@.\-\s\/_]/g;
const offset = opt_offset || 0;
+ if (filter && !filter.includes(':') && filter.match(namePartDelimiters)) {
+ // The query language specifies hyphens as operators. Split the string
+ // by hyphens and 'AND' the parts together as 'inname:' queries.
+ // If the filter includes a semicolon, the user is using a more complex
+ // query so we trust them and don't do any magic under the hood.
+ const originalFilter = filter;
+ filter = '';
+ originalFilter.split(namePartDelimiters).forEach(part => {
+ if (part) {
+ filter += (filter === '' ? 'inname:' : ' AND inname:') + part;
+ }
+ });
+ }
+ // Check if filter is now empty which could be either because the user did
+ // not provide it or because the user provided only a split character.
if (!filter) {
filter = defaultFilter;
}
+
filter = filter.trim();
const encodedFilter = encodeURIComponent(filter);
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
index b7466ef..d9656e4 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
@@ -951,16 +951,46 @@
'/projects/?n=26&S=25&query=test');
});
- test('with filter', () => {
- element.getRepos('test/test/test', 25);
+ test('with blank', () => {
+ element.getRepos('test/test', 25);
assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
- '/projects/?n=26&S=0&query=test%2Ftest%2Ftest');
+ '/projects/?n=26&S=0&query=inname%3Atest%20AND%20inname%3Atest');
});
- test('with regex filter', () => {
- element.getRepos('^test.*', 25);
+ test('with hyphen', () => {
+ element.getRepos('foo-bar', 25);
assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
- '/projects/?n=26&S=0&query=%5Etest.*');
+ '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar');
+ });
+
+ test('with leading hyphen', () => {
+ element.getRepos('-bar', 25);
+ assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
+ '/projects/?n=26&S=0&query=inname%3Abar');
+ });
+
+ test('with trailing hyphen', () => {
+ element.getRepos('foo-bar-', 25);
+ assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
+ '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar');
+ });
+
+ test('with underscore', () => {
+ element.getRepos('foo_bar', 25);
+ assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
+ '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar');
+ });
+
+ test('with underscore', () => {
+ element.getRepos('foo_bar', 25);
+ assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
+ '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar');
+ });
+
+ test('hyphen only', () => {
+ element.getRepos('-', 25);
+ assert.equal(element._fetchSharedCacheURL.lastCall.args[0].url,
+ `/projects/?n=26&S=0&query=${defaultQuery}`);
});
});
