Allow to ignore self approvals for required approval 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 approvals. This means the code owners check did pass if there was an approval from the uploader, even if ignoreSelfApproval was set to true. With this change now, any vote from the uploader is ignored for the code owners check if the required label has ignoreSelfApproval set to true. In this case the enableImplicitApprovals configuration is ignored because there can never be an implicit approval of the uploader if self approvals of the uploader are ignored. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I698a737bc2551e638c11b8cdd4ca3256fa3d45e0
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index 26d67f0..2c4f6de 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -206,9 +206,10 @@ !codeOwnerConfigScannerFactory.create().containsAnyCodeOwnerConfigFile(branch); logger.atFine().log("isBootstrapping = %s", isBootstrapping); - ImmutableSet<Account.Id> reviewerAccountIds = getReviewerAccountIds(changeNotes); + ImmutableSet<Account.Id> reviewerAccountIds = + getReviewerAccountIds(requiredApproval, changeNotes, patchSetUploader); ImmutableSet<Account.Id> approverAccountIds = - getApproverAccountIds(requiredApproval, changeNotes); + getApproverAccountIds(requiredApproval, changeNotes, patchSetUploader); logger.atFine().log("reviewers = %s, approvers = %s", reviewerAccountIds, approverAccountIds); return changedFiles @@ -705,8 +706,19 @@ * * @param changeNotes the change notes */ - private ImmutableSet<Account.Id> getReviewerAccountIds(ChangeNotes changeNotes) { - return changeNotes.getReviewers().byState(ReviewerStateInternal.REVIEWER); + private ImmutableSet<Account.Id> getReviewerAccountIds( + RequiredApproval requiredApproval, ChangeNotes changeNotes, Account.Id patchSetUploader) { + ImmutableSet<Account.Id> reviewerAccountIds = + changeNotes.getReviewers().byState(ReviewerStateInternal.REVIEWER); + if (requiredApproval.labelType().isIgnoreSelfApproval() + && reviewerAccountIds.contains(patchSetUploader)) { + logger.atFine().log( + "Removing patch set uploader %s from reviewers since the label of the required" + + " approval (%s) is configured to ignore self approvals", + patchSetUploader, requiredApproval.labelType()); + return filterOutAccount(reviewerAccountIds, patchSetUploader); + } + return reviewerAccountIds; } /** @@ -718,21 +730,40 @@ * @param changeNotes the change notes */ private ImmutableSet<Account.Id> getApproverAccountIds( - RequiredApproval requiredApproval, ChangeNotes changeNotes) { - return StreamSupport.stream( - approvalsUtil - .byPatchSet( - changeNotes, - changeNotes.getCurrentPatchSet().id(), - /** revWalk */ - null, - /** repoConfig */ - null) - .spliterator(), - /** parallel */ - false) - .filter(requiredApproval::isApprovedBy) - .map(PatchSetApproval::accountId) + RequiredApproval requiredApproval, ChangeNotes changeNotes, Account.Id patchSetUploader) { + ImmutableSet<Account.Id> approverAccountIds = + StreamSupport.stream( + approvalsUtil + .byPatchSet( + changeNotes, + changeNotes.getCurrentPatchSet().id(), + /** revWalk */ + null, + /** repoConfig */ + null) + .spliterator(), + /** parallel */ + false) + .filter(requiredApproval::isApprovedBy) + .map(PatchSetApproval::accountId) + .collect(toImmutableSet()); + + if (requiredApproval.labelType().isIgnoreSelfApproval() + && approverAccountIds.contains(patchSetUploader)) { + logger.atFine().log( + "Removing patch set uploader %s from approvers since the label of the required" + + " approval (%s) is configured to ignore self approvals", + patchSetUploader, requiredApproval.labelType()); + return filterOutAccount(approverAccountIds, patchSetUploader); + } + + return approverAccountIds; + } + + private ImmutableSet<Account.Id> filterOutAccount( + ImmutableSet<Account.Id> accountIds, Account.Id accountIdToFilterOut) { + return accountIds.stream() + .filter(accountId -> !accountId.equals(accountIdToFilterOut)) .collect(toImmutableSet()); }
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java index 57bc488..60c14d2 100644 --- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java +++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
@@ -23,6 +23,7 @@ import com.google.common.collect.Iterables; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.BranchNameKey; +import com.google.gerrit.entities.LabelType; import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; @@ -158,6 +159,14 @@ */ public boolean areImplicitApprovalsEnabled(Project.NameKey project) { requireNonNull(project, "project"); + LabelType requiredLabel = getRequiredApproval(project).labelType(); + if (requiredLabel.isIgnoreSelfApproval()) { + logger.atFine().log( + "ignoring implicit approval configuration on project %s since the label of the required" + + " approval (%s) is configured to ignore self approvals", + project, requiredLabel); + return false; + } return generalConfig.getEnableImplicitApprovals(getPluginConfig(project)); }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java new file mode 100644 index 0000000..3207f67 --- /dev/null +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -0,0 +1,296 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.backend; + +import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatStream; + +import com.google.gerrit.acceptance.TestAccount; +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.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.testing.FileCodeOwnerStatusSubject; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.inject.Inject; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.stream.Stream; +import org.junit.Before; +import org.junit.Test; + +public class CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest extends AbstractCodeOwnersTest { + @Inject private ChangeNotes.Factory changeNotesFactory; + @Inject private RequestScopeOperations requestScopeOperations; + + private CodeOwnerApprovalCheck codeOwnerApprovalCheck; + private CodeOwnerConfigOperations codeOwnerConfigOperations; + + @Before + public void setUpCodeOwnersPlugin() throws Exception { + codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class); + codeOwnerConfigOperations = + plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class); + } + + @Before + public void disableSelfApprovals() throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + } + + @Test + public void notApprovedByUploaderWhoIsChangeOwner() throws Exception { + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(codeOwner.email()) + .create(); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(codeOwner, "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 a self Code-Review+1 (= code owner approval). + requestScopeOperations.setApiUser(codeOwner.id()); + recommend(changeId); + + // Verify that the file is not approved (since self approvals 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 approvedByChangeOwnerThatIsNotUploader() throws Exception { + TestAccount changeOwner = + accountCreator.create( + "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(changeOwner.email()) + .create(); + + 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 a Code-Review+1 (= code owner approval) from the change owner. + requestScopeOperations.setApiUser(changeOwner.id()); + recommend(changeId); + + // Verify that the file is approved now (since the change owner is not the uploader of the + // current patch set). + 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 notApprovedByUploader() throws Exception { + TestAccount changeOwner = + accountCreator.create( + "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null); + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(codeOwner.email()) + .create(); + + 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 a code owner. + amendChange(codeOwner, 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 the code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the file is not pending (the code owner is the uploader of the current patch set + // and self approvals 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); + + // Add a Code-Review+1 (= code owner approval) by the code owner. + requestScopeOperations.setApiUser(codeOwner.id()); + recommend(changeId); + + // Verify that the file is not approved (since the code owner is the uploader of the current + // patch set and self approvals 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 + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByUploaderWhoIsChangeOwner() throws Exception { + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(codeOwner.email()) + .create(); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(codeOwner, "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); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByUploader() throws Exception { + TestAccount changeOwner = + accountCreator.create( + "changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null); + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(codeOwner.email()) + .create(); + + 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 a code owner. + amendChange(codeOwner, 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); + } + + private ChangeNotes getChangeNotes(String changeId) throws Exception { + return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number)); + } +}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java index 6acb268..641bf10 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
@@ -29,6 +29,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.entities.Project.NameKey; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.extensions.common.LabelDefinitionInput; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.registration.PrivateInternals_DynamicMapImpl; import com.google.gerrit.extensions.registration.RegistrationHandle; @@ -910,6 +911,17 @@ .isEqualTo(FallbackCodeOwners.NONE); } + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void implicitApprovalsAreDisabledIfRequiredLabelIgnoresSelfApprovals() throws Exception { + assertThat(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).isTrue(); + + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + assertThat(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).isFalse(); + } + private void configureDisabled(Project.NameKey project, String disabled) throws Exception { setCodeOwnersConfig(project, /* subsection= */ null, StatusConfig.KEY_DISABLED, disabled); }
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md index 24ed4b9..7fbf4c9 100644 --- a/resources/Documentation/config.md +++ b/resources/Documentation/config.md
@@ -78,6 +78,11 @@ <a id="pluginCodeOwnersEnableImplicitApprovals">plugin.@PLUGIN@.enableImplictApprovals</a> : Whether an implicit code owner approval from the last uploader is assumed.\ + This setting has no effect if self approvals from the last uploader are + ignored because the [required label](#pluginCodeOwnersRequiredApproval) + is configured to [ignore self + approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval) + from the uploader.\ If enabled, code owners need to be aware of their implicit approval when they upload new patch sets for other users (e.g. if a contributor pushes a change to a wrong branch and a code owner helps them to get it rebased @@ -147,6 +152,10 @@ rules](../../../Documentation/config-labels.html#label_copyAnyScore) enabled so that votes are sticky across patch sets, also the code owner approvals will be sticky.\ + If the definition of the configured label [ignores self + approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval) + from the uploader, any vote from the uploader is ignored for the code + owners check.\ Can be overridden per project by setting [codeOwners.requiredApproval](#codeOwnersRequiredApproval) in `@PLUGIN@.config`.\ @@ -340,6 +349,11 @@ <a id="codeOwnersEnableImplicitApprovals">codeOwners.enableImplicitApprovals</a> : Whether an implicit code owner approval from the last uploader is assumed.\ + This setting has no effect if self approvals from the last uploader are + ignored because the [required label](#codeOwnersRequiredApproval) + is configured to [ignore self + approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval) + from the uploader.\ If enabled, code owners need to be aware of their implicit approval when they upload new patch sets for other users (e.g. if a contributor pushes a change to a wrong branch and a code owner helps them to get it rebased @@ -409,6 +423,10 @@ rules](../../../Documentation/config-labels.html#label_copyAnyScore) enabled so that votes are sticky across patch sets, also the code owner approvals will be sticky.\ + If the definition of the configured label [ignores self + approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval) + from the uploader, any vote from the uploader is ignored for the code + owners check.\ Overrides the global setting [plugin.@PLUGIN@.requiredApproval](#pluginCodeOwnersRequiredApproval) in `gerrit.config`.\