opendev/gerrit
Code Issues Proposed changes

Cleanup SuggestReviewers

Browse Source 
When matching accounts by email address against an AccountExternalId,
results should use the email address that matches, not the preferred
email address.

This allows users to search for "spearce@spearce.org" on gerrit-review
and not be confused when "sop@google.com" is displayed in suggestions.
Instead my personal email will be displayed in the results.

The active flag is now handled down inside of the reviewer cache for
full text cases, so it does not get considered anymore in the basic
addSuggestion path.

Change-Id: I95e293a50e0c3ac3fe209d8979a23b782b583fab
This commit is contained in:
Shawn Pearce
2014-12-26 20:33:15 -05:00
parent 86f0aae8e4
commit 7b4cd6f5ee
1 changed files with 47 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java
View File

@@ -18,7 +18,6 @@ import com.google.common.base.MoreObjects;
import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.common.AccountInfo;
@@ -49,20 +48,21 @@ import org.eclipse.jgit.lib.Config;
import org.kohsuke.args4j.Option;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
public class SuggestReviewers implements RestReadView<ChangeResource> {
private static final String MAX_SUFFIX = "\u9fa5";
private static final int DEFAULT_MAX_SUGGESTED = 10;
private static final int DEFAULT_MAX_MATCHES = 100;
private final AccountLoader.Factory accountLoaderFactory;
private final AccountControl.Factory accountControlFactory;
private final AccountLoader accountLoader;
private final AccountControl accountControl;
private final GroupMembers.Factory groupMembersFactory;
private final AccountCache accountCache;
private final Provider<ReviewDb> dbProvider;
@@ -105,8 +105,8 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
@GerritServerConfig Config cfg,
GroupBackend groupBackend,
ReviewerSuggestionCache reviewerSuggestionCache) {
this.accountLoaderFactory = accountLoaderFactory;
this.accountControlFactory = accountControlFactory;
this.accountLoader = accountLoaderFactory.create(true);
this.accountControl = accountControlFactory.get();
this.accountCache = accountCache;
this.groupMembersFactory = groupMembersFactory;
this.dbProvider = dbProvider;
@@ -135,7 +135,7 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
}
private interface VisibilityControl {
boolean isVisibleTo(Account account) throws OrmException;
boolean isVisibleTo(Account.Id account) throws OrmException;
}
@Override
@@ -156,7 +156,6 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
} else {
suggestedAccounts = suggestAccount(visibilityControl);
}
accountLoaderFactory.create(true).fill(suggestedAccounts);
List<SuggestedReviewerInfo> reviewer = Lists.newArrayList();
for (AccountInfo a : suggestedAccounts) {
@@ -186,16 +185,16 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
if (rsrc.getControl().getRefControl().isVisibleByRegisteredUsers()) {
return new VisibilityControl() {
@Override
public boolean isVisibleTo(Account account) throws OrmException {
public boolean isVisibleTo(Account.Id account) throws OrmException {
return true;
}
};
} else {
return new VisibilityControl() {
@Override
public boolean isVisibleTo(Account account) throws OrmException {
public boolean isVisibleTo(Account.Id account) throws OrmException {
IdentifiedUser who =
identifiedUserFactory.create(dbProvider, account.getId());
identifiedUserFactory.create(dbProvider, account);
// we can't use changeControl directly as it won't suggest reviewers
// to drafts
return rsrc.getControl().forUser(who).isRefVisible();
@@ -214,16 +213,22 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
String a = query;
String b = a + MAX_SUFFIX;
LinkedHashMap<Account.Id, AccountInfo> r = Maps.newLinkedHashMap();
Map<Account.Id, AccountInfo> r = new LinkedHashMap<>();
Map<Account.Id, String> queryEmail = new HashMap<>();
for (Account p : dbProvider.get().accounts()
.suggestByFullName(a, b, limit)) {
addSuggestion(r, p, new AccountInfo(p.getId().get()), visibilityControl);
if (p.isActive()) {
addSuggestion(r, p.getId(), visibilityControl);
}
}
if (r.size() < limit) {
for (Account p : dbProvider.get().accounts()
.suggestByPreferredEmail(a, b, limit - r.size())) {
addSuggestion(r, p, new AccountInfo(p.getId().get()), visibilityControl);
if (p.isActive()) {
addSuggestion(r, p.getId(), visibilityControl);
}
}
}
@@ -232,21 +237,32 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
.suggestByEmailAddress(a, b, limit - r.size())) {
if (!r.containsKey(e.getAccountId())) {
Account p = accountCache.get(e.getAccountId()).getAccount();
AccountInfo info = new AccountInfo(p.getId().get());
addSuggestion(r, p, info, visibilityControl);
if (p.isActive()) {
if (addSuggestion(r, p.getId(), visibilityControl)) {
queryEmail.put(e.getAccountId(), e.getEmailAddress());
}
}
}
}
}
return Lists.newArrayList(r.values());
accountLoader.fill();
for (Map.Entry<Account.Id, String> p : queryEmail.entrySet()) {
AccountInfo info = r.get(p.getKey());
if (info != null) {
info.email = p.getValue();
}
}
return new ArrayList<>(r.values());
}
private List<AccountInfo> suggestAccountFullTextSearch(
VisibilityControl visibilityControl) throws OrmException {
String str = query.toLowerCase();
LinkedHashMap<Account.Id, AccountInfo> accountMap = Maps.newLinkedHashMap();
List<Account> fullNameMatches = Lists.newArrayListWithCapacity(fullTextMaxMatches);
List<Account> emailMatches = Lists.newArrayListWithCapacity(fullTextMaxMatches);
Map<Account.Id, AccountInfo> accountMap = new LinkedHashMap<>();
List<Account> fullNameMatches = new ArrayList<>(fullTextMaxMatches);
List<Account> emailMatches = new ArrayList<>(fullTextMaxMatches);
for (Account a : reviewerSuggestionCache.get()) {
if (a.getFullName() != null
&& a.getFullName().toLowerCase().contains(str)) {
@@ -261,33 +277,35 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
}
}
for (Account a : fullNameMatches) {
addSuggestion(accountMap, a, new AccountInfo(a.getId().get()), visibilityControl);
addSuggestion(accountMap, a.getId(), visibilityControl);
if (accountMap.size() >= limit) {
break;
}
}
if (accountMap.size() < limit) {
for (Account a : emailMatches) {
addSuggestion(accountMap, a, new AccountInfo(a.getId().get()), visibilityControl);
addSuggestion(accountMap, a.getId(), visibilityControl);
if (accountMap.size() >= limit) {
break;
}
}
}
accountLoader.fill();
return Lists.newArrayList(accountMap.values());
}
private void addSuggestion(Map<Account.Id, AccountInfo> map, Account account,
AccountInfo info, VisibilityControl visibilityControl)
private boolean addSuggestion(Map<Account.Id, AccountInfo> map,
Account.Id account, VisibilityControl visibilityControl)
throws OrmException {
if (!map.containsKey(account.getId())
&& account.isActive()
if (!map.containsKey(account)
// Can the suggestion see the change?
&& visibilityControl.isVisibleTo(account)
// Can the account see the current user?
&& accountControlFactory.get().canSee(account)) {
map.put(account.getId(), info);
&& accountControl.canSee(account)) {
map.put(account, accountLoader.get(account));
return true;
}
return false;
}
private boolean suggestGroupAsReviewer(Project project,
@@ -312,7 +330,7 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
// require that at least one member in the group can see the change
for (Account account : members) {
if (visibilityControl.isVisibleTo(account)) {
if (visibilityControl.isVisibleTo(account.getId())) {
return true;
}
}