Notify only owner if combined check state is updated to non-passing state
Reviewers are only interested in the combined check state when it turns
to a value that makes the change submittable. This is the case for the
states SUCCESSFUL and NOT_RELEVANT. Other combined check states are only
relevant for the change owner, hence for those an email notification
should only be sent to the change owner.
This is just the default for the notifications. Callers can always
override it by setting NotifyHandling in the REST input.
Rerunning a check cannot cause an update of the combined check state to
SUCCESSFUL or NOT_RELEVANT, hence there is no test and documentation for
this case
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I37d407d3336f42d5cd876cccd90ee115cf7fe41d
diff --git a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
index 796bce3..56ece70 100644
--- a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
@@ -143,7 +143,13 @@
CheckKey checkKey,
CombinedCheckState combinedCheckState)
throws BadRequestException, IOException, ConfigInvalidException {
- notifyHandling = notifyHandling != null ? notifyHandling : NotifyHandling.ALL;
+ notifyHandling =
+ notifyHandling != null
+ ? notifyHandling
+ : combinedCheckState == CombinedCheckState.SUCCESSFUL
+ || combinedCheckState == CombinedCheckState.NOT_RELEVANT
+ ? NotifyHandling.ALL
+ : NotifyHandling.OWNER;
NotifyResolver.Result notify = notifyResolver.resolve(notifyHandling, notifyDetails);
try {
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 4b88529..ee824f1 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
@@ -121,7 +121,7 @@
}
@Test
- public void combinedCheckUpdatedEmailAfterCheckCreation() throws Exception {
+ public void combinedCheckUpdatedEmailAfterCheckCreationToOwnerOnly() throws Exception {
// Create a required checker.
CheckerUuid checkerUuid =
checkerOperations.newChecker().repository(project).required().create();
@@ -138,7 +138,9 @@
checksApiFactory.revision(patchSetId).create(input).get();
assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
- // Expect one email because the combined check state was updated.
+ // Expect email because the combined check state was updated.
+ // The email is only sent to the change owner because the new combined check state !=
+ // SUCCESSFUL.
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
@@ -146,6 +148,37 @@
assertThat(message.from().getName()).isEqualTo(bot.fullName() + " (Code Review)");
assertThat(message.body())
.contains("The combined check state has been updated to " + CombinedCheckState.FAILED);
+ assertThat(message.rcpt()).containsExactly(owner.getEmailAddress());
+ }
+
+ @Test
+ public void combinedCheckUpdatedEmailAfterCheckCreationToAll() throws Exception {
+ // Create a required checker.
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).required().create();
+
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.IN_PROGRESS);
+
+ sender.clear();
+
+ // Post a check that changes the combined check state to SUCCESSFUL.
+ requestScopeOperations.setApiUser(bot.id());
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.state = CheckState.SUCCESSFUL;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.SUCCESSFUL);
+
+ // Expect email because the combined check state was updated.
+ // The email is only sent to all users that are involved in the change because the new combined
+ // check state = SUCCESSFUL.
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+
+ Message message = messages.get(0);
+ assertThat(message.from().getName()).isEqualTo(bot.fullName() + " (Code Review)");
+ assertThat(message.body())
+ .contains("The combined check state has been updated to " + CombinedCheckState.SUCCESSFUL);
assertThat(message.rcpt())
.containsExactly(
owner.getEmailAddress(),
@@ -183,7 +216,7 @@
}
@Test
- public void combinedCheckUpdatedEmailAfterCheckUpdate() throws Exception {
+ public void combinedCheckUpdatedEmailAfterCheckUpdateToOwnerOnly() throws Exception {
// Create a required checker.
CheckerUuid checkerUuid =
checkerOperations.newChecker().repository(project).required().create();
@@ -203,7 +236,9 @@
checksApiFactory.revision(patchSetId).create(input).get();
assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.IN_PROGRESS);
- // Expect one email because the combined check state was updated.
+ // Expect email because the combined check state was updated.
+ // The email is only sent to the change owner because the new combined check state !=
+ // SUCCESSFUL.
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
@@ -211,6 +246,40 @@
assertThat(message.from().getName()).isEqualTo(bot.fullName() + " (Code Review)");
assertThat(message.body())
.contains("The combined check state has been updated to " + CombinedCheckState.IN_PROGRESS);
+ assertThat(message.rcpt()).containsExactly(owner.getEmailAddress());
+ }
+
+ @Test
+ public void combinedCheckUpdatedEmailAfterCheckUpdateToAll() throws Exception {
+ // Create a required checker.
+ CheckerUuid checkerUuid =
+ checkerOperations.newChecker().repository(project).required().create();
+
+ // Create a check that sets the combined check state to FAILED.
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid);
+ checkOperations.newCheck(checkKey).state(CheckState.FAILED).upsert();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ sender.clear();
+
+ // Update the new check so that the combined check state is changed to IN_PROGRESS.
+ requestScopeOperations.setApiUser(bot.id());
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.state = CheckState.SUCCESSFUL;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.SUCCESSFUL);
+
+ // Expect email because the combined check state was updated.
+ // The email is only sent to all users that are involved in the change because the new combined
+ // check state = SUCCESSFUL.
+ List<Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+
+ Message message = messages.get(0);
+ assertThat(message.from().getName()).isEqualTo(bot.fullName() + " (Code Review)");
+ assertThat(message.body())
+ .contains("The combined check state has been updated to " + CombinedCheckState.SUCCESSFUL);
assertThat(message.rcpt())
.containsExactly(
owner.getEmailAddress(),
@@ -250,7 +319,7 @@
}
@Test
- public void combinedCheckUpdatedEmailAfterCheckRerun() throws Exception {
+ public void combinedCheckUpdatedEmailAfterCheckRerunToOwnerOnly() throws Exception {
// Create a required checker.
CheckerUuid checkerUuid =
checkerOperations.newChecker().repository(project).required().create();
@@ -267,7 +336,9 @@
checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).rerun();
assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.IN_PROGRESS);
- // Expect one email because the combined check state was updated.
+ // Expect email because the combined check state was updated.
+ // The email is only sent to the change owner because the new combined check state !=
+ // SUCCESSFUL.
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
@@ -275,12 +346,7 @@
assertThat(message.from().getName()).isEqualTo(bot.fullName() + " (Code Review)");
assertThat(message.body())
.contains("The combined check state has been updated to " + CombinedCheckState.IN_PROGRESS);
- assertThat(message.rcpt())
- .containsExactly(
- owner.getEmailAddress(),
- reviewer.getEmailAddress(),
- starrer.getEmailAddress(),
- watcher.getEmailAddress());
+ assertThat(message.rcpt()).containsExactly(owner.getEmailAddress());
}
@Test
diff --git a/resources/Documentation/rest-api-checks.md b/resources/Documentation/rest-api-checks.md
index a152652..b64b32f 100644
--- a/resources/Documentation/rest-api-checks.md
+++ b/resources/Documentation/rest-api-checks.md
@@ -204,7 +204,7 @@
| `url` | optional | A fully-qualified URL pointing to the result of the check on the checker's infrastructure.
| `started` | optional | The [timestamp](../../../Documentation/rest-api.html#timestamp) of when the check started processing.
| `finished` | optional | The [timestamp](../../../Documentation/rest-api.html#timestamp) of when the check finished processing.
-| `notify` | optional | Notify handling that defines to whom email notifications should be sent when the combined check state changes due to posting this check. Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. If not set, the default is `ALL`.
+| `notify` | optional | Notify handling that defines to whom email notifications should be sent when the combined check state changes due to posting this check. Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. If not set, the default is `ALL` if the combined check state is updated to either `SUCCESSFUL` or `NOT_RELEVANT`, otherwise the default is `OWNER`.
| `notify_details`| optional | Additional information about whom to notify when the combined check state changes due to posting this check as a map of recipient type to [NotifyInfo](../../../Documentation/rest-api-changes.html#notify-info) entity.
### <a id="rerun-input"> RerunInput
@@ -212,7 +212,7 @@
| Field Name | | Description |
| --------------- | -------- | ----------- |
-| `notify` | optional | Notify handling that defines to whom email notifications should be sent when the combined check state changes due to rerunning this check. Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. If not set, the default is `ALL`.
+| `notify` | optional | Notify handling that defines to whom email notifications should be sent when the combined check state changes due to rerunning this check. Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. If not set, the default is `OWNER`.
| `notify_details`| optional | Additional information about whom to notify when the combined check state changes due to rerunning this check as a map of recipient type to [NotifyInfo](../../../Documentation/rest-api-changes.html#notify-info) entity.
### <a id="check-state"> CheckState (enum)
