Count sticky approvals on old patch sets as code owner approvals
If an approval was applied on an old patch set which is still valid for
the current patch set because it is sticky we must consider it as code
owner approval the same way as approvals that have been applied on the
current patch set.
The computation of sticky approvals is done by ApprovalInference which
we are now using by retrieving the approvals through ApprovalUtil.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I2131162f5de234a53ba923c9c8111a13d7a555ff
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 7e402c7..36d8cc9 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -30,6 +30,7 @@
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.config.RequiredApproval;
+import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -49,6 +50,7 @@
import java.util.Optional;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -80,6 +82,7 @@
private final CodeOwnerConfigScanner codeOwnerConfigScanner;
private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
private final Provider<CodeOwnerResolver> codeOwnerResolver;
+ private final ApprovalsUtil approvalsUtil;
@Inject
CodeOwnerApprovalCheck(
@@ -89,7 +92,8 @@
ChangedFiles changedFiles,
CodeOwnerConfigScanner codeOwnerConfigScanner,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
- Provider<CodeOwnerResolver> codeOwnerResolver) {
+ Provider<CodeOwnerResolver> codeOwnerResolver,
+ ApprovalsUtil approvalsUtil) {
this.permissionBackend = permissionBackend;
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
@@ -97,6 +101,7 @@
this.codeOwnerConfigScanner = codeOwnerConfigScanner;
this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
this.codeOwnerResolver = codeOwnerResolver;
+ this.approvalsUtil = approvalsUtil;
}
/**
@@ -612,7 +617,18 @@
*/
private ImmutableSet<Account.Id> getApproverAccountIds(
RequiredApproval requiredApproval, ChangeNotes changeNotes) {
- return changeNotes.getApprovals().get(changeNotes.getCurrentPatchSet().id()).stream()
+ return StreamSupport.stream(
+ approvalsUtil
+ .byPatchSet(
+ changeNotes,
+ changeNotes.getCurrentPatchSet().id(),
+ /** revWalk */
+ null,
+ /** repoConfig */
+ null)
+ .spliterator(),
+ /** parallel */
+ false)
.filter(requiredApproval::isApprovedBy)
.map(PatchSetApproval::accountId)
.collect(toImmutableSet());
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 1249301..00b5d1e 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -1870,6 +1870,84 @@
assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void approvedByStickyApprovalOnOldPatchSet() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // Create a code owner config file with 'user' as code owner
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user2, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ // Verify that the file is not approved yet.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+
+ // Let the 'user' approve the change.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId);
+
+ // Check that the file is approved now.
+ requestScopeOperations.setApiUser(admin.id());
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+
+ // Change some other file ('user' who uploads the change is a code owner and hence owner
+ // approvals are implicit for this change)
+ String changeId2 =
+ createChange(user, "Change Other File", "other.txt", "file content").getChangeId();
+ approve(changeId2);
+ gApi.changes().id(changeId2).current().submit();
+
+ // Rebase the first change (trivial rebase).
+ gApi.changes().id(changeId).rebase();
+
+ // Check that the approval from 'user' is still there (since Code-Review is sticky on trivial
+ // rebase).
+ assertThat(gApi.changes().id(changeId).get().labels.get("Code-Review").approved.email)
+ .isEqualTo(user.email());
+
+ // Check that the file is still approved.
+ fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId));
+ fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
