Include override approval and approver into reason if override is present
This way users can see which label/approval counted as override.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I0e891849b87559847158d0447538a91c76afcc60
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index d97faf2..28597fe 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -27,6 +27,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
@@ -333,18 +334,19 @@
logger.atFine().log("requiredApproval = %s", requiredApproval);
ImmutableSet<RequiredApproval> overrideApprovals = codeOwnersConfig.getOverrideApprovals();
- boolean hasOverride =
- hasOverride(currentPatchSetApprovals, overrideApprovals, patchSetUploader);
+ ImmutableSet<PatchSetApproval> overrides =
+ getOverride(currentPatchSetApprovals, overrideApprovals, patchSetUploader);
logger.atFine().log(
- "hasOverride = %s (overrideApprovals = %s)",
- hasOverride,
+ "hasOverride = %s (overrideApprovals = %s, overrides = %s)",
+ !overrides.isEmpty(),
overrideApprovals.stream()
.map(
overrideApproval ->
String.format(
"%s (ignoreSelfApproval = %s)",
overrideApproval, overrideApproval.labelType().isIgnoreSelfApproval()))
- .collect(toImmutableList()));
+ .collect(toImmutableList()),
+ overrides);
BranchNameKey branch = changeNotes.getChange().getDest();
ObjectId revision = getDestBranchRevision(changeNotes.getChange());
@@ -377,7 +379,7 @@
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
- hasOverride,
+ overrides,
changedFile));
}
}
@@ -448,7 +450,7 @@
// Assume an explicit approval of the given account.
/* approverAccountIds= */ ImmutableSet.of(accountId),
fallbackCodeOwners,
- /* hasOverride= */ false,
+ /* overrides= */ ImmutableSet.of(),
changedFile));
}
}
@@ -499,7 +501,7 @@
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
FallbackCodeOwners fallbackCodeOwners,
- boolean hasOverride,
+ ImmutableSet<PatchSetApproval> overrides,
ChangedFile changedFile) {
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatus.start()) {
logger.atFine().log("computing file status for %s", changedFile);
@@ -520,7 +522,7 @@
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
- hasOverride,
+ overrides,
newPath));
// Compute the code owner status for the old path, if the file was deleted or renamed.
@@ -543,7 +545,7 @@
reviewerAccountIds,
approverAccountIds,
fallbackCodeOwners,
- hasOverride,
+ overrides,
changedFile.oldPath().get()));
}
@@ -564,16 +566,23 @@
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
FallbackCodeOwners fallbackCodeOwners,
- boolean hasOverride,
+ ImmutableSet<PatchSetApproval> overrides,
Path absolutePath) {
logger.atFine().log("computing path status for %s", absolutePath);
- if (hasOverride) {
+ if (!overrides.isEmpty()) {
logger.atFine().log(
- "the status for path %s is %s since an override is present",
- absolutePath, CodeOwnerStatus.APPROVED.name());
+ "the status for path %s is %s since an override is present (overrides = %s)",
+ absolutePath, CodeOwnerStatus.APPROVED.name(), overrides);
+ Optional<PatchSetApproval> override = overrides.stream().findAny();
+ checkState(override.isPresent(), "no override found");
return PathCodeOwnerStatus.create(
- absolutePath, CodeOwnerStatus.APPROVED, "override approval is present");
+ absolutePath,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval %s by %s is present",
+ override.get().label() + LabelValue.formatValue(override.get().value()),
+ ChangeMessagesUtil.getAccountTemplate(override.get().accountId())));
}
AtomicReference<CodeOwnerStatus> codeOwnerStatus =
@@ -1084,13 +1093,13 @@
}
/**
- * Checks whether the given change has an override approval.
+ * Gets the overrides that were applied on the change.
*
* @param overrideApprovals approvals that count as override for the code owners submit check.
* @param patchSetUploader account ID of the patch set uploader
- * @return whether the given change has an override approval
+ * @return the overrides that were applied on the change
*/
- private boolean hasOverride(
+ private ImmutableSet<PatchSetApproval> getOverride(
ImmutableList<PatchSetApproval> currentPatchSetApprovals,
ImmutableSet<RequiredApproval> overrideApprovals,
Account.Id patchSetUploader) {
@@ -1118,10 +1127,11 @@
}
return true;
})
- .anyMatch(
+ .filter(
patchSetApproval ->
overrideApprovals.stream()
- .anyMatch(overrideApproval -> overrideApproval.isApprovedBy(patchSetApproval)));
+ .anyMatch(overrideApproval -> overrideApproval.isApprovedBy(patchSetApproval)))
+ .collect(toImmutableSet());
}
/**
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 16401bb..41bbc77 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -1519,9 +1519,17 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path1, CodeOwnerStatus.APPROVED, "override approval is present"),
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))),
FileCodeOwnerStatus.addition(
- path2, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))));
}
@Test
@@ -1563,9 +1571,17 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path1, CodeOwnerStatus.APPROVED, "override approval is present"),
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))),
FileCodeOwnerStatus.addition(
- path2, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))));
// Delete the override approval.
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
@@ -1585,9 +1601,17 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path1, CodeOwnerStatus.APPROVED, "override approval is present"),
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Another-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))),
FileCodeOwnerStatus.addition(
- path2, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Another-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))));
}
@Test
@@ -1818,7 +1842,11 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))));
// Change some other file and submit the change with an override.
String changeId2 =
@@ -1839,7 +1867,11 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))));
}
@Test
@@ -1928,7 +1960,11 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+2 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(user2.id()))));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
index e45e5b3..ef3f830 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithProjectOwnersAsFallbackCodeOwnersTest.java
@@ -534,9 +534,17 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path1, CodeOwnerStatus.APPROVED, "override approval is present"),
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))),
FileCodeOwnerStatus.addition(
- path2, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(admin.id()))));
}
@Test
@@ -579,9 +587,17 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path1, CodeOwnerStatus.APPROVED, "override approval is present"),
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(user.id()))),
FileCodeOwnerStatus.addition(
- path2, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(user.id()))));
// Delete the override approval.
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
@@ -601,9 +617,17 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path1, CodeOwnerStatus.APPROVED, "override approval is present"),
+ path1,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Another-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(user.id()))),
FileCodeOwnerStatus.addition(
- path2, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path2,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Another-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(user.id()))));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
index 81382b9..1dd01d2 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithSelfApprovalsIgnoredTest.java
@@ -342,7 +342,11 @@
assertThatCollection(fileCodeOwnerStatuses)
.containsExactly(
FileCodeOwnerStatus.addition(
- path, CodeOwnerStatus.APPROVED, "override approval is present"));
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ ChangeMessagesUtil.getAccountTemplate(changeOwner.id()))));
}
@Test
