Merge "Support notify settings for posting checks"
diff --git a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
index f29d079..796bce3 100644
--- a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
@@ -15,7 +15,12 @@
package com.google.gerrit.plugins.checks;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.DuplicateKeyException;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.NotifyInfo;
+import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.plugins.checks.email.CombinedCheckStateUpdatedSender;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -23,11 +28,14 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.UserInitiated;
+import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
+import java.util.Map;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/**
* API to update checks.
@@ -50,6 +58,7 @@
private final CombinedCheckStateUpdatedSender.Factory combinedCheckStateUpdatedSenderFactory;
private final ChangeNotes.Factory notesFactory;
private final PatchSetUtil psUtil;
+ private final NotifyResolver notifyResolver;
private final Optional<IdentifiedUser> currentUser;
private final ChecksStorageUpdate checksStorageUpdate;
@@ -60,11 +69,13 @@
CombinedCheckStateUpdatedSender.Factory combinedCheckStateUpdatedSenderFactory,
ChangeNotes.Factory notesFactory,
PatchSetUtil psUtil,
+ NotifyResolver notifyResolver,
@Assisted IdentifiedUser currentUser) {
this.combinedCheckStateCache = combinedCheckStateCache;
this.combinedCheckStateUpdatedSenderFactory = combinedCheckStateUpdatedSenderFactory;
this.notesFactory = notesFactory;
this.psUtil = psUtil;
+ this.notifyResolver = notifyResolver;
this.currentUser = Optional.of(currentUser);
this.checksStorageUpdate = checksStorageUpdate;
}
@@ -75,17 +86,23 @@
CombinedCheckStateCache combinedCheckStateCache,
CombinedCheckStateUpdatedSender.Factory combinedCheckStateUpdatedSenderFactory,
ChangeNotes.Factory notesFactory,
- PatchSetUtil psUtil) {
+ PatchSetUtil psUtil,
+ NotifyResolver notifyResolver) {
this.combinedCheckStateCache = combinedCheckStateCache;
this.combinedCheckStateUpdatedSenderFactory = combinedCheckStateUpdatedSenderFactory;
this.notesFactory = notesFactory;
this.psUtil = psUtil;
+ this.notifyResolver = notifyResolver;
this.currentUser = Optional.empty();
this.checksStorageUpdate = checksStorageUpdate;
}
- public Check createCheck(CheckKey key, CheckUpdate checkUpdate)
- throws DuplicateKeyException, IOException {
+ public Check createCheck(
+ CheckKey key,
+ CheckUpdate checkUpdate,
+ @Nullable NotifyHandling notifyHandling,
+ @Nullable Map<RecipientType, NotifyInfo> notifyDetails)
+ throws DuplicateKeyException, BadRequestException, IOException, ConfigInvalidException {
CombinedCheckState oldCombinedCheckState =
combinedCheckStateCache.get(key.repository(), key.patchSet());
@@ -94,13 +111,18 @@
CombinedCheckState newCombinedCheckState =
combinedCheckStateCache.get(key.repository(), key.patchSet());
if (oldCombinedCheckState != newCombinedCheckState) {
- sendEmail(key, newCombinedCheckState);
+ sendEmail(notifyHandling, notifyDetails, key, newCombinedCheckState);
}
return check;
}
- public Check updateCheck(CheckKey key, CheckUpdate checkUpdate) throws IOException {
+ public Check updateCheck(
+ CheckKey key,
+ CheckUpdate checkUpdate,
+ @Nullable NotifyHandling notifyHandling,
+ @Nullable Map<RecipientType, NotifyInfo> notifyDetails)
+ throws BadRequestException, IOException, ConfigInvalidException {
CombinedCheckState oldCombinedCheckState =
combinedCheckStateCache.get(key.repository(), key.patchSet());
@@ -109,13 +131,21 @@
CombinedCheckState newCombinedCheckState =
combinedCheckStateCache.get(key.repository(), key.patchSet());
if (oldCombinedCheckState != newCombinedCheckState) {
- sendEmail(key, newCombinedCheckState);
+ sendEmail(notifyHandling, notifyDetails, key, newCombinedCheckState);
}
return check;
}
- private void sendEmail(CheckKey checkKey, CombinedCheckState combinedCheckState) {
+ private void sendEmail(
+ @Nullable NotifyHandling notifyHandling,
+ @Nullable Map<RecipientType, NotifyInfo> notifyDetails,
+ CheckKey checkKey,
+ CombinedCheckState combinedCheckState)
+ throws BadRequestException, IOException, ConfigInvalidException {
+ notifyHandling = notifyHandling != null ? notifyHandling : NotifyHandling.ALL;
+ NotifyResolver.Result notify = notifyResolver.resolve(notifyHandling, notifyDetails);
+
try {
CombinedCheckStateUpdatedSender sender =
combinedCheckStateUpdatedSenderFactory.create(
@@ -131,6 +161,7 @@
sender.setPatchSet(patchSet);
sender.setCombinedCheckState(combinedCheckState);
+ sender.setNotify(notify);
sender.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log(
diff --git a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
index 310af7a..6e6de5b 100644
--- a/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
+++ b/java/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImpl.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.plugins.checks.Check;
import com.google.gerrit.plugins.checks.CheckJson;
import com.google.gerrit.plugins.checks.CheckKey;
@@ -66,7 +67,8 @@
@Override
public TestCheckUpdate.Builder newCheck(CheckKey key) {
return TestCheckUpdate.builder(key)
- .checkUpdater(u -> checksUpdate.get().createCheck(key, toCheckUpdate(u)));
+ .checkUpdater(
+ u -> checksUpdate.get().createCheck(key, toCheckUpdate(u), NotifyHandling.NONE, null));
}
final class PerCheckOperationsImpl implements PerCheckOperations {
@@ -117,7 +119,10 @@
public TestCheckUpdate.Builder forUpdate() {
return TestCheckUpdate.builder(key)
.checkUpdater(
- testUpdate -> checksUpdate.get().updateCheck(key, toCheckUpdate(testUpdate)));
+ testUpdate ->
+ checksUpdate
+ .get()
+ .updateCheck(key, toCheckUpdate(testUpdate), NotifyHandling.NONE, null));
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/CheckInput.java b/java/com/google/gerrit/plugins/checks/api/CheckInput.java
index 2162ce3..fd3409d 100644
--- a/java/com/google/gerrit/plugins/checks/api/CheckInput.java
+++ b/java/com/google/gerrit/plugins/checks/api/CheckInput.java
@@ -16,7 +16,11 @@
import com.google.common.base.MoreObjects;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.NotifyInfo;
+import com.google.gerrit.extensions.api.changes.RecipientType;
import java.sql.Timestamp;
+import java.util.Map;
import java.util.Objects;
/** Input to create or update a {@link com.google.gerrit.plugins.checks.Check}. */
@@ -33,6 +37,13 @@
@Nullable public Timestamp started;
/** Date/Time at which the checker finished processing this check. */
@Nullable public Timestamp finished;
+ /**
+ * Whom to send email notifications to when the combined check state changes due to posting this
+ * check.
+ */
+ @Nullable public NotifyHandling notify;
+ /** Additional information about whom to notify regardless of the {@link #notify} setting. */
+ @Nullable public Map<RecipientType, NotifyInfo> notifyDetails;
@Override
public boolean equals(Object o) {
@@ -45,12 +56,14 @@
&& Objects.equals(other.message, message)
&& Objects.equals(other.url, url)
&& Objects.equals(other.started, started)
- && Objects.equals(other.finished, finished);
+ && Objects.equals(other.finished, finished)
+ && Objects.equals(other.notify, notify)
+ && Objects.equals(other.notifyDetails, notifyDetails);
}
@Override
public int hashCode() {
- return Objects.hash(checkerUuid, state, message, url, started, finished);
+ return Objects.hash(checkerUuid, state, message, url, started, finished, notify, notifyDetails);
}
@Override
@@ -62,6 +75,8 @@
.add("url", url)
.add("started", started)
.add("finished", finished)
+ .add("notify", notify)
+ .add("notifyDetails", notifyDetails)
.toString();
}
}
diff --git a/java/com/google/gerrit/plugins/checks/api/PostCheck.java b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
index 5531d9e..aaad490 100644
--- a/java/com/google/gerrit/plugins/checks/api/PostCheck.java
+++ b/java/com/google/gerrit/plugins/checks/api/PostCheck.java
@@ -109,9 +109,15 @@
() ->
new UnprocessableEntityException(
String.format("checker %s not found", checkerUuid)));
- updatedCheck = checksUpdate.get().createCheck(key, toCheckUpdate(input));
+ updatedCheck =
+ checksUpdate
+ .get()
+ .createCheck(key, toCheckUpdate(input), input.notify, input.notifyDetails);
} else {
- updatedCheck = checksUpdate.get().updateCheck(key, toCheckUpdate(input));
+ updatedCheck =
+ checksUpdate
+ .get()
+ .updateCheck(key, toCheckUpdate(input), input.notify, input.notifyDetails);
}
return Response.ok(checkJsonFactory.noOptions().format(updatedCheck));
}
diff --git a/java/com/google/gerrit/plugins/checks/api/RerunCheck.java b/java/com/google/gerrit/plugins/checks/api/RerunCheck.java
index 023e302..487742f 100644
--- a/java/com/google/gerrit/plugins/checks/api/RerunCheck.java
+++ b/java/com/google/gerrit/plugins/checks/api/RerunCheck.java
@@ -103,7 +103,7 @@
.unsetStarted()
.setMessage("")
.setUrl("");
- updatedCheck = checksUpdate.get().updateCheck(key, builder.build());
+ updatedCheck = checksUpdate.get().updateCheck(key, builder.build(), null, null);
}
return Response.ok(checkJsonFactory.noOptions().format(updatedCheck));
}
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 8723614..ca1ce0c 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
@@ -20,10 +20,15 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.NotifyInfo;
+import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.client.ProjectWatchInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
@@ -39,7 +44,9 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.inject.Inject;
+import java.util.Arrays;
import java.util.List;
+import java.util.Set;
import org.junit.Before;
import org.junit.Test;
@@ -227,6 +234,72 @@
assertThat(sender.getMessages()).isEmpty();
}
+ @Test
+ public void postCheckRespectsNotifySettings() throws Exception {
+ testNotifySettingsForPostCheck(NotifyHandling.ALL, owner, reviewer, starrer, watcher);
+ testNotifySettingsForPostCheck(NotifyHandling.OWNER, owner);
+ testNotifySettingsForPostCheck(NotifyHandling.OWNER_REVIEWERS, owner, reviewer);
+ testNotifySettingsForPostCheck(NotifyHandling.NONE);
+
+ testNotifySettingsForPostCheck(
+ ImmutableSet.of(user), NotifyHandling.ALL, user, owner, reviewer, starrer, watcher);
+ testNotifySettingsForPostCheck(ImmutableSet.of(user), NotifyHandling.OWNER, user, owner);
+ testNotifySettingsForPostCheck(
+ ImmutableSet.of(user), NotifyHandling.OWNER_REVIEWERS, user, owner, reviewer);
+ testNotifySettingsForPostCheck(ImmutableSet.of(user), NotifyHandling.NONE, user);
+ }
+
+ private void testNotifySettingsForPostCheck(
+ NotifyHandling notify, TestAccount... expectedRecipients) throws RestApiException {
+ testNotifySettingsForPostCheck(ImmutableSet.of(), notify, expectedRecipients);
+ }
+
+ private void testNotifySettingsForPostCheck(
+ Set<TestAccount> accountsToNotify, NotifyHandling notify, TestAccount... expectedRecipients)
+ throws RestApiException {
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.IN_PROGRESS);
+
+ sender.clear();
+
+ // Post a new check that changes the combined check state to FAILED.
+ requestScopeOperations.setApiUser(bot.id());
+ CheckInput input = new CheckInput();
+ if (!accountsToNotify.isEmpty()) {
+ input.notifyDetails =
+ ImmutableMap.of(
+ RecipientType.TO,
+ new NotifyInfo(
+ accountsToNotify.stream().map(TestAccount::username).collect(toImmutableList())));
+ }
+ input.notify = notify;
+ input.checkerUuid = checkerUuid1.get();
+ input.state = CheckState.FAILED;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ List<Message> messages = sender.getMessages();
+ if (expectedRecipients.length == 0) {
+ assertThat(messages).isEmpty();
+ } else {
+ 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.FAILED);
+ assertThat(message.rcpt())
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedRecipients)
+ .map(TestAccount::getEmailAddress)
+ .collect(toImmutableList()));
+ }
+
+ // reset combined check state
+ input.state = CheckState.SCHEDULED;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.IN_PROGRESS);
+ }
+
private CombinedCheckState getCombinedCheckState() throws RestApiException {
ChangeInfo changeInfo =
gApi.changes()
diff --git a/resources/Documentation/rest-api-checks.md b/resources/Documentation/rest-api-checks.md
index 1c64598..94346d5 100644
--- a/resources/Documentation/rest-api-checks.md
+++ b/resources/Documentation/rest-api-checks.md
@@ -202,6 +202,8 @@
| `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`.
+| `notifyDetails`| 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="check-state"> CheckState (enum)
The `CheckState` enum can have the following values: `NOT_STARTED`, `FAILED`,
