Do not send combined check state updated emails for non-current patch sets
Users are only interested in the combined check state of the current
patch set.
In addition the computation of the combined check state on non-current
patch sets is not accurate because backfilling of checks is only done
for the current patch set. The reason for this is that the checker query
system currently cannot check if a checker is relevant for a non-current
patch set (see comment in
CheckBackfiller#getBackfilledChecksForRelevantCheckers).
On gerrit-review this led to unexpected email contents:
1. On gerrit-review we have 2 optional checkers: "Build/Tests" and
"Code Style"
2. If a successful check for "Code-Style" is posted on a non-current
patch set, we got an email saying that the combined check state was
updated to SUCCESSFUL, but the combined check state is expected to be
IN_PROGRESS since no check for "Build/Tests" was posted yet. That's
because no backfilling is done for "Build/Tests". That's also the
reason why "Build/Tests" is missing in the checks overview of the
email.
If emails are only sent for the current patch set, the issue with the
wrong email contents is no longer occurring.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia15feb64762fbaa7c500529a7746246ae6c0e089
diff --git a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
index 4d6cc54..63df867 100644
--- a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
@@ -159,6 +159,14 @@
return;
}
+ CheckKey checkKey = updatedCheck.key();
+ ChangeNotes changeNotes =
+ notesFactory.create(checkKey.repository(), checkKey.patchSet().changeId());
+ if (!checkKey.patchSet().equals(changeNotes.getCurrentPatchSet().id())) {
+ // do not send an email for non-current patch sets
+ return;
+ }
+
notifyHandling =
notifyHandling != null
? notifyHandling
@@ -168,8 +176,6 @@
: NotifyHandling.OWNER;
NotifyResolver.Result notify = notifyResolver.resolve(notifyHandling, notifyDetails);
- CheckKey checkKey = updatedCheck.key();
-
try {
CombinedCheckStateUpdatedSender sender =
combinedCheckStateUpdatedSenderFactory.create(
@@ -179,8 +185,6 @@
sender.setFrom(currentUser.get().getAccountId());
}
- ChangeNotes changeNotes =
- notesFactory.create(checkKey.repository(), checkKey.patchSet().changeId());
PatchSet patchSet = psUtil.get(changeNotes, checkKey.patchSet());
sender.setPatchSet(patchSet);
sender.setCombinedCheckState(oldCombinedCheckState, newCombinedCheckState);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
index c3c5fd1..2edc6a6 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
@@ -1065,6 +1065,30 @@
+ htmlEmailFooterForCombinedCheckStateUpdate());
}
+ @Test
+ public void noEmailForCombinedCheckStateUpdatesOfNonCurrentPatchSet() throws Exception {
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().name("My Checker").repository(project).create();
+
+ // push a new patch set
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ PushOneCommit.SUBJECT,
+ PushOneCommit.FILE_NAME,
+ "new content " + System.nanoTime(),
+ change.getKey().get());
+ push.to("refs/for/master").assertOkStatus();
+
+ sender.clear();
+
+ // post check on old patch set
+ postCheck(checkerUuid, CheckState.SUCCESSFUL);
+
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
private String combinedCheckStateUpdatedText(CombinedCheckState combinedCheckState) {
return "The combined check state has been updated to "
+ combinedCheckState
