Merge changes I1ec99d8b,I2109d0e3,I39e09c27 * changes: Clarify in docs that approvals >= the configured approvals also count Test that OO+2 counts as code owner override when OO+1 is required Support configuring multiple override approvals
diff --git a/.eslintrc.json b/.eslintrc.json index b9a6517..2c0d24a 100644 --- a/.eslintrc.json +++ b/.eslintrc.json
@@ -90,7 +90,8 @@ 2, { "ignoreComments": true, - "ignorePattern": "^import .*;$" + "ignorePattern": "^import .*;$", + "ignoreTemplateLiterals": true } ], "new-cap": [ @@ -100,7 +101,8 @@ "Polymer", "LegacyElementMixin", "GestureEventListeners", - "LegacyDataMixin" + "LegacyDataMixin", + "CodeOwnersModelMixin" ] } ], @@ -235,4 +237,4 @@ } } ] -} \ No newline at end of file +}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java b/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java index 8562057..8caa0fe 100644 --- a/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java +++ b/java/com/google/gerrit/plugins/codeowners/api/GeneralInfo.java
@@ -14,6 +14,8 @@ package com.google.gerrit.plugins.codeowners.api; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; + /** * Representation of the general code owners configuration in the REST API. * @@ -43,4 +45,7 @@ * code owner override. */ public String overrideInfoUrl; + + /** Policy that controls who should own paths that have no code owners defined. */ + public FallbackCodeOwners fallbackCodeOwners; }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index 8211b7c..26d67f0 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -48,6 +48,7 @@ import java.nio.file.Path; import java.util.Collections; import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Stream; import java.util.stream.StreamSupport; @@ -199,9 +200,8 @@ logger.atFine().log("global code owners = %s", globalCodeOwners); // If the branch doesn't contain any code owner config file yet, we apply special logic - // (project - // owners count as code owners) to allow bootstrapping the code owner configuration in the - // branch. + // (project owners count as code owners) to allow bootstrapping the code owner configuration + // in the branch. boolean isBootstrapping = !codeOwnerConfigScannerFactory.create().containsAnyCodeOwnerConfigFile(branch); logger.atFine().log("isBootstrapping = %s", isBootstrapping); @@ -360,6 +360,20 @@ codeOwnerStatus = CodeOwnerStatus.PENDING; } + // Since there are no code owner config files in bootstrapping mode, fallback code owners also + // apply if they are configured. We can skip checking them if we already found that the file was + // approved. + if (codeOwnerStatus != CodeOwnerStatus.APPROVED) { + codeOwnerStatus = + getCodeOwnerStatusForFallbackCodeOwners( + codeOwnerStatus, + branch.project(), + enableImplicitApprovalFromUploader, + reviewerAccountIds, + approverAccountIds, + absolutePath); + } + PathCodeOwnerStatus pathCodeOwnerStatus = PathCodeOwnerStatus.create(absolutePath, codeOwnerStatus); logger.atFine().log("pathCodeOwnerStatus = %s", pathCodeOwnerStatus); @@ -485,6 +499,8 @@ codeOwnerStatus.set(CodeOwnerStatus.PENDING); } + AtomicBoolean hasRevelantCodeOwnerDefinitions = new AtomicBoolean(false); + AtomicBoolean parentCodeOwnersAreIgnored = new AtomicBoolean(false); codeOwnerConfigHierarchy.visit( branch, revision, @@ -497,6 +513,10 @@ codeOwnerConfig.key().folderPath(), codeOwnerConfig.key().fileName().orElse("<default>")); + if (codeOwners.hasRevelantCodeOwnerDefinitions()) { + hasRevelantCodeOwnerDefinitions.set(true); + } + if (isApproved( absolutePath, codeOwners, @@ -516,7 +536,29 @@ // We need to continue to check if any of the higher-level code owners approved the // change or is a reviewer. return true; + }, + codeOwnerConfigKey -> { + logger.atFine().log( + "code owner config %s ignores parent code owners for %s", + codeOwnerConfigKey, absolutePath); + parentCodeOwnersAreIgnored.set(true); }); + + // If no code owners have been defined for the file and if parent code owners are not ignored, + // the fallback code owners apply if they are configured. We can skip checking them if we + // already found that the file was approved. + if (codeOwnerStatus.get() != CodeOwnerStatus.APPROVED + && !hasRevelantCodeOwnerDefinitions.get() + && !parentCodeOwnersAreIgnored.get()) { + codeOwnerStatus.set( + getCodeOwnerStatusForFallbackCodeOwners( + codeOwnerStatus.get(), + branch.project(), + enableImplicitApprovalFromUploader, + reviewerAccountIds, + approverAccountIds, + absolutePath)); + } } PathCodeOwnerStatus pathCodeOwnerStatus = @@ -525,6 +567,66 @@ return pathCodeOwnerStatus; } + /** + * Computes the code owner status for the given path based on the configured fallback code owners. + */ + private CodeOwnerStatus getCodeOwnerStatusForFallbackCodeOwners( + CodeOwnerStatus codeOwnerStatus, + Project.NameKey project, + boolean enableImplicitApprovalFromUploader, + ImmutableSet<Account.Id> reviewerAccountIds, + ImmutableSet<Account.Id> approverAccountIds, + Path absolutePath) { + FallbackCodeOwners fallbackCodeOwners = + codeOwnersPluginConfiguration.getFallbackCodeOwners(project); + logger.atFine().log( + "getting code owner status for fallback code owners (fallback code owners = %s)", + fallbackCodeOwners); + switch (fallbackCodeOwners) { + case NONE: + logger.atFine().log("no fallback code owners"); + return codeOwnerStatus; + case ALL_USERS: + return getCodeOwnerStatusIfAllUsersAreCodeOwners( + enableImplicitApprovalFromUploader, + reviewerAccountIds, + approverAccountIds, + absolutePath); + } + + throw new StorageException( + String.format("unknown fallback code owners configured: %s", fallbackCodeOwners)); + } + + /** Computes the code owner status for the given path assuming that all users are code owners. */ + private CodeOwnerStatus getCodeOwnerStatusIfAllUsersAreCodeOwners( + boolean enableImplicitApprovalFromUploader, + ImmutableSet<Account.Id> reviewerAccountIds, + ImmutableSet<Account.Id> approverAccountIds, + Path absolutePath) { + logger.atFine().log( + "getting code owner status for fallback code owners (all users are fallback code owners)"); + + if (enableImplicitApprovalFromUploader) { + logger.atFine().log( + "%s was implicitly approved by the patch set uploader since the uploader is a fallback" + + " code owner", + absolutePath); + return CodeOwnerStatus.APPROVED; + } + + if (!approverAccountIds.isEmpty()) { + logger.atFine().log("%s was approved by a fallback code owner", absolutePath); + return CodeOwnerStatus.APPROVED; + } else if (!reviewerAccountIds.isEmpty()) { + logger.atFine().log("%s has a fallback code owner as reviewer", absolutePath); + return CodeOwnerStatus.PENDING; + } + + logger.atFine().log("%s has no fallback code owner as a reviewer", absolutePath); + return CodeOwnerStatus.INSUFFICIENT_REVIEWERS; + } + private boolean isApproved( Path absolutePath, CodeOwnerResolverResult codeOwners,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java index e3bbb85..e9db8c8 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -28,6 +28,7 @@ import java.io.IOException; import java.nio.file.Path; import java.util.Optional; +import java.util.function.Consumer; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; @@ -74,10 +75,38 @@ ObjectId revision, Path absolutePath, CodeOwnerConfigVisitor codeOwnerConfigVisitor) { + visit( + branchNameKey, + revision, + absolutePath, + codeOwnerConfigVisitor, + /* parentCodeOwnersIgnoredCallback= */ codeOwnerConfigKey -> {}); + } + + /** + * Visits the code owner configs in the given branch that apply for the given path by following + * the path hierarchy from the given path up to the root folder. + * + * @param branchNameKey project and branch from which the code owner configs should be visited + * @param revision the branch revision from which the code owner configs should be loaded + * @param absolutePath the path for which the code owner configs should be visited; the path must + * be absolute; can be the path of a file or folder; the path may or may not exist + * @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner + * configs + * @param parentCodeOwnersIgnoredCallback callback that is invoked for the first visited code + * owner config that ignores parent code owners + */ + public void visit( + BranchNameKey branchNameKey, + ObjectId revision, + Path absolutePath, + CodeOwnerConfigVisitor codeOwnerConfigVisitor, + Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) { requireNonNull(branchNameKey, "branch"); requireNonNull(revision, "revision"); requireNonNull(absolutePath, "absolutePath"); requireNonNull(codeOwnerConfigVisitor, "codeOwnerConfigVisitor"); + requireNonNull(parentCodeOwnersIgnoredCallback, "parentCodeOwnersIgnoredCallback"); checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath); logger.atFine().log( @@ -93,14 +122,19 @@ // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config // exists. logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder); + CodeOwnerConfig.Key codeOwnerConfigKey = + CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder); Optional<PathCodeOwners> pathCodeOwners = - pathCodeOwnersFactory.create( - CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder), revision, absolutePath); + pathCodeOwnersFactory.create(codeOwnerConfigKey, revision, absolutePath); if (pathCodeOwners.isPresent()) { logger.atFine().log("visit code owner config for %s", ownerConfigFolder); boolean visitFurtherCodeOwnerConfigs = codeOwnerConfigVisitor.visit(pathCodeOwners.get().getCodeOwnerConfig()); - boolean ignoreParentCodeOwners = pathCodeOwners.get().ignoreParentCodeOwners(); + boolean ignoreParentCodeOwners = + pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners(); + if (ignoreParentCodeOwners) { + parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey); + } logger.atFine().log( "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s", visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java index 6d9d05e..74e2db6 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -45,7 +45,6 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.stream.Stream; /** Class to resolve {@link CodeOwnerReference}s to {@link CodeOwner}s. */ public class CodeOwnerResolver { @@ -154,7 +153,10 @@ .filePath(codeOwnerConfig.key().fileName().orElse("<default>")) .build())) { logger.atFine().log("resolving path code owners for path %s", absolutePath); - return resolve(pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).get()); + PathCodeOwnersResult pathCodeOwnersResult = + pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig(); + return resolve( + pathCodeOwnersResult.getPathCodeOwners(), pathCodeOwnersResult.hasUnresolvedImports()); } } @@ -176,8 +178,22 @@ * @see #resolve(CodeOwnerReference) */ public CodeOwnerResolverResult resolve(Set<CodeOwnerReference> codeOwnerReferences) { + return resolve(codeOwnerReferences, /* hasUnresolvedImports= */ false); + } + + /** + * Resolves the given {@link CodeOwnerReference}s to {@link CodeOwner}s. + * + * @param codeOwnerReferences the code owner references that should be resolved + * @param hasUnresolvedImports whether there are unresolved imports + * @return the {@link CodeOwner} for the given code owner references + * @see #resolve(CodeOwnerReference) + */ + private CodeOwnerResolverResult resolve( + Set<CodeOwnerReference> codeOwnerReferences, boolean hasUnresolvedImports) { requireNonNull(codeOwnerReferences, "codeOwnerReferences"); AtomicBoolean ownedByAllUsers = new AtomicBoolean(false); + AtomicBoolean hasUnresolvedCodeOwners = new AtomicBoolean(false); ImmutableSet<CodeOwner> codeOwners = codeOwnerReferences.stream() .filter( @@ -188,9 +204,19 @@ } return true; }) - .flatMap(this::resolve) + .map(this::resolve) + .filter( + codeOwner -> { + if (!codeOwner.isPresent()) { + hasUnresolvedCodeOwners.set(true); + return false; + } + return true; + }) + .map(Optional::get) .collect(toImmutableSet()); - return CodeOwnerResolverResult.create(codeOwners, ownedByAllUsers.get()); + return CodeOwnerResolverResult.create( + codeOwners, ownedByAllUsers.get(), hasUnresolvedCodeOwners.get(), hasUnresolvedImports); } /** @@ -233,35 +259,34 @@ * @return the {@link CodeOwner} for the code owner reference if it was resolved, otherwise {@link * Optional#empty()} */ - @VisibleForTesting - public Stream<CodeOwner> resolve(CodeOwnerReference codeOwnerReference) { + public Optional<CodeOwner> resolve(CodeOwnerReference codeOwnerReference) { String email = requireNonNull(codeOwnerReference, "codeOwnerReference").email(); logger.atFine().log("resolving code owner reference %s", codeOwnerReference); if (!isEmailDomainAllowed(email)) { logger.atFine().log("domain of email %s is not allowed", email); - return Stream.of(); + return Optional.empty(); } Optional<AccountState> accountState = lookupEmail(email).flatMap(accountId -> lookupAccount(accountId, email)); if (!accountState.isPresent()) { logger.atFine().log("no account for email %s", email); - return Stream.of(); + return Optional.empty(); } if (!accountState.get().account().isActive()) { logger.atFine().log("account for email %s is inactive", email); - return Stream.of(); + return Optional.empty(); } if (enforceVisibility && !isVisible(accountState.get(), email)) { logger.atFine().log( "account %d or email %s not visible", accountState.get().account().id().get(), email); - return Stream.of(); + return Optional.empty(); } CodeOwner codeOwner = CodeOwner.create(accountState.get().account().id()); logger.atFine().log("resolved to code owner %s", codeOwner); - return Stream.of(codeOwner); + return Optional.of(codeOwner); } /** Whether the given account can be seen. */
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java index 226d541..a03f0e1 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -43,17 +43,40 @@ */ public abstract boolean ownedByAllUsers(); + /** Whether there are code owner references which couldn't be resolved. */ + public abstract boolean hasUnresolvedCodeOwners(); + + /** Whether there are imports which couldn't be resolved. */ + public abstract boolean hasUnresolvedImports(); + + /** + * Whether there are any code owners defined for the path, regardless of whether they can be + * resolved or not. + */ + public boolean hasRevelantCodeOwnerDefinitions() { + return !codeOwners().isEmpty() + || ownedByAllUsers() + || hasUnresolvedCodeOwners() + || hasUnresolvedImports(); + } + @Override public String toString() { return MoreObjects.toStringHelper(this) .add("codeOwners", codeOwners()) .add("ownedByAllUsers", ownedByAllUsers()) + .add("hasUnresolvedCodeOwners", hasUnresolvedCodeOwners()) + .add("hasUnresolvedImports", hasUnresolvedImports()) .toString(); } /** Creates a {@link CodeOwnerResolverResult} instance. */ public static CodeOwnerResolverResult create( - ImmutableSet<CodeOwner> codeOwners, boolean ownedByAllUsers) { - return new AutoValue_CodeOwnerResolverResult(codeOwners, ownedByAllUsers); + ImmutableSet<CodeOwner> codeOwners, + boolean ownedByAllUsers, + boolean hasUnresolvedCodeOwners, + boolean hasUnresolvedImports) { + return new AutoValue_CodeOwnerResolverResult( + codeOwners, ownedByAllUsers, hasUnresolvedCodeOwners, hasUnresolvedImports); } }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java new file mode 100644 index 0000000..5ad4079 --- /dev/null +++ b/java/com/google/gerrit/plugins/codeowners/backend/FallbackCodeOwners.java
@@ -0,0 +1,36 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.backend; + +/** Defines who owns paths for which no code owners are defined. */ +public enum FallbackCodeOwners { + /** + * Paths for which no code owners are defined are owned by no one. This means changes that touch + * these files can only be submitted with a code owner override. + */ + NONE, + + /** + * Paths for which no code owners are defined are owned by all users. This means changes to these + * paths can be approved by anyone. If implicit approvals are enabled, these files are always + * automatically approved. + * + * <p>The {@code ALL_USERS} option should only be used with care as it means that any path that is + * not covered by the code owner config files is automatically opened up to everyone and mistakes + * with configuring code owners can easily happen. This is why this option is intended to be only + * used if requiring code owner approvals should not be enforced. + */ + ALL_USERS; +}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java index c8a9b82..078ba17 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -20,7 +20,6 @@ import static java.util.stream.Collectors.toSet; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Project; @@ -123,7 +122,7 @@ private final Path path; private final PathExpressionMatcher pathExpressionMatcher; - private CodeOwnerConfig resolvedCodeOwnerConfig; + private PathCodeOwnersResult pathCodeOwnersResult; private PathCodeOwners( ProjectCache projectCache, @@ -146,32 +145,6 @@ } /** - * Gets the code owners from the code owner config that apply to the path. - * - * <p>Code owners from inherited code owner configs are not considered. - * - * @return the code owners of the path - */ - public ImmutableSet<CodeOwnerReference> get() { - logger.atFine().log("computing path code owners for %s from %s", path, codeOwnerConfig.key()); - ImmutableSet<CodeOwnerReference> pathCodeOwners = - resolveCodeOwnerConfig().codeOwnerSets().stream() - .flatMap(codeOwnerSet -> codeOwnerSet.codeOwners().stream()) - .collect(toImmutableSet()); - logger.atFine().log("pathCodeOwners = %s", pathCodeOwners); - return pathCodeOwners; - } - - /** - * Whether parent code owners should be ignored for the path. - * - * @return whether parent code owners should be ignored for the path - */ - public boolean ignoreParentCodeOwners() { - return resolveCodeOwnerConfig().ignoreParentCodeOwners(); - } - - /** * Resolves the {@link #codeOwnerConfig}. * * <p>Resolving means that: @@ -207,9 +180,9 @@ * * @return the resolved code owner config */ - private CodeOwnerConfig resolveCodeOwnerConfig() { - if (this.resolvedCodeOwnerConfig != null) { - return this.resolvedCodeOwnerConfig; + public PathCodeOwnersResult resolveCodeOwnerConfig() { + if (this.pathCodeOwnersResult != null) { + return this.pathCodeOwnersResult; } try (TraceTimer traceTimer = @@ -233,7 +206,8 @@ getMatchingPerFileCodeOwnerSets(codeOwnerConfig) .forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet); - resolveImports(codeOwnerConfig, resolvedCodeOwnerConfigBuilder); + boolean hasUnresolvedImports = + !resolveImports(codeOwnerConfig, resolvedCodeOwnerConfigBuilder); CodeOwnerConfig resolvedCodeOwnerConfig = resolvedCodeOwnerConfigBuilder.build(); @@ -255,15 +229,24 @@ .build(); } - this.resolvedCodeOwnerConfig = resolvedCodeOwnerConfig; - logger.atFine().log("resolved code owner config = %s", resolvedCodeOwnerConfig); - return this.resolvedCodeOwnerConfig; + this.pathCodeOwnersResult = + PathCodeOwnersResult.create(path, resolvedCodeOwnerConfig, hasUnresolvedImports); + logger.atFine().log("path code owners result = %s", pathCodeOwnersResult); + return this.pathCodeOwnersResult; } } - private void resolveImports( + /** + * Resolve the imports of the given code owner config. + * + * @param importingCodeOwnerConfig the code owner config for which imports should be resolved + * @param resolvedCodeOwnerConfigBuilder the builder for the resolved code owner config + * @return whether all imports have been resolved successfully + */ + private boolean resolveImports( CodeOwnerConfig importingCodeOwnerConfig, CodeOwnerConfig.Builder resolvedCodeOwnerConfigBuilder) { + boolean hasUnresolvedImports = false; try (TraceTimer traceTimer = TraceContext.newTimer( "Resolve code owner config imports", @@ -307,6 +290,7 @@ Optional<ProjectState> projectState = projectCache.get(keyOfImportedCodeOwnerConfig.project()); if (!projectState.isPresent()) { + hasUnresolvedImports = true; logger.atWarning().log( "cannot resolve code owner config %s that is imported by code owner config %s:" + " project %s not found", @@ -316,6 +300,7 @@ continue; } if (!projectState.get().statePermitsRead()) { + hasUnresolvedImports = true; logger.atWarning().log( "cannot resolve code owner config %s that is imported by code owner config %s:" + " state of project %s doesn't permit read", @@ -337,6 +322,7 @@ : codeOwners.getFromCurrentRevision(keyOfImportedCodeOwnerConfig); if (!mayBeImportedCodeOwnerConfig.isPresent()) { + hasUnresolvedImports = true; logger.atWarning().log( "cannot resolve code owner config %s that is imported by code owner config %s" + " (revision = %s)", @@ -404,6 +390,7 @@ } } } + return !hasUnresolvedImports; } public static CodeOwnerConfig.Key createKeyForImportedCodeOwnerConfig(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java new file mode 100644 index 0000000..a463aef --- /dev/null +++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
@@ -0,0 +1,80 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.backend; + +import static com.google.common.collect.ImmutableSet.toImmutableSet; + +import com.google.auto.value.AutoValue; +import com.google.common.base.MoreObjects; +import com.google.common.collect.ImmutableSet; +import com.google.common.flogger.FluentLogger; +import java.nio.file.Path; + +/** The result of resolving path code owners via {@link PathCodeOwners}. */ +@AutoValue +public abstract class PathCodeOwnersResult { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + /** Gets the path for which the code owner config was resolved. */ + abstract Path path(); + + /** Gets the resolved code owner config. */ + abstract CodeOwnerConfig codeOwnerConfig(); + + /** Whether there are unresolved imports. */ + public abstract boolean hasUnresolvedImports(); + + /** + * Gets the code owners from the code owner config that apply to the path. + * + * <p>Code owners from inherited code owner configs are not considered. + * + * @return the code owners of the path + */ + public ImmutableSet<CodeOwnerReference> getPathCodeOwners() { + logger.atFine().log( + "computing path code owners for %s from %s", path(), codeOwnerConfig().key()); + ImmutableSet<CodeOwnerReference> pathCodeOwners = + codeOwnerConfig().codeOwnerSets().stream() + .flatMap(codeOwnerSet -> codeOwnerSet.codeOwners().stream()) + .collect(toImmutableSet()); + logger.atFine().log("pathCodeOwners = %s", pathCodeOwners); + return pathCodeOwners; + } + + /** + * Whether parent code owners should be ignored for the path. + * + * @return whether parent code owners should be ignored for the path + */ + public boolean ignoreParentCodeOwners() { + return codeOwnerConfig().ignoreParentCodeOwners(); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("path", path()) + .add("codeOwnerConfig", codeOwnerConfig()) + .add("hasUnresolvedImports", hasUnresolvedImports()) + .toString(); + } + + /** Creates a {@link CodeOwnerResolverResult} instance. */ + public static PathCodeOwnersResult create( + Path path, CodeOwnerConfig codeOwnerConfig, boolean hasUnresolvedImports) { + return new AutoValue_PathCodeOwnersResult(path, codeOwnerConfig, hasUnresolvedImports); + } +}
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java index a66dc1b..57bc488 100644 --- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java +++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
@@ -30,6 +30,7 @@ import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.server.config.PluginConfigFactory; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; @@ -139,6 +140,17 @@ } /** + * Gets the fallback code owners for the given project. + * + * @param project the project for which the fallback code owners should be retrieved + * @return the fallback code owners for the given project + */ + public FallbackCodeOwners getFallbackCodeOwners(Project.NameKey project) { + requireNonNull(project, "project"); + return generalConfig.getFallbackCodeOwners(project, getPluginConfig(project)); + } + + /** * Checks whether an implicit code owner approval from the last uploader is assumed. * * @param project the project for it should be checked whether implict approvals are enabled
diff --git a/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java index bd137ac..54ddf7c 100644 --- a/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java +++ b/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java
@@ -28,6 +28,7 @@ import com.google.gerrit.plugins.codeowners.api.CodeOwnerConfigValidationPolicy; import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.config.PluginConfigFactory; import com.google.gerrit.server.git.validators.CommitValidationMessage; @@ -59,6 +60,7 @@ @VisibleForTesting public static final String KEY_ALLOWED_EMAIL_DOMAIN = "allowedEmailDomain"; @VisibleForTesting public static final String KEY_FILE_EXTENSION = "fileExtension"; @VisibleForTesting public static final String KEY_READ_ONLY = "readOnly"; + @VisibleForTesting public static final String KEY_FALLBACK_CODE_OWNERS = "fallbackCodeOwners"; @VisibleForTesting public static final String KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED = @@ -118,6 +120,24 @@ ValidationMessage.Type.ERROR)); } + try { + projectLevelConfig + .getConfig() + .getEnum(SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS, FallbackCodeOwners.NONE); + } catch (IllegalArgumentException e) { + validationMessages.add( + new CommitValidationMessage( + String.format( + "The value for fallback code owners '%s' that is configured in %s (parameter %s.%s) is invalid.", + projectLevelConfig + .getConfig() + .getString(SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS), + fileName, + SECTION_CODE_OWNERS, + KEY_FALLBACK_CODE_OWNERS), + ValidationMessage.Type.ERROR)); + } + return ImmutableList.copyOf(validationMessages); } @@ -174,6 +194,46 @@ } /** + * Gets the fallback code owners that own paths that have no defined code owners. + * + * @param project the project for which the fallback code owners should be read + * @param pluginConfig the plugin config from which the fallback code owners should be read + * @return the fallback code owners that own paths that have no defined code owners + */ + FallbackCodeOwners getFallbackCodeOwners(Project.NameKey project, Config pluginConfig) { + requireNonNull(project, "project"); + requireNonNull(pluginConfig, "pluginConfig"); + + String fallbackCodeOwnersString = + pluginConfig.getString(SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS); + if (fallbackCodeOwnersString != null) { + try { + return pluginConfig.getEnum( + SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS, FallbackCodeOwners.NONE); + } catch (IllegalArgumentException e) { + logger.atWarning().log( + "Ignoring invalid value %s for fallback code owners in '%s.config' of project %s." + + " Falling back to global config.", + fallbackCodeOwnersString, pluginName, project.get()); + } + } + + try { + return pluginConfigFromGerritConfig.getEnum( + KEY_FALLBACK_CODE_OWNERS, FallbackCodeOwners.NONE); + } catch (IllegalArgumentException e) { + logger.atWarning().log( + "Ignoring invalid value %s for fallback code owners in gerrit.config (parameter" + + " plugin.%s.%s). Falling back to default value %s.", + pluginConfigFromGerritConfig.getString(KEY_FALLBACK_CODE_OWNERS), + pluginName, + KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, + FallbackCodeOwners.NONE); + return FallbackCodeOwners.NONE; + } + } + + /** * Gets the enable validation on commit received configuration from the given plugin config with * fallback to {@code gerrit.config} and default to {@code true}. *
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java index e7fcd72..6e53e33 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJson.java
@@ -116,6 +116,8 @@ codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(projectName) ? true : null; generalInfo.overrideInfoUrl = codeOwnersPluginConfiguration.getOverrideInfoUrl(projectName).orElse(null); + generalInfo.fallbackCodeOwners = + codeOwnersPluginConfiguration.getFallbackCodeOwners(projectName); return generalInfo; }
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/RenameEmail.java b/java/com/google/gerrit/plugins/codeowners/restapi/RenameEmail.java index 8df9a34..0ed505b 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/RenameEmail.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/RenameEmail.java
@@ -14,13 +14,10 @@ package com.google.gerrit.plugins.codeowners.restapi; -import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.Objects.requireNonNull; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.gerrit.entities.Account; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -56,7 +53,7 @@ @Singleton public class RenameEmail implements RestModifyView<BranchResource, RenameEmailInput> { @VisibleForTesting - public static String DEFAULT_COMMIT_MESSAGE = "Rename email in code owner config files"; + public static final String DEFAULT_COMMIT_MESSAGE = "Rename email in code owner config files"; private final Provider<CurrentUser> currentUser; private final PermissionBackend permissionBackend; @@ -158,19 +155,14 @@ } } - private Account.Id resolveEmail(String email) - throws ResourceConflictException, UnprocessableEntityException { + private Account.Id resolveEmail(String email) throws UnprocessableEntityException { requireNonNull(email, "email"); - ImmutableSet<CodeOwner> codeOwners = - codeOwnerResolver.resolve(CodeOwnerReference.create(email)).collect(toImmutableSet()); - if (codeOwners.isEmpty()) { + Optional<CodeOwner> codeOwner = codeOwnerResolver.resolve(CodeOwnerReference.create(email)); + if (!codeOwner.isPresent()) { throw new UnprocessableEntityException(String.format("cannot resolve email %s", email)); } - if (codeOwners.size() > 1) { - throw new ResourceConflictException(String.format("email %s is ambigious", email)); - } - return Iterables.getOnlyElement(codeOwners).accountId(); + return codeOwner.get().accountId(); } /**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java index 19f9a40..93504a7 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersPluginConfigValidatorIT.java
@@ -26,6 +26,7 @@ import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT; import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend; import com.google.gerrit.plugins.codeowners.config.BackendConfig; import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; @@ -406,6 +407,45 @@ + " (parameter codeOwners.mergeCommitStrategy) is invalid."); } + @Test + public void configureFallbackCodeOwners() throws Exception { + fetchRefsMetaConfig(); + + Config cfg = new Config(); + cfg.setEnum( + CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS, + /* subsection= */ null, + GeneralConfig.KEY_FALLBACK_CODE_OWNERS, + FallbackCodeOwners.ALL_USERS); + setCodeOwnersConfig(cfg); + + PushResult r = pushRefsMetaConfig(); + assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()).isEqualTo(Status.OK); + assertThat(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .isEqualTo(FallbackCodeOwners.ALL_USERS); + } + + @Test + public void cannotSetInvalidFallbackCodeOwners() throws Exception { + fetchRefsMetaConfig(); + + Config cfg = new Config(); + cfg.setString( + CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS, + /* subsection= */ null, + GeneralConfig.KEY_FALLBACK_CODE_OWNERS, + "INVALID"); + setCodeOwnersConfig(cfg); + + PushResult r = pushRefsMetaConfig(); + assertThat(r.getRemoteUpdate(RefNames.REFS_CONFIG).getStatus()) + .isEqualTo(Status.REJECTED_OTHER_REASON); + assertThat(r.getMessages()) + .contains( + "The value for fallback code owners 'INVALID' that is configured in code-owners.config" + + " (parameter codeOwners.fallbackCodeOwners) is invalid."); + } + private void fetchRefsMetaConfig() throws Exception { fetch(testRepo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG); testRepo.reset(RefNames.REFS_CONFIG);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java index 66edb89..9cc77ea 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerBranchConfigIT.java
@@ -30,6 +30,7 @@ import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.plugins.codeowners.config.BackendConfig; import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; import com.google.gerrit.plugins.codeowners.config.GeneralConfig; @@ -89,6 +90,8 @@ assertThat(codeOwnerBranchConfigInfo.general.fileExtension).isNull(); assertThat(codeOwnerBranchConfigInfo.general.mergeCommitStrategy) .isEqualTo(MergeCommitStrategy.ALL_CHANGED_FILES); + assertThat(codeOwnerBranchConfigInfo.general.fallbackCodeOwners) + .isEqualTo(FallbackCodeOwners.NONE); assertThat(codeOwnerBranchConfigInfo.general.implicitApprovals).isNull(); assertThat(codeOwnerBranchConfigInfo.general.overrideInfoUrl).isNull(); assertThat(codeOwnerBranchConfigInfo.disabled).isNull(); @@ -129,6 +132,15 @@ } @Test + public void getConfigWithConfiguredFallbackCodeOwners() throws Exception { + configureFallbackCodeOwners(project, FallbackCodeOwners.ALL_USERS); + CodeOwnerBranchConfigInfo codeOwnerBranchConfigInfo = + projectCodeOwnersApiFactory.project(project).branch("master").getConfig(); + assertThat(codeOwnerBranchConfigInfo.general.fallbackCodeOwners) + .isEqualTo(FallbackCodeOwners.ALL_USERS); + } + + @Test public void getConfigForBranchOfDisabledProject() throws Exception { disableCodeOwnersForProject(project); CodeOwnerBranchConfigInfo codeOwnerBranchConfigInfo = @@ -264,6 +276,11 @@ setConfig(project, null, GeneralConfig.KEY_MERGE_COMMIT_STRATEGY, mergeCommitStrategy.name()); } + private void configureFallbackCodeOwners( + Project.NameKey project, FallbackCodeOwners fallbackCodeOwners) throws Exception { + setConfig(project, null, GeneralConfig.KEY_FALLBACK_CODE_OWNERS, fallbackCodeOwners.name()); + } + private void configureDisabledBranch(Project.NameKey project, String disabledBranch) throws Exception { setCodeOwnersConfig(project, null, StatusConfig.KEY_DISABLED_BRANCH, disabledBranch);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java index de208e9..a6de7df 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerProjectConfigIT.java
@@ -34,6 +34,7 @@ import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.plugins.codeowners.config.BackendConfig; import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; import com.google.gerrit.plugins.codeowners.config.GeneralConfig; @@ -88,6 +89,8 @@ assertThat(codeOwnerProjectConfigInfo.general.fileExtension).isNull(); assertThat(codeOwnerProjectConfigInfo.general.mergeCommitStrategy) .isEqualTo(MergeCommitStrategy.ALL_CHANGED_FILES); + assertThat(codeOwnerProjectConfigInfo.general.fallbackCodeOwners) + .isEqualTo(FallbackCodeOwners.NONE); assertThat(codeOwnerProjectConfigInfo.general.implicitApprovals).isNull(); assertThat(codeOwnerProjectConfigInfo.general.overrideInfoUrl).isNull(); assertThat(codeOwnerProjectConfigInfo.status.disabled).isNull(); @@ -130,6 +133,15 @@ } @Test + public void getConfigWithConfiguredFallbackCodeOwners() throws Exception { + configureFallbackCodeOwners(project, FallbackCodeOwners.ALL_USERS); + CodeOwnerProjectConfigInfo codeOwnerProjectConfigInfo = + projectCodeOwnersApiFactory.project(project).getConfig(); + assertThat(codeOwnerProjectConfigInfo.general.fallbackCodeOwners) + .isEqualTo(FallbackCodeOwners.ALL_USERS); + } + + @Test public void getConfigForDisabledProject() throws Exception { disableCodeOwnersForProject(project); CodeOwnerProjectConfigInfo codeOwnerProjectConfigInfo = @@ -283,6 +295,11 @@ setConfig(project, null, GeneralConfig.KEY_MERGE_COMMIT_STRATEGY, mergeCommitStrategy.name()); } + private void configureFallbackCodeOwners( + Project.NameKey project, FallbackCodeOwners fallbackCodeOwners) throws Exception { + setConfig(project, null, GeneralConfig.KEY_FALLBACK_CODE_OWNERS, fallbackCodeOwners.name()); + } + private void configureDisabledBranch(Project.NameKey project, String disabledBranch) throws Exception { setCodeOwnersConfig(project, null, StatusConfig.KEY_DISABLED_BRANCH, disabledBranch);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java index ec0a368..b9e7502 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java
@@ -22,6 +22,8 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; @@ -527,14 +529,14 @@ // insert comment line at the top of the file b.append("# top comment\n"); - String[] lines = codeOwnerConfigFileContent.split("\\n"); - b.append(lines[0] + "\n"); + Iterable<String> lines = Splitter.on('\n').split(codeOwnerConfigFileContent); + b.append(Iterables.get(lines, /* position= */ 0) + "\n"); // insert comment line in the middle of the file b.append("# middle comment\n"); - for (int n = 1; n < lines.length; n++) { - b.append(lines[n] + "\n"); + for (String line : Iterables.skip(lines, /* numberToSkip= */ 1)) { + b.append(line + "\n"); } // insert comment line at the bottom of the file @@ -560,10 +562,11 @@ // verify that the comments are still present String codeOwnerConfigFileContent = codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getContent(); - String[] lines = codeOwnerConfigFileContent.split("\\n"); - assertThat(lines[0]).isEqualTo("# top comment"); - assertThat(lines[2]).isEqualTo("# middle comment"); - assertThat(lines[lines.length - 1]).isEqualTo("# bottom comment"); + Iterable<String> lines = Splitter.on('\n').split(codeOwnerConfigFileContent); + assertThat(Iterables.get(lines, /* position= */ 0)).isEqualTo("# top comment"); + assertThat(Iterables.get(lines, /* position= */ 2)).isEqualTo("# middle comment"); + assertThat(Iterables.get(lines, /* position= */ Iterables.size(lines) - 2)) + .isEqualTo("# bottom comment"); } @Test @@ -587,7 +590,7 @@ "Insert comments", (codeOwnerConfigFilePath, codeOwnerConfigFileContent) -> { StringBuilder b = new StringBuilder(); - for (String line : codeOwnerConfigFileContent.split("\\n")) { + for (String line : Splitter.on('\n').split(codeOwnerConfigFileContent)) { if (line.contains(user.email())) { b.append(line + "# some comment\n"); continue; @@ -619,7 +622,7 @@ // verify that the inline comments are still present String codeOwnerConfigFileContent = codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getContent(); - for (String line : codeOwnerConfigFileContent.split("\\n")) { + for (String line : Splitter.on('\n').split(codeOwnerConfigFileContent)) { if (line.contains(secondaryEmail)) { assertThat(line).endsWith("# some comment"); } else if (line.contains(admin.email())) { @@ -667,7 +670,8 @@ // verify that the comments are still present String codeOwnerConfigFileContent = codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getContent(); - assertThat(codeOwnerConfigFileContent.split("\\n")[0]) + assertThat( + Iterables.get(Splitter.on('\n').split(codeOwnerConfigFileContent), /* position= */ 0)) .endsWith("# foo " + secondaryEmail + " bar"); }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java index be4d6b0..6386a15 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -51,7 +51,10 @@ import org.junit.Before; import org.junit.Test; -/** Tests for {@link CodeOwnerApprovalCheck}. */ +/** + * Tests for {@link CodeOwnerApprovalCheck}. Further tests with fallback code owners are implemented + * in {@link CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest}}. + */ public class CodeOwnerApprovalCheckTest extends AbstractCodeOwnersTest { @Inject private ChangeNotes.Factory changeNotesFactory; @Inject private RequestScopeOperations requestScopeOperations;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java new file mode 100644 index 0000000..e087873 --- /dev/null +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -0,0 +1,668 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.backend; + +import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatStream; + +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.entities.Change; +import com.google.gerrit.plugins.codeowners.JgitPath; +import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest; +import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations; +import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus; +import com.google.gerrit.plugins.codeowners.config.GeneralConfig; +import com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.testing.ConfigSuite; +import com.google.inject.Inject; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.stream.Stream; +import org.eclipse.jgit.lib.Config; +import org.junit.Before; +import org.junit.Test; + +/** Tests for {@link CodeOwnerApprovalCheck} with fallback code owners. */ +public class CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest + extends AbstractCodeOwnersTest { + @Inject private ChangeNotes.Factory changeNotesFactory; + @Inject private RequestScopeOperations requestScopeOperations; + + 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.setEnum( + "plugin", + "code-owners", + GeneralConfig.KEY_FALLBACK_CODE_OWNERS, + FallbackCodeOwners.ALL_USERS); + return cfg; + } + + @Before + public void setUpCodeOwnersPlugin() throws Exception { + codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class); + codeOwnerConfigOperations = + plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class); + } + + @Test + public void notApprovedByFallbackCodeOwnerIfCodeOwnersDefined() throws Exception { + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(codeOwner.email()) + .create(); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId(); + + // Verify that the file is not approved yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByFallbackCodeOwnerIfCodeOwnersDefined() throws Exception { + TestAccount codeOwner = + accountCreator.create( + "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(codeOwner.email()) + .create(); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId(); + + // Verify that the file is not approved yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + public void notApprovedByFallbackCodeOwnerIfNonResolvableCodeOwnersDefined() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail("non-resolvable-code-owner@example.com") + .create(); + + 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 yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByFallbackCodeOwnerIfNonResolvableCodeOwnersDefined() + throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail("non-resolvable-code-owner@example.com") + .create(); + + 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 yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + public void approvedByFallbackCodeOwner() throws Exception { + // create arbitrary code owner config to avoid entering the bootstrapping code path in + // CodeOwnerApprovalCheck + createArbitraryCodeOwnerConfigFile(); + + 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 yet (the change owner is a code owner, but + // implicit approvals are disabled). + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a user a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the status is pending now . + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.PENDING); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + // Verify that the status is approved now + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.APPROVED); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void implicitlyApprovedByFallbackCodeOwner() throws Exception { + // create arbitrary code owner config to avoid entering the bootstrapping code path in + // CodeOwnerApprovalCheck + createArbitraryCodeOwnerConfigFile(); + + 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 approved (the change owner is a code owner and implicit approvals are + // enabled). + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.APPROVED); + } + + @Test + public void approvedByFallbackCodeOwner_bootstrappingMode() throws Exception { + // since no code owner config exists we are entering the bootstrapping code path in + // CodeOwnerApprovalCheck + + TestAccount user2 = accountCreator.user2(); + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content") + .getChangeId(); + + // Verify that the file is not approved yet (the change owner is a code owner, but + // implicit approvals are disabled). + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a user a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user2.email()); + + // Verify that the status is pending now . + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.PENDING); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user2.id()); + recommend(changeId); + + // Verify that the status is approved now + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.APPROVED); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void implicitlyApprovedByFallbackCodeOwner_bootstrappingMode() throws Exception { + // since no code owner config exists we are entering the bootstrapping code path in + // CodeOwnerApprovalCheck + + Path path = Paths.get("/foo/bar.baz"); + String changeId = + createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content") + .getChangeId(); + + // Verify that the file is approved (the change owner is a code owner and implicit approvals are + // enabled). + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.APPROVED); + } + + @Test + public void notApprovedByFallbackCodeOwnerIfParentCodeOwnersIgnored() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .ignoreParentCodeOwners() + .create(); + + 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 yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByFallbackCodeOwnerIfParentCodeOwnersIgnored() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .ignoreParentCodeOwners() + .create(); + + 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 yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + public void notApprovedByFallbackCodeOwnerIfImportCannotBeResolved() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addImport( + CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, "/non-existing/OWNERS")) + .create(); + + 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 yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByFallbackCodeOwnerIfImportCannotBeResolved() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addImport( + CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, "/non-existing/OWNERS")) + .create(); + + 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 yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + public void notApprovedByFallbackCodeOwnerIfPerFileImportCannotBeResolved() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerSet( + CodeOwnerSet.builder() + .addPathExpression("*.md") + .addImport( + CodeOwnerConfigReference.create( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/bar/OWNERS")) + .autoBuild()) + .create(); + + Path path = Paths.get("/foo/bar.md"); + String changeId = + createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId(); + + // Verify that the file is not approved yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a fallback code owner as reviewer. + gApi.changes().id(changeId).addReviewer(user.email()); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + + // Add a Code-Review+1 (= code owner approval) from a fallback code owner. + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + // Verify that the file is not approved (fallback code owner doesn't apply since a code owner is + // defined). + fileCodeOwnerStatuses = codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + fileCodeOwnerStatusSubject = assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true") + public void notImplicitlyApprovedByFallbackCodeOwnerIfPerFileImportCannotBeResolved() + throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerSet( + CodeOwnerSet.builder() + .addPathExpression("*.md") + .addImport( + CodeOwnerConfigReference.create( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/bar/OWNERS")) + .autoBuild()) + .create(); + + Path path = Paths.get("/foo/bar.md"); + String changeId = + createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId(); + + // Verify that the file is not approved yet. + Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses = + codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)); + FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject = + assertThatStream(fileCodeOwnerStatuses).onlyElement(); + fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path); + fileCodeOwnerStatusSubject + .hasNewPathStatus() + .value() + .hasStatusThat() + .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS); + } + + private ChangeNotes getChangeNotes(String changeId) throws Exception { + return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number)); + } +}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java index 2b30264..af08aa3 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
@@ -33,6 +33,7 @@ import com.google.inject.Inject; import java.io.IOException; import java.nio.file.Paths; +import java.util.function.Consumer; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; @@ -53,6 +54,7 @@ @Rule public final MockitoRule mockito = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS); @Mock private CodeOwnerConfigVisitor visitor; + @Mock private Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback; @Inject private ProjectOperations projectOperations; @@ -145,6 +147,22 @@ } @Test + public void cannotVisitCodeOwnerConfigsWithNullCallback() throws Exception { + BranchNameKey branchNameKey = BranchNameKey.create(project, "master"); + NullPointerException npe = + assertThrows( + NullPointerException.class, + () -> + codeOwnerConfigHierarchy.visit( + branchNameKey, + getCurrentRevision(branchNameKey), + Paths.get("/foo/bar/baz.md"), + visitor, + /* parentCodeOwnersIgnoredCallback= */ null)); + assertThat(npe).hasMessageThat().isEqualTo("parentCodeOwnersIgnoredCallback"); + } + + @Test public void visitorNotInvokedIfNoCodeOwnerConfigExists() throws Exception { visit("master", "/foo/bar/baz.md"); verifyZeroInteractions(visitor); @@ -414,6 +432,9 @@ .verify(visitor) .visit(codeOwnerConfigOperations.codeOwnerConfig(fooBarCodeOwnerConfigKey).get()); verifyNoMoreInteractions(visitor); + + verify(parentCodeOwnersIgnoredCallback).accept(fooBarCodeOwnerConfigKey); + verifyNoMoreInteractions(parentCodeOwnersIgnoredCallback); } @Test @@ -477,6 +498,9 @@ .verify(visitor) .visit(codeOwnerConfigOperations.codeOwnerConfig(fooBarCodeOwnerConfigKey).get()); verifyNoMoreInteractions(visitor); + + verify(parentCodeOwnersIgnoredCallback).accept(fooBarCodeOwnerConfigKey); + verifyNoMoreInteractions(parentCodeOwnersIgnoredCallback); } @Test @@ -533,6 +557,8 @@ .verify(visitor) .visit(codeOwnerConfigOperations.codeOwnerConfig(rootCodeOwnerConfigKey).get()); verifyNoMoreInteractions(visitor); + + verifyZeroInteractions(parentCodeOwnersIgnoredCallback); } @Test @@ -657,6 +683,9 @@ visit("master", "/foo/bar/baz.md"); verify(visitor).visit(codeOwnerConfigOperations.codeOwnerConfig(rootCodeOwnerConfigKey).get()); verifyNoMoreInteractions(visitor); + + verify(parentCodeOwnersIgnoredCallback).accept(rootCodeOwnerConfigKey); + verifyNoMoreInteractions(parentCodeOwnersIgnoredCallback); } @Test @@ -711,7 +740,11 @@ throws InvalidPluginConfigurationException, IOException { BranchNameKey branchNameKey = BranchNameKey.create(project, branchName); codeOwnerConfigHierarchy.visit( - branchNameKey, getCurrentRevision(branchNameKey), Paths.get(path), visitor); + branchNameKey, + getCurrentRevision(branchNameKey), + Paths.get(path), + visitor, + parentCodeOwnersIgnoredCallback); } private ObjectId getCurrentRevision(BranchNameKey branchNameKey) throws IOException {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java index 6e19e28..86652b2 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -34,8 +34,8 @@ import com.google.inject.Key; import com.google.inject.Provider; import java.nio.file.Paths; +import java.util.Optional; import java.util.Set; -import java.util.stream.Stream; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; import org.junit.Before; @@ -89,14 +89,14 @@ @Test public void resolveCodeOwnerReferenceForEmail() throws Exception { - Stream<CodeOwner> codeOwner = + Optional<CodeOwner> codeOwner = codeOwnerResolver.get().resolve(CodeOwnerReference.create(admin.email())); - assertThat(codeOwner).onlyElement().hasAccountIdThat().isEqualTo(admin.id()); + assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(admin.id()); } @Test public void cannotResolveCodeOwnerReferenceForStarAsEmail() throws Exception { - Stream<CodeOwner> codeOwner = + Optional<CodeOwner> codeOwner = codeOwnerResolver .get() .resolve(CodeOwnerReference.create(CodeOwnerResolver.ALL_USERS_WILDCARD)); @@ -160,14 +160,14 @@ // admin has the "Modify Account" global capability and hence can see the secondary email of the // user account. - Stream<CodeOwner> codeOwner = + Optional<CodeOwner> codeOwner = codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail)); - assertThat(codeOwner).onlyElement().hasAccountIdThat().isEqualTo(user.id()); + assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id()); // user can see its own secondary email. requestScopeOperations.setApiUser(user.id()); codeOwner = codeOwnerResolver.get().resolve(CodeOwnerReference.create(secondaryEmail)); - assertThat(codeOwner).onlyElement().hasAccountIdThat().isEqualTo(user.id()); + assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(user.id()); } @Test @@ -192,6 +192,7 @@ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")); assertThat(result.codeOwners()).isEmpty(); assertThat(result.ownedByAllUsers()).isFalse(); + assertThat(result.hasUnresolvedCodeOwners()).isFalse(); } @Test @@ -205,6 +206,7 @@ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")); assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id()); assertThat(result.ownedByAllUsers()).isFalse(); + assertThat(result.hasUnresolvedCodeOwners()).isFalse(); } @Test @@ -219,6 +221,7 @@ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")); assertThat(result.codeOwnersAccountIds()).isEmpty(); assertThat(result.ownedByAllUsers()).isTrue(); + assertThat(result.hasUnresolvedCodeOwners()).isFalse(); } @Test @@ -233,6 +236,23 @@ codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")); assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id()); assertThat(result.ownedByAllUsers()).isFalse(); + assertThat(result.hasUnresolvedCodeOwners()).isTrue(); + } + + @Test + public void resolvePathCodeOwnersNonResolvableCodeOwnersAreFilteredOutIfOwnedByAllUsers() + throws Exception { + CodeOwnerConfig codeOwnerConfig = + CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION) + .addCodeOwnerSet( + CodeOwnerSet.createWithoutPathExpressions( + "*", admin.email(), "non-existing@example.com")) + .build(); + CodeOwnerResolverResult result = + codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md")); + assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id()); + assertThat(result.ownedByAllUsers()).isTrue(); + assertThat(result.hasUnresolvedCodeOwners()).isTrue(); } @Test @@ -297,9 +317,9 @@ assertThat(codeOwnerResolver.get().resolve(adminCodeOwnerReference)).isEmpty(); // if visibility is not enforced the code owner reference can be resolved regardless - Stream<CodeOwner> codeOwner = + Optional<CodeOwner> codeOwner = codeOwnerResolver.get().enforceVisibility(false).resolve(adminCodeOwnerReference); - assertThat(codeOwner).onlyElement().hasAccountIdThat().isEqualTo(admin.id()); + assertThat(codeOwner).value().hasAccountIdThat().isEqualTo(admin.id()); } @Test @@ -311,13 +331,13 @@ // admin is the current user and can see the account assertThat(codeOwnerResolver.get().resolve(CodeOwnerReference.create(user.email()))) - .isNotEmpty(); + .isPresent(); assertThat( codeOwnerResolver .get() .forUser(identifiedUserFactory.create(admin.id())) .resolve(CodeOwnerReference.create(user.email()))) - .isNotEmpty(); + .isPresent(); // user2 cannot see the account assertThat( @@ -386,6 +406,21 @@ CodeOwnerReference.create(user.email()))); assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id(), user.id()); assertThat(result.ownedByAllUsers()).isFalse(); + assertThat(result.hasUnresolvedCodeOwners()).isFalse(); + } + + @Test + public void resolveCodeOwnerReferencesNonResolveableCodeOwnersAreFilteredOut() throws Exception { + CodeOwnerResolverResult result = + codeOwnerResolver + .get() + .resolve( + ImmutableSet.of( + CodeOwnerReference.create(admin.email()), + CodeOwnerReference.create("non-existing@example.com"))); + assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id()); + assertThat(result.ownedByAllUsers()).isFalse(); + assertThat(result.hasUnresolvedCodeOwners()).isTrue(); } @Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersTest.java index cb065a0..fafc620 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersTest.java
@@ -64,7 +64,7 @@ NullPointerException.class, () -> codeOwners.get( - CodeOwnerConfig.Key.create(project, "master", "/"), /* folderPath= */ null)); + CodeOwnerConfig.Key.create(project, "master", "/"), /* revision= */ null)); assertThat(npe).hasMessageThat().isEqualTo("revision"); }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java index e5d06ac..0677ca4 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -205,7 +205,8 @@ CodeOwnerConfig emptyCodeOwnerConfig = createCodeOwnerBuilder().build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(emptyCodeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()).isEmpty(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()).isEmpty(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -216,7 +217,7 @@ .build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()) + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -254,7 +255,7 @@ .build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()) + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -275,7 +276,7 @@ .build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()).isEmpty(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()).isEmpty(); } } @@ -307,7 +308,7 @@ .build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()) + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); } @@ -341,7 +342,7 @@ .build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()) + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -376,7 +377,7 @@ .build(); PathCodeOwners pathCodeOwners = pathCodeOwnersFactory.create(codeOwnerConfig, Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.get()) + assertThat(pathCodeOwners.resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -389,7 +390,7 @@ pathCodeOwnersFactory.create( createCodeOwnerBuilder().setIgnoreParentCodeOwners().build(), Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.ignoreParentCodeOwners()).isTrue(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().ignoreParentCodeOwners()).isTrue(); } @Test @@ -399,7 +400,7 @@ pathCodeOwnersFactory.create( createCodeOwnerBuilder().setIgnoreParentCodeOwners(false).build(), Paths.get("/foo/bar/baz.md")); - assertThat(pathCodeOwners.ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().ignoreParentCodeOwners()).isFalse(); } @Test @@ -415,7 +416,7 @@ .build()) .build(), Paths.get("/foo.md")); - assertThat(pathCodeOwners.ignoreParentCodeOwners()).isTrue(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().ignoreParentCodeOwners()).isTrue(); } @Test @@ -432,7 +433,7 @@ .build()) .build(), Paths.get("/foo.md")); - assertThat(pathCodeOwners.ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.resolveCodeOwnerConfig().ignoreParentCodeOwners()).isFalse(); } @Test @@ -459,9 +460,10 @@ // Expectation: we get the global code owner from the importing code owner config, the // non-resolveable import is silently ignored - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isTrue(); } @Test @@ -504,9 +506,10 @@ // Expectation: we get the global code owners from the importing and the imported code owner // config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -549,9 +552,10 @@ // Expectation: we get the matching per-file code owners from the importing and the imported // code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -595,9 +599,10 @@ // Expectation: we only get the matching per-file code owners from the importing code owner // config, the per-file code owners from the imported code owner config are not relevant since // they do not match - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -642,9 +647,10 @@ // Expectation: we only get the matching per-file code owners from the importing code owner // config, the matching per-file code owners from the imported code owner config are not // relevant with import mode GLOBAL_CODE_OWNER_SETS_ONLY - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -690,10 +696,11 @@ // the matching per-file code owner set in the imported code owner config has the // ignoreGlobalAndParentCodeOwners flag set to true which causes global code owners to be // ignored, in addition this flag causes parent code owners to be ignored - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(user.email()); - assertThat(pathCodeOwners.get().ignoreParentCodeOwners()).isTrue(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners()).isTrue(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -738,10 +745,11 @@ // per-file code owners from the imported code owner config and its // ignoreGlobalAndParentCodeOwners flag are not relevant since the per-file code owner set does // not match - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); - assertThat(pathCodeOwners.get().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -786,10 +794,11 @@ // matching per-file code owners from the imported code owner config and its // ignoreGlobalAndParentCodeOwners flag are not relevant with import mode // GLOBAL_CODE_OWNER_SETS_ONLY - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); - assertThat(pathCodeOwners.get().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -824,7 +833,7 @@ // Expectation: ignoreParentCodeOwners is true because the ignoreParentCodeOwners flag in the // imported code owner config is set to true - assertThat(pathCodeOwners.get().ignoreParentCodeOwners()).isTrue(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners()).isTrue(); } @Test @@ -861,7 +870,7 @@ // Expectation: ignoreParentCodeOwners is false because the ignoreParentCodeOwners flag in the // imported code owner config is not relevant with import mode GLOBAL_CODE_OWNER_SETS_ONLY - assertThat(pathCodeOwners.get().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners()).isFalse(); } @Test @@ -919,9 +928,10 @@ // Expectation: we get the global owners from the importing code owner config, the imported code // owner config and the code owner config that is imported by the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email(), user2.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -977,9 +987,10 @@ // Expectation: we get the global owners from the importing code owner config and the imported // code owner config but not the per file code owner from the code owner config that is imported // by the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1027,7 +1038,7 @@ // Expectation: we get the global code owners from the importing and the imported code owner // config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -1064,7 +1075,7 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing and the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -1111,7 +1122,7 @@ // Expectation: we get the global owners from the importing and the imported code owner config // as they were defined at oldRevision - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); } @@ -1147,9 +1158,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing and the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1176,9 +1188,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isTrue(); } @Test @@ -1220,9 +1233,10 @@ // Expectation: we get the global owners from the importing code owner config, the global code // owners from the imported code owner config are ignored since the project that contains the // code owner config is hidden - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isTrue(); } @Test @@ -1250,9 +1264,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isTrue(); } @Test @@ -1290,9 +1305,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing and the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1337,9 +1353,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing and the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1380,9 +1397,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing and the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1425,9 +1443,10 @@ assertThat(pathCodeOwners).isPresent(); // Expectation: we get the global owners from the importing and the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1459,9 +1478,10 @@ // Expectation: we get the per file code owner from the importing code owner config, the // non-resolveable per file import is silently ignored - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isTrue(); } @Test @@ -1511,12 +1531,13 @@ // Expectation: we get the per file code owners from the importing and the global code owner // from the imported code owner config, but not the per file code owner from the imported code // owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); // Expectation: the ignoreParentCodeOwners flag from the imported code owner config is ignored - assertThat(pathCodeOwners.get().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().ignoreParentCodeOwners()).isFalse(); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1570,9 +1591,10 @@ // Expectation: we get the global owners from the importing code owner config, the imported code // owner config and the code owner config that is imported by the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email(), user2.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test @@ -1629,9 +1651,10 @@ // Expectation: we get the global owners from the importing code owner config and the imported // code owner config, but not the per file code owner from the code owner config that is // imported by the imported code owner config - assertThat(pathCodeOwners.get().get()) + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()) .comparingElementsUsing(hasEmail()) .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().hasUnresolvedImports()).isFalse(); } @Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java index 9fbc92c..f0ec10b 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -614,7 +614,7 @@ NullPointerException.class, () -> FindOwnersCodeOwnerConfigParser.replaceEmail( - /* content= */ null, admin.email(), user.email())); + /* codeOwnerConfigFileContent= */ null, admin.email(), user.email())); assertThat(npe).hasMessageThat().isEqualTo("codeOwnerConfigFileContent"); }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java index 832e491..5b6e80b 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfigurationTest.java
@@ -37,6 +37,7 @@ import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigUpdate; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend; import com.google.gerrit.server.IdentifiedUser; import com.google.inject.Inject; @@ -863,6 +864,52 @@ .isEqualTo(MergeCommitStrategy.ALL_CHANGED_FILES); } + @Test + public void cannotGetFallbackCodeOwnersForNullProject() throws Exception { + NullPointerException npe = + assertThrows( + NullPointerException.class, + () -> codeOwnersPluginConfiguration.getFallbackCodeOwners(/* project= */ null)); + assertThat(npe).hasMessageThat().isEqualTo("project"); + } + + @Test + public void getFallbackCodeOwnersIfNoneIsConfigured() throws Exception { + assertThat(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .isEqualTo(FallbackCodeOwners.NONE); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS") + public void getFallbackCodeOwnersIfNoneIsConfiguredOnProjectLevel() throws Exception { + assertThat(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .isEqualTo(FallbackCodeOwners.ALL_USERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS") + public void fallbackCodeOnwersOnProjectLevelOverridesGlobalFallbackCodeOwners() throws Exception { + configureFallbackCodeOwners(project, FallbackCodeOwners.NONE); + assertThat(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .isEqualTo(FallbackCodeOwners.NONE); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS") + public void fallbackCodeOwnersIsInheritedFromParentProject() throws Exception { + configureFallbackCodeOwners(allProjects, FallbackCodeOwners.NONE); + assertThat(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .isEqualTo(FallbackCodeOwners.NONE); + } + + @Test + public void inheritedFallbackCodeOwnersCanBeOverridden() throws Exception { + configureFallbackCodeOwners(allProjects, FallbackCodeOwners.ALL_USERS); + configureFallbackCodeOwners(project, FallbackCodeOwners.NONE); + assertThat(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .isEqualTo(FallbackCodeOwners.NONE); + } + private void configureDisabled(Project.NameKey project, String disabled) throws Exception { setCodeOwnersConfig(project, /* subsection= */ null, StatusConfig.KEY_DISABLED, disabled); } @@ -919,6 +966,15 @@ mergeCommitStrategy.name()); } + private void configureFallbackCodeOwners( + Project.NameKey project, FallbackCodeOwners fallbackCodeOwners) throws Exception { + setCodeOwnersConfig( + project, + /* subsection= */ null, + GeneralConfig.KEY_FALLBACK_CODE_OWNERS, + fallbackCodeOwners.name()); + } + private AutoCloseable registerTestBackend() { RegistrationHandle registrationHandle = ((PrivateInternals_DynamicMapImpl<CodeOwnerBackend>) codeOwnerBackends)
diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java index 19e3300..2a92d16 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED; +import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_FALLBACK_CODE_OWNERS; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_FILE_EXTENSION; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_GLOBAL_CODE_OWNER; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_MERGE_COMMIT_STRATEGY; @@ -33,6 +34,7 @@ import com.google.gerrit.plugins.codeowners.api.CodeOwnerConfigValidationPolicy; import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.server.git.validators.CommitValidationMessage; import com.google.gerrit.server.git.validators.ValidationMessage; import com.google.gerrit.server.project.ProjectLevelConfig; @@ -395,4 +397,62 @@ cfg.setString(SECTION_CODE_OWNERS, null, KEY_OVERRIDE_INFO_URL, "http://bar.example.com"); assertThat(generalConfig.getOverrideInfoUrl(cfg)).value().isEqualTo("http://bar.example.com"); } + + @Test + public void cannotGetFallbackCodeOwnersForNullProject() throws Exception { + NullPointerException npe = + assertThrows( + NullPointerException.class, + () -> generalConfig.getFallbackCodeOwners(/* project= */ null, new Config())); + assertThat(npe).hasMessageThat().isEqualTo("project"); + } + + @Test + public void cannotGetFallbackCodeOwnersForNullPluginConfig() throws Exception { + NullPointerException npe = + assertThrows( + NullPointerException.class, + () -> generalConfig.getFallbackCodeOwners(project, /* pluginConfig= */ null)); + assertThat(npe).hasMessageThat().isEqualTo("pluginConfig"); + } + + @Test + public void noFallbackCodeOwnersConfigured() throws Exception { + assertThat(generalConfig.getFallbackCodeOwners(project, new Config())) + .isEqualTo(FallbackCodeOwners.NONE); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS") + public void fallbackCodeOwnersIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel() + throws Exception { + assertThat(generalConfig.getFallbackCodeOwners(project, new Config())) + .isEqualTo(FallbackCodeOwners.ALL_USERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS") + public void fallbackCodeOwnersInPluginConfigOverridesFallbackCodeOwnersInGerritConfig() + throws Exception { + Config cfg = new Config(); + cfg.setString(SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS, "NONE"); + assertThat(generalConfig.getFallbackCodeOwners(project, cfg)) + .isEqualTo(FallbackCodeOwners.NONE); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS") + public void globalFallbackOnwersUsedIfInvalidFallbackCodeOwnersConfigured() throws Exception { + Config cfg = new Config(); + cfg.setString(SECTION_CODE_OWNERS, null, KEY_FALLBACK_CODE_OWNERS, "INVALID"); + assertThat(generalConfig.getFallbackCodeOwners(project, cfg)) + .isEqualTo(FallbackCodeOwners.ALL_USERS); + } + + @Test + @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "INVALID") + public void defaultValueUsedIfInvalidGlobalFallbackCodeOwnersConfigured() throws Exception { + assertThat(generalConfig.getFallbackCodeOwners(project, new Config())) + .isEqualTo(FallbackCodeOwners.NONE); + } }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java index 45d957a..bd632c4 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
@@ -35,6 +35,7 @@ import com.google.gerrit.plugins.codeowners.api.RequiredApprovalInfo; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackendId; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigScanner; +import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.plugins.codeowners.backend.findowners.FindOwnersBackend; import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend; import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; @@ -162,6 +163,8 @@ .thenReturn(Optional.of("http://foo.example.com")); when(codeOwnersPluginConfiguration.getMergeCommitStrategy(project)) .thenReturn(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + when(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .thenReturn(FallbackCodeOwners.ALL_USERS); when(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).thenReturn(true); when(codeOwnersPluginConfiguration.getRequiredApproval(project)) .thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2)); @@ -180,6 +183,8 @@ .isEqualTo("http://foo.example.com"); assertThat(codeOwnerProjectConfigInfo.general.mergeCommitStrategy) .isEqualTo(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + assertThat(codeOwnerProjectConfigInfo.general.fallbackCodeOwners) + .isEqualTo(FallbackCodeOwners.ALL_USERS); assertThat(codeOwnerProjectConfigInfo.general.implicitApprovals).isTrue(); assertThat(codeOwnerProjectConfigInfo.backend.id) .isEqualTo(CodeOwnerBackendId.FIND_OWNERS.getBackendId()); @@ -269,6 +274,8 @@ .thenReturn(Optional.of("http://foo.example.com")); when(codeOwnersPluginConfiguration.getMergeCommitStrategy(project)) .thenReturn(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + when(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .thenReturn(FallbackCodeOwners.ALL_USERS); when(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).thenReturn(true); when(codeOwnersPluginConfiguration.getRequiredApproval(project)) .thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2)); @@ -286,6 +293,8 @@ .isEqualTo("http://foo.example.com"); assertThat(codeOwnerBranchConfigInfo.general.mergeCommitStrategy) .isEqualTo(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + assertThat(codeOwnerBranchConfigInfo.general.fallbackCodeOwners) + .isEqualTo(FallbackCodeOwners.ALL_USERS); assertThat(codeOwnerBranchConfigInfo.general.implicitApprovals).isTrue(); assertThat(codeOwnerBranchConfigInfo.backendId) .isEqualTo(CodeOwnerBackendId.FIND_OWNERS.getBackendId()); @@ -324,6 +333,8 @@ .thenReturn(Optional.of("http://foo.example.com")); when(codeOwnersPluginConfiguration.getMergeCommitStrategy(project)) .thenReturn(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + when(codeOwnersPluginConfiguration.getFallbackCodeOwners(project)) + .thenReturn(FallbackCodeOwners.ALL_USERS); when(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).thenReturn(true); when(codeOwnersPluginConfiguration.getRequiredApproval(project)) .thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md index c9e7218..24ed4b9 100644 --- a/resources/Documentation/config.md +++ b/resources/Documentation/config.md
@@ -220,6 +220,35 @@ `@PLUGIN@.config`.\ By default `ALL_CHANGED_FILES`. +<a id="pluginCodeOwnersFallbackCodeOwners">plugin.@PLUGIN@.fallbackCodeOwners</a> +: Policy that controls who should own paths that have no code owners + defined. This policy only applies if the inheritance of parent code + owners hasn't been explicity disabled in a relevant code owner config + file and if there are no unresolved imports.\ + \ + Can be `NONE` or `ALL_USERS`.\ + \ + `NONE`:\ + Paths for which no code owners are defined are owned by no one. This + means changes that touch these files can only be submitted with a code + owner override.\ + \ + `ALL_USERS`:\ + Paths for which no code owners are defined are owned by all users. This + means changes to these paths can be approved by anyone. If [implicit + approvals](#pluginCodeOwnersEnableImplicitApprovals) are enabled, these + files are always automatically approved. The `ALL_USERS` option should + only be used with care as it means that any path that is not covered by + the code owner config files is automatically opened up to everyone and + mistakes with configuring code owners can easily happen. This is why + this option is intended to be only used if requiring code owner + approvals should not be enforced.\ + \ + Can be overridden per project by setting + [codeOwners.fallbackCodeOwners](#codeOwnersFallbackCodeOwners) in + `@PLUGIN@.config`.\ + By default `NONE`. + # <a id="projectConfiguration">Project configuration in @PLUGIN@.config</a> <a id="codeOwnersDisabled">codeOwners.disabled</a> @@ -419,6 +448,21 @@ [plugin.@PLUGIN@.mergeCommitStrategy](#pluginCodeOwnersMergeCommitStrategy) in `gerrit.config` is used. +<a id="codeOwnersFallbackCodeOwners">codeOwners.fallbackCodeOwners</a> +: Policy that controls who should own paths that have no code owners + defined. This policy only applies if the inheritance of parent code + owners hasn't been explicity disabled in a relevant code owner config + file and if there are no unresolved imports.\ + Can be `NONE` or `ALL_USERS` (see + [plugin.@PLUGIN@.fallbackCodeOwners](#pluginCodeOwnersFallbackCodeOwners) + for an explanation of these values).\ + Overrides the global setting + [plugin.@PLUGIN@.fallbackCodeOwners](#pluginCodeOwnersFallbackCodeOwners) + in `gerrit.config`.\ + If not set, the global setting + [plugin.@PLUGIN@.fallbackCodeOwners](#pluginCodeOwnersFallbackCodeOwners) + in `gerrit.config` is used. + --- Back to [@PLUGIN@ documentation index](index.html)
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md index 6ec924e..64a077e 100644 --- a/resources/Documentation/rest-api.md +++ b/resources/Documentation/rest-api.md
@@ -346,6 +346,7 @@ * are referenced by an email with a disallowed domain (see [allowedEmailDomain configuration](config.html#pluginCodeOwnersAllowedEmailDomain)) * do not have read access to the branch +* [fallback code owners](config.html#pluginCodeOwnersFallbackCodeOwners) are omitted from the result. @@ -689,6 +690,7 @@ | `merge_commit_strategy` || Strategy that defines for merge commits which files require code owner approvals. Can be `ALL_CHANGED_FILES` or `FILES_WITH_CONFLICT_RESOLUTION` (see [mergeCommitStrategy](config.html#pluginCodeOwnersMergeCommitStrategy) for an explanation of these values). | `implicit_approvals` | optional | Whether an implicit code owner approval from the last uploader is assumed (see [enableImplicitApprovals](config.html#pluginCodeOwnersEnableImplicitApprovals) for details). When unset, `false`. | `override_info_url` | optional | Optional URL for a page that provides project/host-specific information about how to request a code owner override. +|`fallback_code_owners` || Policy that controls who should own paths that have no code owners defined. Possible values are: `NONE`: Paths for which no code owners are defined are owned by no one. `ALL_USER`: Paths for which no code owners are defined are owned by all users. ### <a id="path-code-owner-status-info"> PathCodeOwnerStatusInfo The `PathCodeOwnerStatusInfo` entity describes the code owner status for a path
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md index 6e31243..1471348 100644 --- a/resources/Documentation/user-guide.md +++ b/resources/Documentation/user-guide.md
@@ -146,8 +146,10 @@ If the code owners functionality is enabled, all touched files require an approval from a code owner. If files are touched for which no code owners are -defined, the change can only be submitted with a [code owner -override](#codeOwnerOverride). +defined, the change can only be submitted with an approval of a fallback code +owner (if [configured](config.html#pluginCodeOwnersFallbackCodeOwners)) or with +a [code owner override](#codeOwnerOverride). Please note that fallback code +owners are not included in the [code owner suggestion](#codeOwnerSuggestion). If the destination branch doesn't contain any [code owner config file](#codeOwnerConfigFiles) at all yet and the project also doesn't have a
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md index d06f5c8..d6160d6 100644 --- a/resources/Documentation/validation.md +++ b/resources/Documentation/validation.md
@@ -145,6 +145,8 @@ before) * the [codeOwners.mergeCommitStrategy](config.html#codeOwnersMergeCommitStrategy) configuration is valid +* the [codeOwners.fallbackCodeOwners](config.html#codeOwnersFallbackCodeOwners) + configuration is valid ---
diff --git a/ui/code-owners-banner.js b/ui/code-owners-banner.js new file mode 100644 index 0000000..a44e258 --- /dev/null +++ b/ui/code-owners-banner.js
@@ -0,0 +1,176 @@ +/** + * @license + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import {CodeOwnersModelMixin} from './code-owners-model-mixin.js'; +import {PluginState} from './code-owners-model.js'; + +// There are 2 elements in this file: +// CodeOwnersBanner - visual elements. This element is shown at the top +// of the page when pluginStatus are changed to failed. +// The CodeOwnersBanner is added to the 'banner' endpoint. The endpoint +// is placed at the top level of the gerrit and doesn't provide a current +// change, so it is impossible to get the pluginStatus directly in the +// CodeOwnersBanner. +// To solve this problem, this files provides non-visual +// CodeOwnersPluginStatusNotifier element. This element is added to the +// change-view-integration endpoint, so it can track the plugin state. +// When a plugin state is changed, the CodeOwnersPluginStatusNotifier updates +// the pluginStatus property of the banner. + +// CodeOwnersBanner and CodeOwnersPluginStatusNotifier can be attached and +// detached in any order. The following piece of code ensures that these +// banner and notifier are always connected correctly. The code assumes, +// that max one banner and one notifier can be connected at any time. +let activeBanner = undefined; +let activeStatusNotifier = undefined; + +function setActiveBanner(banner) { + // banner is null when CodeOwnersBanner has been disconnected + activeBanner = banner; + if (activeStatusNotifier) { + activeStatusNotifier.banner = banner; + } +} + +function setActiveStatusNotifier(notifier) { + // notifier is null when CodeOwnersBanner has been disconnected + if (activeStatusNotifier) { + if (activeStatusNotifier.banner) { + activeStatusNotifier.banner.pluginStatus = undefined; + } + activeStatusNotifier.banner = undefined; + } + activeStatusNotifier = notifier; + if (activeStatusNotifier) { + activeStatusNotifier.banner = activeBanner; + } +} + +export class CodeOwnersBanner extends Polymer.Element { + static get is() { return 'gr-code-owners-banner'; } + + static get template() { + return Polymer.html` + <style include="shared-styles"> + :host { + display: block; + overflow: hidden; + background: red; + } + .text { + color: white; + font-family: var(--header-font-family); + font-size: var(--font-size-h3); + font-weight: var(--font-weight-h3); + line-height: var(--line-height-h3); + margin-left: var(--spacing-l); + } + </style> + <span class="text">Error: Code-owners plugin has failed</span> + <gr-button link on-click="_showFailDetails"> + Details + </gr-button> + `; + } + + static get properties() { + return { + // @internal attributes + hidden: { + type: Boolean, + value: true, + reflectToAttribute: true, + computed: '_computeHidden(pluginStatus)', + }, + pluginStatus: { + type: Object, + }, + }; + } + + connectedCallback() { + super.connectedCallback(); + setActiveBanner(this); + } + + disconnectedCallback() { + super.disconnectedCallback(); + setActiveBanner(undefined); + } + + _computeHidden(pluginStatus) { + return !pluginStatus || pluginStatus.state !== PluginState.Failed; + } + + _showFailDetails() { + showPluginFailedMessage(this, this.pluginStatus); + } +} + +customElements.define(CodeOwnersBanner.is, CodeOwnersBanner); + +export class CodeOwnersPluginStatusNotifier extends + CodeOwnersModelMixin(Polymer.Element) { + static get is() { + return 'owners-plugin-status-notifier'; + } + + static get properties() { + return { + banner: { + type: Object, + }, + }; + } + + static get observers() { + return [ + '_pluginStatusOrBannerChanged(model.pluginStatus, banner)', + ]; + } + + connectedCallback() { + super.connectedCallback(); + setActiveStatusNotifier(this); + } + + disconnectedCallback() { + super.disconnectedCallback(); + setActiveStatusNotifier(undefined); + } + + _pluginStatusOrBannerChanged(status, banner) { + if (!banner) return; + banner.pluginStatus = status; + } + + _loadDataAfterStateChanged() { + this.modelLoader.loadPluginStatus(); + } +} + +customElements.define(CodeOwnersPluginStatusNotifier.is, + CodeOwnersPluginStatusNotifier); + +export function showPluginFailedMessage(sourceEl, pluginStatus) { + sourceEl.dispatchEvent( + new CustomEvent('show-error', { + detail: {message: pluginStatus.failedMessage}, + composed: true, + bubbles: true, + }) + ); +}
diff --git a/ui/code-owners-model-loader.js b/ui/code-owners-model-loader.js new file mode 100644 index 0000000..2cd9095 --- /dev/null +++ b/ui/code-owners-model-loader.js
@@ -0,0 +1,116 @@ +/** + * @license + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {SuggestionsState} from './code-owners-model.js'; + +/** + * ModelLoader provides a method for loading data into the model. + * It is a bridge between an ownersModel and an ownersService. + * When UI elements depends on a model property XXX, the element should + * observe (or bind to) the property and call modelLoader.loadXXX method + * to load the value. + * It is recommended to use CodeOwnersModelMixin and call all load... methods + * in the loadPropertiesAfterModelChanged method + */ +export class ModelLoader { + constructor(ownersService, ownersModel) { + this.ownersService = ownersService; + this.ownersModel = ownersModel; + } + + async _loadProperty(propertyName, propertyLoader, propertySetter) { + // Load property only if it was not already loaded + if (this.ownersModel[propertyName] !== undefined) return; + let newValue; + try { + newValue = await propertyLoader(); + } catch (e) { + this.ownersModel.setPluginFailed(e.message); + return; + } + // It is possible, that several requests is made in parallel. + // Store only the first result and discard all other results. + // (also, due to the CodeOwnersCacheApi all result must be identical) + if (this.ownersModel[propertyName] !== undefined) return; + propertySetter(newValue); + } + + async loadBranchConfig() { + await this._loadProperty('branchConfig', + () => this.ownersService.getBranchConfig(), + value => this.ownersModel.setBranchConfig(value) + ); + } + + async loadStatus() { + await this._loadProperty('status', + () => this.ownersService.getStatus(), + value => this.ownersModel.setStatus(value) + ); + } + + async loadUserRole() { + await this._loadProperty('userRole', + () => this.ownersService.getLoggedInUserInitialRole(), + value => this.ownersModel.setUserRole(value) + ); + } + + async loadPluginStatus() { + await this._loadProperty('pluginStatus', + () => this.ownersService.isCodeOwnerEnabled(), + value => this.ownersModel.setPluginEnabled(value) + ); + } + + async loadAreAllFilesApproved() { + await this._loadProperty('areAllFilesApproved', + () => this.ownersService.areAllFilesApproved(), + value => this.ownersModel.setAreAllFilesApproved(value) + ); + } + + async loadSuggestions() { + // If a loading has been started already, do nothing + if (this.ownersModel.suggestionsState + !== SuggestionsState.NotLoaded) return; + + this.ownersModel.setSuggestionsState(SuggestionsState.Loading); + let suggestedOwners; + try { + suggestedOwners = await this.ownersService.getSuggestedOwners(); + } catch (e) { + this.ownersModel.setSuggestionsState(SuggestionsState.LoadFailed); + this.ownersModel.setPluginFailed(e.message); + return; + } + this.ownersModel.setSuggestions(suggestedOwners.suggestions); + this.ownersModel.setSuggestionsState(SuggestionsState.Loaded); + } + + async updateLoadSuggestionsProgress() { + let suggestedOwners; + try { + suggestedOwners = await this.ownersService.getSuggestedOwnersProgress(); + } catch { + // Ignore any error, keep progress unchanged. + return; + } + this.ownersModel.setSuggestionsLoadProgress(suggestedOwners.progress); + this.ownersModel.setSuggestions(suggestedOwners.suggestions); + } +}
diff --git a/ui/code-owners-model-mixin.js b/ui/code-owners-model-mixin.js new file mode 100644 index 0000000..722af6f --- /dev/null +++ b/ui/code-owners-model-mixin.js
@@ -0,0 +1,100 @@ +/** + * @license + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import {CodeOwnerService} from './code-owners-service.js'; +import {ModelLoader} from './code-owners-model-loader.js'; +import {CodeOwnersModel} from './code-owners-model.js'; + +/** + * The CodeOwnersMixin adds several properties to a class and translates + * 'model-property-changed' events from the model to the notifyPath calls. + * This allows to use properties of the model in observers, calculated + * properties and bindings. + * It is guaranteed, that model and modelLoader are set only if change, + * reporting and restApi properties are set. + */ +export const CodeOwnersModelMixin = Polymer.dedupingMixin(base => { + return class extends base { + constructor(...args) { + super(...args); + /** + * The modelLoader allows an element to request a property + * Typically should be used in loadPropertiesAfterModelChanged + * to ensure that all required model properties are loaded + */ + this.modelLoader = undefined; + } + static get properties() { + return { + /* The following 3 properties (change, reporting, restApi) have to be + * set from the outside for the mixin to work. + */ + change: Object, + reporting: Object, + restApi: Object, + model: { + type: Object, + observer: '_modelChanged', + }, + }; + } + + static get observers() { + return ['onInputChanged(restApi, change, reporting)']; + } + + onInputChanged(restApi, change, reporting) { + if ([restApi, change, reporting].includes(undefined)) { + this.model = undefined; + this.modelLoader = undefined; + return; + } + const ownerService = CodeOwnerService.getOwnerService( + this.restApi, + this.change + ); + const model = CodeOwnersModel.getModel(change); + this.modelLoader = new ModelLoader(ownerService, model); + // Assign model after modelLoader, so modelLoader can be used in + // the _requirePropertiesAfterModelChanged method + this.model = model; + } + + _modelChanged(newModel) { + if (this.modelPropertyChangedUnsubscriber) { + this.modelPropertyChangedUnsubscriber(); + this.modelPropertyChangedUnsubscriber = undefined; + } + if (!newModel) return; + const propertyChangedListener = e => { + this.notifyPath(`model.${e.detail.propertyName}`); + }; + newModel.addEventListener('model-property-changed', + propertyChangedListener); + this.modelPropertyChangedUnsubscriber = () => { + newModel.removeEventListener('model-property-changed', + propertyChangedListener); + }; + this.loadPropertiesAfterModelChanged(); + } + + loadPropertiesAfterModelChanged() { + // The class should override this method and calls appropriate methods + // from this.modelLoader to ensure that all required properties are + // set in model. + } + }; +});
diff --git a/ui/code-owners-model.js b/ui/code-owners-model.js new file mode 100644 index 0000000..cbd9bd2 --- /dev/null +++ b/ui/code-owners-model.js
@@ -0,0 +1,157 @@ +/** + * @license + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export const SuggestionsState = { + NotLoaded: 'NotLoaded', + Loaded: 'Loaded', + Loading: 'Loading', + LoadFailed: 'LoadFailed', +}; + +export const PluginState = { + Enabled: 'Enabled', + Disabled: 'Disabled', + Failed: 'Failed', +}; + +/** + * Maintain the state of code-owners. + * Raises 'model-property-changed' event when a property is changed. + * The plugin shares the same model between all UI elements (if it is not, + * the plugin can't maintain showSuggestions state across different UI elements). + * UI elements use values from this model to display information + * and listens for the model-property-changed event. To do so, UI elements + * add CodeOwnersModelMixin, which is doing the listening and the translation + * from model-property-changed event to Polymer property-changed-event. The + * translation allows to use model properties in observables, bindings, + * computed properties, etc... + * The CodeOwnersModelLoader updates the model. + * + * It would be good to use RxJs Observable for implementing model properties. + * However, RxJs library is imported by Gerrit and there is no + * good way to reuse the same library in the plugin. + */ +export class CodeOwnersModel extends EventTarget { + constructor(change) { + super(); + this.change = change; + this.branchConfig = undefined; + this.status = undefined; + this.userRole = undefined; + this.isCodeOwnerEnabled = undefined; + this.areAllFilesApproved = undefined; + this.suggestions = undefined; + this.suggestionsState = SuggestionsState.NotLoaded; + this.suggestionsLoadProgress = undefined; + this.showSuggestions = false; + this.pluginStatus = undefined; + } + + setBranchConfig(config) { + if (this.branchConfig === config) return; + this.branchConfig = config; + this._firePropertyChanged('branchConfig'); + } + + setStatus(status) { + if (this.status === status) return; + this.status = status; + this._firePropertyChanged('status'); + } + + setUserRole(userRole) { + if (this.userRole === userRole) return; + this.userRole = userRole; + this._firePropertyChanged('userRole'); + } + + setIsCodeOwnerEnabled(enabled) { + if (this.isCodeOwnerEnabled === enabled) return; + this.isCodeOwnerEnabled = enabled; + this._firePropertyChanged('isCodeOwnerEnabled'); + } + + setAreAllFilesApproved(approved) { + if (this.areAllFilesApproved === approved) return; + this.areAllFilesApproved = approved; + this._firePropertyChanged('areAllFilesApproved'); + } + + setSuggestions(suggestions) { + if (this.suggestions === suggestions) return; + this.suggestions = suggestions; + this._firePropertyChanged('suggestions'); + } + + setSuggestionsState(state) { + if (this.suggestionsState === state) return; + this.suggestionsState = state; + this._firePropertyChanged('suggestionsState'); + } + + setSuggestionsLoadProgress(progress) { + if (this.suggestionsLoadProgress === progress) return; + this.suggestionsLoadProgress = progress; + this._firePropertyChanged('suggestionsLoadProgress'); + } + + setShowSuggestions(show) { + if (this.showSuggestions === show) return; + this.showSuggestions = show; + this._firePropertyChanged('showSuggestions'); + } + + setPluginEnabled(enabled) { + this._setPluginStatus({state: enabled ? + PluginState.Enabled : PluginState.Disabled}); + } + + setPluginFailed(failedMessage) { + this._setPluginStatus({state: PluginState.Failed, failedMessage}); + } + + _setPluginStatus(status) { + if (this._arePluginStatusesEqual(this.pluginStatus, status)) return; + this.pluginStatus = status; + this._firePropertyChanged('pluginStatus'); + } + + _arePluginStatusesEqual(status1, status2) { + if (status1 === undefined || status2 === undefined) { + return status1 === status2; + } + if (status1.state !== status2.state) return false; + return status1.state === PluginState.Failed ? + status1.failedMessage === status2.failedMessage : + true; + } + + _firePropertyChanged(propertyName) { + this.dispatchEvent(new CustomEvent('model-property-changed', { + detail: { + propertyName, + }, + })); + } + + static getModel(change) { + if (!this.model || this.model.change !== change) { + this.model = new CodeOwnersModel(change); + } + return this.model; + } +}
diff --git a/ui/code-owners-service.js b/ui/code-owners-service.js index 416b3ec..8bb14d1 100644 --- a/ui/code-owners-service.js +++ b/ui/code-owners-service.js
@@ -111,30 +111,182 @@ } /** - * Returns a promise fetching project_config for code owners. - * - * @doc https://gerrit.googlesource.com/plugins/code-owners/+/refs/heads/master/resources/Documentation/rest-api.md#get-code-owner-project-config - * @param {string} project - */ - getProjectConfig(project) { - return this.restApi.get( - `/projects/${encodeURIComponent(project)}/code_owners.project_config` - ); - } - - /** * Returns a promise fetching the owners config for a given branch. * * @doc https://gerrit.googlesource.com/plugins/code-owners/+/refs/heads/master/resources/Documentation/rest-api.md#branch-endpoints * @param {string} project * @param {string} branch */ - getBranchConfig(project, branch) { - return this.restApi.get( + async getBranchConfig(project, branch) { + const config = await this.restApi.get( `/projects/${encodeURIComponent(project)}/` + `branches/${encodeURIComponent(branch)}/` + `code_owners.branch_config` ); + if (config.override_approval && !(config.override_approval instanceof Array)) { + // In the upcoming backend changes, the override_approval will be changed + // to array with (possible) multiple items. + // While this transition is in progress, the frontend supports both API - + // the old one and the new one. + return {...config, override_approval: [config.override_approval]}; + } + return config; + } +} + +/** + * Wrapper around codeOwnerApi, sends each requests only once and then cache + * the response. A new CodeOwnersCacheApi instance is created every time when a + * new change object is assigned. + * Gerrit never updates existing change object, but instead always assigns a new + * change object. Particularly, a new change object is assigned when a change + * is updated and user clicks reload toasts to see the updated change. + * As a result, the lifetime of a cache is the same as a lifetime of an assigned + * change object. + * Periodical cache invalidation can lead to inconsistency in UI, i.e. + * user can see the old reviewers list (reflects a state when a change was + * loaded) and code-owners status for the current reviewer list. To avoid + * this inconsistency, the cache doesn't invalidate. + */ +export class CodeOwnersCacheApi { + constructor(codeOwnerApi, change) { + this.codeOwnerApi = codeOwnerApi; + this.change = change; + this.promises = {}; + } + + _fetchOnce(cacheKey, asyncFn) { + if (!this.promises[cacheKey]) { + this.promises[cacheKey] = asyncFn(); + } + return this.promises[cacheKey]; + } + + getAccount() { + return this._fetchOnce('getAccount', () => this._getAccount()); + } + + async _getAccount() { + const loggedIn = await this.codeOwnerApi.restApi.getLoggedIn(); + if (!loggedIn) return undefined; + return await this.codeOwnerApi.restApi.getAccount(); + } + + listOwnerStatus() { + return this._fetchOnce('listOwnerStatus', + () => this.codeOwnerApi.listOwnerStatus(this.change._number)); + } + + getBranchConfig() { + return this._fetchOnce('getBranchConfig', + () => this.codeOwnerApi.getBranchConfig(this.change.project, + this.change.branch)); + } + + listOwnersForPath(path) { + return this._fetchOnce(`listOwnersForPath:${path}`, + () => this.codeOwnerApi.listOwnersForPath(this.change.id, path)); + } +} + +export class OwnersFetcher { + constructor(restApi, change, options) { + // fetched files and fetching status + this._fetchedOwners = new Map(); + this._fetchStatus = FetchStatus.NOT_STARTED; + this._totalFetchCount = 0; + this.change = change; + this.options = options; + this.codeOwnerApi = new CodeOwnerApi(restApi); + } + + getStatus() { + return this._fetchStatus; + } + + getProgressString() { + return this._totalFetchCount === 0 ? + `Loading suggested owners ...` : + `${this._fetchedOwners.size} out of ${this._totalFetchCount} files have returned suggested owners.`; + } + + getFiles() { + const result = []; + for (const [path, info] of this._fetchedOwners.entries()) { + result.push({path, info}); + } + return result; + } + + async fetchSuggestedOwners(codeOwnerStatusMap) { + // reset existing temporary storage + this._fetchedOwners = new Map(); + this._fetchStatus = FetchStatus.FETCHING; + this._totalFetchCount = 0; + + // only fetch those not approved yet + const filesGroupByStatus = [...codeOwnerStatusMap.keys()].reduce( + (list, file) => { + const status = codeOwnerStatusMap + .get(file).status; + if (status === OwnerStatus.INSUFFICIENT_REVIEWERS) { + list.missing.push(file); + } else if (status === OwnerStatus.PENDING) { + list.pending.push(file); + } + return list; + } + , {pending: [], missing: []}); + // always fetch INSUFFICIENT_REVIEWERS first and then pending + const filesToFetch = filesGroupByStatus.missing + .concat(filesGroupByStatus.pending); + this._totalFetchCount = filesToFetch.length; + await this._batchFetchCodeOwners(filesToFetch); + this._fetchStatus = FetchStatus.FINISHED; + } + + /** + * Recursively fetches code owners for all files until finished. + * + * @param {!Array<string>} files + */ + async _batchFetchCodeOwners(files) { + if (this._fetchStatus === FetchStatus.ABORT) { + return this._fetchedOwners; + } + + const batchRequests = []; + const maxConcurrentRequests = this.options.maxConcurrentRequests; + for (let i = 0; i < maxConcurrentRequests; i++) { + const filePath = files[i]; + if (filePath) { + this._fetchedOwners.set(filePath, {}); + batchRequests.push(this._fetchOwnersForPath(this.change.id, filePath)); + } + } + const resPromise = Promise.all(batchRequests); + await resPromise; + if (files.length > maxConcurrentRequests) { + return await this._batchFetchCodeOwners( + files.slice(maxConcurrentRequests)); + } + return this._fetchedOwners; + } + + async _fetchOwnersForPath(changeId, filePath) { + try { + const owners = await this.codeOwnerApi.listOwnersForPath(changeId, + filePath); + this._fetchedOwners.get(filePath).owners = new Set(owners); + } catch (e) { + this._fetchedOwners.get(filePath).error = e; + } + } + + abort() { + this._fetchStatus = FetchStatus.ABORT; + this._fetchedOwners = new Map(); + this._totalFetchCount = 0; } } @@ -145,50 +297,29 @@ constructor(restApi, change, options = {}) { this.restApi = restApi; this.change = change; - this.options = {maxConcurrentRequests: 10, ...options}; - this.codeOwnerApi = new CodeOwnerApi(restApi); + const codeOwnerApi = new CodeOwnerApi(restApi); + this.codeOwnerCacheApi = new CodeOwnersCacheApi(codeOwnerApi, change); - // fetched files and fetching status - this._fetchedOwners = new Map(); - this._fetchStatus = FetchStatus.NOT_STARTED; - this._totalFetchCount = 0; - - this.init(); + const fetcherOptions = { + maxConcurrentRequests: options.maxConcurrentRequests || 10, + }; + this.ownersFetcher = new OwnersFetcher(restApi, change, fetcherOptions); } /** - * Initial fetches. + * Prefetch data */ - init() { - this.accountPromise = this.restApi.getLoggedIn().then(loggedIn => { - if (!loggedIn) { - return undefined; - } - return this.restApi.getAccount(); - }); - - this.statusPromise = this.isCodeOwnerEnabled().then(enabled => { - if (!enabled) { - return { - patchsetNumber: 0, - enabled: false, - codeOwnerStatusMap: new Map(), - rawStatuses: [], - }; - } - return this.codeOwnerApi - .listOwnerStatus(this.change._number) - .then(res => { - return { - enabled: true, - patchsetNumber: res.patch_set_number, - codeOwnerStatusMap: this._formatStatuses( - res.file_code_owner_statuses - ), - rawStatuses: res.file_code_owner_statuses, - }; - }); - }); + async prefetch() { + try { + await Promise.all([ + this.codeOwnerCacheApi.getAccount(), + this.getStatus(), + ]); + } catch { + // Ignore any errors during prefetch. + // The same call from a different place throws the same exception + // again. The CodeOwnerService is not responsible for error processing. + } } /** @@ -197,71 +328,92 @@ * For example, if a user removes themselves as a reviewer, the returned * role 'REVIEWER' remains unchanged until the change view is reloaded. */ - getLoggedInUserInitialRole() { - return this.accountPromise.then(account => { - if (!account) { - return UserRole.ANONYMOUS; - } - const change = this.change; + async getLoggedInUserInitialRole() { + const account = await this.codeOwnerCacheApi.getAccount(); + if (!account) { + return UserRole.ANONYMOUS; + } + const change = this.change; + if ( + change.revisions && + change.current_revision && + change.revisions[change.current_revision] + ) { + const commit = change.revisions[change.current_revision].commit; if ( - change.revisions && - change.current_revision && - change.revisions[change.current_revision] + commit && + commit.author && + account.email && + commit.author.email === account.email ) { - const commit = change.revisions[change.current_revision].commit; - if ( - commit && - commit.author && - account.email && - commit.author.email === account.email - ) { - return UserRole.AUTHOR; - } + return UserRole.AUTHOR; } - if (change.owner._account_id === account._account_id) { - return UserRole.CHANGE_OWNER; + } + if (change.owner._account_id === account._account_id) { + return UserRole.CHANGE_OWNER; + } + if (change.reviewers) { + if (this._accountInReviewers(change.reviewers.REVIEWER, account)) { + return UserRole.REVIEWER; + } else if (this._accountInReviewers(change.reviewers.CC, account)) { + return UserRole.CC; + } else if (this._accountInReviewers(change.reviewers.REMOVED, account)) { + return UserRole.REMOVED_REVIEWER; } - if (change.reviewers) { - if (this._accountInReviewers(change.reviewers.REVIEWER, account)) { - return UserRole.REVIEWER; - } else if (this._accountInReviewers(change.reviewers.CC, account)) { - return UserRole.CC; - } else if (this._accountInReviewers(change.reviewers.REMOVED, account)) { - return UserRole.REMOVED_REVIEWER; - } - } - return UserRole.OTHER; - }) + } + return UserRole.OTHER; } _accountInReviewers(reviewers, account) { if (!reviewers) { return false; } - return reviewers.some(reviewer => reviewer._account_id === account._account_id); + return reviewers.some(reviewer => + reviewer._account_id === account._account_id); } - getStatus() { - return this.statusPromise.then(res => { - if (res.enabled && !this.isOnLatestPatchset(res.patchsetNumber)) { - // status is outdated, abort and re-init - this.abort(); - this.init(); - return this.statusPromise; - } - return res; - }); + async getStatus() { + const status = await this._getStatus(); + if (status.enabled && !this.isOnLatestPatchset(status.patchsetNumber)) { + // status is outdated, abort and re-init + this.abort(); + this.prefetch(); + return await this.codeOwnerCacheApi.getStatus(); + } + return status; } - areAllFilesApproved() { - return this.getStatus().then(({rawStatuses}) => { - return !rawStatuses.some(status => { - const oldPathStatus = status.old_path_status; - const newPathStatus = status.new_path_status; - // For deleted files, no new_path_status exists - return (newPathStatus && newPathStatus.status !== OwnerStatus.APPROVED) - || (oldPathStatus && oldPathStatus.status !== OwnerStatus.APPROVED); - }); + async _getStatus() { + const enabled = await this.isCodeOwnerEnabled(); + if (!enabled) { + return { + patchsetNumber: 0, + enabled: false, + codeOwnerStatusMap: new Map(), + rawStatuses: [], + }; + } + + const onwerStatus = await this.codeOwnerCacheApi.listOwnerStatus(); + + return { + enabled: true, + patchsetNumber: onwerStatus.patch_set_number, + codeOwnerStatusMap: this._formatStatuses( + onwerStatus.file_code_owner_statuses + ), + rawStatuses: onwerStatus.file_code_owner_statuses, + }; + } + + async areAllFilesApproved() { + const {rawStatuses} = await this.getStatus(); + return !rawStatuses.some(status => { + const oldPathStatus = status.old_path_status; + const newPathStatus = status.new_path_status; + // For deleted files, no new_path_status exists + return (newPathStatus && newPathStatus.status !== OwnerStatus.APPROVED) + || (oldPathStatus && oldPathStatus.status !== OwnerStatus.APPROVED); }); } @@ -282,57 +434,37 @@ * }> * }} */ - getSuggestedOwners() { + async getSuggestedOwners() { + const {codeOwnerStatusMap} = await this.getStatus(); + // In case its aborted due to outdated patches // should kick start the fetching again // Note: we currently are not reusing the instance when switching changes, // so if its `abort` due to different changes, the whole instance will be // outdated and not used. - if (this._fetchStatus === FetchStatus.NOT_STARTED - || this._fetchStatus === FetchStatus.ABORT) { - this._fetchSuggestedOwners().then(() => { - this._fetchStatus = FetchStatus.FINISHED; - }); + if (this.ownersFetcher.getStatus() === FetchStatus.NOT_STARTED + || this.ownersFetcher.getStatus() === FetchStatus.ABORT) { + await this.ownersFetcher.fetchSuggestedOwners(codeOwnerStatusMap); } - return this.getStatus().then(({codeOwnerStatusMap}) => { - return { - finished: this._fetchStatus === FetchStatus.FINISHED, - status: this._fetchStatus, - progress: this._totalFetchCount === 0 ? - `Loading suggested owners ...` : - `${this._fetchedOwners.size} out of ${this._totalFetchCount} files have returned suggested owners.`, - suggestions: this._groupFilesByOwners(codeOwnerStatusMap), - }; - }); + return { + finished: this.ownersFetcher.getStatus() === FetchStatus.FINISHED, + status: this.ownersFetcher.getStatus(), + progress: this.ownersFetcher.getProgressString(), + suggestions: this._groupFilesByOwners(codeOwnerStatusMap, + this.ownersFetcher.getFiles()), + }; } - _fetchSuggestedOwners() { - // reset existing temporary storage - this._fetchedOwners = new Map(); - this._fetchStatus = FetchStatus.FETCHING; - this._totalFetchCount = 0; - - return this.getStatus() - .then(({codeOwnerStatusMap}) => { - // only fetch those not approved yet - const filesGroupByStatus = [...codeOwnerStatusMap.keys()].reduce( - (list, file) => { - const status = codeOwnerStatusMap - .get(file).status; - if (status === OwnerStatus.INSUFFICIENT_REVIEWERS) { - list.missing.push(file); - } else if (status === OwnerStatus.PENDING) { - list.pending.push(file); - } - return list; - } - , {pending: [], missing: []}); - // always fetch INSUFFICIENT_REVIEWERS first and then pending - const filesToFetch = filesGroupByStatus.missing.concat(filesGroupByStatus.pending); - this._totalFetchCount = filesToFetch.length; - return this._batchFetchCodeOwners(filesToFetch); - }); + async getSuggestedOwnersProgress() { + const {codeOwnerStatusMap} = await this.getStatus(); + return { + finished: this.ownersFetcher.getStatus() === FetchStatus.FINISHED, + status: this.ownersFetcher.getStatus(), + progress: this.ownersFetcher.getProgressString(), + suggestions: this._groupFilesByOwners(codeOwnerStatusMap, + this.ownersFetcher.getFiles()), + }; } _formatStatuses(statuses) { @@ -368,28 +500,27 @@ return; } - _groupFilesByOwners(codeOwnerStatusMap) { + _groupFilesByOwners(codeOwnerStatusMap, files) { // Note: for renamed or moved files, they will have two entries in the map // we will treat them as two entries when group as well - const allFiles = [...this._fetchedOwners.keys()]; const ownersFilesMap = new Map(); const failedToFetchFiles = new Set(); - for (let i = 0; i < allFiles.length; i++) { + for (const file of files) { const fileInfo = { - path: allFiles[i], - status: this._computeFileStatus(codeOwnerStatusMap, allFiles[i]), + path: file.path, + status: this._computeFileStatus(codeOwnerStatusMap, file.path), }; // for files failed to fetch, add them to the special group - if (this._fetchedOwners.get(fileInfo.path).error) { + if (file.info.error) { failedToFetchFiles.add(fileInfo); continue; } // do not include files still in fetching - if (!this._fetchedOwners.get(fileInfo.path).owners) { + if (!file.info.owners) { continue; } - const owners = [...this._fetchedOwners.get(fileInfo.path).owners]; + const owners = [...file.info.owners]; const ownersKey = owners .map(owner => owner.account._account_id) .sort() @@ -415,7 +546,8 @@ groupedItems.push({ groupName: this.getGroupName(failedFiles), files: failedFiles, - error: new Error('Failed to fetch code owner info. Try to refresh the page.'), + error: new Error( + 'Failed to fetch code owner info. Try to refresh the page.'), }); } @@ -435,78 +567,23 @@ return `${latestRevision._number}` === `${patchsetId}`; } - /** - * Recursively fetches code owners for all files until finished. - * - * @param {!Array<string>} files - */ - _batchFetchCodeOwners(files) { - if (this._fetchStatus === FetchStatus.ABORT) { - return Promise.resolve(this._fetchedOwners); - } - - const batchRequests = []; - const maxConcurrentRequests = this.options.maxConcurrentRequests; - for (let i = 0; i < maxConcurrentRequests; i++) { - const filePath = files[i]; - if (filePath) { - this._fetchedOwners.set(filePath, {}); - batchRequests.push( - this.codeOwnerApi - .listOwnersForPath( - this.change.id, - filePath - ) - .then(owners => { - // use Set to de-dup - this._fetchedOwners.get(filePath).owners = new Set(owners); - }) - .catch(e => { - this._fetchedOwners.get(filePath).error = e; - }) - ); - } - } - const resPromise = Promise.all(batchRequests); - if (files.length > maxConcurrentRequests) { - return resPromise.then(() => { - return this._batchFetchCodeOwners(files.slice(maxConcurrentRequests)); - }); - } - return resPromise.then(() => this._fetchedOwners); - } - abort() { - this._fetchStatus = FetchStatus.ABORT; - this._fetchedOwners = new Map(); - this._totalFetchCount = 0; + this.ownersFetcher.abort(); + const codeOwnerApi = new CodeOwnerApi(this.restApi); + this.codeOwnerCacheApi = new CodeOwnersCacheApi(codeOwnerApi, change); } - getProjectConfig() { - if (!this.getProjectConfigPromise) { - this.getProjectConfigPromise = - this.codeOwnerApi.getProjectConfig(this.change.project); - } - return this.getProjectConfigPromise; + async getBranchConfig() { + return this.codeOwnerCacheApi.getBranchConfig(); } - getBranchConfig() { - if (!this.getBranchConfigPromise) { - this.getBranchConfigPromise = - this.codeOwnerApi.getBranchConfig(this.change.project, - this.change.branch); - } - return this.getBranchConfigPromise; - } - - isCodeOwnerEnabled() { + async isCodeOwnerEnabled() { if (this.change.status === ChangeStatus.ABANDONED || this.change.status === ChangeStatus.MERGED) { - return Promise.resolve(false); + return false; } - return this.getBranchConfig().then(config => { - return !(config.status && config.status.disabled); - }); + const config = await this.codeOwnerCacheApi.getBranchConfig(); + return config && !config.disabled; } static getOwnerService(restApi, change) { @@ -515,7 +592,9 @@ // Chrome has a limit of 6 connections per host name, and a max of 10 connections. maxConcurrentRequests: 6, }); + this.ownerService.prefetch(); } return this.ownerService; } } +
diff --git a/ui/code-owners-service_test.html b/ui/code-owners-service_test.html index 6986ea9..884b97f 100644 --- a/ui/code-owners-service_test.html +++ b/ui/code-owners-service_test.html
@@ -98,12 +98,14 @@ suite('basic api request tests', () => { setup(done => { - getApiStub.withArgs(`/changes/${fakeChange._number}/code_owners.status`).returns(Promise.resolve(fakeStatus)); - codeOwnersService = CodeOwnerService.getOwnerService(fakeRestApi, {...fakeChange}); + getApiStub.withArgs(`/changes/${fakeChange._number}/code_owners.status`) + .returns(Promise.resolve(fakeStatus)); + codeOwnersService = CodeOwnerService.getOwnerService(fakeRestApi, + {...fakeChange}); flush(done); }); - test('same change should return the same instance through getOwnerService', () => { + test('getOwnerService - same change returns the same instance', () => { assert.equal( CodeOwnerService.getOwnerService(fakeRestApi, fakeChange), CodeOwnerService.getOwnerService(fakeRestApi, fakeChange) @@ -119,7 +121,8 @@ test('should fetch status after init', () => { assert.isTrue(getApiStub.calledOnce); - assert.equal(getApiStub.lastCall.args[0], `/changes/${fakeChange._number}/code_owners.status`); + assert.equal(getApiStub.lastCall.args[0], + `/changes/${fakeChange._number}/code_owners.status`); }); test('getSuggestion should kickoff the fetch', done => { @@ -144,33 +147,35 @@ suite('all approved case', () => { setup(done => { - getApiStub.withArgs(`/changes/${fakeChange._number}/code_owners.status`).returns(Promise.resolve({ - // fake data with fake files - patch_set_number: 1, - file_code_owner_statuses: [ - { - new_path_status: { - path: 'a.js', - status: 'APPROVED', - }, - }, - { - new_path_status: { - path: 'b.js', - status: 'APPROVED', - }, - }, - { - old_path_status: { - path: 'd.js', - status: 'APPROVED', - }, - change_type: 'DELETED', - }, - ], - })); + getApiStub.withArgs(`/changes/${fakeChange._number}/code_owners.status`) + .returns(Promise.resolve({ + // fake data with fake files + patch_set_number: 1, + file_code_owner_statuses: [ + { + new_path_status: { + path: 'a.js', + status: 'APPROVED', + }, + }, + { + new_path_status: { + path: 'b.js', + status: 'APPROVED', + }, + }, + { + old_path_status: { + path: 'd.js', + status: 'APPROVED', + }, + change_type: 'DELETED', + }, + ], + })); - codeOwnersService = CodeOwnerService.getOwnerService(fakeRestApi, {...fakeChange}); + codeOwnersService = CodeOwnerService.getOwnerService(fakeRestApi, + {...fakeChange}); flush(done); }); @@ -196,11 +201,12 @@ }); suite('abort', () => { - let resolver; setup(done => { - getApiStub.withArgs(`/changes/${fakeChange._number}/code_owners.status`).returns(Promise.resolve(fakeStatus)); + getApiStub.withArgs(`/changes/${fakeChange._number}/code_owners.status`) + .returns(Promise.resolve(fakeStatus)); - codeOwnersService = CodeOwnerService.getOwnerService(fakeRestApi, {...fakeChange}); + codeOwnersService = CodeOwnerService.getOwnerService(fakeRestApi, + {...fakeChange}); flush(done); });
diff --git a/ui/owner-requirement.js b/ui/owner-requirement.js index 7bd74ee..1ad5b4b 100644 --- a/ui/owner-requirement.js +++ b/ui/owner-requirement.js
@@ -15,8 +15,10 @@ * limitations under the License. */ -import {CodeOwnerService, OwnerStatus} from './code-owners-service.js'; -import {ownerState} from './owner-ui-state.js'; +import {OwnerStatus} from './code-owners-service.js'; +import {CodeOwnersModelMixin} from './code-owners-model-mixin.js'; +import {showPluginFailedMessage} from './code-owners-banner.js'; +import {PluginState} from './code-owners-model.js'; /** * Owner requirement control for `submit-requirement-item-code-owners` endpoint. @@ -24,7 +26,8 @@ * This will show the status and suggest owners button next to * the code-owners submit requirement. */ -export class OwnerRequirementValue extends Polymer.Element { +export class OwnerRequirementValue extends + CodeOwnersModelMixin(Polymer.Element) { static get is() { return 'owner-requirement-value'; } @@ -61,16 +64,35 @@ Loading status ... </p> <template is="dom-if" if="[[!_isLoading]]"> - <span>[[_computeStatusText(_statusCount, _isOverriden)]]</span> - <template is="dom-if" if="[[_overrideInfoUrl]]"> - <a on-click="_reportDocClick" href="[[_overrideInfoUrl]]" target="_blank"> - <iron-icon icon="gr-icons:help-outline" title="Documentation for overriding code owners"></iron-icon> - </a> + <template is="dom-if" if="[[!_pluginFailed(model.pluginStatus)]]"> + <template is="dom-if" if="[[!model.branchConfig.no_code_owners_defined]]"> + <span>[[_computeStatusText(_statusCount, _isOverriden)]]</span> + <template is="dom-if" if="[[_overrideInfoUrl]]"> + <a on-click="_reportDocClick" href="[[_overrideInfoUrl]]" + target="_blank"> + <iron-icon icon="gr-icons:help-outline" + title="Documentation for overriding code owners"></iron-icon> + </a> + </template> + <template is="dom-if" if="[[!_allApproved]]"> + <gr-button link on-click="_openReplyDialog"> + Suggest owners + </gr-button> + </template> + </template> + <template is="dom-if" if="[[model.branchConfig.no_code_owners_defined]]"> + <span>No code-owners file</span> + <a href="https://gerrit.googlesource.com/plugins/code-owners/+/master/resources/Documentation/user-guide.md#how-to-submit-changes-with-files-that-have-no-code-owners" target="_blank"> + <iron-icon icon="gr-icons:help-outline" + title="Documentation about submitting changes with files that have no code owners?"></iron-icon> + </a> + </template> </template> - <template is="dom-if" if="[[!_allApproved]]"> - <gr-button link on-click="_openReplyDialog"> - Suggest owners - </gr-button> + <template is="dom-if" if="[[_pluginFailed(model.pluginStatus)]]"> + <span>Code-owners plugin has failed</span> + <gr-button link on-click="_showFailDetails"> + Details + </gr-button> </template> </template> `; @@ -78,87 +100,97 @@ static get properties() { return { - change: Object, - reporting: Object, - restApi: Object, - - ownerService: Object, - _statusCount: Object, _isLoading: { type: Boolean, - value: true, + computed: '_computeIsLoading(model.branchConfig, model.status, ' + + 'model.userRole, model.pluginStatus)', }, _allApproved: { type: Boolean, computed: '_computeAllApproved(_statusCount)', }, - _isOverriden: Boolean, - _overrideInfoUrl: String, + _isOverriden: { + type: Boolean, + computed: '_computeIsOverriden(change, model.branchConfig)', + }, + _overrideInfoUrl: { + type: String, + computed: '_computeOverrideInfoUrl(model.branchConfig)', + }, }; } static get observers() { return [ - 'onInputChanged(restApi, change, reporting)', + '_onStatusChanged(model.status, model.userRole)', ]; } - _checkIfOverriden() { - this.ownerService.getBranchConfig().then(res => { - if (!res['override_approval']) { - // no override label configured - this._isOverriden = false; - return; - } + loadPropertiesAfterModelChanged() { + super.loadPropertiesAfterModelChanged(); + this.reporting.reportLifeCycle('owners-submit-requirement-summary-start'); + this.modelLoader.loadBranchConfig(); + this.modelLoader.loadStatus(); + this.modelLoader.loadUserRole(); + } - const overridenLabel = res['override_approval'].label; - const overridenValue = res['override_approval'].value; + _computeIsLoading(branchConfig, status, userRole, pluginStatus) { + if (this._pluginFailed(pluginStatus)) { + return false; + } + return !branchConfig || !status || !userRole; + } + + _pluginFailed(pluginStatus) { + return pluginStatus && pluginStatus.state === PluginState.Failed; + } + + _onStatusChanged(status, userRole) { + if (!status || !userRole) { + this._statusCount = undefined; + return; + } + const rawStatuses = status.rawStatuses; + this._statusCount = this._getStatusCount(rawStatuses); + this.reporting.reportLifeCycle('owners-submit-requirement-summary-shown', + {...this._statusCount, user_role: userRole}); + } + + _computeOverrideInfoUrl(branchConfig) { + if (!branchConfig) { + return ''; + } + return branchConfig.general && branchConfig.general.override_info_url + ? branchConfig.general.override_info_url : ''; + } + + _computeIsOverriden(change, branchConfig) { + if (!change || !branchConfig || !branchConfig['override_approval']) { + // no override labels configured + return false; + } + + + for(const requiredApprovalInfo of branchConfig['override_approval']) { + const overridenLabel = requiredApprovalInfo.label; + const overridenValue = Number(requiredApprovalInfo.value); + if (isNaN(overridenValue)) continue; if (this.change.labels[overridenLabel]) { - const votes = this.change.labels[overridenLabel].all || []; - if (votes.find(v => `${v.value}` === `${overridenValue}`)) { - this._isOverriden = true; - return; + const votes = change.labels[overridenLabel].all || []; + if (votes.find(v => Number(v.value) >= overridenValue)) { + return true; } } + } - // otherwise always reset it to false - this._isOverriden = false; - }); - } - - _updateStatus() { - this._isLoading = true; - this.reporting.reportLifeCycle('owners-submit-requirement-summary-start'); - - return this.ownerService.getStatus() - .then(({rawStatuses}) => { - this._statusCount = this._getStatusCount(rawStatuses); - this.ownerService.getLoggedInUserInitialRole().then(role => { - // Send a metric with overall summary when code owners submit - // requirement shown and finished fetching status - this.reporting.reportLifeCycle( - 'owners-submit-requirement-summary-shown', - {...this._statusCount, user_role: role} - ); - }); - }) - .finally(() => { - this._isLoading = false; - }); - } - - _updateOverrideInfoUrl() { - this.ownerService.getBranchConfig().then(config => { - this._overrideInfoUrl = config.general && config.general.override_info_url - ? - config.general.override_info_url : ''; - }); + // otherwise always reset it to false + return false; } _computeAllApproved(statusCount) { - return statusCount.missing === 0 + return statusCount && statusCount.missing === 0 && statusCount.pending === 0; } @@ -186,6 +218,7 @@ } _computeStatusText(statusCount, isOverriden) { + if (statusCount === undefined || isOverriden === undefined) return ''; const statusText = []; if (statusCount.missing) { statusText.push(`${statusCount.missing} missing`); @@ -210,15 +243,8 @@ return status === OwnerStatus.PENDING; } - onInputChanged(restApi, change, reporting) { - if ([restApi, change, reporting].includes(undefined)) return; - this.ownerService = CodeOwnerService.getOwnerService(this.restApi, change); - this._updateStatus(); - this._checkIfOverriden(); - this._updateOverrideInfoUrl(); - } - _openReplyDialog() { + this.model.setShowSuggestions(true); this.dispatchEvent( new CustomEvent('open-reply-dialog', { detail: {}, @@ -226,11 +252,12 @@ bubbles: true, }) ); - ownerState.expandSuggestion = true; - this.ownerService.getLoggedInUserInitialRole().then(role => { - this.reporting.reportInteraction( - 'suggest-owners-from-submit-requirement', {user_role: role}); - }); + this.reporting.reportInteraction('suggest-owners-from-submit-requirement', + {user_role: this.model.userRole}); + } + + _showFailDetails() { + showPluginFailedMessage(this, this.model.pluginStatus); } }
diff --git a/ui/owner-status-column.js b/ui/owner-status-column.js index 3fdcd50..31ad654 100644 --- a/ui/owner-status-column.js +++ b/ui/owner-status-column.js
@@ -15,7 +15,8 @@ * limitations under the License. */ -import {CodeOwnerService, OwnerStatus} from './code-owners-service.js'; +import {OwnerStatus} from './code-owners-service.js'; +import {CodeOwnersModelMixin} from './code-owners-model-mixin.js'; const MAGIC_FILES = ['/COMMIT_MSG', '/MERGE_LIST', '/PATCHSET_LEVEL']; const STATUS_CODE = { @@ -38,13 +39,15 @@ const STATUS_TOOLTIP = { [STATUS_CODE.PENDING]: 'Pending code owner approval', [STATUS_CODE.MISSING]: 'Missing code owner approval', - [STATUS_CODE.PENDING_OLD_PATH]: 'Pending code owner approval on pre-renamed file', - [STATUS_CODE.MISSING_OLD_PATH]: 'Missing code owner approval on pre-renamed file', + [STATUS_CODE.PENDING_OLD_PATH]: + 'Pending code owner approval on pre-renamed file', + [STATUS_CODE.MISSING_OLD_PATH]: + 'Missing code owner approval on pre-renamed file', [STATUS_CODE.APPROVED]: 'Approved by code owner', [STATUS_CODE.ERROR]: 'Failed to fetch code owner status', }; -class BaseEl extends Polymer.Element { +class BaseEl extends CodeOwnersModelMixin(Polymer.Element) { computeHidden(change, patchRange) { if ([change, patchRange].includes(undefined)) return true; // if code-owners is not a submit requirement, don't show status column @@ -64,11 +67,6 @@ if (`${patchRange.patchNum}` !== `${latestPatchset._number}`) return true; return false; } - - onInputChanged(restApi, change) { - if ([restApi, change].includes(undefined)) return; - this.ownerService = CodeOwnerService.getOwnerService(this.restApi, change); - } } /** @@ -94,26 +92,15 @@ static get properties() { return { - change: Object, - reporting: Object, patchRange: Object, - restApi: Object, hidden: { type: Boolean, reflectToAttribute: true, computed: 'computeHidden(change, patchRange)', }, - ownerService: Object, }; } - - static get observers() { - return [ - 'onInputChanged(restApi, change)', - 'onOwnerServiceChanged(ownerServive)', - ]; - } } customElements.define(OwnerStatusColumnHeader.is, OwnerStatusColumnHeader); @@ -128,10 +115,6 @@ static get properties() { return { - change: Object, - reporting: Object, - restApi: Object, - path: String, patchRange: Object, hidden: { @@ -186,47 +169,45 @@ static get observers() { return [ - 'onInputChanged(restApi, change)', - 'computeStatusIcon(ownerService, path)', + 'computeStatusIcon(model.status, path)', ]; } - computeStatusIcon(ownerService, path) { - if ([ownerService, path].includes(undefined)) return; + loadPropertiesAfterModelChanged() { + super.loadPropertiesAfterModelChanged(); + this.modelLoader.loadStatus(); + } + + computeStatusIcon(modelStatus, path) { + if ([modelStatus, path].includes(undefined)) return; if (MAGIC_FILES.includes(path)) return; - ownerService.getStatus() - .then(({codeOwnerStatusMap}) => { - const statusItem = codeOwnerStatusMap.get(path); - if (!statusItem) { - this.status = STATUS_CODE.ERROR; - return; - } + const codeOwnerStatusMap = modelStatus.codeOwnerStatusMap; + const statusItem = codeOwnerStatusMap.get(path); + if (!statusItem) { + this.status = STATUS_CODE.ERROR; + return; + } - const status = statusItem.status; - let oldPathStatus = null; - if (statusItem.oldPath) { - const oldStatusItem = codeOwnerStatusMap.get(statusItem.oldPath); - if (!oldStatusItem) { - // should not happen - } else { - oldPathStatus = oldStatusItem.status; - } - } + const status = statusItem.status; + let oldPathStatus = null; + if (statusItem.oldPath) { + const oldStatusItem = codeOwnerStatusMap.get(statusItem.oldPath); + if (!oldStatusItem) { + // should not happen + } else { + oldPathStatus = oldStatusItem.status; + } + } - const newPathStatus = this._computeStatus(status); - if (!oldPathStatus) { - this.status = newPathStatus; - } else { - this.status = newPathStatus === STATUS_CODE.APPROVED - ? this._computeStatus(oldPathStatus, /* oldPath= */ true) - : newPathStatus; - } - }) - .catch(e => { - this.status = STATUS_CODE.ERROR; - throw e; - }); + const newPathStatus = this._computeStatus(status); + if (!oldPathStatus) { + this.status = newPathStatus; + } else { + this.status = newPathStatus === STATUS_CODE.APPROVED + ? this._computeStatus(oldPathStatus, /* oldPath= */ true) + : newPathStatus; + } } _computeIcon(status) { @@ -248,4 +229,4 @@ } } -customElements.define(OwnerStatusColumnContent.is, OwnerStatusColumnContent); \ No newline at end of file +customElements.define(OwnerStatusColumnContent.is, OwnerStatusColumnContent);
diff --git a/ui/owner-ui-state.js b/ui/owner-ui-state.js deleted file mode 100644 index 3d47ef9..0000000 --- a/ui/owner-ui-state.js +++ /dev/null
@@ -1,69 +0,0 @@ -/** - * @license - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/** - * @enum - */ -const OwnerUIStateEventType = { - EXPAND_SUGGESTION: 'expandSuggestion', -}; - -/** - * For states used in code owners plugin across multiple components. - */ -class OwnerUIState { - constructor() { - this._expandSuggestion = false; - this._listeners = new Map(); - this._listeners.set(OwnerUIStateEventType.EXPAND_SUGGESTION, []); - } - - get expandSuggestion() { - return this._expandSuggestion; - } - - set expandSuggestion(value) { - this._expandSuggestion = value; - this._listeners.get(OwnerUIStateEventType.EXPAND_SUGGESTION).forEach(cb => { - try { - cb(value); - } catch (e) { - console.warn(e); - } - }); - } - - _subscribeEvent(eventType, cb) { - this._listeners.get(eventType).push(cb); - return () => { - this._unsubscribeEvent(eventType, cb); - }; - } - - _unsubscribeEvent(eventType, cb) { - this._listeners.set( - eventType, - this._listeners.get(eventType).filter(handler => handler !== cb) - ); - } - - onExpandSuggestionChange(cb) { - return this._subscribeEvent(OwnerUIStateEventType.EXPAND_SUGGESTION, cb); - } -} - -export const ownerState = new OwnerUIState();
diff --git a/ui/plugin.js b/ui/plugin.js index 1a278dd..40bc4e5 100644 --- a/ui/plugin.js +++ b/ui/plugin.js
@@ -19,11 +19,21 @@ import {OwnerStatusColumnContent, OwnerStatusColumnHeader} from './owner-status-column.js'; import {OwnerRequirementValue} from './owner-requirement.js'; import {SuggestOwnersTrigger} from './suggest-owners-trigger.js'; +import {CodeOwnersBanner, CodeOwnersPluginStatusNotifier} from './code-owners-banner.js'; Gerrit.install(plugin => { const restApi = plugin.restApi(); const reporting = plugin.reporting(); + plugin.registerCustomComponent('banner', CodeOwnersBanner.is); + + plugin.registerCustomComponent( + 'change-view-integration', CodeOwnersPluginStatusNotifier.is) + .onAttached(view => { + view.restApi = restApi; + view.reporting = reporting; + }); + // owner status column / rows for file list plugin.registerDynamicCustomComponent( 'change-view-file-list-header-prepend', OwnerStatusColumnHeader.is) @@ -61,4 +71,4 @@ view.restApi = restApi; view.reporting = reporting; }); -}); \ No newline at end of file +});
diff --git a/ui/suggest-owners-trigger.js b/ui/suggest-owners-trigger.js index 2c0ccc0..97f616d 100644 --- a/ui/suggest-owners-trigger.js +++ b/ui/suggest-owners-trigger.js
@@ -14,41 +14,26 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {CodeOwnerService} from './code-owners-service.js'; -import {ownerState} from './owner-ui-state.js'; +import {CodeOwnersModelMixin} from './code-owners-model-mixin.js'; +import {PluginState} from './code-owners-model.js'; -export class SuggestOwnersTrigger extends Polymer.Element { +export class SuggestOwnersTrigger extends + CodeOwnersModelMixin(Polymer.Element) { static get is() { return 'suggest-owners-trigger'; } - constructor(props) { - super(props); - this.expandSuggestionStateUnsubscriber = undefined; - } - static get properties() { return { - change: Object, - reporting: Object, - restApi: Object, - - expanded: { - type: Boolean, - value: false, - }, hidden: { type: Boolean, - value: true, + computed: '_computeHidden(model.pluginStatus,' + + 'model.areAllFilesApproved, model.userRole, model.branchConfig)', reflectToAttribute: true, }, }; } - static get observers() { - return ['onInputChanged(restApi, change)']; - } - static get template() { return Polymer.html` <style include="shared-styles"> @@ -68,7 +53,7 @@ has-tooltip title="Suggest owners for your change" > - [[computeButtonText(expanded)]] + [[computeButtonText(model.showSuggestions)]] </gr-button> <span> <a on-click="_reportBugClick" href="https://bugs.chromium.org/p/gerrit/issues/entry?template=code-owners-plugin" target="_blank"> @@ -81,50 +66,35 @@ `; } - connectedCallback() { - super.connectedCallback(); - this.expandSuggestionStateUnsubscriber = ownerState - .onExpandSuggestionChange(expanded => { - this.expanded = expanded; - }); + loadPropertiesAfterModelChanged() { + super.loadPropertiesAfterModelChanged(); + this.modelLoader.loadUserRole(); + this.modelLoader.loadPluginStatus(); + this.modelLoader.loadAreAllFilesApproved(); + this.modelLoader.loadBranchConfig(); } - disconnnectedCallback() { - super.disconnectedCallback(); - if (this.expandSuggestionStateUnsubscriber) { - this.expandSuggestionStateUnsubscriber(); - this.expandSuggestionStateUnsubscriber = undefined; + _computeHidden(pluginStatus, allFilesApproved, userRole, branchConfig) { + if (pluginStatus === undefined || + allFilesApproved === undefined || + userRole === undefined || + branchConfig === undefined) { + return true; + } + if (branchConfig.no_code_owners_defined) return true; + if (pluginStatus.state === PluginState.Enabled) { + return allFilesApproved; + } else { + return true; } } - onInputChanged(restApi, change) { - if ([restApi, change].includes(undefined)) return; - this.ownerService = CodeOwnerService.getOwnerService( - this.restApi, - this.change - ); - - Promise.all([ - this.ownerService.isCodeOwnerEnabled(), - this.ownerService.areAllFilesApproved(), - this.ownerService.getLoggedInUserInitialRole() - ]) - .then(([enabled, approved, userRole]) => { - if (enabled) { - this.hidden = approved; - } else { - this.hidden = true; - } - this._userRole = userRole; - }); - } - toggleControlContent() { - this.expanded = !this.expanded; - ownerState.expandSuggestion = this.expanded; + this.model.setShowSuggestions(!this.model.showSuggestions); this.reporting.reportInteraction('toggle-suggest-owners', { expanded: this.expanded, - user_role: this._userRole ? this._userRole : 'UNKNOWN', + user_role: this.model.userRole ? + this.model.userRole : 'UNKNOWN', }); }
diff --git a/ui/suggest-owners.js b/ui/suggest-owners.js index 2e1a487..e0bf8a8 100644 --- a/ui/suggest-owners.js +++ b/ui/suggest-owners.js
@@ -14,8 +14,8 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {CodeOwnerService} from './code-owners-service.js'; -import {ownerState} from './owner-ui-state.js'; +import {CodeOwnersModelMixin} from './code-owners-model-mixin.js'; +import {SuggestionsState} from './code-owners-model.js'; const SUGGESTION_POLLING_INTERVAL = 1000; @@ -24,11 +24,6 @@ return 'owner-group-file-list'; } - constructor() { - super(); - this.expandSuggestionStateUnsubscriber = undefined; - } - static get properties() { return { files: Array, @@ -89,7 +84,7 @@ customElements.define(OwnerGroupFileList.is, OwnerGroupFileList); -export class SuggestOwners extends Polymer.Element { +export class SuggestOwners extends CodeOwnersModelMixin(Polymer.Element) { static get is() { return 'suggest-owners'; } @@ -262,6 +257,7 @@ <template is="dom-if" if="[[suggestion.error]]"> <div class="fetch-error-content"> [[suggestion.error]] + <a on-click="_showErrorDetails" </div> </template> <template is="dom-if" if="[[!suggestion.error]]"> @@ -299,18 +295,14 @@ static get properties() { return { - // @input - change: Object, - restApi: Object, - reporting: Object, - // @internal attributes hidden: { type: Boolean, value: true, reflectToAttribute: true, + computed: '_isHidden(model.areAllFilesApproved, ' + + 'model.showSuggestions)', }, - ownerService: Object, suggestedOwners: Array, isLoading: { type: Boolean, @@ -325,93 +317,87 @@ static get observers() { return [ - 'onInputChanged(restApi, change)', - 'onReviewerChange(reviewers)', + '_onReviewerChanged(reviewers)', + '_onShowSuggestionsChanged(model.showSuggestions)', + '_onSuggestionsStateChanged(model.suggestionsState)', + '_onSuggestionsChanged(model.suggestions, model.suggestionsState)', + '_onSuggestionsLoadProgressChanged(model.suggestionsLoadProgress)', ]; } - connectedCallback() { - super.connectedCallback(); - this.expandSuggestionStateUnsubscriber = ownerState - .onExpandSuggestionChange(expanded => { - this.hidden = !expanded; - if (expanded) { - // this is more of a hack to let reivew input lose focus - // to avoid suggestion dropdown - // gr-autocomplete has a internal state for tracking focus - // that will be canceled if any click happens outside of - // it's target - // Can not use `this.async` as it's only available in - // legacy element mixin which not used in this plugin. - Polymer.Async.timeOut.run(() => this.click(), 100); + _onShowSuggestionsChanged(showSuggestions) { + if (!showSuggestions || + this.model.suggestionsLoadProgress === SuggestionsState.NotLoaded) { + return; + } + // this is more of a hack to let review input lose focus + // to avoid suggestion dropdown + // gr-autocomplete has a internal state for tracking focus + // that will be canceled if any click happens outside of + // it's target + // Can not use `this.async` as it's only available in + // legacy element mixin which not used in this plugin. + Polymer.Async.timeOut.run(() => this.click(), 100); - // start fetching suggestions - if (!this._suggestionsTimer) { - this._suggestionsTimer = setInterval(() => { - this._pollingSuggestions(); - }, SUGGESTION_POLLING_INTERVAL); - - // poll immediately to kick start the fetching - this.reporting - .reportLifeCycle('owners-suggestions-fetching-start'); - this._pollingSuggestions(); - } - } - }); + this.modelLoader.loadSuggestions(); + this.reporting.reportLifeCycle('owners-suggestions-fetching-start'); } disconnectedCallback() { super.disconnectedCallback(); - if (this.expandSuggestionStateUnsubscriber) { - this.expandSuggestionStateUnsubscriber(); - this.expandSuggestionStateUnsubscriber = undefined; - } + this._stopUpdateProgressTimer(); } - onInputChanged(restApi, change) { - ownerState.expandSuggestion = false; - if ([restApi, change].includes(undefined)) return; - this.isLoading = true; - const ownerService = CodeOwnerService.getOwnerService(this.restApi, change); - if (this.ownerService && this.ownerService !== ownerService) { - // abort all pending requests - this.ownerService.abort(); - clearInterval(this._suggestionsTimer); - this._suggestionsTimer = null; - } - this.ownerService = ownerService; + _startUpdateProgressTimer() { + if (this._progressUpdateTimer) return; + this._progressUpdateTimer = setInterval(() => { + this.modelLoader.updateLoadSuggestionsProgress(); + }, SUGGESTION_POLLING_INTERVAL); + } + _stopUpdateProgressTimer() { + if (!this._progressUpdateTimer) return; + clearInterval(this._progressUpdateTimer); + this._progressUpdateTimer = undefined; + } + + _onSuggestionsStateChanged(state) { + this._stopUpdateProgressTimer(); + if (state === SuggestionsState.Loading) { + this._startUpdateProgressTimer(); + } + this.isLoading = state === SuggestionsState.Loading; + } + + _isHidden(allFilesApproved, showSuggestions) { + if (!showSuggestions) return true; // if all approved, no need to show the container - this.ownerService.areAllFilesApproved().then(approved => { - if (approved) { - this.hidden = approved; - } - }); + return allFilesApproved === undefined || !!allFilesApproved; } - _pollingSuggestions() { - this.ownerService - .getSuggestedOwners() - .then(res => { - if (res.finished) { - clearInterval(this._suggestionsTimer); - this._suggestionsTimer = null; - const reportDetails = res.suggestions.reduce((details, cur) => { - details.totalGroups++; - details.stats.push([cur.files.length, cur.owners ? cur.owners.length : 0]); - return details; - }, {totalGroups: 0, stats: []}); - this.reporting.reportLifeCycle('owners-suggestions-fetching-finished', reportDetails); - } - this.progressText = res.progress; - this.isLoading = !res.finished; + loadPropertiesAfterModelChanged() { + super.loadPropertiesAfterModelChanged(); + this._stopUpdateProgressTimer(); + this.modelLoader.loadAreAllFilesApproved(); + } - this._updateSuggestions(res.suggestions); + _onSuggestionsChanged(suggestions, suggestionsState) { + // The updateLoadSuggestionsProgress method also updates suggestions + this._updateSuggestions(suggestions || []); + this._updateAllChips(this._currentReviewers); + if (!suggestions || suggestionsState !== SuggestionsState.Loaded) return; + const reportDetails = suggestions.reduce((details, cur) => { + details.totalGroups++; + details.stats.push([cur.files.length, + cur.owners ? cur.owners.length : 0]); + return details; + }, {totalGroups: 0, stats: []}); + this.reporting.reportLifeCycle( + 'owners-suggestions-fetching-finished', reportDetails); + } - // in case `_updateAllChips` called before suggestedOwners ready - // from onReviewerChange - this._updateAllChips(this._currentReviewers); - }); + _onSuggestionsLoadProgressChanged(progress) { + this.progressText = progress; } _updateSuggestions(suggestions) { @@ -421,7 +407,7 @@ }); } - onReviewerChange(reviewers) { + _onReviewerChanged(reviewers) { this._currentReviewers = reviewers; this._updateAllChips(reviewers); } @@ -517,7 +503,8 @@ } _reportDocClick() { - this.reporting.reportInteraction('code-owners-doc-click', {section: 'no owners found'}); + this.reporting.reportInteraction('code-owners-doc-click', + {section: 'no owners found'}); } }