Allow to ignore self approvals for overrides If a label has ignoreSelfApproval set to true, self approvals from the uploader are ignored. However so far this flag was ignored for code owner overrides. This means the code owners check did pass if there was an override from the uploader, even if ignoreSelfApproval was set to true. With this change now, any override from the uploader is ignored for the code owners check if the override label has ignoreSelfApproval set to true. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I5c8486efc2f30152db83b56a04c7234e07ca68cf
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index 2c4f6de..4055d28 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -171,20 +171,6 @@ .projectName(changeNotes.getProjectName().get()) .changeId(changeNotes.getChangeId().get()) .build())) { - RequiredApproval requiredApproval = - codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName()); - logger.atFine().log("requiredApproval = %s", requiredApproval); - - ImmutableSet<RequiredApproval> overrideApprovals = - codeOwnersPluginConfiguration.getOverrideApproval(changeNotes.getProjectName()); - boolean hasOverride = hasOverride(overrideApprovals, changeNotes); - logger.atFine().log( - "hasOverride = %s (overrideApprovals = %s)", hasOverride, overrideApprovals); - - BranchNameKey branch = changeNotes.getChange().getDest(); - ObjectId revision = getDestBranchRevision(changeNotes.getChange()); - logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name()); - boolean enableImplicitApprovalFromUploader = codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(changeNotes.getProjectName()); Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader(); @@ -192,6 +178,20 @@ "patchSetUploader = %d, implicit approval from uploader is %s", patchSetUploader.get(), enableImplicitApprovalFromUploader ? "enabled" : "disabled"); + RequiredApproval requiredApproval = + codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName()); + logger.atFine().log("requiredApproval = %s", requiredApproval); + + ImmutableSet<RequiredApproval> overrideApprovals = + codeOwnersPluginConfiguration.getOverrideApproval(changeNotes.getProjectName()); + boolean hasOverride = hasOverride(overrideApprovals, changeNotes, patchSetUploader); + logger.atFine().log( + "hasOverride = %s (overrideApprovals = %s)", hasOverride, overrideApprovals); + + BranchNameKey branch = changeNotes.getChange().getDest(); + ObjectId revision = getDestBranchRevision(changeNotes.getChange()); + logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name()); + CodeOwnerResolverResult globalCodeOwners = codeOwnerResolver .get() @@ -772,11 +772,34 @@ * * @param overrideApprovals approvals that count as override for the code owners submit check. * @param changeNotes the change notes + * @param patchSetUploader account ID of the patch set uploader * @return whether the given change has an override approval */ private boolean hasOverride( - ImmutableSet<RequiredApproval> overrideApprovals, ChangeNotes changeNotes) { + ImmutableSet<RequiredApproval> overrideApprovals, + ChangeNotes changeNotes, + Account.Id patchSetUploader) { + ImmutableSet<RequiredApproval> overrideApprovalsThatIgnoreSelfApprovals = + overrideApprovals.stream() + .filter(overrideApproval -> overrideApproval.labelType().isIgnoreSelfApproval()) + .collect(toImmutableSet()); return changeNotes.getApprovals().get(changeNotes.getCurrentPatchSet().id()).stream() + .filter( + approval -> { + // If the approval is from the patch set uploader and if it matches any of the labels + // for which self approvals are ignored, filter it out. + if (approval.accountId().equals(patchSetUploader) + && overrideApprovalsThatIgnoreSelfApprovals.stream() + .anyMatch( + requiredApproval -> + requiredApproval + .labelType() + .getLabelId() + .equals(approval.key().labelId()))) { + return false; + } + return true; + }) .anyMatch( patchSetApproval -> overrideApprovals.stream()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java index 3207f67..66c3958 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -20,17 +20,21 @@ import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.Change; +import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.common.LabelDefinitionInput; import com.google.gerrit.plugins.codeowners.JgitPath; import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest; import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations; import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus; +import com.google.gerrit.plugins.codeowners.config.OverrideApprovalConfig; import com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.testing.ConfigSuite; import com.google.inject.Inject; import java.nio.file.Path; import java.nio.file.Paths; import java.util.stream.Stream; +import org.eclipse.jgit.lib.Config; import org.junit.Before; import org.junit.Test; @@ -41,6 +45,15 @@ private CodeOwnerApprovalCheck codeOwnerApprovalCheck; private CodeOwnerConfigOperations codeOwnerConfigOperations; + /** Returns a {@code gerrit.config} that configures all users as fallback code owners. */ + @ConfigSuite.Default + public static Config defaultConfig() { + Config cfg = new Config(); + cfg.setString( + "plugin", "code-owners", OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL, "Owners-Override+1"); + return cfg; + } + @Before public void setUpCodeOwnersPlugin() throws Exception { codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class); @@ -49,10 +62,16 @@ } @Before + public void defineOwnersOverrideLabel() throws Exception { + createOwnersOverrideLabel(); + } + + @Before public void disableSelfApprovals() throws Exception { LabelDefinitionInput input = new LabelDefinitionInput(); input.ignoreSelfApproval = true; gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + gApi.projects().name(project.get()).label("Owners-Override").update(input); } @Test @@ -290,6 +309,139 @@ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); } + @Test + public void notOverriddenByUploaderWhoIsChangeOwner() throws Exception { + // create arbitrary code owner config to avoid entering the bootstrapping code path in + // CodeOwnerApprovalCheck + createArbitraryCodeOwnerConfigFile(); + + TestAccount changeOwner = + accountCreator.create( + "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(changeOwner, "Change Adding A File", JgitPath.of(path).get(), "file content") + .getChangeId(); + + // Verify that the file is not approved. + 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); + + // Add an override approval. + requestScopeOperations.setApiUser(changeOwner.id()); + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + // Verify that the file is not approved (since self approvals on the override label are + // ignored). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + public void overridenByChangeOwnerThatIsNotUploader() throws Exception { + // create arbitrary code owner config to avoid entering the bootstrapping code path in + // CodeOwnerApprovalCheck + createArbitraryCodeOwnerConfigFile(); + + TestAccount changeOwner = + accountCreator.create( + "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(changeOwner, "Change Adding A File", JgitPath.of(path).get(), "file content") + .getChangeId(); + + // Upload another patch set by another user. + amendChange(admin, changeId); + + // Verify that the file is not approved. + 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); + + // Add an override approval from the change owner. + requestScopeOperations.setApiUser(changeOwner.id()); + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + // Verify that the file is approved now (since the change owner is not the uploader of the + // current patch set and hence the override counts). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.APPROVED); + } + + @Test + public void notOverridenByUploader() throws Exception { + // create arbitrary code owner config to avoid entering the bootstrapping code path in + // CodeOwnerApprovalCheck + createArbitraryCodeOwnerConfigFile(); + + TestAccount changeOwner = + accountCreator.create( + "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(changeOwner, "Change Adding A File", JgitPath.of(path).get(), "file content") + .getChangeId(); + + // Upload another patch set by another user. + amendChange(admin, changeId); + + // Verify that the file is not approved. + 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); + + // Add an override approval. + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + // Verify that the file is not approved (since the override from the uploader is ignored). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + private ChangeNotes getChangeNotes(String changeId) throws Exception { return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number)); }
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md index afeb2bf..d80939f 100644 --- a/resources/Documentation/config.md +++ b/resources/Documentation/config.md
@@ -177,6 +177,10 @@ rules](../../../Documentation/config-labels.html#label_copyAnyScore) enabled so that votes are sticky across patch sets, also the code owner overrides will be sticky.\ + If the definition of a configured label [ignores self + approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval) + from the uploader, any override vote from the uploader on that label is + ignored for the code owners check.\ Can be overridden per project by setting [codeOwners.overrideApproval](#codeOwnersOverrideApproval) in `@PLUGIN@.config`.\ @@ -454,6 +458,10 @@ rules](../../../Documentation/config-labels.html#label_copyAnyScore) enabled so that votes are sticky across patch sets, also the code owner overrides will be sticky.\ + If the definition of a configured label [ignores self + approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval) + from the uploader, any override vote from the uploader on that label is + ignored for the code owners check.\ Overrides the global setting [plugin.@PLUGIN@.overrideApproval](#pluginCodeOwnersOverrideApproval) in `gerrit.config`.\