Apply implicit approvals only when change owner == last patch set uploader
So far implicit approvals are applied on all patch sets that are
uploaded by a code owner. This has some implications that are unexpected
to users. E.g. if a non-code-owner uploads a change and a code owner
edits its commit message, the change gets implicitly code owner approved
(because editing the commit message creates a new patch set that is
uploaded by the code owner, and hence is implicitly code owner
approved).
Applying implicit approvals only based on the change owner (all patch
sets of changes that are owned by a code owner are automatically
approved) is even worse, as a non-code-owner could upload a new patch
set to a change that is owned by a code owner and then get their patch
set implicitly approved by the change owner. This would allow
non-code-onwers to get arbitrary code implicitly code owner approved.
To avoid both issues we now apply implicit code owner approvals only if
a change is owned by a code owner and if the last patch set was uploaded
by the change owner (change owner == last patch set uploader).
This doesn't resolve all security concerns about enabling implicit code
owner approvals, but is much safer than what we have now. It remains an
issue that code owners must be aware of their implicit code owner
approval when creating changes. E.g. if a code owner helps a contributor
to rebase a patch to another branch, they implicitly approve the change
on the other branch (since the code owner is the change owner).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I33d5e7433172847d6ee419a7a17740353cd34a99
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 4317c77..65f4d78 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -277,6 +277,7 @@
CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
+ Account.Id changeOwner = changeNotes.getChange().getOwner();
Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
ImmutableSet<Account.Id> exemptedAccounts = codeOwnersConfig.getExemptedAccounts();
logger.atFine().log("exemptedAccounts = %s", exemptedAccounts);
@@ -295,10 +296,15 @@
return getAllPathsAsApproved(changeNotes, changeNotes.getCurrentPatchSet());
}
- boolean enableImplicitApprovalFromUploader = codeOwnersConfig.areImplicitApprovalsEnabled();
+ boolean implicitApprovalConfig = codeOwnersConfig.areImplicitApprovalsEnabled();
+ boolean enableImplicitApproval =
+ implicitApprovalConfig && changeOwner.equals(patchSetUploader);
logger.atFine().log(
- "patchSetUploader = %d, implicit approval from uploader is %s",
- patchSetUploader.get(), enableImplicitApprovalFromUploader ? "enabled" : "disabled");
+ "changeOwner = %d, patchSetUploader = %d, implict approval config = %s\n=> implicit approval is %s",
+ changeOwner.get(),
+ patchSetUploader.get(),
+ implicitApprovalConfig,
+ enableImplicitApproval ? "enabled" : "disabled");
ImmutableList<PatchSetApproval> currentPatchSetApprovals =
getCurrentPatchSetApprovals(changeNotes);
@@ -349,7 +355,7 @@
branch,
revision,
globalCodeOwners,
- enableImplicitApprovalFromUploader ? patchSetUploader : null,
+ enableImplicitApproval ? changeOwner : null,
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 234e4a0..ea7c88b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -450,7 +450,12 @@
return backendConfig.getDefaultBackend();
}
- /** Checks whether an implicit code owner approval from the last uploader is assumed. */
+ /**
+ * Checks whether implicit code owner approvals are enabled.
+ *
+ * <p>If enabled, an implict code owner approval from the change owner is assumed if the last
+ * patch set was uploaded by the change owner.
+ */
public boolean areImplicitApprovalsEnabled() {
if (implicitApprovalsEnabled == null) {
implicitApprovalsEnabled = readImplicitApprovalsEnabled();
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
index 72e2022..7ab6706 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -740,8 +740,11 @@
}
/**
- * Gets whether an implicit code owner approval from the last uploader is assumed from the given
- * plugin config with fallback to {@code gerrit.config}.
+ * Gets whether an implicit code owner approvals are enabled from the given plugin config with
+ * fallback to {@code gerrit.config}.
+ *
+ * <p>If enabled, an implict code owner approval from the change owner is assumed if the last
+ * patch set was uploaded by the change owner.
*
* @param project the name of the project for which the configuration should be read
* @param pluginConfig the plugin config from which the configuration should be read.
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 566c5cd..98268cb 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -490,126 +490,183 @@
}
@Test
- public void getStatusForFileAddition_noImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatusForFileAddition(
- /* implicitApprovalsEnabled= */ false);
+ public void getStatusForFileAddition_noImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatusForFileAddition(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileAddition_withImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatusForFileAddition(
- /* implicitApprovalsEnabled= */ true);
+ public void getStatusForFileAddition_noImplicitApproval_uploaderDoesntMatchChangeOwner()
+ throws Exception {
+ testImplicitApprovalOnGetStatusForFileAddition(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
- private void testImplicitApprovalByPatchSetUploaderOnGetStatusForFileAddition(
- boolean implicitApprovalsEnabled) throws Exception {
- setAsRootCodeOwners(user);
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void getStatusForFileAddition_withImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatusForFileAddition(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ private void testImplicitApprovalOnGetStatusForFileAddition(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner =
+ accountCreator.create(
+ "code_owner", "code.owner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsRootCodeOwners(changeOwner, otherCodeOwner);
Path path = Paths.get("/foo/bar.baz");
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- amendChange(user, changeId);
+
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
- public void getStatusForFileModification_noImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatusForFileModification(
- /* implicitApprovalsEnabled= */ false);
+ public void getStatusForFileModification_noImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatusForFileModification(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileModification_withImplicitApprovalByPatchSetUploader()
+ public void getStatusForFileModification_noImplicitApproval_uploaderDoesntMatchChangeOwner()
throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatusForFileModification(
- /* implicitApprovalsEnabled= */ true);
+ testImplicitApprovalOnGetStatusForFileModification(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
- private void testImplicitApprovalByPatchSetUploaderOnGetStatusForFileModification(
- boolean implicitApprovalsEnabled) throws Exception {
- setAsRootCodeOwners(user);
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void getStatusForFileModification_withImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatusForFileModification(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ private void testImplicitApprovalOnGetStatusForFileModification(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner =
+ accountCreator.create(
+ "code_owner", "code.owner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsRootCodeOwners(changeOwner, otherCodeOwner);
Path path = Paths.get("/foo/bar.baz");
- createChange("Test Change", JgitPath.of(path).get(), "file content").getChangeId();
+ createChange("Test Change", JgitPath.of(path).get(), "file content");
String changeId =
createChange("Change Modifying A File", JgitPath.of(path).get(), "new file content")
.getChangeId();
- amendChange(user, changeId);
+
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.modification(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
- public void getStatusForFileDeletion_noImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatusForFileDeletion(
- /* implicitApprovalsEnabled= */ false);
+ public void getStatusForFileDeletion_noImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatusForFileDeletion(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileDeletion_withImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatusForFileDeletion(
- /* implicitApprovalsEnabled= */ true);
+ public void getStatusForFileDeletion_noImplicitApproval_uploaderDoesntMatchChangeOwner()
+ throws Exception {
+ testImplicitApprovalOnGetStatusForFileDeletion(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
- private void testImplicitApprovalByPatchSetUploaderOnGetStatusForFileDeletion(
- boolean implicitApprovalsEnabled) throws Exception {
- setAsRootCodeOwners(user);
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void getStatusForFileDeletion_withImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatusForFileDeletion(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ private void testImplicitApprovalOnGetStatusForFileDeletion(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner =
+ accountCreator.create(
+ "code_owner", "code.owner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsRootCodeOwners(changeOwner, otherCodeOwner);
Path path = Paths.get("/foo/bar.baz");
String changeId = createChangeWithFileDeletion(path);
- amendChange(user, changeId);
+
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.deletion(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@Test
- public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnOldPath()
- throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ false);
+ public void getStatusForFileRename_noImplicitApprovalOnOldPath() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ false,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ false);
}
@Test
@GerritConfig(
name = "experiments.enabled",
value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnOldPath_useDiffCache()
- throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ true);
+ public void getStatusForFileRename_noImplicitApprovalOnOldPath_useDiffCache() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ false,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ true);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnOldPath()
+ public void getStatusForFileRename_noImplicitApprovalOnOldPath_uploaderDoesntMatchChangeOwner()
throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ false);
+ testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ false,
+ /* useDiffCache= */ false);
}
@Test
@@ -617,20 +674,55 @@
@GerritConfig(
name = "experiments.enabled",
value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnOldPath_useDiffCache()
- throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ true);
+ public void
+ getStatusForFileRename_noImplicitApprovalOnOldPath_uploaderDoesntMatchChangeOwner_useDiffCache()
+ throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ false,
+ /* useDiffCache= */ true);
}
- private void testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnOldPath(
- boolean implicitApprovalsEnabled, boolean useDiffCache) throws Exception {
- setAsCodeOwners("/foo/bar/", user);
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void getStatusForFileRename_withImplicitApprovalOnOldPath() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_withImplicitApprovalOnOldPath_useDiffCache() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ true);
+ }
+
+ private void testImplicitApprovalOnGetStatusForFileRenameOnOldPath(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner, boolean useDiffCache)
+ throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner =
+ accountCreator.create(
+ "code_owner", "code.owner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsCodeOwners("/foo/bar/", changeOwner, otherCodeOwner);
Path oldPath = Paths.get("/foo/bar/abc.txt");
Path newPath = Paths.get("/foo/baz/abc.txt");
String changeId = createChangeWithFileRename(oldPath, newPath);
- amendChange(user, changeId);
+
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
if (useDiffCache) {
@@ -638,7 +730,7 @@
.containsExactly(
FileCodeOwnerStatus.rename(
oldPath,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
newPath,
@@ -648,7 +740,7 @@
.containsExactly(
FileCodeOwnerStatus.deletion(
oldPath,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
FileCodeOwnerStatus.addition(newPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
@@ -656,28 +748,33 @@
}
@Test
- public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnNewPath()
- throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ false);
+ public void testImplicitApprovalOnGetStatusForFileRenameOnNewPath() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ false,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ false);
}
@Test
@GerritConfig(
name = "experiments.enabled",
value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_noImplicitApprovalByPatchSetUploaderOnNewPath_useDiffCache()
+ public void testImplicitApprovalOnGetStatusForFileRenameOnNewPath_useDiffCache()
throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ false, /* useDiffCache= */ true);
+ testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ false,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ true);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnNewPath()
+ public void getStatusForFileRename_noImplicitApprovalOnNewPath_uploaderDoesntMatchChangeOwner()
throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ false);
+ testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ false,
+ /* useDiffCache= */ false);
}
@Test
@@ -685,20 +782,55 @@
@GerritConfig(
name = "experiments.enabled",
value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
- public void getStatusForFileRename_withImplicitApprovalByPatchSetUploaderOnNewPath_useDiffCache()
- throws Exception {
- testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- /* implicitApprovalsEnabled= */ true, /* useDiffCache= */ true);
+ public void
+ getStatusForFileRename_noImplicitApprovalOnNewPath_uploaderDoesntMatchChangeOwner_useDiffCache()
+ throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ false,
+ /* useDiffCache= */ true);
}
- private void testImplicitApprovalByPatchSetUploaderOnStatusForFileRenameOnNewPath(
- boolean implicitApprovalsEnabled, boolean useDiffCache) throws Exception {
- setAsCodeOwners("/foo/baz/", user);
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void getStatusForFileRename_withImplicitApprovalOnNewPath() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ false);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ @GerritConfig(
+ name = "experiments.enabled",
+ value = CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)
+ public void getStatusForFileRename_withImplicitApprovalOnNewPath_useDiffCache() throws Exception {
+ testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ /* implicitApprovalsEnabled= */ true,
+ /* uploaderMatchesChangeOwner= */ true,
+ /* useDiffCache= */ true);
+ }
+
+ private void testImplicitApprovalOnGetStatusForFileRenameOnNewPath(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner, boolean useDiffCache)
+ throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner =
+ accountCreator.create(
+ "code_owner", "code.owner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsCodeOwners("/foo/baz/", changeOwner, otherCodeOwner);
Path oldPath = Paths.get("/foo/bar/abc.txt");
Path newPath = Paths.get("/foo/baz/abc.txt");
String changeId = createChangeWithFileRename(oldPath, newPath);
- amendChange(user, changeId);
+
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
if (useDiffCache) {
@@ -708,7 +840,7 @@
oldPath,
CodeOwnerStatus.INSUFFICIENT_REVIEWERS,
newPath,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
} else {
@@ -717,7 +849,7 @@
FileCodeOwnerStatus.deletion(oldPath, CodeOwnerStatus.INSUFFICIENT_REVIEWERS),
FileCodeOwnerStatus.addition(
newPath,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@@ -725,7 +857,9 @@
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileAddition_noImplicitlyApprovalByChangeOwner() throws Exception {
+ public void
+ getStatusForFileAddition_noImplicitlyApprovalByPatchSetUploaderThatDoesntOwnTheChange()
+ throws Exception {
setAsRootCodeOwners(admin);
Path path = Paths.get("/foo/bar.baz");
@@ -740,26 +874,6 @@
}
@Test
- @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatusForFileAddition_noImplicitlyApprovalByPreviousPatchSetUploader()
- throws Exception {
- TestAccount user2 = accountCreator.user2();
-
- setAsRootCodeOwners(user);
-
- Path path = Paths.get("/foo/bar.baz");
- String changeId =
- createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- amendChange(user, changeId);
- amendChange(user2, changeId);
-
- ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
- assertThatCollection(fileCodeOwnerStatuses)
- .containsExactly(
- FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
- }
-
- @Test
public void approvedByAnyoneWhenEveryoneIsCodeOwner() throws Exception {
// Create a code owner config file that makes everyone a code owner.
codeOwnerConfigOperations
@@ -795,17 +909,30 @@
@Test
public void everyoneIsCodeOwner_noImplicitApproval() throws Exception {
- testImplicitlyApprovedWhenEveryoneIsCodeOwner(/* implicitApprovalsEnabled= */ false);
+ testImplicitlyApprovedWhenEveryoneIsCodeOwner(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void everyoneIsCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner()
+ throws Exception {
+ testImplicitlyApprovedWhenEveryoneIsCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
public void everyoneIsCodeOwner_withImplicitApproval() throws Exception {
- testImplicitlyApprovedWhenEveryoneIsCodeOwner(/* implicitApprovalsEnabled= */ true);
+ testImplicitlyApprovedWhenEveryoneIsCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
}
- private void testImplicitlyApprovedWhenEveryoneIsCodeOwner(boolean implicitApprovalsEnabled)
- throws Exception {
+ private void testImplicitlyApprovedWhenEveryoneIsCodeOwner(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner = user;
+
// Create a code owner config file that makes everyone a code owner.
codeOwnerConfigOperations
.newCodeOwnerConfig()
@@ -820,12 +947,18 @@
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
+
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@@ -898,34 +1031,59 @@
}
@Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"bot@example.com", "otherBot@example.com"})
public void globalCodeOwner_noImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(/* implicitApprovalsEnabled= */ false);
+ testImplicitlyApprovedByGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"bot@example.com", "otherBot@example.com"})
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void globalCodeOwner_withImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(/* implicitApprovalsEnabled= */ true);
+ public void globalCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
- private void testImplicitlyApprovedByGlobalCodeOwner(boolean implicitApprovalsEnabled)
- throws Exception {
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.globalCodeOwner",
+ values = {"bot@example.com", "otherBot@example.com"})
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void globalCodeOwner_withImplicitApproval() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ private void testImplicitlyApprovedByGlobalCodeOwner(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
TestAccount bot =
accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+ TestAccount otherBot =
+ accountCreator.create(
+ "other_bot", "otherBot@example.com", "Other Bot", /* displayName= */ null);
Path path = Paths.get("/foo/bar.baz");
String changeId =
createChange(bot, "Change Adding A File", JgitPath.of(path).get(), "file content")
.getChangeId();
+ if (uploaderMatchesChangeOwner) {
+ amendChange(bot, changeId);
+ } else {
+ amendChange(otherBot, changeId);
+ }
+
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@@ -1004,7 +1162,16 @@
@GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
public void everyoneIsGlobalCodeOwner_noImplicitApproval() throws Exception {
testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ false);
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void everyoneIsGlobalCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner()
+ throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
@Test
@@ -1012,22 +1179,30 @@
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
public void everyoneIsGlobalCodeOwner_withImplicitApproval() throws Exception {
testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ true);
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
}
private void testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- boolean implicitApprovalsEnabled) throws Exception {
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner = user;
// Create a change as a user that is a code owner only through the global code ownership.
Path path = Paths.get("/foo/bar.baz");
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
+
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
@@ -1539,23 +1714,43 @@
@Test
public void defaultCodeOwner_noImplicitApproval() throws Exception {
- testImplicitlyApprovedByDefaultCodeOwner(/* implicitApprovalsEnabled= */ false);
+ testImplicitlyApprovedByDefaultCodeOwner(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void defaultCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner()
+ throws Exception {
+ testImplicitlyApprovedByDefaultCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
public void defaultCodeOwner_withImplicitApproval() throws Exception {
- testImplicitlyApprovedByDefaultCodeOwner(/* implicitApprovalsEnabled= */ true);
+ testImplicitlyApprovedByDefaultCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
}
- private void testImplicitlyApprovedByDefaultCodeOwner(boolean implicitApprovalsEnabled)
- throws Exception {
- setAsDefaultCodeOwners(user);
+ private void testImplicitlyApprovedByDefaultCodeOwner(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount changeOwner = admin;
+ TestAccount otherCodeOwner =
+ accountCreator.create(
+ "code_owner", "code.owner@example.com", "Code Owner", /* displayName= */ null);
+
+ setAsDefaultCodeOwners(changeOwner, otherCodeOwner);
Path path = Paths.get("/foo/bar.baz");
String changeId =
- createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ if (uploaderMatchesChangeOwner) {
+ amendChange(changeOwner, changeId);
+ } else {
+ amendChange(otherCodeOwner, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
@@ -1565,7 +1760,7 @@
.containsExactly(
FileCodeOwnerStatus.addition(
path,
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
index 68382f9..4ab563c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
@@ -115,28 +115,45 @@
}
@Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", values = "bot@example.com")
public void globalCodeOwner_noImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(/* implicitApprovalsEnabled= */ false);
+ testImplicitlyApprovedByGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", values = "bot@example.com")
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void globalCodeOwner_withImplicitApproval() throws Exception {
- testImplicitlyApprovedByGlobalCodeOwner(/* implicitApprovalsEnabled= */ true);
+ public void globalCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
- private void testImplicitlyApprovedByGlobalCodeOwner(boolean implicitApprovalsEnabled)
- throws Exception {
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", values = "bot@example.com")
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void globalCodeOwner_withImplicitApproval() throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ private void testImplicitlyApprovedByGlobalCodeOwner(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
TestAccount bot =
accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+ TestAccount projectOwner = admin;
Path path = Paths.get("/foo/bar.baz");
String changeId =
createChange(bot, "Change Adding A File", JgitPath.of(path).get(), "file content")
.getChangeId();
+ if (uploaderMatchesChangeOwner) {
+ amendChange(bot, changeId);
+ } else {
+ amendChange(projectOwner, changeId);
+ }
+
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
@@ -146,7 +163,7 @@
.value()
.hasStatusThat()
.isEqualTo(
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
@@ -250,7 +267,16 @@
@GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
public void everyoneIsGlobalCodeOwner_noImplicitApproval() throws Exception {
testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ false);
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*")
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void everyoneIsGlobalCodeOwner_noImplicitApproval_uploaderDoesntMatchChangeOwner()
+ throws Exception {
+ testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
@Test
@@ -258,16 +284,25 @@
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
public void everyoneIsGlobalCodeOwner_withImplicitApproval() throws Exception {
testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- /* implicitApprovalsEnabled= */ true);
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
}
private void testImplicitlyApprovedByGlobalCodeOwnerWhenEveryoneIsGlobalCodeOwner(
- boolean implicitApprovalsEnabled) throws Exception {
- // Create a change as a user that is a code owner only through the global code ownership.
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount projectOwner = admin;
+ TestAccount otherProjectOwner = accountCreator.admin2();
+
+ // Create a change as a user that is a project code owner.
Path path = Paths.get("/foo/bar.baz");
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+ if (uploaderMatchesChangeOwner) {
+ amendChange(projectOwner, changeId);
+ } else {
+ amendChange(otherProjectOwner, changeId);
+ }
+
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
assertThatCollection(fileCodeOwnerStatuses).onlyElement();
@@ -277,7 +312,7 @@
.value()
.hasStatusThat()
.isEqualTo(
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
@@ -407,26 +442,39 @@
}
@Test
- public void getStatus_noImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatus(/* implicitApprovalsEnabled= */ false);
+ public void getStatus_noImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatus(
+ /* implicitApprovalsEnabled= */ false, /* uploaderMatchesChangeOwner= */ true);
}
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatus_withImplicitApprovalByPatchSetUploader() throws Exception {
- testImplicitApprovalByPatchSetUploaderOnGetStatus(/* implicitApprovalsEnabled= */ true);
+ public void getStatus_noImplicitApproval_uploaderDoesntMatchChangeOwner() throws Exception {
+ testImplicitApprovalOnGetStatus(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ false);
}
- private void testImplicitApprovalByPatchSetUploaderOnGetStatus(boolean implicitApprovalsEnabled)
- throws Exception {
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
+ public void getStatus_withImplicitApproval() throws Exception {
+ testImplicitApprovalOnGetStatus(
+ /* implicitApprovalsEnabled= */ true, /* uploaderMatchesChangeOwner= */ true);
+ }
+
+ private void testImplicitApprovalOnGetStatus(
+ boolean implicitApprovalsEnabled, boolean uploaderMatchesChangeOwner) throws Exception {
+ TestAccount projectOwner = admin;
+
// Create change with a user that is not a project owner.
Path path = Paths.get("/foo/bar.baz");
String changeId =
- createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
- .getChangeId();
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- // Amend change with a user that is a project owner.
- amendChange(admin, changeId);
+ if (uploaderMatchesChangeOwner) {
+ amendChange(projectOwner, changeId);
+ } else {
+ amendChange(user, changeId);
+ }
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
@@ -437,7 +485,7 @@
.value()
.hasStatusThat()
.isEqualTo(
- implicitApprovalsEnabled
+ implicitApprovalsEnabled && uploaderMatchesChangeOwner
? CodeOwnerStatus.APPROVED
: CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
fileCodeOwnerStatusSubject.hasOldPathStatus().isEmpty();
@@ -447,14 +495,17 @@
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void getStatus_noImplicitlyApprovalByChangeOwner() throws Exception {
+ public void getStatus_noImplicitlyApprovalByPatchSetUploaderThatDoesntOwnTheChange()
+ throws Exception {
+ TestAccount admin2 = accountCreator.admin2();
+
// Create change with a user that is a project owner.
Path path = Paths.get("/foo/bar.baz");
String changeId =
createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
- // Amend change with a user that is not a project owner.
- amendChange(user, changeId);
+ // Amend change with a user that is another project owner.
+ amendChange(admin2, changeId);
ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
index 6cdf2b1..f09a9e8 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -212,7 +212,7 @@
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "true")
- public void notImplicitlyApprovedByUploaderWhoIsChangeOwner() throws Exception {
+ public void notImplicitlyApproved() throws Exception {
TestAccount codeOwner =
accountCreator.create(
"codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
@@ -269,7 +269,7 @@
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "forced")
- public void implicitlyApprovedByUploaderWhoIsChangeOwner() throws Exception {
+ public void implicitlyApproved() throws Exception {
TestAccount codeOwner =
accountCreator.create(
"codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
@@ -294,7 +294,7 @@
@Test
@GerritConfig(name = "plugin.code-owners.enableImplicitApprovals", value = "forced")
- public void implicitlyApprovedByUploader() throws Exception {
+ public void notImplicitlyApprovedByUploader_forcedImplicitApprovals() throws Exception {
TestAccount changeOwner =
accountCreator.create(
"changeOwner", "changeOwner@example.com", "ChangeOwner", /* displayName= */ null);
@@ -321,7 +321,7 @@
.hasNewPathStatus()
.value()
.hasStatusThat()
- .isEqualTo(CodeOwnerStatus.APPROVED);
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
}
@Test
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index 473de83..86b447f 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -33,16 +33,17 @@
### <a id="implicitApprovals">Implicit code owner approvals
It's possible to [enable implicit approvals](config.html#pluginCodeOwnersEnableImplicitApprovals)
-of code owners on their own changes. If enabled and the uploader of a patch set
-is a code owner, an approval of the uploader is assumed for all owned files.
-This means if a code owner uploads a change / patch set that only touches files
-that they own, no approval from other code owners is required for submitting the
-change.
+of code owners on their own changes. If enabled, changes of code owners are
+automatically code owner approved, but only if the last patch set was uploaded
+by the change owner (change owner == last patch set uploader). This implict code
+owner approval covers all files that are owned by the change owner. This means
+if a code owner uploads a change that only touches files that they own, no
+approval from other code owners is required for submitting the change.
If implicit approvals are enabled, paths can be exempted from requiring code
owner approvals by assigning the code ownership to [all
users](backend-find-owners.html#allUsers), as then any modification to the path
-is always implicitly approved by the uploader.
+is always implicitly approved by the change owner.
**NOTE:** If implicit approvals are disabled, users can still self-approve their
own changes by voting on the required label.
@@ -187,17 +188,14 @@
### <a id="securityImplicitApprovals">Implicit approvals
If [implicit approvals](#implicitApprovals) are enabled, it is important that
-code owners are aware of their implicit approval when they upload new patch sets
+code owners are aware of their implicit approval when they upload new changes
for other users.
-Examples:
+Example:
* If a contributor pushes a change to a wrong branch and a code owner helps them
to get it rebased onto the correct branch, the rebased change has implicit
- approvals from the code owner, since the code owner is the uploader.
-* If a code owner edits the commit message of change owned by a non-code-owner,
- the change gets implicitly approved since editing the commit message creates a
- new patch set and the code owner is the uploader that patch set.
+ approvals from the code owner, since the code owner is the change owner.
To avoid situations like this it is recommended to not enable implicit
approvals.
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 53c39d2..90e4e5c 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -181,33 +181,34 @@
By default unset (no invalid code owner config info URL).
<a id="pluginCodeOwnersEnableImplicitApprovals">plugin.@PLUGIN@.enableImplicitApprovals</a>
-: Whether an implicit code owner approval from the last uploader is
+: Whether an implicit code owner approval from the change owner is
assumed.\
+ Implicit code owner approvals are only applied on patch sets that are
+ uploaded by the change owner (change owner == last patch set uploader).\
\
Can be `FALSE`, `TRUE` or `FORCED`.\
\
`FALSE`:\
- Implicit code-owner approvals of the the patch set uploader are
- disabled.\
+ Implicit code-owner approvals of change owners are disabled.\
\
`TRUE`:\
- Implicit code-owner approvals of the patch set uploader are enabled, but
- only if the configured [required
- label](#pluginCodeOwnersRequiredApproval) is not configured to [ignore
- self approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
+ Implicit code-owner approvals of change owners are enabled, but only if
+ the configured [required label](#pluginCodeOwnersRequiredApproval) is
+ not configured to [ignore self
+ approvals](../../../Documentation/config-labels.html#label_ignoreSelfApproval)
from the uploader.\
\
`FORCED`:\
- Implicit code-owner approvals of the patch set uploader are enabled,
- even if the configured [required
- label](#pluginCodeOwnersRequiredApproval) disallows self approvals.\
+ Implicit code-owner approvals of change owners are enabled, even if the
+ configured [required label](#pluginCodeOwnersRequiredApproval) disallows
+ self approvals.\
\
If enabled/enforced, code owners need to be aware of their implicit
- approval when they upload new patch sets for other users (e.g. if a
+ approval when they upload new changes for other users (e.g. if a
contributor pushes a change to a wrong branch and a code owner helps
them to get it rebased onto the correct branch, the rebased change has
implicit approvals from the code owner, since the code owner is the
- uploader).\
+ change owner).\
If implicit code owner approvals are disabled, code owners can still
self-approve their own changes by voting on the change.\
Can be overridden per project by setting
@@ -656,17 +657,19 @@
in `gerrit.config` is used.
<a id="codeOwnersEnableImplicitApprovals">codeOwners.enableImplicitApprovals</a>
-: Whether an implicit code owner approval from the last uploader is
+: Whether an implicit code owner approval from the change owner is
assumed.\
+ Implicit code owner approvals are only applied on patch sets that are
+ uploaded by the change owner (change owner == last patch set uploader).\
Can be `FALSE`, `TRUE` or `FORCED` (see
[plugin.@PLUGIN@.enableImplicitApprovals](#pluginCodeOwnersEnableImplicitApprovals)
for an explanation of these values).\
If enabled/enforced, code owners need to be aware of their implicit
- approval when they upload new patch sets for other users (e.g. if a
+ approval when they upload new changes for other users (e.g. if a
contributor pushes a change to a wrong branch and a code owner helps
them to get it rebased onto the correct branch, the rebased change has
implicit approvals from the code owner, since the code owner is the
- uploader).\
+ change owner).\
If implicit code owner approvals are disabled, code owners can still
self-approve their own changes by voting on the change.\
Overrides the global setting
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md
index bd67ed2..d912ea4 100644
--- a/resources/Documentation/setup-guide.md
+++ b/resources/Documentation/setup-guide.md
@@ -336,8 +336,9 @@
Examples (not an exhaustive list):
* [Global code owners](config.html#pluginCodeOwnersGlobalCodeOwner)
-* Whether [an implicit code owner approval from the last uploader is
- assumed](config.html#codeOwnersEnableImplicitApprovals)
+* Whether [an implicit code owner approval from the change owner is
+ assumed](config.html#codeOwnersEnableImplicitApprovals) (only for patch sets
+ that are uploaded by the change owner)
* [Merge commit strategy](config.html#codeOwnersMergeCommitStrategy) that
decides which files of merge commits require code owner approvals
* [File extension](config.html#codeOwnersFileExtension) that should be used for
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md
index f7318ee..5a665cb 100644
--- a/resources/Documentation/user-guide.md
+++ b/resources/Documentation/user-guide.md
@@ -64,14 +64,20 @@
block the submission (unless it's a veto vote which is configured independently
of the `@PLUGIN@` plugin).
-It's possible to configure that for changes/patch-sets that are uploaded by a
-code owner an implicit code owner approval from the uploader is assumed. In this
-case, if a code owner only touches files that they own, no approval from other
-code owners is required. If this is configured, it is important that code owners
-are aware of their implicit approval when they upload new patch sets for other
-users (e.g. if a contributor pushes a change to a wrong branch and a code owner
-helps them to get it rebased onto the correct branch, the rebased change has
-implicit approvals from the code owner, since the code owner is the uploader).
+It's possible to [configure implicit
+approvals](config.html#codeOwnersEnableImplicitApprovals) for changes/patch-sets
+that are owned and uploaded by a code owner. In this case, if a code owner only
+touches files that they own, no approval from other code owners is required. If
+this is configured, it is important that code owners are aware of their implicit
+approval when they upload new changes for other users (e.g. if a contributor
+pushes a change to a wrong branch and a code owner helps them to get it rebased
+onto the correct branch, i.e. the code owner performs a cherry-pick, the rebased
+change has implicit approvals from the code owner, since the code owner is the
+change owner and uploader).
+
+**NOTE:** Implicit approvals are applied on changes that are owned by a code
+owner, but only if the current patch set was uploader by the change owner
+(change owner == last patch set uploader).
For files that are [renamed/moved](#renames) Gerrit requires a code owner
approval for the old and the new path of the files.
