Merge branch 'stable-3.14' * stable-3.14: Improve documentation for auto-owners-approval Convert FilesOwnersResponse to a record Show auto-approved owner approvals in the UI Expose auto-approved files in the owners REST API Extract shared auto-owners-approved logic Swtich auto-owners-approved to a allowlist model Default auto-owners-approved to false Change-Id: If71c81eed1a825138652aa72883d1080d5f80ada
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java index 102f954..d6dbc13 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
@@ -31,7 +31,7 @@ private Map<String, Set<Account.Id>> fileOwners = Maps.newHashMap(); private Map<String, Set<Account.Id>> fileReviewers = Maps.newHashMap(); private Map<String, Set<String>> fileGroupOwners = Maps.newHashMap(); - private Set<String> fileOwnersBannedAutoApproval = Sets.newHashSet(); + private Set<String> fileOwnersAllowedAutoApproval = Sets.newHashSet(); private Optional<LabelDefinition> label = Optional.empty(); @Override @@ -87,8 +87,8 @@ return fileGroupOwners; } - public Set<String> getFileOwnersBannedAutoApproval() { - return fileOwnersBannedAutoApproval; + public Set<String> getFileOwnersAllowedAutoApproval() { + return fileOwnersAllowedAutoApproval; } public void addFileOwners(String file, Set<Id> owners) { @@ -127,12 +127,12 @@ fileGroupOwners.computeIfAbsent(file, (f) -> Sets.newHashSet()).addAll(groupOwners); } - public void banFileFromOwnersAutoApproval(String file) { - fileOwnersBannedAutoApproval.add(file); + public void addAllowedFileForOwnersAutoApproval(String file) { + fileOwnersAllowedAutoApproval.add(file); } - public void allowFileForAutoApproval(String file) { - fileOwnersBannedAutoApproval.remove(file); + public void removeAllowedFileForOwnersAutoApproval(String file) { + fileOwnersAllowedAutoApproval.remove(file); } public Optional<LabelDefinition> getLabel() {
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java index c7d33fe..71ce117 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -91,7 +91,7 @@ private final Map<String, Set<String>> fileGroupOwners; - private final Set<String> fileOwnersBannedAutoApproval; + private final Set<String> fileOwnersAllowedAutoApproval; private final boolean expandGroups; @@ -178,7 +178,7 @@ matchers = map.getMatchers(); fileOwners = map.getFileOwners(); fileGroupOwners = map.getFileGroupOwners(); - fileOwnersBannedAutoApproval = map.getFileOwnersBannedAutoApproval(); + fileOwnersAllowedAutoApproval = map.getFileOwnersAllowedAutoApproval(); label = globalLabel.or(map::getLabel); } @@ -212,8 +212,8 @@ return fileGroupOwners; } - public Set<String> getFileOwnersBannedAutoApproval() { - return fileOwnersBannedAutoApproval; + public Set<String> getFileOwnersAllowedAutoApproval() { + return fileOwnersAllowedAutoApproval; } public boolean expandGroups() { @@ -265,10 +265,9 @@ ownersMap.addFileOwners(path, currentEntry.getOwners()); ownersMap.addFileReviewers(path, currentEntry.getReviewers()); ownersMap.addFileGroupOwners(path, currentEntry.getGroupOwners()); - if (!currentEntry.isAutoOwnersApproved()) { - ownersMap.banFileFromOwnersAutoApproval(path); + if (currentEntry.isAutoOwnersApproved()) { + ownersMap.addAllowedFileForOwnersAutoApproval(path); } - // Only add the path to the OWNERS file to reduce the number of // entries in the result if (currentEntry.getOwnersPath() != null) { @@ -392,18 +391,17 @@ ownersMap.addFileGroupOwners(path, matcher.getGroupOwners()); ownersMap.addFileReviewers(path, matcher.getReviewers()); switch (matcher.getAutoOwnersApproved()) { - // We have an explicit allowance for this matcher - // Make sure that anything added at OWNERS level is removed + // We have an explicit allowance for this matcher. case InheritableBoolean.TRUE: - ownersMap.allowFileForAutoApproval(path); + ownersMap.addAllowedFileForOwnersAutoApproval(path); break; - // We have an explicit ban for this matcher + // We have an explicit disable for this matcher. case InheritableBoolean.FALSE: - ownersMap.banFileFromOwnersAutoApproval(path); + ownersMap.removeAllowedFileForOwnersAutoApproval(path); break; - // There is no matcher-level specification of auto-owner-approved - // therefore the global OWNER-level still applies + // There is no matcher-level specification of auto-owner-approved, + // therefore the OWNERS-level resolution still applies. default: break; }
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java index 7b42709..8402642 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
@@ -79,8 +79,8 @@ this.label = config.getLabel().or(() -> inheritedLabel); } else { this.label = config.getLabel(); - // Default to true unless the OWNERS file explicitly sets it to false. - this.setAutoOwnersApproved(config.getAutoOwnersApproved() != InheritableBoolean.FALSE); + // Default to false unless the OWNERS file explicitly sets it to true. + this.setAutoOwnersApproved(config.getAutoOwnersApproved() == InheritableBoolean.TRUE); } } @@ -129,7 +129,7 @@ protected String ownersPath; protected Map<String, Matcher> matchers = Maps.newHashMap(); protected Set<String> groupOwners = Sets.newHashSet(); - protected boolean autoOwnersApproved = true; + protected boolean autoOwnersApproved; protected boolean explicitAutoOwnersApproved; protected ReadOnlyPathOwnersEntry(boolean inherited) {
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java index ca09d57..134eeda 100644 --- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java +++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -512,7 +512,7 @@ CACHE_MOCK, Optional.empty()); - assertThat(owners.getFileOwnersBannedAutoApproval()).isEmpty(); + assertThat(owners.getFileOwnersAllowedAutoApproval()).isEmpty(); assertThat(owners.getFileOwners()).isEmpty(); } @@ -520,7 +520,7 @@ public void testAutoOwnersApprovedInheritedFromRoot() throws Exception { expectConfig( "OWNERS", - "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + "inherited: true\nauto-owners-approved: true\nowners:\n- " + USER_A_EMAIL_COM + "\n"); expectConfig("dir/OWNERS", "inherited: true\nowners:\n- " + USER_B_EMAIL_COM + "\n"); replayAll(); @@ -538,14 +538,14 @@ CACHE_MOCK, Optional.empty()); - assertThat(owners.getFileOwnersBannedAutoApproval()).contains("dir/file.txt"); + assertThat(owners.getFileOwnersAllowedAutoApproval()).contains("dir/file.txt"); } @Test public void testAutoOwnersApprovedDefaultsWhenInheritanceStopped() throws Exception { expectConfig( "OWNERS", - "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + "inherited: true\nauto-owners-approved: true\nowners:\n- " + USER_A_EMAIL_COM + "\n"); expectConfig("dir/OWNERS", "inherited: false\nowners:\n- " + USER_B_EMAIL_COM + "\n"); replayAll(); @@ -563,7 +563,7 @@ CACHE_MOCK, Optional.empty()); - assertThat(owners.getFileOwnersBannedAutoApproval()).isEmpty(); + assertThat(owners.getFileOwnersAllowedAutoApproval()).isEmpty(); } @Test @@ -574,7 +574,7 @@ "OWNERS", RefNames.REFS_CONFIG, parentRepository1, - "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + "inherited: true\nauto-owners-approved: true\nowners:\n- " + USER_A_EMAIL_COM + "\n"); mockParentRepository(parentRepository1NameKey, parentRepository1); replayAll(); @@ -592,7 +592,7 @@ CACHE_MOCK, Optional.empty()); - assertThat(owners.getFileOwnersBannedAutoApproval()).contains("file.txt"); + assertThat(owners.getFileOwnersAllowedAutoApproval()).contains("file.txt"); } @Test @@ -622,7 +622,7 @@ CACHE_MOCK, Optional.empty()); - assertThat(owners.getFileOwnersBannedAutoApproval()).contains("file.txt"); + assertThat(owners.getFileOwnersAllowedAutoApproval()).isEmpty(); } private void mockOwners(String... owners) throws IOException {
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java b/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java index e74e402..eb3ca43 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java
@@ -18,10 +18,12 @@ import static com.google.common.flogger.LazyArgs.lazy; import static com.googlesource.gerrit.owners.AlreadyApprovedByOperand.FULL_OPERAND_WITH_PLUGIN_NAME; import static com.googlesource.gerrit.owners.AlreadyApprovedByOperand.OPERAND; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.allowsAutoApprovalOnPatch; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.modifiedFilesBetweenPatchSets; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.touchedPaths; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Account; -import com.google.gerrit.entities.Patch; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; @@ -30,7 +32,6 @@ import com.google.gerrit.server.git.InMemoryInserter; import com.google.gerrit.server.patch.DiffNotAvailableException; import com.google.gerrit.server.patch.DiffOperations; -import com.google.gerrit.server.patch.DiffOptions; import com.google.gerrit.server.patch.filediff.FileDiffOutput; import com.google.gerrit.server.patch.gitdiff.ModifiedFile; import com.google.gerrit.server.query.approval.ApprovalContext; @@ -39,10 +40,7 @@ import com.googlesource.gerrit.owners.restapi.GetFilesOwners; import java.io.IOException; import java.util.Map; -import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.revwalk.RevCommit; @@ -56,8 +54,6 @@ private final UserInPredicate.Field predicateField; private static final boolean DISABLE_RENAME_DETECTION = false; - private static final DiffOptions DO_NOT_IGNORE_REBASE = - DiffOptions.builder().skipFilesWithAllEditsDueToRebase(false).build(); public AlreadyApprovedByPredicate( GetFilesOwners getFilesOwners, @@ -94,38 +90,27 @@ project); Map<String, FileDiffOutput> priorVsCurrent = - diffOperations - .listModifiedFiles( - project, - sourcePatchSet.commitId(), - targetPatchSet.commitId(), - DO_NOT_IGNORE_REBASE) - .entrySet() - .stream() - // COMMIT_MSG has never an owner, we don't ever want to consider it, even if it - // was modified as part of this patch-set. - .filter(entry -> !Patch.COMMIT_MSG.equals(entry.getKey())) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + modifiedFilesBetweenPatchSets(diffOperations, project, sourcePatchSet, targetPatchSet); // We can't simply look at keys because it won't contain the old name of renamed-files. - Set<String> allFilePathsInDiff = - priorVsCurrent.values().stream() - .flatMap(v -> Stream.of(v.newPath(), v.oldPath())) - .filter(Optional::isPresent) - .map(Optional::get) - .collect(Collectors.toSet()); + Set<String> allFilePathsInDiff = touchedPaths(priorVsCurrent); String branch = ctx.changeData().branchOrThrow().branch(); Set<String> filesOwnedByApprover = getFilesOwners.filterFilesOwnedBy(currentApprover, allFilePathsInDiff, project, branch); - if (isApproverAlsoOwnerAndUploader(currentApprover, changeOwner, uploader) - && allTouchedFilesAreOwned(filesOwnedByApprover, allFilePathsInDiff) - && getFilesOwners.noOwnedFileIsBannedFromAutoApproval( - filesOwnedByApprover, project, branch)) { + if (allowsAutoApprovalOnPatch( + currentApprover, + changeOwner, + uploader, + filesOwnedByApprover, + allFilePathsInDiff, + getFilesOwners, + project, + branch)) { logger.atFinest().log( "Approver '%s' is change owner and uploader. only owned files have been modified and" - + " none of them has auto-owners-approved=false. Label WILL be copied.", + + " all of them allow auto-owners-approved. Label WILL be copied.", currentApprover); return true; } @@ -219,17 +204,6 @@ return !d.oldPath().equals(d.newPath()); } - private static boolean isApproverAlsoOwnerAndUploader( - Account.Id currentApprover, Account.Id changeOwner, Account.Id uploader) { - return currentApprover.equals(changeOwner) && currentApprover.equals(uploader); - } - - private static boolean allTouchedFilesAreOwned( - Set<String> filesOwnedByApprover, Set<String> allFilePathsInDiff) { - return !filesOwnedByApprover.isEmpty() - && filesOwnedByApprover.size() == allFilePathsInDiff.size(); - } - private int getParentNum(ObjectId objectId, RevWalk revWalk) { try { RevCommit commit = revWalk.parseCommit(objectId);
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/AutoOwnersApprovalFunctions.java b/owners/src/main/java/com/googlesource/gerrit/owners/AutoOwnersApprovalFunctions.java new file mode 100644 index 0000000..7b5db51 --- /dev/null +++ b/owners/src/main/java/com/googlesource/gerrit/owners/AutoOwnersApprovalFunctions.java
@@ -0,0 +1,79 @@ +// Copyright (C) 2026 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.googlesource.gerrit.owners; + +import com.google.gerrit.entities.Account; +import com.google.gerrit.entities.Patch; +import com.google.gerrit.entities.PatchSet; +import com.google.gerrit.entities.Project; +import com.google.gerrit.server.patch.DiffNotAvailableException; +import com.google.gerrit.server.patch.DiffOperations; +import com.google.gerrit.server.patch.DiffOptions; +import com.google.gerrit.server.patch.filediff.FileDiffOutput; +import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; +import com.googlesource.gerrit.owners.restapi.GetFilesOwners; +import java.io.IOException; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +public final class AutoOwnersApprovalFunctions { + private static final DiffOptions DO_NOT_IGNORE_REBASE = + DiffOptions.builder().skipFilesWithAllEditsDueToRebase(false).build(); + + public static Map<String, FileDiffOutput> modifiedFilesBetweenPatchSets( + DiffOperations diffOperations, + Project.NameKey project, + PatchSet sourcePatchSet, + PatchSet targetPatchSet) + throws DiffNotAvailableException { + return diffOperations + .listModifiedFiles( + project, sourcePatchSet.commitId(), targetPatchSet.commitId(), DO_NOT_IGNORE_REBASE) + .entrySet() + .stream() + // COMMIT_MSG has never an owner, we don't ever want to consider it, even if it + // was modified as part of this patch-set. + .filter(entry -> !Patch.COMMIT_MSG.equals(entry.getKey())) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + } + + public static Set<String> touchedPaths(Map<String, FileDiffOutput> priorVsCurrent) { + return priorVsCurrent.values().stream() + .flatMap(v -> Stream.of(v.newPath(), v.oldPath())) + .filter(Optional::isPresent) + .map(Optional::get) + .collect(Collectors.toSet()); + } + + public static boolean allowsAutoApprovalOnPatch( + Account.Id approver, + Account.Id changeOwner, + Account.Id uploader, + Set<String> filesOwnedByApprover, + Set<String> allTouchedFiles, + GetFilesOwners getFilesOwners, + Project.NameKey project, + String branch) + throws IOException, InvalidOwnersFileException { + return approver.equals(changeOwner) + && approver.equals(uploader) + && !filesOwnedByApprover.isEmpty() + && filesOwnedByApprover.size() == allTouchedFiles.size() + && getFilesOwners.allOwnedFilesAllowAutoApproval(filesOwnedByApprover, project, branch); + } +}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java b/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java index 3f6c937..819b741 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java
@@ -15,50 +15,12 @@ package com.googlesource.gerrit.owners.entities; -import com.google.common.base.Objects; import java.util.Map; import java.util.Set; /* Files to Owners response API representation */ -public class FilesOwnersResponse { - - public final Map<String, Set<GroupOwner>> files; - public final Map<Integer, Map<String, Integer>> ownersLabels; - public final Map<String, Set<GroupOwner>> filesApproved; - - public FilesOwnersResponse( - Map<Integer, Map<String, Integer>> ownersLabels, - Map<String, Set<GroupOwner>> files, - Map<String, Set<GroupOwner>> filesApproved) { - this.ownersLabels = ownersLabels; - this.files = files; - this.filesApproved = filesApproved; - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - FilesOwnersResponse that = (FilesOwnersResponse) o; - return Objects.equal(files, that.files) - && Objects.equal(ownersLabels, that.ownersLabels) - && Objects.equal(filesApproved, that.filesApproved); - } - - @Override - public int hashCode() { - return Objects.hashCode(files, ownersLabels, filesApproved); - } - - @Override - public String toString() { - return "FilesOwnersResponse{" - + "files=" - + files - + ", ownersLabels=" - + ownersLabels - + ", filesApproved=" - + filesApproved - + '}'; - } -} +public record FilesOwnersResponse( + Map<Integer, Map<String, Integer>> ownersLabels, + Map<String, Set<GroupOwner>> files, + Map<String, Set<GroupOwner>> filesApproved, + Map<String, Set<GroupOwner>> filesAutoApproved) {}
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java index af62387..c61c901 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -15,11 +15,18 @@ package com.googlesource.gerrit.owners.restapi; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.allowsAutoApprovalOnPatch; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.modifiedFilesBetweenPatchSets; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.touchedPaths; + import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.LabelId; +import com.google.gerrit.entities.PatchSet; +import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.client.ListChangesOption; @@ -34,6 +41,8 @@ import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.patch.DiffNotAvailableException; +import com.google.gerrit.server.patch.DiffOperations; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; @@ -53,8 +62,10 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.jgit.lib.Repository; @@ -65,6 +76,7 @@ private final AccountCache accountCache; private final ProjectCache projectCache; private final GitRepositoryManager repositoryManager; + private final DiffOperations diffOperations; private final PluginSettings pluginSettings; private final GerritApi gerritApi; private final PathOwnersEntriesCache cache; @@ -79,6 +91,7 @@ AccountCache accountCache, ProjectCache projectCache, GitRepositoryManager repositoryManager, + DiffOperations diffOperations, PluginSettings pluginSettings, GerritApi gerritApi, PathOwnersEntriesCache cache) { @@ -86,6 +99,7 @@ this.accountCache = accountCache; this.projectCache = projectCache; this.repositoryManager = repositoryManager; + this.diffOperations = diffOperations; this.pluginSettings = pluginSettings; this.gerritApi = gerritApi; this.cache = cache; @@ -108,13 +122,13 @@ .collect(Collectors.toSet()); } - public boolean noOwnedFileIsBannedFromAutoApproval( + public boolean allOwnedFilesAllowAutoApproval( Set<String> ownedPaths, Project.NameKey project, String branch) throws IOException, InvalidOwnersFileException { PathOwners owners = getPathOwners(project, branch, ownedPaths); - Set<String> filesBannedFromAutoOwnersApproval = owners.getFileOwnersBannedAutoApproval(); + Set<String> filesAllowedForOwnersAutoApproval = owners.getFileOwnersAllowedAutoApproval(); - return ownedPaths.stream().noneMatch(filesBannedFromAutoOwnersApproval::contains); + return filesAllowedForOwnersAutoApproval.containsAll(ownedPaths); } @Override @@ -165,8 +179,29 @@ isApprovedByOwner( fileExpandedOwners.get(fileOwnerEntry.getKey()), ownersLabels, label)); + Map<String, Set<GroupOwner>> filesAutoApprovedByOwners; + Map<String, Set<GroupOwner>> filesExplicitlyApprovedByOwners; + Set<String> filesAllowedAutoApproval = owners.getFileOwnersAllowedAutoApproval(); + + if (Sets.intersection(filesAllowedAutoApproval, filesApprovedByOwners.keySet()).isEmpty()) { + filesAutoApprovedByOwners = Map.of(); + filesExplicitlyApprovedByOwners = filesApprovedByOwners; + } else { + Set<String> filesAutoApproved = + getFilesAutoApproved(revision, changeData, filesApprovedByOwners); + filesAutoApprovedByOwners = + Maps.filterKeys(filesApprovedByOwners, filesAutoApproved::contains); + filesExplicitlyApprovedByOwners = + Maps.filterKeys( + filesApprovedByOwners, filePath -> !filesAutoApproved.contains(filePath)); + } + return Response.ok( - new FilesOwnersResponse(ownersLabels, filesWithPendingOwners, filesApprovedByOwners)); + new FilesOwnersResponse( + ownersLabels, + filesWithPendingOwners, + filesExplicitlyApprovedByOwners, + filesAutoApprovedByOwners)); } catch (InvalidOwnersFileException e) { logger.atSevere().withCause(e).log("Reading/parsing OWNERS file error."); throw new ResourceConflictException(e.getMessage(), e); @@ -240,6 +275,98 @@ .anyMatch(value -> value >= label.getScore()); } + private Set<String> getFilesAutoApproved( + RevisionResource revision, + ChangeData changeData, + Map<String, Set<GroupOwner>> filesApprovedByOwners) + throws IOException, InvalidOwnersFileException, DiffNotAvailableException { + PatchSet sourcePatchSet = getPreviousPatchSet(revision); + if (sourcePatchSet == null) { + return Set.of(); + } + Account.Id ownerId = revision.getChange().getOwner(); + String branch = changeData.change().getDest().branch(); + Project.NameKey project = changeData.project(); + + Set<String> allFilesTouchedInTheLastPatchSet = + touchedPaths( + modifiedFilesBetweenPatchSets( + diffOperations, project, sourcePatchSet, revision.getPatchSet())); + + Map<Account.Id, List<PatchSetApproval>> approvalsByAccount = + changeData.currentApprovals().stream() + .collect(Collectors.groupingBy(PatchSetApproval::accountId)); + + List<PatchSetApproval> changeOwnerApprovals = approvalsByAccount.get(ownerId); + + // If the change owner didn't approve the label or the label was not copied from the previous + // patch set then auto-owners-approvals cannot qualify for this patch-set. + if (changeOwnerApprovals == null + || changeOwnerApprovals.stream().anyMatch(isNotCopiedApproval())) { + return Set.of(); + } + + // Otherwise we check if the change owner was eligible for auto-owners-approved + Set<String> filesOwnedByChangeOwnerInTheLastPatchSet = + filterFilesOwnedBy(ownerId, allFilesTouchedInTheLastPatchSet, project, branch); + + if (!allowsAutoApprovalOnPatch( + ownerId, + ownerId, + revision.getPatchSet().uploader(), + filesOwnedByChangeOwnerInTheLastPatchSet, + allFilesTouchedInTheLastPatchSet, + this, + project, + branch)) { + return Set.of(); + } + + return getAutoApprovedFiles(filesApprovedByOwners, approvalsByAccount); + } + + private Set<String> getAutoApprovedFiles( + Map<String, Set<GroupOwner>> filesApprovedByOwners, + Map<Account.Id, List<PatchSetApproval>> currentApprovalsByAccount) { + return filesApprovedByOwners.keySet().stream() + .filter(notExplicitlyApprovedByAnOwner(filesApprovedByOwners, currentApprovalsByAccount)) + .collect(Collectors.toSet()); + } + + private Predicate<String> notExplicitlyApprovedByAnOwner( + Map<String, Set<GroupOwner>> fileExpandedOwners, + Map<Account.Id, List<PatchSetApproval>> currentApprovalsByAccount) { + return filePath -> + ownerIds(fileExpandedOwners.get(filePath)) + .map(currentApprovalsByAccount::get) + .filter(Objects::nonNull) + .flatMap(List::stream) + .noneMatch(isNotCopiedApproval()); + } + + private PatchSet getPreviousPatchSet(RevisionResource revision) { + int sourcePatchSetNumber = revision.getPatchSet().id().get() - 1; + if (sourcePatchSetNumber < 1) { + return null; + } + + return revision + .getNotes() + .getPatchSets() + .get(PatchSet.id(revision.getChange().getId(), sourcePatchSetNumber)); + } + + private static Predicate<PatchSetApproval> isNotCopiedApproval() { + Predicate<PatchSetApproval> func = PatchSetApproval::copied; + return func.negate(); + } + + private Stream<Account.Id> ownerIds(Set<GroupOwner> fileOwners) { + return fileOwners.stream() + .filter(owner -> owner instanceof Owner) + .map(owner -> Account.id(((Owner) owner).getId())); + } + private Stream<Integer> codeReviewLabelValue( Map<Integer, Map<String, Integer>> ownersLabels, int ownerId, String labelId) { return Stream.ofNullable(ownersLabels.get(ownerId))
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-1.1.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-1.1.png new file mode 100644 index 0000000..bfa20f5 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-1.1.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-1.2.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-1.2.png new file mode 100644 index 0000000..d9aaa1d --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-1.2.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-2.1.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-2.1.png new file mode 100644 index 0000000..8c2c801 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-2.1.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-2.2.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-2.2.png new file mode 100644 index 0000000..cc1b9bd --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-2.2.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-3.1.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-3.1.png new file mode 100644 index 0000000..c7ee811 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-3.1.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-3.2.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-3.2.png new file mode 100644 index 0000000..ea28999 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-3.2.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-4.1.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-4.1.png new file mode 100644 index 0000000..463d6fd --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-4.1.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-4.2.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-4.2.png new file mode 100644 index 0000000..94c6576 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-4.2.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-5.1.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-5.1.png new file mode 100644 index 0000000..48ae37a --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-5.1.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-5.2.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-5.2.png new file mode 100644 index 0000000..8e215c1 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-5.2.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.1.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.1.png new file mode 100644 index 0000000..8c2c801 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.1.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.2.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.2.png new file mode 100644 index 0000000..a22de19 --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.2.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.3.png b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.3.png new file mode 100644 index 0000000..7d7f67f --- /dev/null +++ b/owners/src/main/resources/Documentation/auto-owners-approval-images/example-6.3.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md index 553e540..9bc33cd 100644 --- a/owners/src/main/resources/Documentation/config.md +++ b/owners/src/main/resources/Documentation/config.md
@@ -206,58 +206,11 @@ ## auto-owners-approved -The `auto-owners-approved` field controls a specific exception to the default -`approverin:already-approved-by_owners` behavior. It applies when a new patch-set updates only files -that are owned by the change owner or patch-set committer, in a situation where the normal -`approverin:already-approved-by_owners` logic would otherwise drop that owner's previous vote. +The optional `auto-owners-approved` field controls a specific exception to the default +`approverin:already-approved-by_owners` copy condition behavior. -The rationale is simple: if an owner already approved a change that stays entirely within code they -own, and the next patch set is uploaded by that same owner, forcing that same person to re-apply -the same vote adds little review value. - -See [copy-conditions.md](copy-conditions.md) for predicate evaluation details. - -This field can be configured at `OWNERS` file level and on individual matchers. -If it is not set, it defaults to `true`. - -If `auto-owners-approved` is `false` for any touched file, the predicate does not use that -self-update shortcut for the patch set. The usual `approverin:already-approved-by_owners` logic -still applies. - -When a matcher defines `auto-owners-approved`, that matcher-specific value takes precedence for the -files it matches over the surrounding `OWNERS` value. - -### Inheritance - -The usual `OWNERS` [inheritance](#global-project-owners) logic applies to `auto-owners-approved` as -well. This includes directory `OWNERS` lookup, project `refs/meta/config` `OWNERS`, and -parent project `OWNERS` when inheritance continues up the project hierarchy. - -### auto-owners-approved example - -Disable at `OWNERS` level: - - inherited: true - auto-owners-approved: false - -With this setting, the predicate will not copy an owner's vote just because the owner is updating -only files they own on their own change. Paths under that `OWNERS` file still participate in the -normal copy-condition behavior. - -Override that setting for matched files: - -```yaml -inherited: true -auto-owners-approved: false -matchers: - - suffix: .java - auto-owners-approved: true - owners: - - user-backend -``` - -Here, `.java` files matched by that rule use `auto-owners-approved: true` even though the enclosing -`OWNERS` file sets it to `false`. +Please refer to the [relevant paragraph](./copy-conditions.md#auto-owners-approved) in +the [copy-conditions.md](./copy-conditions.md) documentation for details on this. ## Example 1 - OWNERS file without matchers
diff --git a/owners/src/main/resources/Documentation/copy-conditions.md b/owners/src/main/resources/Documentation/copy-conditions.md index b318028..8628208 100644 --- a/owners/src/main/resources/Documentation/copy-conditions.md +++ b/owners/src/main/resources/Documentation/copy-conditions.md
@@ -119,12 +119,202 @@ copyCondition = approverin:already-approved-by_owners ``` -## Customizing the copy condition behaviour with the `auto-owners-approved` in `OWNERS` +## auto-owners-approved -The `approverin:already-approved-by_owners` can be fine-grained enabled or disabled using -`auto-owners-approved` in `OWNERS`, and more narrowly on individual matchers. -When both are present, the matcher value wins for the files that matcher selects. -See the examples in [config.md](./config.md#auto-owners-approved). +The `auto-owners-approved` field controls a specific exception to the default +`approverin:already-approved-by_owners` behavior. It applies when: -Details on the `auto-owners-approved` behaviour can be -found [here](./config.md#auto-owners-approved). +1. The new patch-set updates only files that are owned by the change owner. +2. The change owner, the patch-set committer and the label approver are the same person. + +Under these conditions, if the `auto-owners-approved` field is set, and it applies to all files +updated in the patchset, then the vote will be copied over, whilst the normal +`approverin:already-approved-by_owners` logic would drop that owner's previous vote. + +The rationale is simple: if an owner already approved a change that stays entirely within code they +own, and the next patch set is uploaded by that same owner, forcing that same person to re-apply +the same vote adds little review value. + +If it is not set, it defaults to `false`. + +This field can be configured at `OWNERS` file level and on individual matchers. +When a matcher defines `auto-owners-approved`, that matcher-specific value takes precedence for the +files it matches over the surrounding `OWNERS` value. + +If `auto-owners-approved` is `true` for every touched file, the predicate can use that self-update +shortcut for the patch set. Otherwise, the usual `approverin:already-approved-by_owners` logic +still applies. + +When a vote was copied over to the new patchset due to the `auto-owners-approved` applying, the UI +will clearly show a dedicated icon on the UI, highlighting this. +The [examples](#auto-owners-approved-examples) section shows some screenshots of what this looks +like. + +### Inheritance + +The usual `OWNERS` [inheritance](./config.md#global-project-owners) logic applies to +`auto-owners-approved` as well. This includes directory `OWNERS` lookup, project `refs/meta/config` +`OWNERS`, and parent project `OWNERS` when inheritance continues up the project hierarchy. + +### auto-owners-approved examples + +Let's imagine a repository `main-repo` for which the `approverin:already-approved-by_owners` copy +condition logic has been configured. + +The `main-repo` repo has the following `OWNERS` files configurations. + +``` +➜ main-repo tree +. +├── OWNERS +└── subdir + ├── OWNERS + └── subsub + └── OWNERS +``` + +* OWNERS + +The root `OWNERS` file enables `auto-owners-approved` behaviour for Java (`.java`) files, which are +owned by the `user-backend` and `user-security`. +JavaScript (`.js`) files are owned by `user-frontend` and auto-approval logic does not apply for +them, as per default. + +```yaml +inherited: true +matchers: + - suffix: .js + owners: + - user-frontend + - suffix: .java + auto-owners-approved: true + owners: + - user-backend + - user-security +``` + +* subdir/OWNERS + +The `subdir/OWNERS` file attributes ownership on the entire `subdir` directory to the `user-backend` +and _enables_ `auto-approval` behaviour at directory level. + +```yaml +inherited: true +auto-owners-approved: true +owners: + - user-backend +``` + +* subdir/subsub/OWNERS + +The `subdir/subsub/OWNERS` file attributes ownership on the entire `subsub` directory to the +`user-backend` but _disables_ `auto-approval` behaviour at directory level. + +```yaml +inherited: true +auto-owners-approved: false +owners: + - user-backend +``` + +#### Example 1 - Default behavior. auto owners approval logic does NOT apply + +1. The `user-frontend` adds a `.js` file and votes `+2`. The owner approval is satisfied (since + `user-frontend` is an owner). + +  +2. The `user-frontend` modifies the content of the `.js` file and uploads a new patchset. The + approval is **not copied over**. + +  + +**EXPLANATION**: by default, the `auto-owners-approved` is `false`, so the standard +`approverin:already-approved-by_owners` logic applies: owned files have been modified, the vote is +not copied over. + +#### Example 2 - auto owners approval logic applies: Vote is copied over to new patchset + +1. The `user-backend` adds a `.java` file and votes `+2`. The owner approval is satisfied (since + `user-backend` is an owner). + +  +2. The `user-backend` modifies the content of the `.java` file and uploads a new patchset. The + approval **is** copied over (and a special icon is displayed). + +  + +**EXPLANATION**: the root `OWNERS` file has a specific matcher that enables the +`auto-owners-approved` for `.java` files. + +Since the change owner, the uploader and the approver are all the same person **and** only owned +files have been modified in the new patchset, the vote is copied over. + +The UI shows a special icon to indicate that the approval was carried automatically because the +`auto-owners-approved` applied. + +#### Example 3 - Owned and non-owned files are modified. auto owners approval logic does NOT apply + +1. The `user-backend` adds a `.java` file and votes `+2`. The owner approval is satisfied (since + `user-backend` is an owner). + +  +2. The `user-backend` modifies the content of the `.java` and of a `.txt` file and uploads a new + patchset. The approval is **NOT copied over**. + +  + +**EXPLANATION**: Even though the root `OWNERS` enables `auto-owners-approved` for `.java` files, the +change owner's new patch also touched non-owned files (the `.txt`): the conditions for which the +patch-set is eligible for auto-approval did not apply and thus the owner approval is lost. + +#### Example 4 - a different owner approved the change. auto owners approval logic does NOT apply + +1. The `user-backend` adds a `.java` file. `user-security` votes `+2`. The owner approval is + satisfied (since `user-security` is also an owner of `java` files). + +  +2. The `user-backend` modifies the content of the `.java` file and uploads a new patchset. The + approval is **NOT** copied over. + +  + +**EXPLANATION**: Even though the root `OWNERS` enables `auto-owners-approved` for `.java` files, the +approval vote was given by a _different_ owner (i.e. not by the change owner): the conditions for +which the patch-set is eligible for auto-approval did not apply and thus the owner approval is lost. + +#### Example 5 - auto approval not enabled for every file. auto owners approval logic does NOT apply + +1. The `user-backend` adds a file in the `subdir` file and votes `+2`. The owner approval is + satisfied (since `user-backend` is an owner of the `subdir` directory). + +  +2. The `user-backend` modifies a `subdir/subsub` file and uploads a new patchset. + The approval is **NOT copied over**. + +  + +**EXPLANATION**: The `subdir/OWNERS` enables `auto-owners-approved`, whilst the +`subdir/subsub/OWNERS` disables it. Even though the change owner is the uploader and the approver of +the new patchset and only owned files have been touched, the `auto-owners-approved` flag was not +enabled for every file in the patch. the conditions for which the patch-set is eligible for +auto-approval did not apply and thus the owner approval is lost. + +#### Example 6 - Explicit owner approval override. auto owners approval icon is not displayed + +1. The `user-backend` adds a `.java` file and votes `+2`. The owner approval is satisfied (since + `user-backend` is an owner). + +  +2. The `user-backend` modifies the content of the `.java` file and uploads a new patchset. The + approval **is** copied over (and a special icon is displayed). + +  +3. The `user-security` now also approves the change. The explicit approval icon is displayed. + +  + +**EXPLANATION**: The auto owners approval icon +is meant to highlight the fact the patch-set was _implicitly_ approved due to the +`auto-owners-approval` logic, rather than an _explicit_ owners approval. In this context, since the +`user-security` cast an explicit approval, the special icon would be misleading, and thus it is +not displayed.
diff --git a/owners/src/main/resources/Documentation/rest-api.md b/owners/src/main/resources/Documentation/rest-api.md index 3505d6c..490772f 100644 --- a/owners/src/main/resources/Documentation/rest-api.md +++ b/owners/src/main/resources/Documentation/rest-api.md
@@ -1,8 +1,9 @@ # Rest API The @PLUGIN@ exposes a Rest API endpoint to list the owners associated with each file that -needs approval (`file` field), is approved (`files_approved`) and, for each owner, -its current labels and votes (`owners_labels`): +needs approval (`files`), is approved (`files_approved`), is approved through +`auto-owners-approved` (`files_auto_approved`) and, for each owner, its current labels and +votes (`owners_labels`): ```bash GET /changes/{change-id}/revisions/{revision-id}/owners~files-owners @@ -25,6 +26,12 @@ { "name":"Release Engineer", "id": 1000001 } ] }, + "files_auto_approved": { + "Repository.java":[ + { "name":"John", "id": 1000004 }, + { "name":"Release Engineer", "id": 1000001 } + ] + }, "owners_labels" : { "1000002": { "Verified": 1, @@ -38,5 +45,14 @@ ``` +`files_auto_approved` contains the files whose approval on the current patch set comes from a vote +that was copied forward because the `auto-owners-approved` logic applies. +See [the relevant section](./copy-conditions.md#auto-owners-approved) for more details on this. + +`files_auto_approved` and `files_approved` are mutually exclusive. A file that is auto-approved is +returned only in `files_auto_approved`, and both sections return the full owner set for each file. +If a file also has a sufficient explicit owner vote on the current patch set, it is treated as +explicitly approved and returned only in `files_approved`. + > __NOTE__: The API does not work in the case when custom label is in > rules.pl configuration as described in [the config.md docs](https://gerrit.googlesource.com/plugins/owners/+/refs/heads/stable-3.4/owners/src/main/resources/Documentation/config.md#example-3-owners-file-without-matchers-and-custom-owner_approves-label) \ No newline at end of file
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java index 70ded46..f9d8f04 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java
@@ -348,7 +348,7 @@ } @Test - public void shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsDefault() + public void shouldNotCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsDefault() throws Exception { pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER)); @@ -358,16 +358,12 @@ vote(BACKEND_FILES_OWNER, changeId.toString(), 2); createPatchSet(changeId, BACKEND_FILES_OWNER.id(), BACKEND_OWNED_FILE, "updated java content"); - assertVote(changeId, BACKEND_FILES_OWNER, 2); + assertVote(changeId, BACKEND_FILES_OWNER, 0); } @Test - public void - shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsDisabledOnRepoAndEnabledOnRoot() - throws Exception { - pushOwnersToRef( - "inherited: true\nauto-owners-approved: false\n", "OWNERS", RefNames.REFS_CONFIG); - + public void shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsEnabledOnRoot() + throws Exception { pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVED_ENABLED)); ChangeIdentifier changeId = @@ -380,12 +376,8 @@ } @Test - public void - shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsDisabledOnRootAndEnabledOnPath() - throws Exception { - pushOwnersToRef( - "inherited: true\nauto-owners-approved: false\n", "OWNERS", RefNames.fullName("master")); - + public void shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsEnabledOnPath() + throws Exception { pushOwnersToRef( ownersConfigFor(BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVED_ENABLED), BACKEND_OWNED_FILE_PATH + "OWNERS", @@ -403,7 +395,7 @@ @Test public void shouldNotCopyApprovalWhenAllModifiedFilesAreOwnedButApproverIsNotChangeOwner() throws Exception { - pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER)); + pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVED_ENABLED)); ChangeIdentifier changeId = createChange(NON_OWNER.id(), BACKEND_OWNED_FILE, "java content"); @@ -416,7 +408,7 @@ @Test public void shouldNotCopyApprovalWhenAllModifiedFilesAreOwnedButUploaderNotOwner() throws Exception { - pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER)); + pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVED_ENABLED)); ChangeIdentifier changeId = createChange(BACKEND_FILES_OWNER.id(), BACKEND_OWNED_FILE, "java content"); @@ -456,9 +448,9 @@ } @Test - public void shouldCopyApprovalWhenAutoOwnersApprovedIsFalseButOwnedEditsAreRebaseOnly() + public void shouldCopyApprovalWhenAutoOwnersApprovedIsDefaultAndOwnedEditsAreRebaseOnly() throws Exception { - pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVAL_DISABLED)); + pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER)); ObjectId initialCommitId = createInitialContentFor(BACKEND_OWNED_FILE); PushOneCommit.Result amendL3 = @@ -490,10 +482,13 @@ } @Test - public void shouldNotCopyApprovalWhenMatcherDisablesAutoOwnersApproved() throws Exception { - pushOwnersToMaster( - matcherOwnersConfig( - suffixMatcherConfig(".java", BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVAL_DISABLED))); + public void shouldNotCopyApprovalWhenChildMatcherUsesDefaultAutoOwnersApproved() + throws Exception { + pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER)); + pushOwnersToRef( + matcherOwnersConfig(suffixMatcherConfig(".java", BACKEND_FILES_OWNER)), + BACKEND_OWNED_FILE_PATH + "OWNERS", + RefNames.fullName("master")); ChangeIdentifier changeId = createChange(BACKEND_FILES_OWNER.id(), BACKEND_OWNED_FILE, "java content"); @@ -649,7 +644,7 @@ @Test public void shouldNotCopyApprovalWhenPathAndMatcherFilesDoNotAllAllowAutoOwnersApproved() throws Exception { - pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVAL_DISABLED)); + pushOwnersToMaster(ownersConfigFor(BACKEND_FILES_OWNER)); pushOwnersToRef( matcherOwnersConfig( suffixMatcherConfig(".java", BACKEND_FILES_OWNER, AUTO_OWNERS_APPROVED_ENABLED)),
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java index ad46da6..3ee36e6 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java
@@ -16,7 +16,10 @@ package com.googlesource.gerrit.owners.restapi; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import static com.googlesource.gerrit.owners.AlreadyApprovedByOperand.FULL_OPERAND_WITH_PLUGIN_NAME; import com.google.common.collect.Sets; import com.google.gerrit.acceptance.GitUtil; @@ -25,11 +28,16 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.acceptance.config.GlobalPluginConfig; +import com.google.gerrit.acceptance.testsuite.change.ChangeOperations; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; +import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.LabelId; import com.google.gerrit.entities.LabelType; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.Project.NameKey; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.extensions.api.changes.ChangeIdentifier; +import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -37,13 +45,17 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.server.project.testing.TestLabels; +import com.google.inject.Inject; import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; import com.googlesource.gerrit.owners.common.LabelDefinition; import com.googlesource.gerrit.owners.entities.FilesOwnersResponse; import com.googlesource.gerrit.owners.entities.GroupOwner; import com.googlesource.gerrit.owners.entities.Owner; import com.googlesource.gerrit.owners.restapi.GetFilesOwners.LabelNotFoundException; +import java.util.Arrays; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import javax.servlet.http.HttpServletResponse; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; @@ -53,6 +65,12 @@ public abstract class GetFilesOwnersITAbstract extends LightweightPluginDaemonTest { private static final String REFS_META_CONFIG = RefNames.REFS_META + "config"; + private static final String UNOWNED_TXT_FILE = "a.txt"; + private static final String OWNED_JAVA_FILE = "foo.java"; + private static final Set<GroupOwner> NO_AUTO_APPROVED_OWNERS = Set.of(); + @Inject protected ProjectOperations projectOperations; + @Inject protected RequestScopeOperations requestScopeOperations; + @Inject protected ChangeOperations changeOperations; protected GetFilesOwners ownersApi; private Owner rootOwner; private Owner projectOwner; @@ -107,10 +125,10 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("a.txt", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); - assertThat(resp.value().ownersLabels).isEmpty(); + assertThat(resp.value().ownersLabels()).isEmpty(); } @Test @@ -122,7 +140,7 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().ownersLabels) + assertThat(resp.value().ownersLabels()) .containsExactly(admin.id().get(), Map.of(LabelId.CODE_REVIEW, 2)); } @@ -136,12 +154,139 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).isEmpty(); - assertThat(resp.value().filesApproved) + assertThat(resp.value().files()).isEmpty(); + assertThat(resp.value().filesApproved()) .containsExactly("a.txt", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); } @Test + public void shouldReturnFilesAutoApprovedWhenOwnerVoteIsCopied() throws Exception { + setupAutoApprovalFor(admin); + + ChangeIdentifier changeId = createChangeWithCopiedOwnerVote(admin); + + assertFilesApproval( + changeId.toString(), OWNED_JAVA_FILE, NO_AUTO_APPROVED_OWNERS, owners(admin)); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenOwnerExplicitlyVotesOnCurrentPatchSet() + throws Exception { + setupAutoApprovalFor(admin); + + ChangeIdentifier changeId = createChangeWithCopiedOwnerVote(admin); + vote(admin, changeId.toString(), 2); + + assertFilesApproval( + changeId.toString(), OWNED_JAVA_FILE, owners(admin), NO_AUTO_APPROVED_OWNERS); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenAnotherOwnerExplicitlyVotesOnCurrentPatchSet() + throws Exception { + TestAccount explicitVoteOwner = accountCreator.create("user-backend"); + allowCodeReviewForRegisteredUsers(); + setupAutoApprovalFor(admin, explicitVoteOwner); + + ChangeIdentifier changeId = createChangeWithCopiedOwnerVote(admin); + vote(explicitVoteOwner, changeId.toString(), 2); + + assertFilesApproval( + changeId.toString(), + OWNED_JAVA_FILE, + owners(admin, explicitVoteOwner), + NO_AUTO_APPROVED_OWNERS); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenOwnedAndUnownedFilesAreModifiedTogether() + throws Exception { + setupAutoApprovalForJavaMatcher(admin); + + ChangeIdentifier changeId = + changeOperations + .newChange() + .project(project) + .owner(admin.id()) + .file(OWNED_JAVA_FILE) + .content("v1") + .create(); + vote(admin, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(admin.id()) + .file(OWNED_JAVA_FILE) + .content("v2") + .file(UNOWNED_TXT_FILE) + .content("unowned") + .create(); + + Response<FilesOwnersResponse> response = + assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId.toString()))); + assertThat(response.value().files()).containsExactly(OWNED_JAVA_FILE, owners(admin)); + assertThat(response.value().filesApproved()).isEmpty(); + assertThat(response.value().filesAutoApproved()).isEmpty(); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenOnlyUnownedFileIsAdded() throws Exception { + setupAutoApprovalForJavaMatcher(admin); + + ChangeIdentifier changeId = + changeOperations + .newChange() + .project(project) + .owner(admin.id()) + .file(OWNED_JAVA_FILE) + .content("v1") + .create(); + vote(admin, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(admin.id()) + .file(UNOWNED_TXT_FILE) + .content("unowned") + .create(); + + assertFilesApproval( + changeId.toString(), OWNED_JAVA_FILE, owners(admin), NO_AUTO_APPROVED_OWNERS); + } + + @Test + public void shouldReturnFilesAutoApprovedWhenNextPatchSetAddsOnlyOwnedFile() throws Exception { + setupAutoApprovalForJavaMatcher(admin); + String ANOTHER_OWNED_JAVA_FILE = "another-" + OWNED_JAVA_FILE; + + ChangeIdentifier changeId = + changeOperations + .newChange() + .project(project) + .owner(admin.id()) + .file(OWNED_JAVA_FILE) + .content("owned") + .file(UNOWNED_TXT_FILE) + .content("unowned") + .create(); + vote(admin, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(admin.id()) + .file(ANOTHER_OWNED_JAVA_FILE) + .content("owned") + .create(); + + Response<FilesOwnersResponse> response = + assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId.toString()))); + assertThat(response.value().files()).isEmpty(); + assertThat(response.value().filesApproved()).isEmpty(); + assertThat(response.value().filesAutoApproved()) + .containsExactly(OWNED_JAVA_FILE, owners(admin), ANOTHER_OWNED_JAVA_FILE, owners(admin)); + } + + @Test @GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false") public void shouldReturnResponseWithUnexpandedFileOwners() throws Exception { addOwnerFileToRoot(true); @@ -150,9 +295,9 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("a.txt", Sets.newHashSet(new GroupOwner(admin.username()))); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().filesApproved()).isEmpty(); } @Test @@ -167,8 +312,8 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).isEmpty(); - assertThat(resp.value().filesApproved) + assertThat(resp.value().files()).isEmpty(); + assertThat(resp.value().filesApproved()) .containsExactly("a.txt", Sets.newHashSet(new GroupOwner(admin.username()))); } @@ -181,9 +326,9 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("a.txt", Sets.newHashSet(new GroupOwner(admin.username()))); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().filesApproved()).isEmpty(); } @Test @@ -195,9 +340,9 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("a.txt", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().filesApproved()).isEmpty(); } @Test @@ -220,12 +365,12 @@ String changeId = createChange().getChangeId(); Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner)); + assertThat(resp.value().files()).containsExactly("a.txt", Sets.newHashSet(rootOwner)); addOwnerFileToProjectConfig(allProjects, true, user); resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(projectOwner)); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().files()).containsExactly("a.txt", Sets.newHashSet(projectOwner)); + assertThat(resp.value().filesApproved()).isEmpty(); } @Test @@ -298,8 +443,8 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).containsExactly("a.txt", Sets.newHashSet(rootOwner)); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().files()).containsExactly("a.txt", Sets.newHashSet(rootOwner)); + assertThat(resp.value().filesApproved()).isEmpty(); } private void assertInheritFromProject(Project.NameKey projectNameKey) throws Exception { @@ -310,15 +455,89 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("a.txt", Sets.newHashSet(rootOwner, projectOwner)); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().filesApproved()).isEmpty(); } private void addBrokenOwnersFileToRoot() throws Exception { merge(createChange(testRepo, "master", "Add OWNER file", "OWNERS", "{foo", "")); } + private ChangeIdentifier createChangeWithCopiedOwnerVote(TestAccount owner) throws Exception { + ChangeIdentifier changeId = + changeOperations + .newChange() + .project(project) + .owner(owner.id()) + .file(OWNED_JAVA_FILE) + .content("v1") + .create(); + vote(owner, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(owner.id()) + .file(OWNED_JAVA_FILE) + .content("v2") + .create(); + return changeId; + } + + private void setupAutoApprovalFor(TestAccount... owners) throws Exception { + String ownersYaml = + Arrays.stream(owners) + .map(owner -> String.format("- %s\n", owner.username())) + .collect(Collectors.joining()); + setupAutoApproval( + String.format("inherited: true\nauto-owners-approved: true\nowners:\n%s", ownersYaml)); + } + + private void setupAutoApprovalForJavaMatcher(TestAccount owner) throws Exception { + setupAutoApproval( + String.format( + "inherited: true\nmatchers:\n" + + "- suffix: .java\n" + + " auto-owners-approved: true\n" + + " owners:\n" + + " - %s\n", + owner.username())); + } + + private void setupAutoApproval(String ownersContent) throws Exception { + updateLabel(b -> b.setCopyCondition("approverin:" + FULL_OPERAND_WITH_PLUGIN_NAME)); + merge(createChange(testRepo, "master", "Add OWNER file", "OWNERS", ownersContent, "")); + } + + private void assertFilesApproval( + String changeId, + String filePath, + java.util.Set<GroupOwner> explicitlyApprovedOwners, + java.util.Set<GroupOwner> autoApprovedOwners) + throws Exception { + Response<FilesOwnersResponse> response = + assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); + assertThat(response.value().files()).isEmpty(); + if (explicitlyApprovedOwners.isEmpty()) { + assertThat(response.value().filesApproved()).isEmpty(); + } else { + assertThat(response.value().filesApproved()) + .containsExactly(filePath, explicitlyApprovedOwners); + } + if (autoApprovedOwners.isEmpty()) { + assertThat(response.value().filesAutoApproved()).isEmpty(); + } else { + assertThat(response.value().filesAutoApproved()) + .containsExactly(filePath, autoApprovedOwners); + } + } + + private Set<GroupOwner> owners(TestAccount... accounts) { + return java.util.Arrays.stream(accounts) + .map(account -> new Owner(account.fullName(), account.id().get())) + .collect(Collectors.toSet()); + } + private void addOwnerFileToProjectConfig(Project.NameKey projectNameKey, boolean inherit) throws Exception { addOwnerFileToProjectConfig(projectNameKey, inherit, user); @@ -422,4 +641,27 @@ } return clonedProject; } + + private void vote(TestAccount user, String changeId, int vote) throws Exception { + requestScopeOperations.setApiUser(user.id()); + gApi.changes() + .id(changeId) + .current() + .review(new ReviewInput().label(LabelId.CODE_REVIEW, vote)); + } + + private void updateLabel(java.util.function.Consumer<LabelType.Builder> update) throws Exception { + try (ProjectConfigUpdate u = updateProject(allProjects)) { + u.getConfig().updateLabelType(LabelId.CODE_REVIEW, update); + u.save(); + } + } + + private void allowCodeReviewForRegisteredUsers() throws Exception { + projectOperations + .project(allProjects) + .forUpdate() + .add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, 2)) + .update(); + } }
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersSubmitRequirementsIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersSubmitRequirementsIT.java index 3915997..026ebdb 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersSubmitRequirementsIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersSubmitRequirementsIT.java
@@ -70,19 +70,19 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("foo", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); - assertThat(resp.value().ownersLabels).isEmpty(); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().ownersLabels()).isEmpty(); + assertThat(resp.value().filesApproved()).isEmpty(); // give CR+1 as requested recommend(changeId); resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).isEmpty(); - assertThat(resp.value().ownersLabels) + assertThat(resp.value().files()).isEmpty(); + assertThat(resp.value().ownersLabels()) .containsExactly(admin.id().get(), Map.of(LabelId.CODE_REVIEW, 1)); - assertThat(resp.value().filesApproved) + assertThat(resp.value().filesApproved()) .containsExactly("foo", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); } @@ -97,18 +97,18 @@ Response<FilesOwnersResponse> resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files) + assertThat(resp.value().files()) .containsExactly("foo", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); - assertThat(resp.value().ownersLabels).isEmpty(); - assertThat(resp.value().filesApproved).isEmpty(); + assertThat(resp.value().ownersLabels()).isEmpty(); + assertThat(resp.value().filesApproved()).isEmpty(); // give LabelFoo+1 as requested gApi.changes().id(changeId).current().review(new ReviewInput().label(label, 1)); resp = assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); - assertThat(resp.value().files).isEmpty(); - assertThat(resp.value().ownersLabels).containsEntry(admin.id().get(), Map.of(label, 1)); - assertThat(resp.value().filesApproved) + assertThat(resp.value().files()).isEmpty(); + assertThat(resp.value().ownersLabels()).containsEntry(admin.id().get(), Map.of(label, 1)); + assertThat(resp.value().filesApproved()) .containsExactly("foo", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get()))); }
diff --git a/owners/web/gr-files.ts b/owners/web/gr-files.ts index dbacad8..b13594d 100644 --- a/owners/web/gr-files.ts +++ b/owners/web/gr-files.ts
@@ -48,21 +48,25 @@ const STATUS_CODE = { MISSING: 'missing', APPROVED: 'approved', + AUTO_APPROVED: 'autoApproved', }; const STATUS_ICON = { [STATUS_CODE.MISSING]: 'schedule', [STATUS_CODE.APPROVED]: 'check', + [STATUS_CODE.AUTO_APPROVED]: 'published_with_changes', }; const FILE_STATUS = { [FileStatus.NEEDS_APPROVAL]: STATUS_CODE.MISSING, [FileStatus.APPROVED]: STATUS_CODE.APPROVED, + [FileStatus.AUTO_APPROVED]: STATUS_CODE.AUTO_APPROVED, }; const HOVER_HEADING = { [STATUS_CODE.MISSING]: "Needs Owners' Approval", [STATUS_CODE.APPROVED]: 'Approved by Owners', + [STATUS_CODE.AUTO_APPROVED]: 'Auto-approved by Owners', }; const DISPLAY_OWNERS_FOR_FILE_LIMIT = 5; @@ -251,6 +255,9 @@ :host([file-status='approved']) gr-icon.status { color: var(--positive-green-text-color); } + :host([file-status='autoApproved']) gr-icon.status { + color: var(--positive-green-text-color); + } :host([file-status='missing']) gr-icon.status { color: #ffa62f; } @@ -459,7 +466,9 @@ if ( hasOwnersSubmitRequirement(change) && filesOwners && - (filesOwners.files || filesOwners.files_approved) + (filesOwners.files || + filesOwners.files_approved || + filesOwners.files_auto_approved) ) { return !userRole || userRole === UserRole.ANONYMOUS; } @@ -476,7 +485,13 @@ const fileOwners = (filesOwners.files ?? {})[path]; const fileApprovers = (filesOwners.files_approved ?? {})[path]; - if (fileApprovers) { + const fileAutoApprovers = (filesOwners.files_auto_approved ?? {})[path]; + if (fileAutoApprovers) { + return { + fileStatus: FileStatus.AUTO_APPROVED, + owners: fileAutoApprovers, + }; + } else if (fileApprovers) { return { fileStatus: FileStatus.APPROVED, owners: fileApprovers,
diff --git a/owners/web/gr-files_test.ts b/owners/web/gr-files_test.ts index d05f6e7..d8ede81 100644 --- a/owners/web/gr-files_test.ts +++ b/owners/web/gr-files_test.ts
@@ -46,9 +46,11 @@ suite('owners status tests', () => { const path = 'readme.md'; const approvedPath = 'db.sql'; + const autoApprovedPath = 'foo.java'; const filesOwners = { files: {[path]: [{name: 'John', id: 1}]}, files_approved: {[approvedPath]: [{name: 'Merry', id: 2}]}, + files_auto_approved: {[autoApprovedPath]: [{name: 'Merry', id: 2}]}, } as unknown as FilesOwners; suite('shouldHide tests', () => { @@ -261,6 +263,34 @@ ); }); + test('getFileOwnership - should return owners from `files_auto_approved` when file is auto-approved', () => { + const filesOwnersWithExplicitAndAutoApprovers = { + files_approved: { + [approvedPath]: [ + {name: 'Merry', id: 2}, + {name: 'John', id: 1}, + ], + }, + files_auto_approved: { + [autoApprovedPath]: [{name: 'Merry', id: 2}], + }, + } as unknown as FilesOwners; + + assert.equal( + deepEqual( + getFileOwnership( + autoApprovedPath, + filesOwnersWithExplicitAndAutoApprovers + ), + { + fileStatus: FileStatus.AUTO_APPROVED, + owners: [{name: 'Merry', id: 2}], + } as FileOwnership + ), + true + ); + }); + test('getFileOwnership - should return `FileOwnership` with `NOT_OWNED` fileStatus when file has no owner', () => { assert.equal( deepEqual(getFileOwnership(path, emptyFilesOwners), {
diff --git a/owners/web/gr-owned-files.ts b/owners/web/gr-owned-files.ts index 1e64398..428e85d 100644 --- a/owners/web/gr-owned-files.ts +++ b/owners/web/gr-owned-files.ts
@@ -53,11 +53,13 @@ export enum FileStatus { NEEDS_APPROVAL = 'missing', APPROVED = 'approved', + AUTO_APPROVED = 'autoApproved', } const STATUS_ICON = { [FileStatus.NEEDS_APPROVAL]: 'schedule', [FileStatus.APPROVED]: 'check', + [FileStatus.AUTO_APPROVED]: 'published_with_changes', }; interface OwnedFileInfo { @@ -125,7 +127,8 @@ padding: var(--spacing-xs) 0px; margin-left: 3px; } - :host([files-status='approved']) gr-icon.status { + :host([files-status='approved']) gr-icon.status, + :host([files-status='autoApproved']) gr-icon.status { color: var(--positive-green-text-color); } :host([files-status='missing']) gr-icon.status { @@ -207,6 +210,9 @@ filesApproved: number, filesPending: number ): [string, string] { + const allFilesApproved = + filesStatus === FileStatus.APPROVED || + filesStatus === FileStatus.AUTO_APPROVED; const pendingInfo = filesPending > 0 ? `Missing approval for ${filesPending} file${ @@ -220,13 +226,11 @@ } already approved.` : ''; const info = `${ - FileStatus.APPROVED === filesStatus + allFilesApproved ? approvedInfo : `${pendingInfo}${filesApproved > 0 ? ` and ${approvedInfo}` : '.'}` }`; - const summary = `${ - FileStatus.APPROVED === filesStatus ? 'Approved' : 'Missing' - }`; + const summary = `${allFilesApproved ? 'Approved' : 'Missing'}`; return [info, summary]; } } @@ -284,6 +288,9 @@ gr-icon.status.approved { color: var(--positive-green-text-color); } + gr-icon.status.autoApproved { + color: var(--positive-green-text-color); + } gr-icon.status.missing { color: #ffa62f; } @@ -499,7 +506,9 @@ if ( !owner || !filesOwners || - (!filesOwners.files && !filesOwners.files_approved) + (!filesOwners.files && + !filesOwners.files_approved && + !filesOwners.files_auto_approved) ) { return; } @@ -520,10 +529,17 @@ FileStatus.APPROVED, emailWithoutDomain ); + const autoApprovedFiles = collectOwnedFiles( + owner, + groupPrefix, + filesOwners.files_auto_approved ?? {}, + FileStatus.AUTO_APPROVED, + emailWithoutDomain + ); return { - ownedFiles: [...pendingFiles, ...approvedFiles], + ownedFiles: [...pendingFiles, ...approvedFiles, ...autoApprovedFiles], numberOfPending: pendingFiles.length, - numberOfApproved: approvedFiles.length, + numberOfApproved: approvedFiles.length + autoApprovedFiles.length, } as OwnedFilesInfo; }
diff --git a/owners/web/gr-owned-files_test.ts b/owners/web/gr-owned-files_test.ts index fd37f67..540017e 100644 --- a/owners/web/gr-owned-files_test.ts +++ b/owners/web/gr-owned-files_test.ts
@@ -93,6 +93,40 @@ ); }); + test('ownedFiles - should return auto-approved files', () => { + const autoApprovedFilesOwners = { + files_auto_approved: { + [ownedApprovedFile]: [fileOwner(1)], + }, + } as unknown as FilesOwners; + assert.equal( + deepEqual(ownedFiles(owner, autoApprovedFilesOwners), { + ownedFiles: [ + {file: ownedApprovedFile, status: FileStatus.AUTO_APPROVED}, + ], + numberOfApproved: 1, + numberOfPending: 0, + }), + true + ); + }); + + test('ownedFiles - should return explicitly approved files without auto approval flag', () => { + const explicitlyApprovedFilesOwners = { + files_approved: { + [ownedApprovedFile]: [fileOwner(1)], + }, + } as unknown as FilesOwners; + assert.equal( + deepEqual(ownedFiles(owner, explicitlyApprovedFilesOwners), { + ownedFiles: [{file: ownedApprovedFile, status: FileStatus.APPROVED}], + numberOfApproved: 1, + numberOfPending: 0, + }), + true + ); + }); + test('ownedFiles - should match file owner through email without domain name', () => { const filesOwners = { files: {
diff --git a/owners/web/owners-model.ts b/owners/web/owners-model.ts index 48e7e08..1edb46b 100644 --- a/owners/web/owners-model.ts +++ b/owners/web/owners-model.ts
@@ -50,6 +50,7 @@ export enum FileStatus { NEEDS_APPROVAL = 'NEEDS_APPROVAL', APPROVED = 'APPROVED', + AUTO_APPROVED = 'AUTO_APPROVED', NOT_OWNED = 'NOT_OWNED', }
diff --git a/owners/web/owners-service.ts b/owners/web/owners-service.ts index 0a57362..974aa58 100644 --- a/owners/web/owners-service.ts +++ b/owners/web/owners-service.ts
@@ -52,6 +52,7 @@ export interface FilesOwners { files: OwnedFiles; files_approved: OwnedFiles; + files_auto_approved?: OwnedFiles; owners_labels: OwnersLabels; }
