Return current reviewers first in code owner suggestion
The frontend only suggests the best 5 code owners for a file. In the
suggestion dialog the frontend shows checkmarks for those files for
which one of the suggested code owners has been added or selected as
reviewer. If the change already has a reviewer that is a code owner of a
file, but this reviewer doesn't appear in suggested 5 code owners, the
frontend wrongly shows the file as not covered yet (no checkmark). To
fix this we are now returning code owners that are reviewers first when
suggesting code owners. If the current reviewers are returned first,
they appear in the suggested code owners and hence the checkmarking
logic in the frontend will just work.
To return code owners that are reviewers first, we add a IS_REVIEWER
score next to the existing DISTANCE score. The IS_REVIEWER score is only
applied if code owners are listed for a change (since otherwise there
are no reviewers). The scoring for the IS_REVIEWER score can be 0.0
(code owner is not a reviewer) and 1.0 (code owner is a reviewer). To
ensure that the IS_REVIEWER score takes precedence over the DISTANCE
score it has a higher weight. The IS_REVIEWER score has a weight of 2,
while the DISTANCE score has a weight of 1. This means for code owners
that are reviewers the total score is >= 2.0 (2 * 1.0 + 1 * distance
scoring, 0.0 <= distance scoring <= 1.0) while for code owners that are
no reviewers the total score is <= 1.0 (2 * 0.0 + 1 * distance scoring,
0.0 <= distance scoring <= 1.0).
In order for the sorting to work, we cannot abort the lookup of code
owners once we found the best 5 code owners by distance score (or
whatever limit is provided), but we must always iterate over all
relevant OWNERS files. We already need to do this in most cases to find
out if the file is owned by all users, so doing it always now shouldn't
make a big difference.
Having multiple scores with different weights was planned from the very
beginning, just so far we didn't have a need to implement weights as
there was only a single score.
Bug: Issue 14089
Change-Id: Id2d9f0ebe08be7b613033bf03946190e8d0d849f
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java
index 065425d..523c2ee 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScore.java
@@ -14,6 +14,11 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.gerrit.common.Nullable;
+import java.util.Optional;
+
/**
* Scores by which we rate how good we consider a code owner as reviewer/approver for a certain
* path.
@@ -36,7 +41,30 @@
* user Y is a better reviewer/approver for '/foo/bar/baz/' than user X as they have a lower
* distance.
*/
- DISTANCE(Kind.LOWER_VALUE_IS_BETTER);
+ DISTANCE(Kind.LOWER_VALUE_IS_BETTER, /* weight= */ 1, /* maxValue= */ null),
+
+ /**
+ * Score to take into account whether a code owner is a reviewer.
+ *
+ * <p>Code owners that are reviewers get scored with 1 (see {@link #IS_REVIEWER_SCORING_VALUE}),
+ * while code owners that are not a reviewer get scored with 0 (see {@link
+ * #NO_REVIEWER_SCORING_VALUE}).
+ *
+ * <p>The IS_REVIEWER score has a higher weight than the {@link #DISTANCE} score so that it takes
+ * precedence and code owners that are reviewers are always returned first.
+ */
+ IS_REVIEWER(Kind.GREATER_VALUE_IS_BETTER, /* weight= */ 2, /* maxValue= */ 1);
+
+ /**
+ * Scoring value for the {@link #IS_REVIEWER} score for users that are not a reviewer of the
+ * change.
+ */
+ public static int NO_REVIEWER_SCORING_VALUE = 0;
+
+ /**
+ * Scoring value for the {@link #IS_REVIEWER} score for users that are a reviewer of the change.
+ */
+ public static int IS_REVIEWER_SCORING_VALUE = 1;
/**
* Score kind.
@@ -57,17 +85,58 @@
*/
private final Kind kind;
- private CodeOwnerScore(Kind kind) {
+ /**
+ * The weight that this score should have when sorting code owners.
+ *
+ * <p>The higher the weight the larger the impact that this score has on the sorting.
+ */
+ private final double weight;
+
+ /**
+ * The max value that this score can have.
+ *
+ * <p>Not set if max value is not hard-coded, but is different case by case.
+ *
+ * <p>For scores that have a max value set scorings must be created by the {@link
+ * #createScoring()} method, for scores with flexible max values (maxValue = null) scorings must
+ * be created by the {@link #createScoring(int)} method.
+ */
+ @Nullable private final Integer maxValue;
+
+ private CodeOwnerScore(Kind kind, double weight, @Nullable Integer maxValue) {
this.kind = kind;
+ this.weight = weight;
+ this.maxValue = maxValue;
}
/**
* Creates a {@link CodeOwnerScoring.Builder} instance for this score.
*
+ * <p>Use {@link #createScoring()} instead if the score has a max value set.
+ *
* @param maxValue the max possible scoring value
* @return the created {@link CodeOwnerScoring.Builder} instance
*/
public CodeOwnerScoring.Builder createScoring(int maxValue) {
+ checkState(
+ this.maxValue == null,
+ "score %s has defined a maxValue, setting maxValue not allowed",
+ name());
+ return CodeOwnerScoring.builder(this, maxValue);
+ }
+
+ /**
+ * Creates a {@link CodeOwnerScoring.Builder} instance for this score.
+ *
+ * <p>Use {@link #createScoring(int)} instead if the score doesn't have a max value set.
+ *
+ * @return the created {@link CodeOwnerScoring.Builder} instance
+ */
+ public CodeOwnerScoring.Builder createScoring() {
+ checkState(
+ maxValue != null,
+ "score %s doesn't have a maxValue defined, setting maxValue is required",
+ name());
return CodeOwnerScoring.builder(this, maxValue);
}
@@ -79,4 +148,12 @@
boolean isLowerValueBetter() {
return Kind.LOWER_VALUE_IS_BETTER.equals(kind);
}
+
+ double weight() {
+ return weight;
+ }
+
+ Optional<Integer> maxValue() {
+ return Optional.ofNullable(maxValue);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoring.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoring.java
index 4694fd8..ce89e07 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoring.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoring.java
@@ -110,14 +110,15 @@
}
/**
- * Returns a comparator to sort code owners by the scorings collected in this {@link
- * CodeOwnerScoring} instance.
+ * Computes the weighted scoring for a code owner.
*
- * <p>Code owners with higher scoring come first. The order of code owners with the same scoring
- * is undefined.
+ * <p>The result of {@link #scoring(CodeOwner)} multiplied with the {@code score().weight()}.
+ *
+ * @param codeOwner for which the weighted scoring should be computed
+ * @return the weighted scoring for the code owner
*/
- public Comparator<CodeOwner> comparingByScoring() {
- return Comparator.comparingDouble(this::scoring).reversed();
+ public double weightedScoring(CodeOwner codeOwner) {
+ return score().weight() * scoring(codeOwner);
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
new file mode 100644
index 0000000..84a263b
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScorings.java
@@ -0,0 +1,68 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * Class to sort code owners based on their scorings on different {@link CodeOwnerScore}s.
+ *
+ * <p>To determine the sort order the scorings are weighted based on the {@link
+ * CodeOwnerScore#weight()} of the {@link CodeOwnerScore} on which the scoring was done.
+ */
+@AutoValue
+public abstract class CodeOwnerScorings {
+ /** The scorings that should be taken into account for sorting the code owners. */
+ public abstract ImmutableSet<CodeOwnerScoring> scorings();
+
+ public static CodeOwnerScorings create(CodeOwnerScoring... codeOwnerScorings) {
+ return new AutoValue_CodeOwnerScorings(ImmutableSet.copyOf(codeOwnerScorings));
+ }
+
+ public static CodeOwnerScorings create(Set<CodeOwnerScoring> codeOwnerScorings) {
+ return new AutoValue_CodeOwnerScorings(ImmutableSet.copyOf(codeOwnerScorings));
+ }
+
+ public static CodeOwnerScorings appendScoring(
+ CodeOwnerScorings codeOwnerScorings, CodeOwnerScoring codeOwnerScoringToBeAdded) {
+ Set<CodeOwnerScoring> codeOwnerScoringSet = new HashSet<>(codeOwnerScorings.scorings());
+ codeOwnerScoringSet.add(codeOwnerScoringToBeAdded);
+ return create(codeOwnerScoringSet);
+ }
+
+ /**
+ * Returns a comparator to sort code owners by the collected scorings.
+ *
+ * <p>Code owners with higher scoring come first. The order of code owners with the same scoring
+ * is undefined.
+ */
+ public Comparator<CodeOwner> comparingByScorings() {
+ return Comparator.comparingDouble(this::sumWeightedScorings).reversed();
+ }
+
+ /** Returns the sum of all weighted scorings that available for the given code owner. */
+ private double sumWeightedScorings(CodeOwner codeOwner) {
+ double sum =
+ scorings().stream()
+ .map(scoring -> scoring.weightedScoring(codeOwner))
+ .collect(Collectors.summingDouble(Double::doubleValue));
+ return sum;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 990ecb7..d406be1 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -38,6 +38,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolverResult;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.account.AccountControl;
@@ -190,9 +191,7 @@
limit, codeOwners.size());
}
- // We already found that the path is owned by all users. Hence we do not need to check
- // if there are further code owners in higher-level code owner configs.
- return false;
+ return true;
}
int distance =
@@ -204,9 +203,6 @@
.forEach(
localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance));
- // If codeOwners.size() >= limit we have gathered enough code owners, but we still need to
- // inspect the further code owner configs to know if the path is maybe owned by all user
- // (in which case we need to set the ownedByAllUsers flag in the response).
return true;
});
@@ -228,7 +224,11 @@
codeOwnersInfo.codeOwners =
codeOwnerJsonFactory
.create(getFillOptions())
- .format(sortAndLimit(distanceScoring.build(), ImmutableSet.copyOf(codeOwners)));
+ .format(
+ sortAndLimit(
+ rsrc,
+ CodeOwnerScorings.create(distanceScoring.build()),
+ ImmutableSet.copyOf(codeOwners)));
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
return Response.ok(codeOwnersInfo);
}
@@ -348,10 +348,8 @@
}
private ImmutableList<CodeOwner> sortAndLimit(
- CodeOwnerScoring distanceScoring, ImmutableSet<CodeOwner> codeOwners) {
- return sortCodeOwners(seed, distanceScoring, codeOwners)
- .limit(limit)
- .collect(toImmutableList());
+ R rsrc, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
+ return sortCodeOwners(rsrc, seed, scorings, codeOwners).limit(limit).collect(toImmutableList());
}
/**
@@ -361,14 +359,15 @@
*
* <p>The order of code owners with the same distance score is random.
*
+ * @param rsrc resource on which this REST endpoint is invoked
* @param seed seed that should be used to randomize the order
- * @param distanceScoring the distance scorings for the code owners
+ * @param scorings the scorings for the code owners
* @param codeOwners the code owners that should be sorted
* @return the sorted code owners
*/
- private static Stream<CodeOwner> sortCodeOwners(
- Optional<Long> seed, CodeOwnerScoring distanceScoring, ImmutableSet<CodeOwner> codeOwners) {
- return randomizeOrder(seed, codeOwners).sorted(distanceScoring.comparingByScoring());
+ protected Stream<CodeOwner> sortCodeOwners(
+ R rsrc, Optional<Long> seed, CodeOwnerScorings scorings, ImmutableSet<CodeOwner> codeOwners) {
+ return randomizeOrder(seed, codeOwners).sorted(scorings.comparingByScorings());
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 5e188b8..f475550 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -14,7 +14,12 @@
package com.google.gerrit.plugins.codeowners.restapi;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_REVIEWER_SCORING_VALUE;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
+
+import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.common.AccountVisibility;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -22,10 +27,14 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwner;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
@@ -83,6 +92,35 @@
return Optional.of(Long.valueOf(rsrc.getRevisionResource().getChange().getId().get()));
}
+ /**
+ * This method is overridden to add scorings for the {@link CodeOwnerScore#IS_REVIEWER} score that
+ * only applies if code owners are suggested on changes.
+ */
+ @Override
+ protected Stream<CodeOwner> sortCodeOwners(
+ CodeOwnersInChangeCollection.PathResource rsrc,
+ Optional<Long> seed,
+ CodeOwnerScorings scorings,
+ ImmutableSet<CodeOwner> codeOwners) {
+ // Add scorings for IS_REVIEWER score.
+ ImmutableSet<Account.Id> reviewers =
+ rsrc.getRevisionResource()
+ .getNotes()
+ .getReviewers()
+ .byState(ReviewerStateInternal.REVIEWER);
+ CodeOwnerScoring.Builder isReviewerScoring = CodeOwnerScore.IS_REVIEWER.createScoring();
+ codeOwners.forEach(
+ codeOwner ->
+ isReviewerScoring.putValueForCodeOwner(
+ codeOwner,
+ reviewers.contains(codeOwner.accountId())
+ ? IS_REVIEWER_SCORING_VALUE
+ : NO_REVIEWER_SCORING_VALUE));
+ scorings = CodeOwnerScorings.appendScoring(scorings, isReviewerScoring.build());
+
+ return super.sortCodeOwners(rsrc, seed, scorings, codeOwners);
+ }
+
@Override
protected Stream<CodeOwner> filterCodeOwners(
CodeOwnersInChangeCollection.PathResource rsrc, Stream<CodeOwner> codeOwners) {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
index c3e7ee1..e0802a1 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -324,4 +324,69 @@
.comparingElementsUsing(hasAccountId())
.containsExactly(user.id());
}
+
+ @Test
+ public void codeOwnersThatAreReviewersAreReturnedFirst() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ // None of the code owners is a reviewer, hence the sorting is done only by distance.
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id(), user.id(), admin.id())
+ .inOrder();
+
+ // Add admin as reviewer, now admin should be returned first.
+ gApi.changes().id(changeId).addReviewer(admin.email());
+ codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user2.id(), user.id())
+ .inOrder();
+
+ // Add user as reviewer, now user and admin should be returned first (user before admin, because
+ // user has a better distance score).
+ gApi.changes().id(changeId).addReviewer(user.email());
+ codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), admin.id(), user2.id())
+ .inOrder();
+
+ // If all code owners are reviewers, only the distance score matters for the sorting.
+ gApi.changes().id(changeId).addReviewer(user2.email());
+ codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user2.id(), user.id(), admin.id())
+ .inOrder();
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoreTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoreTest.java
new file mode 100644
index 0000000..f0072f0
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoreTest.java
@@ -0,0 +1,55 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnerScore}. */
+public class CodeOwnerScoreTest extends AbstractCodeOwnersTest {
+ @Test
+ public void createScoringForScoreThatDefinesAMaxValue() throws Exception {
+ assertThat(CodeOwnerScore.IS_REVIEWER.createScoring().build().maxValue())
+ .isEqualTo(CodeOwnerScore.IS_REVIEWER.maxValue().get());
+ }
+
+ @Test
+ public void cannotCreateScoringWithMaxValueForScoreThatDefinesAMaxValue() throws Exception {
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class, () -> CodeOwnerScore.IS_REVIEWER.createScoring(5));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("score IS_REVIEWER has defined a maxValue, setting maxValue not allowed");
+ }
+
+ @Test
+ public void createScoringWithMaxValueForScoreThatDosntDefineAMaxValue() throws Exception {
+ assertThat(CodeOwnerScore.DISTANCE.createScoring(5).build().maxValue()).isEqualTo(5);
+ }
+
+ @Test
+ public void cannotCreateScoringWithoutMaxValueForScoreThatDoesntDefinesAMaxValue()
+ throws Exception {
+ IllegalStateException exception =
+ assertThrows(IllegalStateException.class, () -> CodeOwnerScore.DISTANCE.createScoring());
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("score DISTANCE doesn't have a maxValue defined, setting maxValue is required");
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringTest.java
index 0cab290..43498c6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringTest.java
@@ -19,8 +19,6 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
-import java.util.ArrayList;
-import java.util.Comparator;
import java.util.Optional;
import org.junit.Test;
@@ -108,45 +106,4 @@
.scoring(CodeOwner.create(admin.id())))
.isZero();
}
-
- @Test
- public void sortCodeOwnersByScorings() throws Exception {
- CodeOwner codeOwner1 = CodeOwner.create(admin.id());
- CodeOwner codeOwner2 = CodeOwner.create(user.id());
- CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
-
- ArrayList<CodeOwner> codeOwners = new ArrayList<>();
- codeOwners.add(codeOwner1);
- codeOwners.add(codeOwner2);
- codeOwners.add(codeOwner3);
-
- // lower distance is better
- Comparator<CodeOwner> comparator =
- CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
- .putValueForCodeOwner(codeOwner1, 50)
- .putValueForCodeOwner(codeOwner2, 100)
- .putValueForCodeOwner(codeOwner3, 0)
- .build()
- .comparingByScoring();
- codeOwners.sort(comparator);
- assertThat(codeOwners).containsExactly(codeOwner3, codeOwner1, codeOwner2).inOrder();
- }
-
- @Test
- public void sortCodeOwnersByScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
- CodeOwner codeOwner1 = CodeOwner.create(admin.id());
- CodeOwner codeOwner2 = CodeOwner.create(user.id());
-
- ArrayList<CodeOwner> codeOwners = new ArrayList<>();
- codeOwners.add(codeOwner1);
- codeOwners.add(codeOwner2);
-
- Comparator<CodeOwner> comparator =
- CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
- .putValueForCodeOwner(codeOwner2, 50)
- .build()
- .comparingByScoring();
- codeOwners.sort(comparator);
- assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
- }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
new file mode 100644
index 0000000..1bb7090
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerScoringsTest.java
@@ -0,0 +1,128 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_REVIEWER_SCORING_VALUE;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NO_REVIEWER_SCORING_VALUE;
+
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import java.util.ArrayList;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnerScorings}. */
+public class CodeOwnerScoringsTest extends AbstractCodeOwnersTest {
+ @Test
+ public void sortCodeOwnersByLowerValueIsBetterScore() throws Exception {
+ CodeOwner codeOwner1 = CodeOwner.create(admin.id());
+ CodeOwner codeOwner2 = CodeOwner.create(user.id());
+ CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
+
+ ArrayList<CodeOwner> codeOwners = new ArrayList<>();
+ codeOwners.add(codeOwner1);
+ codeOwners.add(codeOwner2);
+ codeOwners.add(codeOwner3);
+
+ // lower distance is better
+ CodeOwnerScoring distanceScoring =
+ CodeOwnerScore.DISTANCE
+ .createScoring(100)
+ .putValueForCodeOwner(codeOwner1, 50)
+ .putValueForCodeOwner(codeOwner2, 100)
+ .putValueForCodeOwner(codeOwner3, 0)
+ .build();
+ codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
+ assertThat(codeOwners).containsExactly(codeOwner3, codeOwner1, codeOwner2).inOrder();
+ }
+
+ @Test
+ public void sortCodeOwnersByGreaterValueIsBetterScore() throws Exception {
+ CodeOwner codeOwner1 = CodeOwner.create(admin.id());
+ CodeOwner codeOwner2 = CodeOwner.create(user.id());
+
+ ArrayList<CodeOwner> codeOwners = new ArrayList<>();
+ codeOwners.add(codeOwner1);
+ codeOwners.add(codeOwner2);
+
+ // for the IS_REVIEWER score a greater score is better
+ CodeOwnerScoring isReviewerScoring =
+ CodeOwnerScore.IS_REVIEWER
+ .createScoring()
+ .putValueForCodeOwner(codeOwner1, 0)
+ .putValueForCodeOwner(codeOwner2, 1)
+ .build();
+ codeOwners.sort(CodeOwnerScorings.create(isReviewerScoring).comparingByScorings());
+ assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+ }
+
+ @Test
+ public void sortCodeOwnersByMultipleScore() throws Exception {
+ CodeOwner codeOwner1 = CodeOwner.create(admin.id());
+ CodeOwner codeOwner2 = CodeOwner.create(user.id());
+ CodeOwner codeOwner3 = CodeOwner.create(accountCreator.user2().id());
+
+ ArrayList<CodeOwner> codeOwners = new ArrayList<>();
+ codeOwners.add(codeOwner1);
+ codeOwners.add(codeOwner2);
+ codeOwners.add(codeOwner3);
+
+ // lower distance is better
+ CodeOwnerScoring distanceScoring =
+ CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
+ .putValueForCodeOwner(codeOwner1, 50)
+ .putValueForCodeOwner(codeOwner2, 75)
+ .putValueForCodeOwner(codeOwner3, 0)
+ .build();
+
+ // for the IS_REVIEWER score a greater score is better
+ CodeOwnerScoring isReviewerScoring =
+ CodeOwnerScore.IS_REVIEWER
+ .createScoring()
+ .putValueForCodeOwner(codeOwner1, NO_REVIEWER_SCORING_VALUE)
+ .putValueForCodeOwner(codeOwner2, IS_REVIEWER_SCORING_VALUE)
+ .putValueForCodeOwner(codeOwner3, NO_REVIEWER_SCORING_VALUE)
+ .build();
+
+ codeOwners.sort(
+ CodeOwnerScorings.create(distanceScoring, isReviewerScoring).comparingByScorings());
+
+ // Expected scorings:
+ // codeOwner1: DISTANCE(weight=1)=0.5, IS_REVIEWER(weight=2)=0.0
+ // -> total scoring: 0.5 * 1 + 0.0 * 2 = 0.5
+ // codeOwner2: DISTANCE(weight=1)=0.25, IS_REVIEWER(weight=2)=1.0
+ // -> total scoring: 0.25 * 1 + 1.0 * 2 = 2.25
+ // codeOwner3: DISTANCE(weight=1)=1.0, IS_REVIEWER(weight=2)=0.0
+ // -> total scoring: 1.0 * 1 + 0.0 * 2 = 1.0
+ // => expected order (highest total score first): codeOwner2, codeOwner3, codeOwner1
+ assertThat(codeOwners).containsExactly(codeOwner2, codeOwner3, codeOwner1).inOrder();
+ }
+
+ @Test
+ public void sortCodeOwnersByScoringsIfAnyCodeOwnerHasNoScoring() throws Exception {
+ CodeOwner codeOwner1 = CodeOwner.create(admin.id());
+ CodeOwner codeOwner2 = CodeOwner.create(user.id());
+
+ ArrayList<CodeOwner> codeOwners = new ArrayList<>();
+ codeOwners.add(codeOwner1);
+ codeOwners.add(codeOwner2);
+
+ CodeOwnerScoring distanceScoring =
+ CodeOwnerScoring.builder(CodeOwnerScore.DISTANCE, 100)
+ .putValueForCodeOwner(codeOwner2, 50)
+ .build();
+ codeOwners.sort(CodeOwnerScorings.create(distanceScoring).comparingByScorings());
+ assertThat(codeOwners).containsExactly(codeOwner2, codeOwner1).inOrder();
+ }
+}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 68d8f50..b977f3c 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -479,6 +479,12 @@
* distance of the code owner config file that defines the code owner to the
path for which code owners are listed (the lower the distance the better the
code owner)
+* whether the code owner is a reviewer of the change (only when listing code
+ owners for a change)
+
+The distance score has a lower weight than the is-reviewer score, hence when
+listing code owners for a change, code owners that are reviewers are always
+returned first.
Other factors like OOO state, recent review activity or code authorship are not
considered.
