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'});
}
}
