diff --git a/core/src/main/java/google/registry/bsa/BsaDownloadAction.java b/core/src/main/java/google/registry/bsa/BsaDownloadAction.java index afd5e3e08..4e25e894f 100644 --- a/core/src/main/java/google/registry/bsa/BsaDownloadAction.java +++ b/core/src/main/java/google/registry/bsa/BsaDownloadAction.java @@ -14,6 +14,7 @@ package google.registry.bsa; +import static com.google.common.base.Throwables.getStackTraceAsString; import static google.registry.bsa.BlockListType.BLOCK; import static google.registry.bsa.BlockListType.BLOCK_PLUS; import static google.registry.bsa.api.JsonSerializations.toCompletedOrdersReport; @@ -65,6 +66,7 @@ public class BsaDownloadAction implements Runnable { private final BsaReportSender bsaReportSender; private final GcsClient gcsClient; private final Lazy lazyIdnChecker; + private final BsaEmailSender emailSender; private final BsaLock bsaLock; private final Clock clock; private final int transactionBatchSize; @@ -78,6 +80,7 @@ public class BsaDownloadAction implements Runnable { BsaReportSender bsaReportSender, GcsClient gcsClient, Lazy lazyIdnChecker, + BsaEmailSender emailSender, BsaLock bsaLock, Clock clock, @Config("bsaTxnBatchSize") int transactionBatchSize, @@ -88,6 +91,7 @@ public class BsaDownloadAction implements Runnable { this.bsaReportSender = bsaReportSender; this.gcsClient = gcsClient; this.lazyIdnChecker = lazyIdnChecker; + this.emailSender = emailSender; this.bsaLock = bsaLock; this.clock = clock; this.transactionBatchSize = transactionBatchSize; @@ -98,12 +102,13 @@ public class BsaDownloadAction implements Runnable { public void run() { try { if (!bsaLock.executeWithLock(this::runWithinLock)) { - logger.atInfo().log("Job is being executed by another worker."); + String message = "BSA download did not run: another BSA related task is running"; + logger.atInfo().log("%s.", message); + emailSender.sendNotification(message, /* body= */ ""); } } catch (Throwable throwable) { - // TODO(12/31/2023): consider sending an alert email. - // TODO: if unretriable errors, log at severe and send email. - logger.atWarning().withCause(throwable).log("Failed to update block lists."); + logger.atWarning().withCause(throwable).log("Failed to download and process BSA data."); + emailSender.sendNotification("BSA download aborted", getStackTraceAsString(throwable)); } // Always return OK. Let the next cron job retry. response.setStatus(SC_OK); diff --git a/core/src/main/java/google/registry/bsa/BsaRefreshAction.java b/core/src/main/java/google/registry/bsa/BsaRefreshAction.java index 0222bd6c1..d79e7eb74 100644 --- a/core/src/main/java/google/registry/bsa/BsaRefreshAction.java +++ b/core/src/main/java/google/registry/bsa/BsaRefreshAction.java @@ -90,8 +90,6 @@ public class BsaRefreshAction implements Runnable { String message = "BSA refresh did not run: another BSA related task is running"; logger.atInfo().log("%s.", message); emailSender.sendNotification(message, /* body= */ ""); - } else { - emailSender.sendNotification("BSA refreshed successfully", ""); } } catch (Throwable throwable) { logger.atWarning().withCause(throwable).log("Failed to refresh BSA data."); diff --git a/core/src/main/java/google/registry/bsa/BsaValidateAction.java b/core/src/main/java/google/registry/bsa/BsaValidateAction.java index e93f636fd..6df8da570 100644 --- a/core/src/main/java/google/registry/bsa/BsaValidateAction.java +++ b/core/src/main/java/google/registry/bsa/BsaValidateAction.java @@ -132,9 +132,10 @@ public class BsaValidateAction implements Runnable { errors.isEmpty() ? "BSA validation completed: no errors found" : "BSA validation completed with errors"; - - emailValidationResults(resultSummary, downloadJobName.get(), errors); - logger.atInfo().log("Finished validating BSA with latest download: %s", downloadJobName.get()); + if (!errors.isEmpty()) { + emailValidationResults(resultSummary, downloadJobName.get(), errors); + } + logger.atInfo().log("%s (latest download: %s)", resultSummary, downloadJobName.get()); return null; } diff --git a/core/src/test/java/google/registry/bsa/BsaDownloadFunctionalTest.java b/core/src/test/java/google/registry/bsa/BsaDownloadFunctionalTest.java index 0021f82b5..a8baa2587 100644 --- a/core/src/test/java/google/registry/bsa/BsaDownloadFunctionalTest.java +++ b/core/src/test/java/google/registry/bsa/BsaDownloadFunctionalTest.java @@ -66,6 +66,8 @@ class BsaDownloadFunctionalTest { @Mock BlockListFetcher blockListFetcher; @Mock BsaReportSender bsaReportSender; + @Mock BsaEmailSender bsaEmailSender; + private final FakeClock fakeClock = new FakeClock(TEST_START_TIME); @RegisterExtension @@ -95,6 +97,7 @@ class BsaDownloadFunctionalTest { bsaReportSender, gcsClient, () -> new IdnChecker(fakeClock), + bsaEmailSender, new BsaLock( new FakeLockHandler(/* lockSucceeds= */ true), Duration.standardSeconds(30)), fakeClock, diff --git a/core/src/test/java/google/registry/bsa/BsaRefreshActionTest.java b/core/src/test/java/google/registry/bsa/BsaRefreshActionTest.java index 4cce0b74a..03fb95cfe 100644 --- a/core/src/test/java/google/registry/bsa/BsaRefreshActionTest.java +++ b/core/src/test/java/google/registry/bsa/BsaRefreshActionTest.java @@ -16,6 +16,7 @@ package google.registry.bsa; import static com.google.common.base.Throwables.getStackTraceAsString; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -96,14 +97,13 @@ public class BsaRefreshActionTest { } @Test - void notificationSent_success() { + void notification_notSent_whenNoError() { when(bsaLock.executeWithLock(any())) .thenAnswer( args -> { return true; }); action.run(); - verify(gmailClient, times(1)) - .sendEmail(EmailMessage.create("BSA refreshed successfully", "", emailRecipient)); + verify(gmailClient, never()).sendEmail(any()); } } diff --git a/core/src/test/java/google/registry/bsa/BsaRefreshFunctionalTest.java b/core/src/test/java/google/registry/bsa/BsaRefreshFunctionalTest.java index 2119b790f..7638e65a9 100644 --- a/core/src/test/java/google/registry/bsa/BsaRefreshFunctionalTest.java +++ b/core/src/test/java/google/registry/bsa/BsaRefreshFunctionalTest.java @@ -149,7 +149,7 @@ class BsaRefreshFunctionalTest { verify(bsaReportSender, times(1)) .addUnblockableDomainsUpdates("{\n \"reserved\": [\n \"blocked1.app\"\n ]\n}"); - verify(emailSender, times(1)).sendNotification("BSA refreshed successfully", ""); + verify(emailSender, never()).sendNotification(anyString(), anyString()); } @Test diff --git a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java index e5251171f..5bea623c9 100644 --- a/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java +++ b/core/src/test/java/google/registry/bsa/BsaValidateActionTest.java @@ -26,6 +26,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -345,7 +346,7 @@ public class BsaValidateActionTest { } @Test - void notificationSent_noError() { + void notification_notSent_WhenNoError() { when(bsaLock.executeWithLock(any())) .thenAnswer( args -> { @@ -356,9 +357,6 @@ public class BsaValidateActionTest { action = spy(action); doReturn(ImmutableList.of()).when(action).checkBsaLabels(anyString()); action.run(); - verify(gmailClient, times(1)).sendEmail(emailCaptor.capture()); - EmailMessage message = emailCaptor.getValue(); - assertThat(message.subject()).isEqualTo("BSA validation completed: no errors found"); - assertThat(message.body()).isEqualTo("Most recent download is 2023-11-09t020857.880z.\n\n"); + verify(gmailClient, never()).sendEmail(any()); } }