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 {