Suggest owners that are reviewers even if annotated with LAST_RESORT_SUGGESTION
So far code owners that are annotated with LAST_RESORT_SUGGESTION were
only suggested if the suggestion result would be empty otherwise. This
meant that these code owners were also not suggested after they have
been manually added as a reviewer. Due to this the UI couldn't show
files that were owned by these users with a check mark (= covered by a
reviewer) as the UI only considers code owners that are part of the
suggestion. To fix this issue, code owners that are annotated with
LAST_RESORT_SUGGESTION are now also included in the suggestion result if
they are already a reviewer. This means if a code owner is already a
reviewer the LAST_RESORT_SUGGESTION annotation is ignored.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I6b6a0a7d776e166d655076019bbc3596a713517f
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 0b3d3fc..3ed9a51 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -152,7 +152,7 @@
filteredCodeOwners.stream()
.filter(
filterOutCodeOwnersThatAreAnnotatedWithLastResortSuggestion(
- annotations, debugLogs))
+ rsrc, annotations, debugLogs))
.collect(toImmutableList());
if (filteredCodeOwnersWithoutCodeOwnersThatAreAnnotatedWithLastResortSuggestion.isEmpty()) {
// The result would be empty, hence return code owners that are annotated with
@@ -177,12 +177,27 @@
}
private Predicate<CodeOwner> filterOutCodeOwnersThatAreAnnotatedWithLastResortSuggestion(
+ CodeOwnersInChangeCollection.PathResource rsrc,
ImmutableMultimap<CodeOwner, CodeOwnerAnnotation> annotations,
ImmutableList.Builder<String> debugLogs) {
return codeOwner -> {
boolean lastResortSuggestion =
annotations.containsEntry(
codeOwner, CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION);
+
+ // If the code owner is already a reviewer, the code owner should always be suggested, even
+ // if annotated with LAST_RESORT_SUGGESTION_ANNOTATION.
+ if (isReviewer(rsrc, codeOwner)) {
+ if (lastResortSuggestion) {
+ debugLogs.add(
+ String.format(
+ "ignoring %s annotation for %s because this code owner is a reviewer",
+ CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION.key(), codeOwner));
+ }
+
+ // Returning true from the Predicate here means that the code owner should be kept.
+ return true;
+ }
if (!lastResortSuggestion) {
// Returning true from the Predicate here means that the code owner should be kept.
return true;
@@ -196,6 +211,14 @@
};
}
+ private boolean isReviewer(CodeOwnersInChangeCollection.PathResource rsrc, CodeOwner codeOwner) {
+ return rsrc.getRevisionResource()
+ .getNotes()
+ .getReviewers()
+ .byState(ReviewerStateInternal.REVIEWER)
+ .contains(codeOwner.accountId());
+ }
+
private Predicate<CodeOwner> filterOutServiceUsers(ImmutableList.Builder<String> debugLogs) {
return codeOwner -> {
if (!isServiceUser(codeOwner)) {
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 1f09ce1..a4d69f4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -401,6 +401,51 @@
}
@Test
+ public void
+ codeOwnersWithLastResortSuggestionAnnotation_annotationIgnoredIfCodeOwnerIsAlreadyAReviewer()
+ throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .addAnnotation(user.email(), CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION)
+ .addCodeOwnerEmail(user2.email())
+ .addAnnotation(
+ user2.email(), CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION)
+ .build())
+ .create();
+
+ // Expectation: admin is suggested, user and user2 get filtered out due to the
+ // LAST_RESORT_SUGGESTION annotation
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id());
+
+ // Add user as a reviewer.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Expectation: user is being suggested now despite of the LAST_RESORT_SUGGESTION annotation
+ // because user is a reviewer, admin is still suggested and user2 is still filtered out due to
+ // the LAST_RESORT_SUGGESTION annotation
+ codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id());
+ }
+
+ @Test
public void codeOwnersWithLastResortSuggestionAnnotation_annotationSetForAllUsersWildcard()
throws Exception {
skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
@@ -507,6 +552,52 @@
}
@Test
+ public void
+ perFileCodeOwnersWithLastResortSuggestionAnnotation_annotationIgnoredIfCodeOwnerIsAlreadyAReviewer()
+ throws Exception {
+ skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .addAnnotation(user.email(), CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION)
+ .addCodeOwnerEmail(user2.email())
+ .addAnnotation(
+ user2.email(), CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION)
+ .build())
+ .create();
+
+ // Expectation: admin is suggested, user and user2 get filtered out due to the
+ // LAST_RESORT_SUGGESTION annotation
+ CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id());
+
+ // Add user as a reviewer.
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ // Expectation: user is being suggested now despite of the LAST_RESORT_SUGGESTION annotation
+ // because user is a reviewer, admin is still suggested and user2 is still filtered out due to
+ // the LAST_RESORT_SUGGESTION annotation
+ codeOwnersInfo = queryCodeOwners("foo/bar/baz.md");
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id());
+ }
+
+ @Test
public void perFileCodeOwnersWithLastResortSuggestionAnnotation_annotationSetForAllUsersWildcard()
throws Exception {
skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md
index 21125a9..6680d40 100644
--- a/resources/Documentation/backend-find-owners.md
+++ b/resources/Documentation/backend-find-owners.md
@@ -318,11 +318,12 @@
* `LAST_RESORT_SUGGESTION`:
Code owners with this annotation are omitted when [suggesting code
owners](rest-api.html#list-code-owners-for-path-in-change), except if dropping
- these code owners would make the suggestion result empty. If code ownership is
- assigned to the same code owner through multiple relevant access grants in the
- same code owner config file or in other relevant code owner config files the
- code owner gets omitted from the suggestion if it has the
- `LAST_RESORT_SUGGESTION` set on any of the access grants.
+ these code owners would make the suggestion result empty or if these code
+ owners are already reviewers of the change. If code ownership is assigned to
+ the same code owner through multiple relevant access grants in the same code
+ owner config file or in other relevant code owner config files the code owner
+ gets omitted from the suggestion if it has the `LAST_RESORT_SUGGESTION` set on
+ any of the access grants.
Unknown annotations are silently ignored.
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 0227a5f..f08a932 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -700,7 +700,8 @@
* the change owner (since the change owner cannot be added as reviewer)
* code owners that are annotated with
[LAST_RESORT_SUGGESTION](backend-find-owners.html#lastResortSuggestion),
- except if dropping these code owners would make the suggestion result empty
+ except if dropping these code owners would make the suggestion result empty or
+ if these code owners are already reviewers of the change
In addition, by default the change number is used as seed if none was specified.
This way the sort order on a change is always the same for files that have the
