Send email notification if combined check state is updated
When the combined check state of a patch set changes by
creating/updating a check we now send an email notification to the
change owner, the reviewer, users who starred the change and project
watchers.
We only send a notification when the combined check state changes
because sending a notification each time a check is created or updated
would result in too many emails and we don't want to spam users.
To handle the sending of the email notifications we introduce a new
layer for updating checks that is in between the REST layer and the
storage layer. This way we neither need to reimplement sending email
notifications for all REST callers nor for all storage backends. The
new ChecksUpdate layer delegates the persistence of checks to the checks
storage backend and implements business logic (such a s sending emails)
that is the same for all storage backends. The interface for the
storage layer was renamed from ChecksUpdate to ChecksStorageUpdate (to
free up the ChecksUpdate name for the new layer).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ibb5cd800b786f6754fcf4f3f28f26c2cad7ded94
diff --git a/java/com/google/gerrit/plugins/checks/ChecksStorageUpdate.java b/java/com/google/gerrit/plugins/checks/ChecksStorageUpdate.java
new file mode 100644
index 0000000..bdf8693
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/ChecksStorageUpdate.java
@@ -0,0 +1,47 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.checks;
+
+import com.google.gerrit.exceptions.DuplicateKeyException;
+import java.io.IOException;
+
+/**
+ * API for updating checks in the storage backend.
+ *
+ * <p>To be implemented by check storage backends.
+ */
+public interface ChecksStorageUpdate {
+ /**
+ * Creates a new check in the storage backend.
+ *
+ * @param key the key of the check that should be created
+ * @param checkUpdate the update describing the properties of the new check
+ * @return the newly created check
+ * @throws DuplicateKeyException thrown if a check with the given key already exists
+ * @throws IOException thrown in case of an I/O error
+ */
+ public Check createCheck(CheckKey key, CheckUpdate checkUpdate)
+ throws DuplicateKeyException, IOException;
+
+ /**
+ * Updates an existing check in the storage backend.
+ *
+ * @param key the key of the check that should be updated
+ * @param checkUpdate the update describing the check properties that should be updated
+ * @return the updated check
+ * @throws IOException thrown in case of an I/O error
+ */
+ public Check updateCheck(CheckKey key, CheckUpdate checkUpdate) throws IOException;
+}
diff --git a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
index ba58afa..f29d079 100644
--- a/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/ChecksUpdate.java
@@ -14,12 +14,127 @@
package com.google.gerrit.plugins.checks;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.DuplicateKeyException;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.plugins.checks.email.CombinedCheckStateUpdatedSender;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.UserInitiated;
+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.Optional;
-public interface ChecksUpdate {
- Check createCheck(CheckKey key, CheckUpdate checkUpdate)
- throws DuplicateKeyException, IOException;
+/**
+ * API to update checks.
+ *
+ * <p>Delegates the persistence of checks to the storage layer (see {@link ChecksStorageUpdate}).
+ *
+ * <p>This class contains additional business logic for updating checks which is independent of the
+ * used storage layer (e.g. sending email notifications).
+ */
+public class ChecksUpdate {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- Check updateCheck(CheckKey key, CheckUpdate checkUpdate) throws IOException;
+ interface Factory {
+ ChecksUpdate create(IdentifiedUser currentUser);
+
+ ChecksUpdate createWithServerIdent();
+ }
+
+ private final CombinedCheckStateCache combinedCheckStateCache;
+ private final CombinedCheckStateUpdatedSender.Factory combinedCheckStateUpdatedSenderFactory;
+ private final ChangeNotes.Factory notesFactory;
+ private final PatchSetUtil psUtil;
+ private final Optional<IdentifiedUser> currentUser;
+ private final ChecksStorageUpdate checksStorageUpdate;
+
+ @AssistedInject
+ ChecksUpdate(
+ @UserInitiated ChecksStorageUpdate checksStorageUpdate,
+ CombinedCheckStateCache combinedCheckStateCache,
+ CombinedCheckStateUpdatedSender.Factory combinedCheckStateUpdatedSenderFactory,
+ ChangeNotes.Factory notesFactory,
+ PatchSetUtil psUtil,
+ @Assisted IdentifiedUser currentUser) {
+ this.combinedCheckStateCache = combinedCheckStateCache;
+ this.combinedCheckStateUpdatedSenderFactory = combinedCheckStateUpdatedSenderFactory;
+ this.notesFactory = notesFactory;
+ this.psUtil = psUtil;
+ this.currentUser = Optional.of(currentUser);
+ this.checksStorageUpdate = checksStorageUpdate;
+ }
+
+ @AssistedInject
+ ChecksUpdate(
+ @ServerInitiated ChecksStorageUpdate checksStorageUpdate,
+ CombinedCheckStateCache combinedCheckStateCache,
+ CombinedCheckStateUpdatedSender.Factory combinedCheckStateUpdatedSenderFactory,
+ ChangeNotes.Factory notesFactory,
+ PatchSetUtil psUtil) {
+ this.combinedCheckStateCache = combinedCheckStateCache;
+ this.combinedCheckStateUpdatedSenderFactory = combinedCheckStateUpdatedSenderFactory;
+ this.notesFactory = notesFactory;
+ this.psUtil = psUtil;
+ this.currentUser = Optional.empty();
+ this.checksStorageUpdate = checksStorageUpdate;
+ }
+
+ public Check createCheck(CheckKey key, CheckUpdate checkUpdate)
+ throws DuplicateKeyException, IOException {
+ CombinedCheckState oldCombinedCheckState =
+ combinedCheckStateCache.get(key.repository(), key.patchSet());
+
+ Check check = checksStorageUpdate.createCheck(key, checkUpdate);
+
+ CombinedCheckState newCombinedCheckState =
+ combinedCheckStateCache.get(key.repository(), key.patchSet());
+ if (oldCombinedCheckState != newCombinedCheckState) {
+ sendEmail(key, newCombinedCheckState);
+ }
+
+ return check;
+ }
+
+ public Check updateCheck(CheckKey key, CheckUpdate checkUpdate) throws IOException {
+ CombinedCheckState oldCombinedCheckState =
+ combinedCheckStateCache.get(key.repository(), key.patchSet());
+
+ Check check = checksStorageUpdate.updateCheck(key, checkUpdate);
+
+ CombinedCheckState newCombinedCheckState =
+ combinedCheckStateCache.get(key.repository(), key.patchSet());
+ if (oldCombinedCheckState != newCombinedCheckState) {
+ sendEmail(key, newCombinedCheckState);
+ }
+
+ return check;
+ }
+
+ private void sendEmail(CheckKey checkKey, CombinedCheckState combinedCheckState) {
+ try {
+ CombinedCheckStateUpdatedSender sender =
+ combinedCheckStateUpdatedSenderFactory.create(
+ checkKey.repository(), checkKey.patchSet().changeId());
+
+ if (currentUser.isPresent()) {
+ 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(combinedCheckState);
+ sender.send();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Cannot email update for change %s", checkKey.patchSet().changeId());
+ }
+ }
}
diff --git a/java/com/google/gerrit/plugins/checks/Module.java b/java/com/google/gerrit/plugins/checks/Module.java
index 140497b..3431ec1 100644
--- a/java/com/google/gerrit/plugins/checks/Module.java
+++ b/java/com/google/gerrit/plugins/checks/Module.java
@@ -25,8 +25,12 @@
import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory.GetChangeOptions;
import com.google.gerrit.plugins.checks.api.ChangeCheckAttributeFactory.QueryChangesOptions;
import com.google.gerrit.plugins.checks.db.NoteDbCheckersModule;
+import com.google.gerrit.plugins.checks.email.ChecksEmailModule;
import com.google.gerrit.plugins.checks.rules.ChecksSubmitRule;
import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.UserInitiated;
import com.google.gerrit.server.change.ChangeAttributeFactory;
import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.git.validators.CommitValidationListener;
@@ -35,11 +39,13 @@
import com.google.gerrit.server.restapi.change.GetChange;
import com.google.gerrit.server.restapi.change.QueryChanges;
import com.google.gerrit.sshd.commands.Query;
+import com.google.inject.Provides;
public class Module extends FactoryModule {
@Override
protected void configure() {
factory(CheckJson.AssistedFactory.class);
+ factory(ChecksUpdate.Factory.class);
install(new NoteDbCheckersModule());
install(CombinedCheckStateCache.module());
@@ -74,5 +80,19 @@
install(new ApiModule());
install(new ChecksSubmitRule.Module());
+ install(new ChecksEmailModule());
+ }
+
+ @Provides
+ @ServerInitiated
+ ChecksUpdate provideServerInitiatedChecksUpdate(ChecksUpdate.Factory factory) {
+ return factory.createWithServerIdent();
+ }
+
+ @Provides
+ @UserInitiated
+ ChecksUpdate provideUserInitiatedChecksUpdate(
+ ChecksUpdate.Factory factory, IdentifiedUser currentUser) {
+ return factory.create(currentUser);
}
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersModule.java b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersModule.java
index 39e8d7c..efc278b 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersModule.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbCheckersModule.java
@@ -18,7 +18,7 @@
import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.CheckersUpdate;
import com.google.gerrit.plugins.checks.Checks;
-import com.google.gerrit.plugins.checks.ChecksUpdate;
+import com.google.gerrit.plugins.checks.ChecksStorageUpdate;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.UserInitiated;
@@ -51,13 +51,13 @@
@Provides
@ServerInitiated
- ChecksUpdate provideServerInitiatedChecksUpdate(NoteDbChecksUpdate.Factory factory) {
+ ChecksStorageUpdate provideServerInitiatedChecksUpdate(NoteDbChecksUpdate.Factory factory) {
return factory.createWithServerIdent();
}
@Provides
@UserInitiated
- ChecksUpdate provideUserInitiatedChecksUpdate(
+ ChecksStorageUpdate provideUserInitiatedChecksUpdate(
NoteDbChecksUpdate.Factory factory, IdentifiedUser currentUser) {
return factory.create(currentUser);
}
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
index 980a1fe..467a7bd 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecksUpdate.java
@@ -29,7 +29,7 @@
import com.google.gerrit.plugins.checks.CheckerRef;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
-import com.google.gerrit.plugins.checks.ChecksUpdate;
+import com.google.gerrit.plugins.checks.ChecksStorageUpdate;
import com.google.gerrit.plugins.checks.CombinedCheckStateCache;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -56,7 +56,7 @@
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevWalk;
-public class NoteDbChecksUpdate implements ChecksUpdate {
+public class NoteDbChecksUpdate implements ChecksStorageUpdate {
interface Factory {
NoteDbChecksUpdate create(IdentifiedUser currentUser);
diff --git a/java/com/google/gerrit/plugins/checks/email/ChecksEmailModule.java b/java/com/google/gerrit/plugins/checks/email/ChecksEmailModule.java
new file mode 100644
index 0000000..3b4e016
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/email/ChecksEmailModule.java
@@ -0,0 +1,32 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.checks.email;
+
+import static com.google.inject.Scopes.SINGLETON;
+
+import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.mail.send.MailSoyTemplateProvider;
+
+public class ChecksEmailModule extends FactoryModule {
+ @Override
+ protected void configure() {
+ DynamicSet.bind(binder(), MailSoyTemplateProvider.class)
+ .to(ChecksMailSoyTemplateProvider.class)
+ .in(SINGLETON);
+
+ factory(CombinedCheckStateUpdatedSender.Factory.class);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/email/ChecksMailSoyTemplateProvider.java b/java/com/google/gerrit/plugins/checks/email/ChecksMailSoyTemplateProvider.java
new file mode 100644
index 0000000..6bcfe08
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/email/ChecksMailSoyTemplateProvider.java
@@ -0,0 +1,33 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.checks.email;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.server.mail.send.MailSoyTemplateProvider;
+import com.google.inject.Singleton;
+import java.util.Set;
+
+@Singleton
+public class ChecksMailSoyTemplateProvider implements MailSoyTemplateProvider {
+ @Override
+ public String getPath() {
+ return "com/google/gerrit/plugins/checks/email/";
+ }
+
+ @Override
+ public Set<String> getFileNames() {
+ return ImmutableSet.of("CombinedCheckStateUpdated.soy", "CombinedCheckStateUpdatedHtml.soy");
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdatedSender.java b/java/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdatedSender.java
new file mode 100644
index 0000000..560e97b
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdatedSender.java
@@ -0,0 +1,78 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.checks.email;
+
+import com.google.gerrit.exceptions.EmailException;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.account.ProjectWatches.NotifyType;
+import com.google.gerrit.server.mail.send.ChangeEmail;
+import com.google.gerrit.server.mail.send.EmailArguments;
+import com.google.gerrit.server.mail.send.ReplyToChangeSender;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+
+/** Send notice about an update of the combined check state of a change. */
+public class CombinedCheckStateUpdatedSender extends ReplyToChangeSender {
+ public interface Factory extends ReplyToChangeSender.Factory<CombinedCheckStateUpdatedSender> {
+ @Override
+ CombinedCheckStateUpdatedSender create(Project.NameKey project, Change.Id changeId);
+ }
+
+ private CombinedCheckState combinedCheckState;
+
+ @Inject
+ public CombinedCheckStateUpdatedSender(
+ EmailArguments args, @Assisted Project.NameKey project, @Assisted Change.Id changeId) {
+ super(args, "combinedCheckStateUpdate", ChangeEmail.newChangeData(args, project, changeId));
+ }
+
+ @Override
+ protected void init() throws EmailException {
+ super.init();
+
+ ccAllApprovals();
+ bccStarredBy();
+ includeWatchers(NotifyType.ALL_COMMENTS);
+ removeUsersThatIgnoredTheChange();
+ }
+
+ public void setCombinedCheckState(CombinedCheckState combinedCheckState) {
+ this.combinedCheckState = combinedCheckState;
+ }
+
+ @Override
+ protected void setupSoyContext() {
+ super.setupSoyContext();
+
+ if (combinedCheckState != null) {
+ soyContext.put("combinedCheckState", combinedCheckState.name());
+ }
+ }
+
+ @Override
+ protected void formatChange() throws EmailException {
+ appendText(textTemplate("CombinedCheckStateUpdated"));
+ if (useHtml()) {
+ appendHtml(soyHtmlTemplate("CombinedCheckStateUpdatedHtml"));
+ }
+ }
+
+ @Override
+ protected boolean supportsHtml() {
+ return true;
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
index a00c2cd..7bbf3ee 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChangeCheckInfoIT.java
@@ -186,14 +186,14 @@
checksApiFactory.revision(psId).id(checkerUuid).update(checkInput);
// Incurs reload after updating check state.
- assertThat(cache.getStats()).since(start).hasHitCount(2);
+ assertThat(cache.getStats()).since(start).hasHitCount(4);
assertThat(cache.getStats()).since(start).hasMissCount(0);
assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
assertThat(queryChangeCheckInfo(changeId))
.hasValue(new ChangeCheckInfo("checks", CombinedCheckState.WARNING));
- assertThat(cache.getStats()).since(start).hasHitCount(3);
+ assertThat(cache.getStats()).since(start).hasHitCount(5);
assertThat(cache.getStats()).since(start).hasMissCount(0);
assertThat(cache.getReloadCount(false) - startReloadsFalse).isEqualTo(0);
assertThat(cache.getReloadCount(true) - startReloadsTrue).isEqualTo(1);
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
new file mode 100644
index 0000000..8723614
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/ChecksEmailIT.java
@@ -0,0 +1,241 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.checks.acceptance.api;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+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.client.ProjectWatchInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.plugins.checks.CheckKey;
+import com.google.gerrit.plugins.checks.CheckerUuid;
+import com.google.gerrit.plugins.checks.acceptance.AbstractCheckersTest;
+import com.google.gerrit.plugins.checks.api.ChangeCheckInfo;
+import com.google.gerrit.plugins.checks.api.CheckInput;
+import com.google.gerrit.plugins.checks.api.CheckState;
+import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.testing.FakeEmailSender.Message;
+import com.google.inject.Inject;
+import java.util.List;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ChecksEmailIT extends AbstractCheckersTest {
+ @Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private GroupOperations groupOperations;
+ @Inject private ProjectOperations projectOperations;
+
+ private TestAccount bot;
+ private CheckerUuid checkerUuid1;
+ private CheckerUuid checkerUuid2;
+ private TestAccount owner;
+ private TestAccount reviewer;
+ private TestAccount starrer;
+ private TestAccount watcher;
+ private PatchSet.Id patchSetId;
+
+ @Before
+ public void setup() throws Exception {
+ // Create a bot account, create a bots group and add the bot as member and allow the bots group
+ // to post checks.
+ bot = accountCreator.create("bot", "bot@test.com", "Bot");
+ AccountGroup.UUID botsAccountGroupUuid =
+ groupOperations.newGroup().name("bots").addMember(bot.id()).create();
+ projectOperations
+ .project(allProjects)
+ .forUpdate()
+ .add(allowCapability("checks-administrateCheckers").group(botsAccountGroupUuid))
+ .update();
+
+ // Create two required checkers.
+ checkerUuid1 = checkerOperations.newChecker().repository(project).required().create();
+ checkerUuid2 = checkerOperations.newChecker().repository(project).required().create();
+
+ // Create a change.
+ owner = admin;
+ patchSetId = createChange().getPatchSetId();
+
+ // Add a reviewer.
+ reviewer = accountCreator.create("reviewer", "reviewer@test.com", "Reviewer");
+ gApi.changes().id(patchSetId.changeId().get()).addReviewer(reviewer.username());
+
+ // Star the change from some user.
+ starrer = accountCreator.create("starred", "starrer@test.com", "Starrer");
+ requestScopeOperations.setApiUser(starrer.id());
+ gApi.accounts().self().starChange(patchSetId.changeId().toString());
+
+ // Watch all comments of change from some user.
+ watcher = accountCreator.create("watcher", "watcher@test.com", "Watcher");
+ requestScopeOperations.setApiUser(watcher.id());
+ ProjectWatchInfo projectWatchInfo = new ProjectWatchInfo();
+ projectWatchInfo.project = project.get();
+ projectWatchInfo.filter = "*";
+ projectWatchInfo.notifyAllComments = true;
+ gApi.accounts().self().setWatchedProjects(ImmutableList.of(projectWatchInfo));
+
+ // Watch only change creations from some user --> user doesn't get notified by checks plugin.
+ TestAccount changeCreationWatcher =
+ accountCreator.create(
+ "changeCreationWatcher", "changeCreationWatcher@test.com", "Change Creation Watcher");
+ requestScopeOperations.setApiUser(changeCreationWatcher.id());
+ projectWatchInfo = new ProjectWatchInfo();
+ projectWatchInfo.project = project.get();
+ projectWatchInfo.filter = "*";
+ projectWatchInfo.notifyNewChanges = true;
+ gApi.accounts().self().setWatchedProjects(ImmutableList.of(projectWatchInfo));
+
+ // Add a reviewer that ignores the change --> user doesn't get notified by checks plugin.
+ TestAccount ignorer = accountCreator.create("ignorer", "ignorer@test.com", "Ignorer");
+ requestScopeOperations.setApiUser(admin.id());
+ gApi.changes().id(patchSetId.changeId().get()).addReviewer(ignorer.username());
+ requestScopeOperations.setApiUser(ignorer.id());
+ gApi.changes().id(patchSetId.changeId().get()).ignore(true);
+
+ // Reset request scope to admin.
+ requestScopeOperations.setApiUser(admin.id());
+ }
+
+ @Test
+ public void combinedCheckUpdatedEmailAfterCheckCreation() throws Exception {
+ 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();
+ input.checkerUuid = checkerUuid1.get();
+ input.state = CheckState.FAILED;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ // Except one email because the combined check state was updated.
+ 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.FAILED);
+ assertThat(message.rcpt())
+ .containsExactly(
+ owner.getEmailAddress(),
+ reviewer.getEmailAddress(),
+ starrer.getEmailAddress(),
+ watcher.getEmailAddress());
+ }
+
+ @Test
+ public void noCombinedCheckUpdatedEmailOnCheckCreationIfCombinedCheckStateIsNotChanged()
+ throws Exception {
+ // Create a check that sets the combined check state to FAILED.
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid1);
+ checkOperations.newCheck(checkKey).state(CheckState.FAILED).upsert();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ sender.clear();
+
+ // Post a new check that doesn't change the combined check state..
+ requestScopeOperations.setApiUser(bot.id());
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid2.get();
+ input.state = CheckState.SCHEDULED;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ // Except that no email was sent because the combined check state was not updated.
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
+ @Test
+ public void combinedCheckUpdatedEmailAfterCheckUpdate() throws Exception {
+ // Create a check that sets the combined check state to FAILED.
+ CheckKey checkKey = CheckKey.create(project, patchSetId, checkerUuid1);
+ 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 = checkerUuid1.get();
+ input.state = CheckState.RUNNING;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.IN_PROGRESS);
+
+ // Except one email because the combined check state was updated.
+ 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.IN_PROGRESS);
+ assertThat(message.rcpt())
+ .containsExactly(
+ owner.getEmailAddress(),
+ reviewer.getEmailAddress(),
+ starrer.getEmailAddress(),
+ watcher.getEmailAddress());
+ }
+
+ @Test
+ public void noCombinedCheckUpdatedEmailOnCheckUpdateIfCombinedCheckStateIsNotChanged()
+ throws Exception {
+ // Create 2 checks that set the combined check state to FAILED.
+ CheckKey checkKey1 = CheckKey.create(project, patchSetId, checkerUuid1);
+ checkOperations.newCheck(checkKey1).state(CheckState.FAILED).upsert();
+ CheckKey checkKey2 = CheckKey.create(project, patchSetId, checkerUuid2);
+ checkOperations.newCheck(checkKey2).state(CheckState.FAILED).upsert();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ sender.clear();
+
+ // Update one of the checks in a way so that doesn't change the combined check state..
+ requestScopeOperations.setApiUser(bot.id());
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid2.get();
+ input.state = CheckState.SCHEDULED;
+ checksApiFactory.revision(patchSetId).create(input).get();
+ assertThat(getCombinedCheckState()).isEqualTo(CombinedCheckState.FAILED);
+
+ // Except that no email was sent because the combined check state was not updated.
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
+ private CombinedCheckState getCombinedCheckState() throws RestApiException {
+ ChangeInfo changeInfo =
+ gApi.changes()
+ .id(patchSetId.changeId().get())
+ .get(ImmutableListMultimap.of("checks--combined", "true"));
+ ImmutableList<PluginDefinedInfo> infos =
+ changeInfo.plugins.stream().filter(i -> i.name.equals("checks")).collect(toImmutableList());
+ assertThat(infos).hasSize(1);
+ assertThat(infos.get(0)).isInstanceOf(ChangeCheckInfo.class);
+ return ((ChangeCheckInfo) infos.get(0)).combinedState;
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
index 6458099..1948adb 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/testsuite/CheckOperationsImplTest.java
@@ -41,7 +41,6 @@
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Optional;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
@@ -91,22 +90,27 @@
CheckKey.create(
Project.nameKey("non-existing"), createChange().getPatchSetId(), checkerUuid);
- IllegalStateException thrown =
- assertThrows(
- IllegalStateException.class, () -> checkOperations.newCheck(checkKey).upsert());
- assertThat(thrown.getCause(), instanceOf(RepositoryNotFoundException.class));
+ assertThrows(IllegalStateException.class, () -> checkOperations.newCheck(checkKey).upsert());
+ }
+
+ @Test
+ public void checkCannotBeCreatedForNonExistingChange() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().create();
+ CheckKey checkKey = CheckKey.create(project, PatchSet.id(Change.id(1), 1), checkerUuid);
+
+ assertThrows(IllegalStateException.class, () -> checkOperations.newCheck(checkKey).upsert());
}
@Test
public void checkCannotBeCreatedForNonExistingPatchSet() throws Exception {
+ Change.Id changeId = createChange().getChange().getId();
CheckerUuid checkerUuid = checkerOperations.newChecker().create();
- CheckKey checkKey = CheckKey.create(project, PatchSet.id(Change.id(1), 1), checkerUuid);
+ CheckKey checkKey = CheckKey.create(project, PatchSet.id(changeId, 99), checkerUuid);
IllegalStateException thrown =
assertThrows(
IllegalStateException.class, () -> checkOperations.newCheck(checkKey).upsert());
- assertThat(thrown.getCause(), instanceOf(IOException.class));
- assertThat(thrown).hasMessageThat().contains("patchset 1,1 not found");
+ assertThat(thrown).hasMessageThat().contains("patch set not found: " + changeId.get() + ",99");
}
@Test
diff --git a/resources/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdated.soy b/resources/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdated.soy
new file mode 100644
index 0000000..83d67b1
--- /dev/null
+++ b/resources/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdated.soy
@@ -0,0 +1,41 @@
+/**
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+{namespace com.google.gerrit.server.mail.template}
+
+/**
+ * The .CombinedCheckStateUpdated template will determine the contents of the email related to a
+ * change for which the combined check state was updated.
+ */
+{template .CombinedCheckStateUpdated kind="text"}
+ {@param change: ?}
+ {@param combinedCheckState: ?}
+ {@param coverLetter: ?}
+ {@param email: ?}
+ {@param patchSet: ?}
+ The combined check state has been updated to {$combinedCheckState} for patch set
+ {$patchSet.patchSetId} of this change.
+ {if $email.changeUrl} ( {$email.changeUrl} ){/if}{\n}
+ {\n}
+ Change subject: {$change.subject}{\n}
+ ......................................................................{\n}
+ {if $coverLetter}
+ {\n}
+ {\n}
+ {$coverLetter}
+ {\n}
+ {/if}
+{/template}
diff --git a/resources/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdatedHtml.soy b/resources/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdatedHtml.soy
new file mode 100644
index 0000000..8e7e5ff
--- /dev/null
+++ b/resources/com/google/gerrit/plugins/checks/email/CombinedCheckStateUpdatedHtml.soy
@@ -0,0 +1,42 @@
+/**
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+{namespace com.google.gerrit.server.mail.template}
+
+/**
+ * The .CombinedCheckStateUpdatedHtml template will determine the contents of the email related to
+ * a change for which the combined check state was updated.
+ */
+{template .CombinedCheckStateUpdatedHtml}
+ {@param combinedCheckState: ?}
+ {@param coverLetter: ?}
+ {@param email: ?}
+ {@param patchSet: ?}
+ <p>
+ The combined check state has been updated to <strong>{$combinedCheckState}</strong> for patch
+ set {$patchSet.patchSetId} of this change.
+ </p>
+
+ {if $email.changeUrl}
+ <p>
+ {call .ViewChangeButton data="all" /}
+ </p>
+ {/if}
+
+ {if $coverLetter}
+ <div style="white-space:pre-wrap">{$coverLetter}</div>
+ {/if}
+{/template}
