Omit change owner from code owner suggestion
The change owner cannot be added as reviewer, hence it should be omitted
from the suggestion.
Some of the existing tests had to be adapted as they relied on the
change owner being suggested.
Change-Id: I297c9990e9dd1b6573acd47227718750c4a036c8
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
index e4a9455..ad0b1ad 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInChangeCollection.java
@@ -142,8 +142,15 @@
return new PathResource(revisionResource, branchRevision, parsePath(pathId));
}
+ private final RevisionResource revisionResource;
+
private PathResource(RevisionResource revisionResource, ObjectId branchRevision, Path path) {
super(revisionResource, branchRevision, path);
+ this.revisionResource = revisionResource;
+ }
+
+ public RevisionResource getRevisionResource() {
+ return revisionResource;
}
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 0595177..c0465f3 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -80,7 +80,21 @@
@Override
protected Stream<CodeOwner> filterCodeOwners(
CodeOwnersInChangeCollection.PathResource rsrc, Stream<CodeOwner> codeOwners) {
- return codeOwners.filter(filterOutServiceUsers());
+ return codeOwners.filter(filterOutChangeOwner(rsrc)).filter(filterOutServiceUsers());
+ }
+
+ private Predicate<CodeOwner> filterOutChangeOwner(
+ CodeOwnersInChangeCollection.PathResource rsrc) {
+ return codeOwner -> {
+ if (!codeOwner.accountId().equals(rsrc.getRevisionResource().getChange().getOwner())) {
+ // Returning true from the Predicate here means that the code owner should be kept.
+ return true;
+ }
+ logger.atFine().log(
+ "Filtering out %s because this code owner is the change owner", codeOwner);
+ // Returning false from the Predicate here means that the code owner should be filtered out.
+ return false;
+ };
}
private Predicate<CodeOwner> filterOutServiceUsers() {
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 7e82e20..9b05857 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -35,6 +35,8 @@
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.inject.Inject;
import java.util.List;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -53,16 +55,25 @@
@Inject private ProjectOperations projectOperations;
@Inject private GroupOperations groupOperations;
+ private TestAccount changeOwner;
private String changeId;
@Before
public void createTestChange() throws Exception {
+ changeOwner =
+ accountCreator.create(
+ "changeOwner",
+ "changeOwner@example.com",
+ "ChangeOwner",
+ /** displayName = */
+ null);
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, changeOwner);
// Create a change that contains files for all paths that are used in the tests. This is
// necessary since CodeOwnersInChangeCollection rejects requests for paths that are not present
// in the change.
PushOneCommit push =
pushFactory.create(
- admin.newIdent(),
+ changeOwner.newIdent(),
testRepo,
"test change",
TEST_PATHS.stream()
@@ -96,7 +107,7 @@
.project(project)
.branch("master")
.folderPath("/foo/bar/")
- .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
.create();
String path = "/foo/bar/baz.txt";
@@ -104,17 +115,19 @@
List<CodeOwnerInfo> codeOwnerInfos =
codeOwnersApiFactory.change(changeId, "current").query().get(path);
- assertThat(codeOwnerInfos).comparingElementsUsing(hasAccountId()).containsExactly(admin.id());
+ assertThat(codeOwnerInfos).comparingElementsUsing(hasAccountId()).containsExactly(user.id());
}
@Test
public void getCodeOwnersForRenamedFile() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
codeOwnerConfigOperations
.newCodeOwnerConfig()
.project(project)
.branch("master")
.folderPath("/foo/new/")
- .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
.create();
codeOwnerConfigOperations
@@ -122,7 +135,7 @@
.project(project)
.branch("master")
.folderPath("/foo/old/")
- .addCodeOwnerEmail(user.email())
+ .addCodeOwnerEmail(user2.email())
.create();
String oldPath = "/foo/old/bar.txt";
@@ -133,13 +146,13 @@
codeOwnersApiFactory.change(changeId, "current").query().get(newPath);
assertThat(codeOwnerInfosNewPath)
.comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id());
+ .containsExactly(user.id());
List<CodeOwnerInfo> codeOwnerInfosOldPath =
codeOwnersApiFactory.change(changeId, "current").query().get(oldPath);
assertThat(codeOwnerInfosOldPath)
.comparingElementsUsing(hasAccountId())
- .containsExactly(user.id());
+ .containsExactly(user2.id());
}
@Test
@@ -174,7 +187,7 @@
// Check that 'user' is anyway suggested as code owner for the file in the private change since
// by adding 'user' as reviewer the change would get visible to 'user'.
- requestScopeOperations.setApiUser(admin.id());
+ requestScopeOperations.setApiUser(changeOwner.id());
List<CodeOwnerInfo> codeOwnerInfos =
codeOwnersApiFactory.change(changeId, "current").query().get(TEST_PATHS.get(0));
assertThat(codeOwnerInfos).comparingElementsUsing(hasAccountId()).containsExactly(user.id());
@@ -225,4 +238,20 @@
.update();
assertThat(queryCodeOwners("/foo/bar/baz.md")).isEmpty();
}
+
+ @Test
+ public void changeOwnerIsFilteredOut() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(changeOwner.email())
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ assertThat(queryCodeOwners("/foo/bar/baz.md"))
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id());
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnersForPathInChangeRestIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnersForPathInChangeRestIT.java
index 55a1e05..5583fdc 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnersForPathInChangeRestIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnersForPathInChangeRestIT.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.plugins.codeowners.JgitPath;
import org.junit.Before;
@@ -37,10 +38,18 @@
@Before
public void createTestChange() throws Exception {
+ TestAccount changeOwner =
+ accountCreator.create(
+ "changeOwner",
+ "changeOwner@example.com",
+ "ChangeOwner",
+ /** displayName = */
+ null);
// Create a change that contains the file that is used in the tests. This is necessary since
// CodeOwnersInChangeCollection rejects requests for paths that are not present in the change.
changeId =
- createChange("Test Change", JgitPath.of(TEST_PATH).get(), "some content").getChangeId();
+ createChange(changeOwner, "Test Change", JgitPath.of(TEST_PATH).get(), "some content")
+ .getChangeId();
}
@Override
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 26cf341..1978bd0 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -406,6 +406,7 @@
The following code owners are filtered out additionally:
* service users (members of the `Service Users` group)
+* the change owner (since the change owner cannot be added as reviewer)
### <a id="check-code-owner-config-files-in-revision">Check Code Owner Config Files In Revision
_'POST /changes/[\{change-id}](../../../Documentation/rest-api-changes.html#change-id)/revisions/[\{revison-id\}](../../../Documentation/rest-api-changes.html#revision-id)/code_owners.check_config'_
