Filter out the files owners based on the owners' code-review value
The GetFilesOwners did not process the ownership's review labels for
filtering out the total set of files owners requirements, making the
list of owners' requirements less effective.
The GetFilesOwners should not display anymore the files that have their
owners requirements already satisfied. Once all the owners have provided
their feedback, the payload of GetFilesOwners, limited to the files
list, it should be empty.
Bug: Issue 40015590
Change-Id: I7b513985c0711d9e3c318e629f8529effcd1a190
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
index b2d7458..9030bfc 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -18,6 +18,7 @@
import com.google.common.collect.Maps;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.GerritApi;
@@ -90,6 +91,13 @@
throws AuthException, BadRequestException, ResourceConflictException, Exception {
PatchSet ps = revision.getPatchSet();
Change change = revision.getChange();
+ short codeReviewMaxValue =
+ revision
+ .getChangeResource()
+ .getChangeData()
+ .getLabelTypes()
+ .byLabel(LabelId.CODE_REVIEW)
+ .getMaxPositive();
int id = revision.getChangeResource().getChange().getChangeId();
List<Project.NameKey> projectParents =
@@ -112,24 +120,55 @@
patchList,
pluginSettings.expandGroups());
+ Map<String, Set<GroupOwner>> fileExpandedOwners =
+ Maps.transformValues(
+ owners.getFileOwners(),
+ ids ->
+ ids.stream()
+ .map(this::getOwnerFromAccountId)
+ .flatMap(Optional::stream)
+ .collect(Collectors.toSet()));
+
Map<String, Set<GroupOwner>> fileToOwners =
pluginSettings.expandGroups()
- ? Maps.transformValues(
- owners.getFileOwners(),
- ids ->
- ids.stream()
- .map(this::getOwnerFromAccountId)
- .flatMap(Optional::stream)
- .collect(Collectors.toSet()))
+ ? fileExpandedOwners
: Maps.transformValues(
owners.getFileGroupOwners(),
groupNames ->
groupNames.stream().map(GroupOwner::new).collect(Collectors.toSet()));
- return Response.ok(new FilesOwnersResponse(getLabels(id), fileToOwners));
+ Map<Integer, Map<String, Integer>> ownersLabels = getLabels(id);
+
+ Map<String, Set<GroupOwner>> filesWithPendingOwners =
+ Maps.filterEntries(
+ fileToOwners,
+ (fileOwnerEntry) ->
+ !isApprovedByOwner(
+ fileExpandedOwners.get(fileOwnerEntry.getKey()),
+ ownersLabels,
+ codeReviewMaxValue));
+
+ return Response.ok(new FilesOwnersResponse(ownersLabels, filesWithPendingOwners));
}
}
+ private boolean isApprovedByOwner(
+ Set<GroupOwner> fileOwners,
+ Map<Integer, Map<String, Integer>> ownersLabels,
+ short codeReviewMaxValue) {
+ return fileOwners.stream()
+ .filter(owner -> owner instanceof Owner)
+ .map(owner -> ((Owner) owner).getId())
+ .map(ownerId -> codeReviewLabelValue(ownersLabels, ownerId))
+ .anyMatch(value -> value.filter(v -> v == codeReviewMaxValue).isPresent());
+ }
+
+ private Optional<Integer> codeReviewLabelValue(
+ Map<Integer, Map<String, Integer>> ownersLabels, int ownerId) {
+ return Optional.ofNullable(ownersLabels.get(ownerId))
+ .flatMap(m -> Optional.ofNullable(m.get(LabelId.CODE_REVIEW)));
+ }
+
/**
* This method returns ta Map representing the "owners_labels" object of the response. When
* serialized the Map, has to to return the following JSON: the following JSON:
diff --git a/owners/src/main/resources/Documentation/rest-api.md b/owners/src/main/resources/Documentation/rest-api.md
index 4fb4285..68fb463 100644
--- a/owners/src/main/resources/Documentation/rest-api.md
+++ b/owners/src/main/resources/Documentation/rest-api.md
@@ -1,7 +1,7 @@
# Rest API
-The @PLUGIN@ exposes a Rest API endpoint to list the owners associated to each file and, for each owner,
-its associated labels and votes:
+The @PLUGIN@ exposes a Rest API endpoint to list the owners associated to each file that
+needs a review, and, for each owner, its current labels and votes:
```bash
GET /changes/{change-id}/revisions/{revision-id}/owners~files-owners
@@ -29,4 +29,7 @@
}
}
-```
\ No newline at end of file
+```
+
+> __NOTE__: The API does not work in the case when custom label is in
+> rules.pl configuration as described in [the config.md docs](https://gerrit.googlesource.com/plugins/owners/+/refs/heads/stable-3.4/owners/src/main/resources/Documentation/config.md#example-3-owners-file-without-matchers-and-custom-owner_approves-label)
\ No newline at end of file
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
index e79f156..a6d7276 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertThat;
-import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit.Result;
@@ -98,7 +97,21 @@
}
@Test
- public void shouldReturnOwnersLabels() throws Exception {
+ public void shouldReturnOwnersLabelsWhenNotApprovedByOwners() throws Exception {
+ addOwnerFileToRoot(true);
+ String changeId = createChange().getChangeId();
+
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+
+ assertThat(resp.value().files)
+ .containsExactly("a.txt", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get())));
+
+ assertThat(resp.value().ownersLabels).isEmpty();
+ }
+
+ @Test
+ public void shouldReturnEmptyResponseWhenApprovedByOwners() throws Exception {
addOwnerFileToRoot(true);
String changeId = createChange().getChangeId();
approve(changeId);
@@ -106,8 +119,7 @@
Response<FilesOwnersResponse> resp =
assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
- assertThat(resp.value().ownersLabels)
- .containsExactly(admin.id().get(), ImmutableMap.builder().put("Code-Review", 2).build());
+ assertThat(resp.value().files).isEmpty();
}
@Test
@@ -125,6 +137,20 @@
@Test
@GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false")
+ public void shouldReturnEmptyResponseWhenApprovedByOwnersWithUnexpandedFileOwners()
+ throws Exception {
+ addOwnerFileToRoot(true);
+ String changeId = createChange().getChangeId();
+ approve(changeId);
+
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+
+ assertThat(resp.value().files).isEmpty();
+ }
+
+ @Test
+ @GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false")
public void shouldReturnResponseWithUnexpandedFileMatchersOwners() throws Exception {
addOwnerFileWithMatchersToRoot(true);
String changeId = createChange().getChangeId();
