SuggestService visibility logic updated. The SuggestService previously used the project to determine if a suggested user should be returned. The RPC was updated to use the change to determine if a suggested user can see the change. This is implemented as a new RPC, deprecating the old one, so the API does not break unexpectly for direct API users. This change is moving toward the goal of not using GroupMembership.getKnownGroups(), so as to support almost any group system. Change-Id: I63a8bca720d0cc77b4a9f3820e1d6ebe8966afc4
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java index 58e8bc3..25d05e1 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java
@@ -15,6 +15,7 @@ package com.google.gerrit.common.data; import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gwt.user.client.rpc.AsyncCallback; import com.google.gwtjsonrpc.client.RemoteJsonService; @@ -35,13 +36,20 @@ AsyncCallback<List<GroupReference>> callback); /** + * @see #suggestChangeReviewer(com.google.gerrit.reviewdb.client.Change.Id, String, int, AsyncCallback) + */ + @Deprecated + void suggestReviewer(Project.NameKey project, String query, int limit, + AsyncCallback<List<ReviewerInfo>> callback); + + /** * Suggests reviewers. A reviewer can be a user or a group. Inactive users, * the system groups {@link AccountGroup#ANONYMOUS_USERS} and * {@link AccountGroup#REGISTERED_USERS} and groups that have more than the * configured <code>addReviewer.maxAllowed</code> members are not suggested as * reviewers. - * @param project the project for which reviewers should be suggested + * @param changeId the change for which reviewers should be suggested */ - void suggestReviewer(Project.NameKey project, String query, int limit, + void suggestChangeReviewer(Change.Id changeId, String query, int limit, AsyncCallback<List<ReviewerInfo>> callback); }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java index 87b266c..09716cc 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
@@ -134,13 +134,12 @@ } void display(ChangeDetail detail) { - reviewerSuggestOracle.setProject(detail.getChange().getProject()); + changeId = detail.getChange().getId(); + reviewerSuggestOracle.setChange(changeId); List<String> columns = new ArrayList<String>(); List<ApprovalDetail> rows = detail.getApprovals(); - changeId = detail.getChange().getId(); - final Element missingList = missing.getElement(); while (DOM.getChildCount(missingList) > 0) { DOM.removeChild(missingList, DOM.getChild(missingList, 0));
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java index b2a686a..747ef40 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java
@@ -20,7 +20,7 @@ import com.google.gerrit.client.rpc.GerritCallback; import com.google.gerrit.common.data.AccountInfo; import com.google.gerrit.common.data.ReviewerInfo; -import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.Change; import com.google.gwt.user.client.ui.SuggestOracle; import com.google.gwtexpui.safehtml.client.HighlightSuggestOracle; @@ -30,13 +30,13 @@ /** Suggestion Oracle for reviewers. */ public class ReviewerSuggestOracle extends HighlightSuggestOracle { - private Project.NameKey project; + private Change.Id changeId; @Override protected void onRequestSuggestions(final Request req, final Callback callback) { RpcStatus.hide(new Runnable() { public void run() { - SuggestUtil.SVC.suggestReviewer(project, req.getQuery(), + SuggestUtil.SVC.suggestChangeReviewer(changeId, req.getQuery(), req.getLimit(), new GerritCallback<List<ReviewerInfo>>() { public void onSuccess(final List<ReviewerInfo> result) { final List<ReviewerSuggestion> r = @@ -52,8 +52,8 @@ }); } - public void setProject(final Project.NameKey project) { - this.project = project; + public void setChange(Change.Id changeId) { + this.changeId = changeId; } private static class ReviewerSuggestion implements SuggestOracle.Suggestion {
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java index b347ead..bd74f45 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java
@@ -23,6 +23,7 @@ import com.google.gerrit.reviewdb.client.AccountExternalId; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroupName; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; @@ -35,6 +36,8 @@ import com.google.gerrit.server.account.GroupMembers; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.patch.AddReviewer; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; @@ -47,7 +50,6 @@ import java.util.ArrayList; import java.util.Collections; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -57,13 +59,15 @@ SuggestService { private static final String MAX_SUFFIX = "\u9fa5"; + private final Provider<ReviewDb> reviewDbProvider; private final ProjectControl.Factory projectControlFactory; private final ProjectCache projectCache; private final AccountCache accountCache; private final GroupControl.Factory groupControlFactory; private final GroupMembers.Factory groupMembersFactory; - private final IdentifiedUser.GenericFactory userFactory; + private final IdentifiedUser.GenericFactory identifiedUserFactory; private final AccountControl.Factory accountControlFactory; + private final ChangeControl.Factory changeControlFactory; private final Config cfg; private final GroupCache groupCache; private final boolean suggestAccounts; @@ -74,18 +78,21 @@ final ProjectCache projectCache, final AccountCache accountCache, final GroupControl.Factory groupControlFactory, final GroupMembers.Factory groupMembersFactory, - final IdentifiedUser.GenericFactory userFactory, final Provider<CurrentUser> currentUser, + final IdentifiedUser.GenericFactory identifiedUserFactory, final AccountControl.Factory accountControlFactory, + final ChangeControl.Factory changeControlFactory, @GerritServerConfig final Config cfg, final GroupCache groupCache) { super(schema, currentUser); + this.reviewDbProvider = schema; this.projectControlFactory = projectControlFactory; this.projectCache = projectCache; this.accountCache = accountCache; this.groupControlFactory = groupControlFactory; this.groupMembersFactory = groupMembersFactory; - this.userFactory = userFactory; + this.identifiedUserFactory = identifiedUserFactory; this.accountControlFactory = accountControlFactory; + this.changeControlFactory = changeControlFactory; this.cfg = cfg; this.groupCache = groupCache; @@ -126,17 +133,27 @@ callback.onSuccess(r); } + private interface VisibilityControl { + boolean isVisible(Account account) throws OrmException; + } + public void suggestAccount(final String query, final Boolean active, final int limit, final AsyncCallback<List<AccountInfo>> callback) { run(callback, new Action<List<AccountInfo>>() { public List<AccountInfo> run(final ReviewDb db) throws OrmException { - return suggestAccount(db, query, active, limit); + return suggestAccount(db, query, active, limit, new VisibilityControl() { + @Override + public boolean isVisible(Account account) throws OrmException { + return accountControlFactory.get().canSee(account); + } + }); } }); } private List<AccountInfo> suggestAccount(final ReviewDb db, - final String query, final Boolean active, final int limit) + final String query, final Boolean active, final int limit, + VisibilityControl visibilityControl) throws OrmException { if (!suggestAccounts) { return Collections.<AccountInfo> emptyList(); @@ -150,12 +167,12 @@ final LinkedHashMap<Account.Id, AccountInfo> r = new LinkedHashMap<Account.Id, AccountInfo>(); for (final Account p : db.accounts().suggestByFullName(a, b, n)) { - addSuggestion(r, p, new AccountInfo(p), active); + addSuggestion(r, p, new AccountInfo(p), active, visibilityControl); } if (r.size() < n) { for (final Account p : db.accounts().suggestByPreferredEmail(a, b, n - r.size())) { - addSuggestion(r, p, new AccountInfo(p), active); + addSuggestion(r, p, new AccountInfo(p), active, visibilityControl); } } if (r.size() < n) { @@ -165,7 +182,7 @@ final Account p = accountCache.get(e.getAccountId()).getAccount(); final AccountInfo info = new AccountInfo(p); info.setPreferredEmail(e.getEmailAddress()); - addSuggestion(r, p, info, active); + addSuggestion(r, p, info, active, visibilityControl); } } } @@ -173,14 +190,15 @@ } private void addSuggestion(Map<Account.Id, AccountInfo> map, Account account, - AccountInfo info, Boolean active) { + AccountInfo info, Boolean active, VisibilityControl visibilityControl) + throws OrmException { if (map.containsKey(account.getId())) { return; } if (active != null && active != account.isActive()) { return; } - if (accountControlFactory.get().canSee(account)) { + if (visibilityControl.isVisible(account)) { map.put(account.getId(), info); } } @@ -217,13 +235,35 @@ } @Override - public void suggestReviewer(final Project.NameKey project, + public void suggestReviewer(Project.NameKey project, String query, int limit, + AsyncCallback<List<ReviewerInfo>> callback) { + // The RPC is deprecated, but return an empty list for RPC API compatibility. + callback.onSuccess(Collections.<ReviewerInfo>emptyList()); + } + + @Override + public void suggestChangeReviewer(final Change.Id change, final String query, final int limit, final AsyncCallback<List<ReviewerInfo>> callback) { run(callback, new Action<List<ReviewerInfo>>() { public List<ReviewerInfo> run(final ReviewDb db) throws OrmException { + final ChangeControl changeControl; + try { + changeControl = changeControlFactory.controlFor(change); + } catch (NoSuchChangeException e) { + return Collections.emptyList(); + } + VisibilityControl visibilityControl = new VisibilityControl() { + @Override + public boolean isVisible(Account account) throws OrmException { + IdentifiedUser who = + identifiedUserFactory.create(reviewDbProvider, account.getId()); + return changeControl.forUser(who).isVisible(reviewDbProvider.get()); + } + }; + final List<AccountInfo> suggestedAccounts = - suggestAccount(db, query, Boolean.TRUE, limit); + suggestAccount(db, query, Boolean.TRUE, limit, visibilityControl); final List<ReviewerInfo> reviewer = new ArrayList<ReviewerInfo>(suggestedAccounts.size()); for (final AccountInfo a : suggestedAccounts) { @@ -232,7 +272,7 @@ final List<GroupReference> suggestedAccountGroups = suggestAccountGroup(db, query, limit); for (final GroupReference g : suggestedAccountGroups) { - if (suggestGroupAsReviewer(project, g)) { + if (suggestGroupAsReviewer(changeControl.getProject().getNameKey(), g)) { reviewer.add(new ReviewerInfo(g)); } }