Merge changes Ie8370684,I70304e87
* changes:
Add Gerrit Config for number of relevant changes to suggest reviewers
Suggest reviewers based on past reviewers instead of approvals
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 880626e..6968e83 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -4784,6 +4784,15 @@
used for suggesting accounts when adding members to a group.
+
By default 0.
+[[suggest.relevantChanges]]suggest.relevantChanges::
++
+When suggesting reviewers, we go over recent changes of the user, and
+give priority to users that are present as reviewers in any of those
+changes. The number of changes we go over is `sugggest.relevantChanges`.
++
+By default 50. This nubmer is a tradeoff between speed and accuracy.
+A high number would be accurate but slow, and a low number would be
+fast but inaccurate.
[[tracing]]
=== Section tracing
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
index f07d815..39df82d 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java
@@ -22,7 +22,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.ApprovalsUtil;
@@ -202,23 +201,25 @@
private Map<Account.Id, MutableDouble> baseRanking(
double baseWeight, String query, List<Account.Id> candidateList)
throws IOException, ConfigInvalidException {
- // Get the user's last 25 changes, check approvals
+ int numberOfRelevantChanges = config.getInt("suggest", "relevantChanges", 50);
+ // Get the user's last 25 changes, check reviewers
try {
List<ChangeData> result =
queryProvider
.get()
- .setLimit(25)
- .setRequestedFields(ChangeField.APPROVAL)
+ .setLimit(numberOfRelevantChanges)
+ .setRequestedFields(ChangeField.REVIEWER)
.query(changeQueryBuilder.owner("self"));
Map<Account.Id, MutableDouble> suggestions = new LinkedHashMap<>();
// Put those candidates at the bottom of the list
candidateList.stream().forEach(id -> suggestions.put(id, new MutableDouble(0)));
for (ChangeData cd : result) {
- for (PatchSetApproval approval : cd.currentApprovals()) {
- Account.Id id = approval.accountId();
- if (Strings.isNullOrEmpty(query) || accountMatchesQuery(id, query)) {
- suggestions.computeIfAbsent(id, (ignored) -> new MutableDouble(0)).add(baseWeight);
+ for (Account.Id reviewer : cd.reviewers().all()) {
+ if (Strings.isNullOrEmpty(query) || accountMatchesQuery(reviewer, query)) {
+ suggestions
+ .computeIfAbsent(reviewer, (ignored) -> new MutableDouble(0))
+ .add(baseWeight);
}
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index 568c63b..1bd2d99 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -39,7 +39,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
-import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
@@ -397,19 +396,13 @@
requestScopeOperations.setApiUser(user1.id());
String changeId1 = createChangeFromApi();
- requestScopeOperations.setApiUser(reviewer1.id());
- reviewChange(changeId1);
+ reviewChange(changeId1, reviewer1);
- requestScopeOperations.setApiUser(user1.id());
String changeId2 = createChangeFromApi();
- requestScopeOperations.setApiUser(reviewer1.id());
- reviewChange(changeId2);
+ reviewChange(changeId2, reviewer1);
+ reviewChange(changeId2, reviewer2);
- requestScopeOperations.setApiUser(reviewer2.id());
- reviewChange(changeId2);
-
- requestScopeOperations.setApiUser(user1.id());
String changeId3 = createChangeFromApi();
List<SuggestedReviewerInfo> reviewers = suggestReviewers(changeId3, null, 4);
assertThat(reviewers.stream().map(r -> r.account._accountId).collect(toList()))
@@ -440,13 +433,11 @@
String name = name("foo");
TestAccount foo1 = accountCreator.create(name + "-1");
- requestScopeOperations.setApiUser(foo1.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo1);
assertThat(gApi.accounts().id(foo1.username()).getActive()).isTrue();
TestAccount foo2 = accountCreator.create(name + "-2");
- requestScopeOperations.setApiUser(foo2.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo2);
assertThat(gApi.accounts().id(foo2.username()).getActive()).isTrue();
assertReviewers(
@@ -466,12 +457,10 @@
String name = name("foo");
TestAccount foo1 = accountCreator.create(name + "-1");
- requestScopeOperations.setApiUser(foo1.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo1);
TestAccount foo2 = accountCreator.create(name + "-2");
- requestScopeOperations.setApiUser(foo2.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo2);
assertReviewers(
suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
@@ -488,12 +477,10 @@
String name = name("foo");
TestAccount foo1 = accountCreator.create(name + "-1");
- requestScopeOperations.setApiUser(foo1.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo1);
TestAccount foo2 = accountCreator.create(name + "-2");
- requestScopeOperations.setApiUser(foo2.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo2);
assertReviewers(
suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
@@ -514,12 +501,10 @@
String name = name("foo");
TestAccount foo1 = accountCreator.create(name + "-1");
- requestScopeOperations.setApiUser(foo1.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo1);
TestAccount foo2 = accountCreator.create(name + "-2");
- requestScopeOperations.setApiUser(foo2.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, foo2);
assertReviewers(suggestCcs(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
@@ -575,8 +560,7 @@
String changeIdReviewed = createChangeFromApi();
TestAccount reviewer = accountCreator.create("newReviewer");
- requestScopeOperations.setApiUser(reviewer.id());
- reviewChange(changeIdReviewed);
+ reviewChange(changeIdReviewed, reviewer);
List<SuggestedReviewerInfo> reviewers = suggestReviewers(changeId, "new", 4);
assertThat(reviewers.stream().map(r -> r.account._accountId).collect(toList()))
@@ -624,10 +608,8 @@
return user(name, fullName, name);
}
- private void reviewChange(String changeId) throws RestApiException {
- ReviewInput ri = new ReviewInput();
- ri.label("Code-Review", 1);
- gApi.changes().id(changeId).current().review(ri);
+ private void reviewChange(String changeId, TestAccount reviewer) throws RestApiException {
+ gApi.changes().id(changeId).addReviewer(reviewer.id().toString());
}
private String createChangeFromApi() throws RestApiException {
