Merge "Allow making code owner approvals sticky per file"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BUILD b/java/com/google/gerrit/plugins/codeowners/backend/BUILD
index 83671a0..04aaf14 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BUILD
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BUILD
@@ -6,6 +6,7 @@
srcs = glob(["**/*.java"]),
visibility = ["//visibility:public"],
deps = PLUGIN_DEPS_NEVERLINK + [
+ "//lib/errorprone:annotations",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/common",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/metrics",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/util",
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
index 89f493f..cba9409 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
@@ -36,6 +36,7 @@
public class BackendModule extends FactoryModule {
@Override
protected void configure() {
+ factory(ChangedFilesByPatchSetCache.Factory.class);
factory(CodeOwnerApprovalCheckInput.Loader.Factory.class);
factory(CodeOwnersUpdate.Factory.class);
factory(CodeOwnerConfigScanner.Factory.class);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java
new file mode 100644
index 0000000..3d5fc9c
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFilesByPatchSetCache.java
@@ -0,0 +1,100 @@
+// Copyright (C) 2022 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.common.base.Preconditions.checkState;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
+import com.google.gerrit.plugins.codeowners.common.ChangedFile;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Computes and caches the {@link ChangedFile}s for the patch sets of a change.
+ *
+ * <p>The changed files for a patch set are computed lazily. This way we do not compute changed
+ * files unnecessarily that are never requested.
+ *
+ * <p>This class is not thread-safe.
+ */
+public class ChangedFilesByPatchSetCache {
+ interface Factory {
+ ChangedFilesByPatchSetCache create(
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig, ChangeNotes changeNotes);
+ }
+
+ private final ChangedFiles changedFiles;
+ private final CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig;
+ private final ChangeNotes changeNotes;
+
+ private Map<PatchSet.Id, ImmutableList<ChangedFile>> cache = new HashMap<>();
+
+ @Inject
+ public ChangedFilesByPatchSetCache(
+ ChangedFiles changedFiles,
+ @Assisted CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ @Assisted ChangeNotes changeNotes) {
+ this.changedFiles = changedFiles;
+ this.codeOwnersConfig = codeOwnersConfig;
+ this.changeNotes = changeNotes;
+ }
+
+ public ImmutableList<ChangedFile> get(PatchSet.Id patchSetId) {
+ return cache.computeIfAbsent(patchSetId, this::compute);
+ }
+
+ private ImmutableList<ChangedFile> compute(PatchSet.Id patchSetId) {
+ checkState(
+ patchSetId.changeId().equals(changeNotes.getChange().getId()),
+ "patch set %s belongs to other change than change %s",
+ patchSetId,
+ changeNotes.getChange().getId().get());
+ PatchSet patchSet = getPatchSet(patchSetId);
+ try {
+ return changedFiles.getFromDiffCache(
+ changeNotes.getProjectName(),
+ patchSet.commitId(),
+ codeOwnersConfig.getMergeCommitStrategy());
+ } catch (IOException | DiffNotAvailableException e) {
+ throw new StorageException(
+ String.format(
+ "failed to retrieve changed files for patch set %d of change %d"
+ + " in project %s (commit=%s)",
+ patchSetId.get(),
+ patchSetId.changeId().get(),
+ changeNotes.getProjectName(),
+ patchSet.commitId()),
+ e);
+ }
+ }
+
+ private PatchSet getPatchSet(PatchSet.Id patchSetId) {
+ PatchSet patchSet = changeNotes.getPatchSets().get(patchSetId);
+ if (patchSet == null) {
+ throw new StorageException(
+ String.format(
+ "patch set %s not found in change %d", patchSetId, changeNotes.getChangeId().get()));
+ }
+ return patchSet;
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 792f641..71c0044 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -35,6 +35,7 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
@@ -89,6 +90,7 @@
private final Provider<CodeOwnerResolver> codeOwnerResolverProvider;
private final CodeOwnerApprovalCheckInput.Loader.Factory inputLoaderFactory;
private final CodeOwnerMetrics codeOwnerMetrics;
+ private final ChangedFilesByPatchSetCache.Factory changedFilesByPatchSetCacheFactory;
@Inject
CodeOwnerApprovalCheck(
@@ -99,7 +101,8 @@
Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider,
Provider<CodeOwnerResolver> codeOwnerResolverProvider,
CodeOwnerApprovalCheckInput.Loader.Factory codeOwnerApprovalCheckInputLoaderFactory,
- CodeOwnerMetrics codeOwnerMetrics) {
+ CodeOwnerMetrics codeOwnerMetrics,
+ ChangedFilesByPatchSetCache.Factory changedFilesByPatchSetCacheFactory) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.changedFiles = changedFiles;
@@ -108,6 +111,7 @@
this.codeOwnerResolverProvider = codeOwnerResolverProvider;
this.inputLoaderFactory = codeOwnerApprovalCheckInputLoaderFactory;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.changedFilesByPatchSetCacheFactory = changedFilesByPatchSetCacheFactory;
}
/**
@@ -212,9 +216,12 @@
changeNotes.getChangeId().get(), changeNotes.getProjectName());
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get().enforceVisibility(false);
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
try {
boolean isSubmittable =
- !getFileStatuses(codeOwnerConfigHierarchy, codeOwnerResolver, changeNotes)
+ !getFileStatuses(
+ codeOwnersConfig, codeOwnerConfigHierarchy, codeOwnerResolver, changeNotes)
.anyMatch(
fileStatus ->
(fileStatus.newPathStatus().isPresent()
@@ -247,17 +254,22 @@
*
* @param start number of file statuses to skip
* @param limit the max number of file statuses that should be returned (0 = unlimited)
- * @see #getFileStatuses(CodeOwnerConfigHierarchy, CodeOwnerResolver, ChangeNotes)
+ * @see #getFileStatuses(CodeOwnersPluginProjectConfigSnapshot, CodeOwnerConfigHierarchy,
+ * CodeOwnerResolver, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
ChangeNotes changeNotes, int start, int limit) throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
- try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
+ codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+ try (Timer1.Context<Boolean> ctx =
+ codeOwnerMetrics.computeFileStatuses.start(codeOwnersConfig.areStickyApprovalsEnabled())) {
logger.atFine().log(
"compute file statuses (project = %s, change = %d, start = %d, limit = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get(), start, limit);
Stream<FileCodeOwnerStatus> fileStatuses =
getFileStatuses(
+ codeOwnersConfig,
codeOwnerConfigHierarchyProvider.get(),
codeOwnerResolverProvider.get().enforceVisibility(false),
changeNotes);
@@ -300,6 +312,7 @@
* returned
*/
private Stream<FileCodeOwnerStatus> getFileStatuses(
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
ChangeNotes changeNotes)
@@ -310,9 +323,6 @@
"prepare stream to compute file statuses (project = %s, change = %d)",
changeNotes.getProjectName(), changeNotes.getChangeId().get());
- CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
- codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
-
Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
ImmutableSet<Account.Id> exemptedAccounts = codeOwnersConfig.getExemptedAccounts();
logger.atFine().log("exemptedAccounts = %s", exemptedAccounts);
@@ -360,6 +370,8 @@
getFileStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes),
branch,
revision.orElse(null),
changedFile,
@@ -430,6 +442,8 @@
getFileStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCacheFactory.create(codeOwnersConfig, changeNotes),
branch,
revision.orElse(null),
changedFile,
@@ -476,6 +490,8 @@
private FileCodeOwnerStatus getFileStatus(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
BranchNameKey branch,
@Nullable ObjectId revision,
ChangedFile changedFile,
@@ -492,6 +508,8 @@
getPathCodeOwnerStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
branch,
revision,
newPath,
@@ -510,6 +528,8 @@
getPathCodeOwnerStatus(
codeOwnerConfigHierarchy,
codeOwnerResolver,
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
branch,
revision,
changedFile.oldPath().get(),
@@ -526,6 +546,8 @@
private PathCodeOwnerStatus getPathCodeOwnerStatus(
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
BranchNameKey branch,
@Nullable ObjectId revision,
Path absolutePath,
@@ -554,10 +576,12 @@
boolean isGloballyApproved =
isApproved(
+ absolutePath,
input.globalCodeOwners(),
CodeOwnerKind.GLOBAL_CODE_OWNER,
- input.approvers(),
- input.implicitApprover().orElse(null),
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
+ input,
reason);
if (isGloballyApproved) {
@@ -626,10 +650,12 @@
}
if (isApproved(
+ absolutePath,
codeOwners,
codeOwnerKind,
- input.approvers(),
- input.implicitApprover().orElse(null),
+ codeOwnersConfig,
+ changedFilesByPatchSetCache,
+ input,
reason)) {
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
// No need to recurse if we are not checking all owners or all owners are
@@ -798,31 +824,36 @@
}
/**
- * Checks whether the given path was implicitly or explicitly approved.
+ * Checks whether the given path was approved implicitly, explicitly or by sticky approvals.
*
+ * @param absolutePath the absolute path for which it should be checked whether it is code owner
+ * approved
* @param codeOwners users that own the path
* @param codeOwnerKind the kind of the given {@code codeOwners}
- * @param approverAccountIds the IDs of the accounts that have approved the change
- * @param implicitApprover the ID of the account the could be an implicit approver (aka last patch
- * set uploader)
+ * @param codeOwnersConfig the code-owners plugin configuration that applies to the project that
+ * contains the change for which the code owner statuses are checked
+ * @param changedFilesByPatchSetCache cache that allows to lookup changed files by patch set
+ * @param input input data for checking if a path is code owner approved
* @param reason {@link AtomicReference} on which the reason is being set if the path is approved
* @return whether the path was approved
*/
private boolean isApproved(
+ Path absolutePath,
CodeOwnerResolverResult codeOwners,
CodeOwnerKind codeOwnerKind,
- ImmutableSet<Account.Id> approverAccountIds,
- @Nullable Account.Id implicitApprover,
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
+ CodeOwnerApprovalCheckInput input,
AtomicReference<String> reason) {
- if (implicitApprover != null) {
- if (codeOwners.codeOwnersAccountIds().contains(implicitApprover)
+ if (input.implicitApprover().isPresent()) {
+ if (codeOwners.codeOwnersAccountIds().contains(input.implicitApprover().get())
|| codeOwners.ownedByAllUsers()) {
// If the uploader of the patch set owns the path, there is an implicit code owner
// approval from the patch set uploader so that the path is automatically approved.
reason.set(
String.format(
"implicitly approved by the patch set uploader %s who is a %s%s",
- AccountTemplateUtil.getAccountTemplate(implicitApprover),
+ AccountTemplateUtil.getAccountTemplate(input.implicitApprover().get()),
codeOwnerKind.getDisplayName(),
codeOwners.ownedByAllUsers()
? String.format(" (all users are %ss)", codeOwnerKind.getDisplayName())
@@ -831,13 +862,14 @@
}
}
- if (!Collections.disjoint(approverAccountIds, codeOwners.codeOwnersAccountIds())
- || (codeOwners.ownedByAllUsers() && !approverAccountIds.isEmpty())) {
+ ImmutableSet<Account.Id> approvers = input.approvers();
+ if (!Collections.disjoint(approvers, codeOwners.codeOwnersAccountIds())
+ || (codeOwners.ownedByAllUsers() && !approvers.isEmpty())) {
// At least one of the code owners approved the change.
Optional<Account.Id> approver =
codeOwners.ownedByAllUsers()
- ? approverAccountIds.stream().findAny()
- : approverAccountIds.stream()
+ ? approvers.stream().findAny()
+ : approvers.stream()
.filter(accountId -> codeOwners.codeOwnersAccountIds().contains(accountId))
.findAny();
checkState(approver.isPresent(), "no approver found");
@@ -852,6 +884,58 @@
return true;
}
+ return codeOwnersConfig.areStickyApprovalsEnabled()
+ && isApprovedByStickyApproval(
+ absolutePath, codeOwners, codeOwnerKind, changedFilesByPatchSetCache, input, reason);
+ }
+
+ /**
+ * Checks whether the given path is code owner approved by a sticky approval on a previous patch
+ * set.
+ */
+ private boolean isApprovedByStickyApproval(
+ Path absolutePath,
+ CodeOwnerResolverResult codeOwners,
+ CodeOwnerKind codeOwnerKind,
+ ChangedFilesByPatchSetCache changedFilesByPatchSetCache,
+ CodeOwnerApprovalCheckInput input,
+ AtomicReference<String> reason) {
+ for (PatchSet.Id patchSetId : input.previouslyApprovedPatchSetsInReverseOrder()) {
+ if (changedFilesByPatchSetCache.get(patchSetId).stream()
+ .anyMatch(
+ changedFile ->
+ changedFile.hasNewPath(absolutePath) || changedFile.hasOldPath(absolutePath))) {
+ logger.atFine().log(
+ "previously approved patch set %d contains path %s", patchSetId.get(), absolutePath);
+ Optional<Account.Id> approver =
+ input.approversFromPreviousPatchSets().get(patchSetId).stream()
+ .filter(
+ accountId ->
+ codeOwners.codeOwnersAccountIds().contains(accountId)
+ || codeOwners.ownedByAllUsers())
+ .findAny();
+ if (!approver.isPresent()) {
+ logger.atFine().log(
+ "none of the approvals on previous patch set %d is from a user that owns path %s"
+ + " (approvers=%s)",
+ patchSetId.get(),
+ absolutePath,
+ input.approversFromPreviousPatchSets().get(patchSetId));
+ continue;
+ }
+
+ reason.set(
+ String.format(
+ "approved on patch set %d by %s who is a %s%s",
+ patchSetId.get(),
+ AccountTemplateUtil.getAccountTemplate(approver.get()),
+ codeOwnerKind.getDisplayName(),
+ codeOwners.ownedByAllUsers()
+ ? String.format(" (all users are %ss)", codeOwnerKind.getDisplayName())
+ : ""));
+ return true;
+ }
+ }
return false;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
index cd2ef0e..9335e5b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
@@ -14,14 +14,23 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
+import static java.util.Comparator.comparing;
import com.google.auto.value.AutoValue;
+import com.google.auto.value.extension.memoized.Memoized;
+import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -33,6 +42,8 @@
import com.google.gerrit.server.util.LabelVote;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.HashMap;
+import java.util.Map;
import java.util.Optional;
/**
@@ -63,6 +74,23 @@
public abstract ImmutableSet<Account.Id> approvers();
/**
+ * Gets a map of previous patch sets to the IDs of the accounts that have an approval on that
+ * patch set that is sticky and possibly counts as code owner approval (if they are code owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out for all patch sets
+ * since in this case the approval of the patch set uploader is ignored even if they are a code
+ * owner.
+ */
+ public abstract ImmutableMultimap<PatchSet.Id, Account.Id> approversFromPreviousPatchSets();
+
+ @Memoized
+ public ImmutableSortedSet<PatchSet.Id> previouslyApprovedPatchSetsInReverseOrder() {
+ return ImmutableSortedSet.orderedBy(comparing(PatchSet.Id::get).reversed())
+ .addAll(approversFromPreviousPatchSets().keySet())
+ .build();
+ }
+
+ /**
* Account from which an implicit code owner approval should be assumed.
*
* @see CodeOwnersPluginProjectConfigSnapshot#areImplicitApprovalsEnabled()
@@ -119,6 +147,7 @@
return create(
/* reviewers= */ ImmutableSet.of(),
/* approvers= */ accounts,
+ /* approversFromPreviousPatchSets= */ ImmutableMultimap.of(),
// Do not check for implicit approvals since implicit approvals of other users
// should be ignored. For the given account we do not need to check for
// implicit approvals since all owned files are already covered by the
@@ -152,6 +181,7 @@
private static CodeOwnerApprovalCheckInput create(
ImmutableSet<Account.Id> reviewers,
ImmutableSet<Account.Id> approvers,
+ ImmutableMultimap<PatchSet.Id, Account.Id> approversFromPreviousPatchSets,
Optional<Account.Id> implicitApprover,
ImmutableSet<PatchSetApproval> overrides,
CodeOwnerResolverResult globalCodeOwners,
@@ -160,6 +190,7 @@
return new AutoValue_CodeOwnerApprovalCheckInput(
reviewers,
approvers,
+ approversFromPreviousPatchSets,
implicitApprover,
overrides,
globalCodeOwners,
@@ -207,6 +238,7 @@
return CodeOwnerApprovalCheckInput.create(
getReviewers(),
getApprovers(),
+ getApproversFromPreviousPatchSets(),
getImplicitApprover(),
getOverrides(),
getGlobalCodeOwners(),
@@ -284,6 +316,108 @@
}
/**
+ * Gets a map of previous patch sets to the IDs of the accounts that have an approval on that
+ * patch set that is sticky and possibly counts as code owner approval (if they are code
+ * owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out for all patch sets
+ * since in this case the approval of the patch set uploader is ignored even if they are a code
+ * owner.
+ */
+ private ImmutableMultimap<PatchSet.Id, Account.Id> getApproversFromPreviousPatchSets() {
+ if (!codeOwnersConfig.areStickyApprovalsEnabled()) {
+ logger.atFine().log("sticky approvals are disabled");
+ return ImmutableMultimap.of();
+ }
+
+ // Filter out approvals on the current patch set, since here we are only interested in code
+ // owner approvals on previous patch sets that should be considered as sticky.
+ PatchSet.Id currentPatchSetId = changeNotes.getCurrentPatchSet().id();
+ ImmutableSetMultimap<PatchSet.Id, Account.Id> approversFromPreviousPatchSets =
+ getLastCodeOwnerApprovalsByAccount().values().stream()
+ .filter(psa -> psa.patchSetId().get() < currentPatchSetId.get())
+ .collect(
+ toImmutableSetMultimap(
+ PatchSetApproval::patchSetId, PatchSetApproval::accountId));
+ logger.atFine().log(
+ "sticky approvals are enabled, approversFromPreviousPatchSets=%s",
+ approversFromPreviousPatchSets);
+ return approversFromPreviousPatchSets;
+ }
+
+ /**
+ * Returns the last code owner approvals by account.
+ *
+ * <p>The returned map contains for each user their last approval on the change that counts as a
+ * code owner approval. Approvals that are invalidated by code owner votes on newer patch sets
+ * are filtered out.
+ */
+ private ImmutableMap<Account.Id, PatchSetApproval> getLastCodeOwnerApprovalsByAccount() {
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+
+ Map<Account.Id, PatchSetApproval> lastCodeOwnerVotesByAccount = new HashMap<>();
+ ImmutableSetMultimap<PatchSet.Id, PatchSetApproval> allCodeOwnerApprovals =
+ changeNotes.getApprovals().all().entries().stream()
+ // Only look at approvals on the label that is configured for code owner approvals.
+ .filter(e -> e.getValue().label().equals(requiredApproval.labelType().getName()))
+ .collect(toImmutableSetMultimap(Map.Entry::getKey, Map.Entry::getValue));
+ logger.atFine().log("allCodeOwnerApprovals=%s", allCodeOwnerApprovals);
+ // Iterate over the patch sets in reverse order (latest patch set first).
+ for (PatchSet.Id patchSetId : getPatchSetIdsInReverseOrder()) {
+ // Only store the code owner approval if we didn't find a code owner approval for that
+ // account on a newer patch set yet.
+ // If a code owner approval on a newer patch set exist, it invalidated the code owner
+ // approval on the older patch set and we can ignore it.
+ allCodeOwnerApprovals
+ .get(patchSetId)
+ .forEach(psa -> lastCodeOwnerVotesByAccount.putIfAbsent(psa.accountId(), psa));
+ }
+
+ ImmutableMap<Account.Id, PatchSetApproval> lastCodeOwnerApprovalsByAccount =
+ lastCodeOwnerVotesByAccount.entrySet().stream()
+ // Remove all approvals which do not count as a code owner approval because the voting
+ // value is insufficient.
+ .filter(e -> requiredApproval.isApprovedBy(e.getValue()))
+ .filter(filterOutSelfApprovalsIfSelfApprovalsAreIgnored())
+ .collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
+ logger.atFine().log(
+ "lastCodeOwnerApprovalsByAccount=%s, lastCodeOwnerVotesByAccount=%s",
+ lastCodeOwnerApprovalsByAccount, lastCodeOwnerVotesByAccount);
+ return lastCodeOwnerApprovalsByAccount;
+ }
+
+ /**
+ * Creates a filter that filters out self approvals by the patch set uploader if self approvals
+ * are ignored
+ */
+ private Predicate<Map.Entry<Account.Id, PatchSetApproval>>
+ filterOutSelfApprovalsIfSelfApprovalsAreIgnored() {
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+ if (!requiredApproval.labelType().isIgnoreSelfApproval()) {
+ logger.atFine().log("s");
+ return e -> true;
+ }
+
+ Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
+ return e -> {
+ if (e.getKey().equals(patchSetUploader)) {
+ logger.atFine().log(
+ "Removing approvals of the patch set uploader %s since the label of the required"
+ + " approval (%s) is configured to ignore self approvals",
+ patchSetUploader, requiredApproval.labelType());
+ return false;
+ }
+ return true;
+ };
+ }
+
+ private ImmutableSortedSet<PatchSet.Id> getPatchSetIdsInReverseOrder() {
+ return ImmutableSortedSet.orderedBy(comparing(PatchSet.Id::get).reversed())
+ .addAll(changeNotes.getPatchSets().keySet())
+ .build();
+ }
+
+ /**
* Gets the approvals from the current patch set that count as code owner overrides.
*
* <p>If self approvals are ignored an override of the patch set uploader is filtered out since
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 89fc56e..f02e54d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -95,6 +95,7 @@
private Map<String, Optional<PathExpressions>> pathExpressionsByBranch = new HashMap<>();
@Nullable private Optional<PathExpressions> pathExpressions;
@Nullable private Boolean implicitApprovalsEnabled;
+ @Nullable private Boolean stickyApprovalsEnabled;
@Nullable private RequiredApproval requiredApproval;
@Nullable private ImmutableSortedSet<RequiredApproval> overrideApprovals;
@@ -618,6 +619,27 @@
}
/**
+ * Checks whether sticky code owner approvals are enabled.
+ *
+ * <p>If enabled, a code owner approval on a previous patch set is sticky (if the approver didn't
+ * alter or remove it on a later patch set).
+ */
+ public boolean areStickyApprovalsEnabled() {
+ if (stickyApprovalsEnabled == null) {
+ stickyApprovalsEnabled = readStickyApprovalsEnabled();
+ }
+ return stickyApprovalsEnabled;
+ }
+
+ private boolean readStickyApprovalsEnabled() {
+ boolean enableStickyApprovals = generalConfig.enableStickyApprovals(projectName, pluginConfig);
+ logger.atFine().log(
+ "sticky approvals on project %s are %s",
+ projectName, enableStickyApprovals ? "enabled" : "disabled");
+ return enableStickyApprovals;
+ }
+
+ /**
* Returns the approval that is required from code owners to approve the files in a change.
*
* <p>Defines which approval counts as code owner approval.
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 00574bc..810486f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -80,6 +80,7 @@
public static final String KEY_GLOBAL_CODE_OWNER = "globalCodeOwner";
public static final String KEY_EXEMPTED_USER = "exemptedUser";
public static final String KEY_ENABLE_IMPLICIT_APPROVALS = "enableImplicitApprovals";
+ public static final String KEY_ENABLE_STICKY_APPROVALS = "enableStickyApprovals";
public static final String KEY_OVERRIDE_INFO_URL = "overrideInfoUrl";
public static final String KEY_INVALID_CODE_OWNER_CONFIG_INFO_URL =
"invalidCodeOwnerConfigInfoUrl";
@@ -875,6 +876,22 @@
}
/**
+ * Gets whether sticky code owner approvals are enabled from the given plugin config with fallback
+ * to {@code gerrit.config}.
+ *
+ * <p>If enabled, a code owner approval on a previous patch set is sticky (if the approver didn't
+ * alter or remove it on a later patch set).
+ *
+ * @param project the name of the project for which the configuration should be read
+ * @param pluginConfig the plugin config from which the configuration should be read.
+ * @return whether a sticky code owner approvals are enabled
+ */
+ boolean enableStickyApprovals(Project.NameKey project, Config pluginConfig) {
+ return getBooleanConfig(
+ project, pluginConfig, KEY_ENABLE_STICKY_APPROVALS, /* defaultValue= */ false);
+ }
+
+ /**
* Gets the users which are configured as global code owners from the given plugin config with
* fallback to {@code gerrit.config}.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index d6ac638..fa90564 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -34,7 +34,7 @@
// latency metrics
public final Timer1<String> addChangeMessageOnAddReviewer;
public final Timer0 computeFileStatus;
- public final Timer0 computeFileStatuses;
+ public final Timer1<Boolean> computeFileStatuses;
public final Timer0 computeOwnedPaths;
public final Timer0 computePatchSetApprovals;
public final Timer0 extendChangeMessageOnPostReview;
@@ -90,7 +90,10 @@
this.computeFileStatuses =
createTimer(
"compute_file_statuses",
- "Latency for computing file statuses for all files in a change");
+ "Latency for computing file statuses for all files in a change",
+ Field.ofBoolean("sticky_approvals", (metadataBuilder, stickyApprovals) -> {})
+ .description("Whether sticky approvals on file level are enabled.")
+ .build());
this.computeOwnedPaths =
createTimer(
"compute_owned_paths",
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
index 3c41f14..0054f2e 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInputTest.java
@@ -17,13 +17,19 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.ChangeApi;
@@ -34,6 +40,7 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import org.junit.Before;
import org.junit.Test;
@@ -41,6 +48,7 @@
/** Tests for {@link CodeOwnerApprovalCheckInput}. */
public class CodeOwnerApprovalCheckInputTest extends AbstractCodeOwnersTest {
@Inject private ChangeNotes.Factory changeNotesFactory;
+ @Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
private CodeOwnerApprovalCheckInput.Loader.Factory inputLoaderFactory;
@@ -48,6 +56,7 @@
private CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig;
private TestAccount user2;
private Change.Id changeId;
+ private Change.Key changeKey;
private Account.Id changeOwner;
@Before
@@ -63,7 +72,9 @@
@Before
public void setUp() throws Exception {
user2 = accountCreator.user2();
- changeId = createChange().getChange().getId();
+ ChangeData changeData = createChange().getChange();
+ changeId = changeData.getId();
+ changeKey = changeData.change().getKey();
changeOwner = admin.id();
}
@@ -143,6 +154,448 @@
assertThat(loadInput().approvers()).containsExactly(user.id(), user2.id());
}
+ /** Test that current approvals do not count for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApprovers() throws Exception {
+ // self approve current patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user current patch set
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2 current patch set
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /** Test that previous approvals on other labels do not count for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfApprovalIsOnUnrelatedLabel() throws Exception {
+ // Create Foo-Review label.
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.values = ImmutableMap.of("+1", "Approved", " 0", "Not Approved");
+ gApi.projects().name(project.get()).label("Foo-Review").create(input).get();
+
+ // Allow to vote on the Foo-Review label.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Foo-Review")
+ .range(0, 1)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ // approve on Foo-Review label
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(changeId.get()).current().review(new ReviewInput().label("Foo-Review", 1));
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that previous votes with insufficient values do not count for computing previous
+ * approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+2")
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfVoteIsNotAnApproval() throws Exception {
+ // vote with Code-Review+1, but only Code-Review+2 counts as a code owner approval
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /** Test that previous approvals are ignored if sticky approvals are disabled. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "false")
+ public void noPreviousApproversIfEnableStickyApprovalsDisabled() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /** Test that the approvals on the previous patch set count for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApprovers() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.putAll(
+ PatchSet.id(changeId, 1), ImmutableSet.of(changeOwner, user.id(), user2.id()));
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that a self-approval on the previous patch set is ignored for computing previous
+ * approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApprovers_selfApprovalsIgnored() throws Exception {
+ disableSelfCodeReviewApprovals();
+
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.putAll(
+ PatchSet.id(changeId, 1), ImmutableSet.of(user.id(), user2.id()));
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that the approvals on different previous patch sets count for computing previous
+ * approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApproversOnDifferentPatchSets() throws Exception {
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 4th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), changeOwner);
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 2), user.id());
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 3), user2.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /** Test that a self-approval on an old patch set is ignored for computing previous approvers. */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void withPreviousApproversOnDifferentPatchSets_selfApprovalsIgnored() throws Exception {
+ disableSelfCodeReviewApprovals();
+
+ // self approve
+ requestScopeOperations.setApiUser(changeOwner);
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 4th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 2), user.id());
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 3), user2.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that sticky approvals do not count for computing previous approvers (because if the
+ * approval is sticky it's a current approval).
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfApprovalIsCopied() throws Exception {
+ // Make Code-Review approvals sticky
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ input.copyCondition = "is:ANY";
+ gApi.projects().name(allProjects.get()).label("Code-Review").update(input);
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that a previous approval still counts for computing previous approvers if the approver
+ * comments on the current patch set without applying a vote.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void previousApproversIsPreservedWhenThePreviousApproverCommentsOnTheChange()
+ throws Exception {
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // check that the previous approver on patch set 1 is found
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+
+ // comment on the change
+ requestScopeOperations.setApiUser(user.id());
+ ReviewInput reviewInput = ReviewInput.noScore();
+ reviewInput.message = "a comment";
+ gApi.changes().id(changeId.get()).current().review(reviewInput);
+
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ /**
+ * Test that a previous approval doesn't count for computing previous approvers if the approver
+ * downgrades the vote.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+2")
+ public void noPreviousApproversIfApprovalIsDowngraded() throws Exception {
+ // Allow all users to vote with Code-Review+2.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Code-Review")
+ .range(0, 2)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ approve(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // check that the previous approver on patch set 1 is found
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+
+ // change vote from Code-Review+2 to Code-Review+1 as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // the Code-Review+1 vote on the current patch set overrode the previous approval
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // the Code-Review+1 vote on the previous patch set overrode the previous approval
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that a previous approval doesn't count for computing previous approvers if the approver
+ * re-applies the approval (because now it's a current approval).
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviousApproversIfApprovalIsReapplied() throws Exception {
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // check that the previous approver on patch set 1 is found
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 1), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+
+ // re-apply the approval
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ assertThat(loadInput().approversFromPreviousPatchSets()).isEmpty();
+ }
+
+ /**
+ * Test that only the last previous approval of a user counts for computing previous approvers.
+ */
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void onlyLastPreviousApprovalOfAUserIsConsideredForComputingPreviousApprovers()
+ throws Exception {
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // re-approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ ArrayListMultimap<PatchSet.Id, Account.Id> expectedPreviousApprovers =
+ ArrayListMultimap.create();
+ expectedPreviousApprovers.put(PatchSet.id(changeId, 2), user.id());
+ assertThat(loadInput().approversFromPreviousPatchSets())
+ .containsExactlyEntriesIn(expectedPreviousApprovers);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void noPreviouslyApprovedPatchSets() throws Exception {
+ // approve current patch set
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ assertThat(loadInput().previouslyApprovedPatchSetsInReverseOrder()).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void previouslyApprovedPatchSetsAreReturnedInReverseOrder() throws Exception {
+ // create a second patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId.toString());
+
+ // create a third patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // approve as user2, ignored since overridden on patch set 4
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 4th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ // re- approve as user2
+ requestScopeOperations.setApiUser(user2.id());
+ recommend(changeId.toString());
+
+ // create a 5th patch set
+ requestScopeOperations.setApiUser(changeOwner);
+ amendChange(changeKey.get()).assertOkStatus();
+
+ assertThat(loadInput().previouslyApprovedPatchSetsInReverseOrder())
+ .containsExactly(PatchSet.id(changeId, 4), PatchSet.id(changeId, 2));
+ }
+
@Test
public void noImplicitApprover() {
assertThat(loadInput().implicitApprover()).isEmpty();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
new file mode 100644
index 0000000..fc73235
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithStickyApprovalsTest.java
@@ -0,0 +1,696 @@
+// Copyright (C) 2022 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.assertThatCollection;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
+import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
+import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.util.JgitPath;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.util.AccountTemplateUtil;
+import com.google.gerrit.testing.ConfigSuite;
+import com.google.inject.Inject;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Map;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnerApprovalCheck} with sticky approvals enabled. */
+public class CodeOwnerApprovalCheckWithStickyApprovalsTest extends AbstractCodeOwnersTest {
+ @Inject private ChangeNotes.Factory changeNotesFactory;
+ @Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ProjectOperations projectOperations;
+
+ 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.setBoolean(
+ "plugin", "code-owners", GeneralConfig.KEY_ENABLE_STICKY_APPROVALS, /* value= */ true);
+ return cfg;
+ }
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class);
+ codeOwnerConfigOperations =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ }
+
+ @Test
+ public void notApproved_noStickyApproval() throws Exception {
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file is not approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // create a second patch set so that there is a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is not approved.
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
+
+ @Test
+ public void notApproved_byPreviousApprovalOfNonCodeOwner() throws Exception {
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve as user who is not a code owner
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is not approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOnPreviousPatchSet() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOnOldPatchSet() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create several new patch sets so that the approval becomes an approval on an old patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newer file content")
+ .assertOkStatus();
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newest file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalsOfDifferentUsersOnDifferentPreviousPatchSets()
+ throws Exception {
+ TestAccount codeOwner1 =
+ accountCreator.create(
+ "codeOwner1", "codeOwner1@example.com", "CodeOwner1", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner1);
+ TestAccount codeOwner2 =
+ accountCreator.create(
+ "codeOwner2", "codeOwner2@example.com", "CodeOwner2", /* displayName= */ null);
+ setAsCodeOwners("/bar/", codeOwner2);
+
+ Path path1 = Paths.get("/foo/bar.baz");
+ Path path2 = Paths.get("/bar/foo.baz");
+ String changeId =
+ createChange(
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "file content",
+ JgitPath.of(path2).get(),
+ "file content"))
+ .getChangeId();
+
+ // code owner approve first path
+ requestScopeOperations.setApiUser(codeOwner1.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(
+ changeId,
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "new file content",
+ JgitPath.of(path2).get(),
+ "new file content"))
+ .assertOkStatus();
+
+ // Verify that the path1 is approved, but path2 isn't.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner1.id()))),
+ FileCodeOwnerStatus.addition(path2, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // code owner approve second path
+ requestScopeOperations.setApiUser(codeOwner2.id());
+ recommend(changeId);
+
+ // create another patch set so that the second approval becomes an approval on a previous patch
+ // set
+ amendChange(
+ changeId,
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path1).get(),
+ "newer file content",
+ JgitPath.of(path2).get(),
+ "newer file content"))
+ .assertOkStatus();
+
+ // Verify that both paths approved now.
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner1.id()))),
+ FileCodeOwnerStatus.addition(
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 2 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner2.id()))));
+ }
+
+ @Test
+ public void notApproved_byPreviousApprovalThatHasBeenDeleted() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // delete the approval
+ adminRestSession
+ .delete(
+ "/changes/"
+ + changeId
+ + "/reviewers/"
+ + codeOwner.id().toString()
+ + "/votes/Code-Review")
+ .assertNoContent();
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is not approved. The expected status is PENDING since the code owner is
+ // a reviewer now.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.requiredApproval", value = "Code-Review+2")
+ public void notApproved_byPreviousApprovalThatHasBeenDowngraded() throws Exception {
+ // Allow all users to vote with Code-Review+2.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Code-Review")
+ .range(0, 2)
+ .ref("refs/heads/*")
+ .group(REGISTERED_USERS)
+ .build())
+ .update();
+
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ approve(changeId);
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // downgrade approval
+ recommend(changeId);
+
+ // Verify that the file is not approved. The expected status is PENDING since the code owner is
+ // a reviewer now.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+
+ // create another patch set so that the downgraded approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newer file content")
+ .assertOkStatus();
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_reapprovalTrumpsPreviousApproval() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // re-approve
+ recommend(changeId);
+
+ // Verify that the file is approved by the current approval.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+
+ // create another patch set so that the re-approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "newer file content")
+ .assertOkStatus();
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 2 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void approved_implicitApprovalTrumpsPreviousApproval() throws Exception {
+ TestAccount implicitCodeOwner = admin; // the changes is created by the admit user
+ setAsRootCodeOwners(implicitCodeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(implicitCodeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the deleted approval becomes an approval on a previous
+ // patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved by the current implicit approval.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "implicitly approved by the patch set uploader %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(implicitCodeOwner.id()))));
+ }
+
+ @Test
+ public void notApproved_fileThatIsNotPresentInApprovedPatchSetIsNotCoveredByTheApproval()
+ throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsRootCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set that adds a new file
+ Path path2 = Paths.get("/foo/abc.xyz");
+ amendChange(
+ changeId,
+ "Change Adding A File",
+ ImmutableMap.of(
+ JgitPath.of(path).get(),
+ "new file content",
+ JgitPath.of(path2).get(),
+ "file content"))
+ .assertOkStatus();
+
+ // Verify that the new file is not approved. The expected status is PENDING since the code owner
+ // is a reviewer.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))),
+ FileCodeOwnerStatus.addition(
+ path2,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+
+ // re-approve to cover all files
+ recommend(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))),
+ FileCodeOwnerStatus.addition(
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOnPreviousPatchSet_everyoneIsCodeOwner() throws Exception {
+ // Create a code owner config file that makes everyone a code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a code owner (all users are code owners)",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOfDefaultCodeOnPreviousPatchSet() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+ setAsDefaultCodeOwners(codeOwner);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(codeOwner.id()))));
+ }
+
+ @Test
+ public void approved_byStickyApprovalOfDefaultCodeOnPreviousPatchSet_everyoneIsDefaultCodeOwner()
+ throws Exception {
+ // Create a code owner config file that makes everyone a default code owner.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch(RefNames.REFS_CONFIG)
+ .folderPath("/")
+ .addCodeOwnerEmail("*")
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a default code owner"
+ + " (all users are default code owners)",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void approved_byStickyApprovalOfGlobalCodeOnPreviousPatchSet() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(bot.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(bot.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ public void approved_byStickyApprovalOfGlobalCodeOnPreviousPatchSet_everyoneIsGlobalCodeOwner()
+ throws Exception {
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // code owner approve
+ requestScopeOperations.setApiUser(user.id());
+ recommend(changeId);
+
+ // create a second patch set so that the approval becomes an approval on a previous patch set
+ amendChange(changeId, "Change Adding A File", JgitPath.of(path).get(), "new file content")
+ .assertOkStatus();
+
+ // Verify that the file is approved.
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved on patch set 1 by %s who is a global code owner"
+ + " (all users are global code owners)",
+ AccountTemplateUtil.getAccountTemplate(user.id()))));
+ }
+
+ private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
+ throws Exception {
+ return codeOwnerApprovalCheck.getFileStatusesAsSet(
+ getChangeNotes(changeId), /* start= */ 0, /* limit= */ 0);
+ }
+
+ private ChangeNotes getChangeNotes(String changeId) throws Exception {
+ return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
+ }
+
+ private PushOneCommit.Result amendChange(
+ String changeId, String subject, Map<String, String> files) throws Exception {
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, subject, files, changeId);
+ return push.to("refs/for/master");
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
index 86801cc..970aa74 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfigTest.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_ASYNC_MESSAGE_ON_ADD_REVIEWER;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_CODE_OWNER_CONFIG_FILES_WITH_FILE_EXTENSIONS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS;
+import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_STICKY_APPROVALS;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_BRANCH_CREATION;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED;
import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_SUBMIT;
@@ -1642,6 +1643,63 @@
}
@Test
+ public void cannotGetEnableStickyApprovalsForNullProject() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.enableStickyApprovals(/* project= */ null, new Config()));
+ assertThat(npe).hasMessageThat().isEqualTo("project");
+ }
+
+ @Test
+ public void cannotGetEnableStickyApprovalsForNullPluginConfig() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> generalConfig.enableStickyApprovals(project, /* pluginConfig= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pluginConfig");
+ }
+
+ @Test
+ public void noEnableStickyApprovals() throws Exception {
+ assertThat(generalConfig.enableStickyApprovals(project, new Config())).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void
+ enableStickyApprovalsConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel()
+ throws Exception {
+ assertThat(generalConfig.enableStickyApprovals(project, new Config())).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void
+ enableStickyApprovalsConfigurationInPluginConfigOverridesEnableStickyApprovalsConfigurationInGerritConfig()
+ throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS, /* subsection= */ null, KEY_ENABLE_STICKY_APPROVALS, "false");
+ assertThat(generalConfig.enableStickyApprovals(project, cfg)).isFalse();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "true")
+ public void invalidEnableStickyApprovalsConfigurationInPluginConfigIsIgnored() throws Exception {
+ Config cfg = new Config();
+ cfg.setString(
+ SECTION_CODE_OWNERS, /* subsection= */ null, KEY_ENABLE_STICKY_APPROVALS, "INVALID");
+ assertThat(generalConfig.enableStickyApprovals(project, cfg)).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableStickyApprovals", value = "INVALID")
+ public void invalidEnableStickyApprovalsConfigurationInGerritConfigIsIgnored() throws Exception {
+ assertThat(generalConfig.enableStickyApprovals(project, new Config())).isFalse();
+ }
+
+ @Test
public void cannotGetGlobalCodeOwnersForNullPluginConfig() throws Exception {
NullPointerException npe =
assertThrows(
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index 2a20478..3eaefca 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -30,6 +30,9 @@
approval](config.html#pluginCodeOwnersRequiredApproval) and [override
approval](config.html#pluginCodeOwnersOverrideApproval).
+Alternatively code owner approvals can be made sticky per file by [enabling
+sticky approvals in the plugin configuration](config.html#pluginCodeOwnersEnableStickyApprovals).
+
### <a id="implicitApprovals">Implicit code owner approvals
It's possible to [enable implicit approvals](config.html#pluginCodeOwnersEnableImplicitApprovals)
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index b4ea424..80c5b59 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -261,6 +261,66 @@
in `@PLUGIN@.config`.\
By default `FALSE`.
+<a id="pluginCodeOwnersEnableStickyApprovals">plugin.@PLUGIN@.enableStickyApprovals</a>
+: Whether code owner approvals should be sticky on the files they approve,
+ even if these files are changed in follow-up patch sets.\
+ \
+ With this setting previous code owner approvals on files only get
+ invalidated if the approval is revoked, changed (e.g. from
+ `Code-Review+1` to `Code-Review-1`) or re-applied, but not by the upload
+ of a new patch set regardless of whether the new patch sets changes
+ these files.\
+ \
+ Code owner approvals are sticky per file but not on change level. This
+ means they do not show up as (copied) approvals on the change screen
+ like regular sticky approvals, but only count for the computation of the
+ code owner file statuses. In contrast to this setting, making code owner
+ approvals sticky on change level (by setting a [copy
+ condition](../../../Documentation/config-labels.html#label_copyCondition)
+ on the label that is used for code owner approvals) would make the
+ sticky approvals count for all files in the current patch set that are
+ owned by the approvers, regardless of whether these files existed in the
+ patch sets that were originally approved.\
+ \
+ Since code owner approvals that are sticky on file level are not shown
+ in the UI, users need to inspect the [per-file code owner
+ statuses](how-to-use.html#perFileCodeOwnerStatuses) to know which files
+ are code owner approved.\
+ \
+ Example:\
+ If patch set 1 contains the files A, B and C and a user that owns the
+ files A and B approves the patch set by voting with `Code-Review+1`, A
+ and B are code owner approved for this patch set and all future patch
+ sets unless the approval is revoked, changed or re-applied. This means
+ if a second patch set is uploaded that touches files A and B, A and B
+ are still code owner approved. If a third patch set is uploaded that
+ adds file D that is also owned by the same code owner, file D is not
+ code owner approved since it is not present in patch set 1 on which the
+ user applied the code owner approval. If the user changes their vote on
+ patch set 3 from `Code-Review+1` to `Code-Review-1`, the new vote
+ invalidates the approval on patch set 1, so that in this example the
+ files A and B would no longer be code owner approved by this user. If
+ the user re-applies the `Code-Review+1` approval now all owned files
+ that are present in the current patch set, files A, B and D, are code
+ owner approved.\
+ \
+ Enabling sticky code owner approvals on file level can improve the
+ overall developer productivity significantly, since it makes
+ re-approvals on new patch sets unecessary in many cases. With sticky
+ code owner approvals on files new patch sets only need to be re-approved
+ if they touch additional files, and then only by the users that own
+ these files. In contrast to this, if code owner approvals are not
+ sticky (neither on file level via this setting, nor on change level via
+ a copy condition) a new patch set must always be re-approved by all code
+ owners that approved any of the contained files, no matter if these
+ files are touched in the new patch set.\
+ \
+ Can be overridden per project by setting
+ [codeOwners.enableStickyApprovals](#codeOwnersEnableStickyApprovals)
+ in `@PLUGIN@.config`.\
+ \
+ By default `false`.
+
<a id="pluginCodeOwnersGlobalCodeOwner">plugin.@PLUGIN@.globalCodeOwner</a>
: The email of a user that should be a code owner globally across all
branches.\
@@ -854,11 +914,24 @@
If implicit code owner approvals are disabled, code owners can still
self-approve their own changes by voting on the change.\
Overrides the global setting
- [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersenableImplicitApprovals)
+ [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersEnableImplicitApprovals)
in `gerrit.config` and the `codeOwners.enableImplicitApprovals` setting
from parent projects.\
If not set, the global setting
- [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersenableImplicitApprovals)
+ [plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersEnableImplicitApprovals)
+ in `gerrit.config` is used.
+
+<a id="codeOwnersEnableStickyApprovals">codeOwners.enableStickyApprovals</a>
+: Whether code owner approvals should be sticky on the files they approve,
+ even if these files are changed in follow-up patch sets.\
+ For details see
+ [plugin.@PLUGIN@.enableStickyApprovals](#pluginCodeOwnersEnableStickyApprovals).\
+ Overrides the global setting
+ [plugin.@PLUGIN@.enableStickyApprovals](#pluginCodeOwnersEnableStickyApprovals)
+ in `gerrit.config` and the `codeOwners.enableStickyApprovals` setting
+ from parent projects.\
+ If not set, the global setting
+ [plugin.@PLUGIN@.enableStickyApprovals](#pluginCodeOwnersEnableStickyApprovals)
in `gerrit.config` is used.
<a id="codeOwnersGlobalCodeOwner">codeOwners.globalCodeOwner</a>
diff --git a/resources/Documentation/how-to-use.md b/resources/Documentation/how-to-use.md
index 552f675..1635034 100644
--- a/resources/Documentation/how-to-use.md
+++ b/resources/Documentation/how-to-use.md
@@ -191,7 +191,7 @@
\
-### <a id="perFilCodeOwnerStatuses">Per file code owner statuses
+### <a id="perFileCodeOwnerStatuses">Per file code owner statuses
The `@PLUGIN@` plugin also shows the code owner statuses per file in the file
list.
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index 7c15669..b5f7765 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -16,6 +16,8 @@
Latency for computing the file status for one file.
* `compute_file_statuses`:
Latency for computing file statuses for all files in a change.
+ * `sticky_approvals`:
+ Whether sticky approvals on file level are enabled.
* `compute_owned_paths`:
Latency for computing file statuses.
* `compute_owned_paths`:
