Merge "Ignore account visibility when parsing Reviewers Collection"
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 988d871..a41a36c 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -516,12 +516,22 @@
* @throws IOException if an error occurs.
*/
public Result resolve(String input) throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate());
+ return searchImpl(input, searchers, visibilitySupplierCanSee(), accountActivityPredicate());
}
public Result resolve(String input, Predicate<AccountState> accountActivityPredicate)
throws ConfigInvalidException, IOException {
- return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate);
+ return searchImpl(input, searchers, visibilitySupplierCanSee(), accountActivityPredicate);
+ }
+
+ public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
+ return searchImpl(input, searchers, visibilitySupplierAll(), accountActivityPredicate());
+ }
+
+ public Result resolveIgnoreVisibility(
+ String input, Predicate<AccountState> accountActivityPredicate)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(input, searchers, visibilitySupplierAll(), accountActivityPredicate);
}
/**
@@ -550,13 +560,23 @@
@Deprecated
public Result resolveByNameOrEmail(String input) throws ConfigInvalidException, IOException {
return searchImpl(
- input, nameOrEmailSearchers, visibilitySupplier(), accountActivityPredicate());
+ input, nameOrEmailSearchers, visibilitySupplierCanSee(), accountActivityPredicate());
}
- private Supplier<Predicate<AccountState>> visibilitySupplier() {
+ private Supplier<Predicate<AccountState>> visibilitySupplierCanSee() {
return () -> accountControlFactory.get()::canSee;
}
+ private Supplier<Predicate<AccountState>> visibilitySupplierAll() {
+ return () -> all();
+ }
+
+ private Predicate<AccountState> all() {
+ return accountState -> {
+ return true;
+ };
+ }
+
private Predicate<AccountState> accountActivityPredicate() {
return (AccountState accountState) -> accountState.account().isActive();
}
diff --git a/java/com/google/gerrit/server/restapi/change/Reviewers.java b/java/com/google/gerrit/server/restapi/change/Reviewers.java
index b2714da..d702142 100644
--- a/java/com/google/gerrit/server/restapi/change/Reviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/Reviewers.java
@@ -21,12 +21,11 @@
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestView;
-import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ReviewerResource;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -37,22 +36,22 @@
public class Reviewers implements ChildCollection<ChangeResource, ReviewerResource> {
private final DynamicMap<RestView<ReviewerResource>> views;
private final ApprovalsUtil approvalsUtil;
- private final AccountsCollection accounts;
private final ReviewerResource.Factory resourceFactory;
private final ListReviewers list;
+ private final AccountResolver accountResolver;
@Inject
Reviewers(
ApprovalsUtil approvalsUtil,
- AccountsCollection accounts,
ReviewerResource.Factory resourceFactory,
DynamicMap<RestView<ReviewerResource>> views,
- ListReviewers list) {
+ ListReviewers list,
+ AccountResolver accountResolver) {
this.approvalsUtil = approvalsUtil;
- this.accounts = accounts;
this.resourceFactory = resourceFactory;
this.views = views;
this.list = list;
+ this.accountResolver = accountResolver;
}
@Override
@@ -68,22 +67,18 @@
@Override
public ReviewerResource parse(ChangeResource rsrc, IdString id)
throws ResourceNotFoundException, AuthException, IOException, ConfigInvalidException {
- Address address = Address.tryParse(id.get());
-
- Account.Id accountId = null;
try {
- accountId = accounts.parse(TopLevelResource.INSTANCE, id).getUser().getAccountId();
- } catch (ResourceNotFoundException e) {
- if (address == null) {
- throw e;
+
+ AccountResolver.Result result = accountResolver.resolveIgnoreVisibility(id.get());
+ if (fetchAccountIds(rsrc).contains(result.asUniqueUser().getAccountId())) {
+ return resourceFactory.create(rsrc, result.asUniqueUser().getAccountId());
+ }
+ } catch (AccountResolver.UnresolvableAccountException e) {
+ if (e.isSelf()) {
+ throw new AuthException(e.getMessage(), e);
}
}
- // See if the id exists as a reviewer for this change
- if (accountId != null && fetchAccountIds(rsrc).contains(accountId)) {
- return resourceFactory.create(rsrc, accountId);
- }
-
- // See if the address exists as a reviewer on the change
+ Address address = Address.tryParse(id.get());
if (address != null && rsrc.getNotes().getReviewersByEmail().all().contains(address)) {
return new ReviewerResource(rsrc, address);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 477ec38..74f753d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -2204,7 +2204,7 @@
gApi.changes().id(r.getChangeId()).reviewer(admin.id().toString()).votes();
assertThat(m).hasSize(1);
- assertThat(m).containsEntry("Code-Review", Short.valueOf((short) 2));
+ assertThat(m).containsExactly("Code-Review", Short.valueOf((short) 2));
requestScopeOperations.setApiUser(user.id());
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.dislike());
@@ -2212,7 +2212,26 @@
m = gApi.changes().id(r.getChangeId()).reviewer(user.id().toString()).votes();
assertThat(m).hasSize(1);
- assertThat(m).containsEntry("Code-Review", Short.valueOf((short) -1));
+ assertThat(m).containsExactly("Code-Review", Short.valueOf((short) -1));
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void listVotesEvenWhenAccountsAreNotVisible() throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // check finding by address works
+ Map<String, Short> m = gApi.changes().id(r.getChangeId()).reviewer(admin.email()).votes();
+ assertThat(m).hasSize(1);
+ assertThat(m).containsEntry("Code-Review", Short.valueOf((short) 2));
+
+ // check finding by id works
+ m = gApi.changes().id(r.getChangeId()).reviewer(admin.id().toString()).votes();
+ assertThat(m).hasSize(1);
+ assertThat(m).containsEntry("Code-Review", Short.valueOf((short) 2));
}
@Test
