diff --git a/application/common/components/Emailer.php b/application/common/components/Emailer.php index f45a4011..4c0a8acb 100644 --- a/application/common/components/Emailer.php +++ b/application/common/components/Emailer.php @@ -484,6 +484,26 @@ public function shouldSendAbandonedUsersMessage(): bool return !$haveSentAbandonedUsersEmailRecently; } + /** + * Whether we should send an external-groups sync-errors email to the given + * email address. + * + * @param string $emailAddress + * @return bool + */ + public function shouldSendExternalGroupsSyncErrorsEmailTo(string $emailAddress): bool + { + if (empty($emailAddress)) { + return false; + } + + $haveSentEmailRecently = $this->hasNonUserReceivedMessageRecently( + $emailAddress, + EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS + ); + + return !$haveSentEmailRecently; + } /** * Whether we should send an invite message to the given User. @@ -819,28 +839,38 @@ public function sendExternalGroupSyncErrorsEmail( ) { $logData = [ 'action' => 'send external-groups sync errors email', - 'status' => 'starting', + 'prefix' => $appPrefix, ]; - $this->logger->info(array_merge($logData, [ - 'errors' => count($errors) - ])); - - $this->sendMessageTo( - EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS, - null, - [ - 'toAddress' => $recipient, - 'appPrefix' => $appPrefix, - 'errors' => $errors, - 'googleSheetUrl' => $googleSheetUrl, - 'idpDisplayName' => \Yii::$app->params['idpDisplayName'], - ] - ); + if (!$this->shouldSendExternalGroupsSyncErrorsEmailTo($recipient)) { + $this->logger->info(array_merge($logData, [ + 'errors' => count($errors), + 'recipient' => $recipient, + 'status' => 'skipping (too soon to resend)', + ])); + } else { + $this->logger->info(array_merge($logData, [ + 'errors' => count($errors), + 'recipient' => $recipient, + 'status' => 'starting', + ])); + + $this->sendMessageTo( + EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS, + null, + [ + 'toAddress' => $recipient, + 'appPrefix' => $appPrefix, + 'errors' => $errors, + 'googleSheetUrl' => $googleSheetUrl, + 'idpDisplayName' => \Yii::$app->params['idpDisplayName'], + ] + ); - $this->logger->info(array_merge($logData, [ - 'status' => 'finished', - ])); + $this->logger->info(array_merge($logData, [ + 'status' => 'finished', + ])); + } } /** diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index 9b8a98cc..39feb630 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -118,6 +118,53 @@ public function rules() 'default', 'value' => MySqlDateTime::now(), ], + [ + 'non_user_address', + 'emptyIfUserIsSet', + ], + [ + 'non_user_address', + 'requiredIfUserIsEmpty', + 'skipOnEmpty' => false, + ], ], parent::rules()); } + + /** + * Validate an attribute to ensure it is empty (unset) if `user_id` is set. + * + * @param string $attribute Attribute name to be validated + * @return bool + */ + public function emptyIfUserIsSet($attribute) + { + if (!empty($this->user_id) && !empty($this[$attribute])) { + $this->addError( + $attribute, + sprintf( + 'Only a user_id or a %s may be set, not both', + $attribute + ) + ); + } + } + + /** + * Validate an attribute to ensure it is not empty if `user_id` is not set. + * + * @param string $attribute Attribute name to be validated + * @return bool + */ + public function requiredIfUserIsEmpty($attribute) + { + if (empty($this->user_id) && empty($this[$attribute])) { + $this->addError( + $attribute, + sprintf( + 'Either a user_id or a %s must be set', + $attribute + ) + ); + } + } } diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index 262c28b3..9f8c3897 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -11,7 +11,6 @@ use common\models\MfaBackupcode; use common\models\MfaWebauthn; use common\models\User; -use Sil\SilIdBroker\Behat\Context\YiiContext; use Webmozart\Assert\Assert; class EmailContext extends YiiContext @@ -59,6 +58,10 @@ class EmailContext extends YiiContext /** @var array */ protected $matchingFakeEmails; + private string $dummyExtGroupsAppPrefix = 'ext-dummy'; + + private ?EmailLog $tempEmailLog; + public const METHOD_EMAIL_ADDRESS = 'method@example.com'; public const MANAGER_EMAIL = 'manager@example.com'; @@ -1243,7 +1246,7 @@ public function theUserHasNotLoggedInFor($months) } /** - * @When I send abandoned user email + * @When I send abandoned user email (again) */ public function iSendAbandonedUserEmail() { @@ -1292,4 +1295,76 @@ public function theDatabaseHasBeenPurged() User::deleteAll(); $this->fakeEmailer->forgetFakeEmailsSent(); } + + /** + * @Then the abandoned user email has been sent :expectedCount time(s) + */ + public function theAbandonedUserEmailHasBeenSentTime($expectedCount) + { + $actualCount = $this->countEmailsSent(EmailLog::MESSAGE_TYPE_ABANDONED_USERS); + Assert::eq($actualCount, $expectedCount); + } + + /** + * @Given I send an external-groups sync-error email (again) + */ + public function iSendAnExternalGroupsSyncErrorEmail() + { + $this->fakeEmailer->sendExternalGroupSyncErrorsEmail( + $this->dummyExtGroupsAppPrefix, + ['dummy error'], + 'dummy@example.com', + '' + ); + } + + /** + * @Then the external-groups sync-error email has been sent :expectedCount time(s) + */ + public function theExternalGroupsSyncErrorEmailHasBeenSentTime($expectedCount) + { + // The $appPrefix is needed by the FakeEmailer, to find the appropriate + // emails (by being able to accurately generate the expected subject). + $this->fakeEmailer->otherDataForEmails['appPrefix'] = $this->dummyExtGroupsAppPrefix; + + $actualCount = $this->countEmailsSent( + EmailLog::MESSAGE_TYPE_EXT_GROUP_SYNC_ERRORS + ); + Assert::eq($actualCount, $expectedCount); + } + + /** + * @When I try to log an email as sent to that User and to a non-user address + */ + public function iTryToLogAnEmailAsSentToThatUserAndToANonUserAddress() + { + $this->tempEmailLog = new EmailLog([ + 'message_type' => EmailLog::MESSAGE_TYPE_ABANDONED_USERS, + 'non_user_address' => 'dummy@example.com', + 'user_id' => $this->tempUser->id, + ]); + $this->tempEmailLog->save(); + } + + /** + * @Then an email log validation error should have occurred + */ + public function anEmailLogValidationErrorShouldHaveOccurred() + { + Assert::notEmpty( + $this->tempEmailLog->errors, + 'No EmailLog validation errors were found' + ); + } + + /** + * @When I try to log an email as sent to neither a User nor a non-user address + */ + public function iTryToLogAnEmailAsSentToNeitherAUserNorANonUserAddress() + { + $this->tempEmailLog = new EmailLog([ + 'message_type' => EmailLog::MESSAGE_TYPE_ABANDONED_USERS, + ]); + $this->tempEmailLog->save(); + } } diff --git a/application/features/email.feature b/application/features/email.feature index b6d21892..d385669a 100644 --- a/application/features/email.feature +++ b/application/features/email.feature @@ -404,7 +404,6 @@ Feature: Email | to send | -1 | has | should NOT | | NOT to send | -1 | has NOT | should NOT | - Scenario Outline: HR notification users Given hr notification email set And the database has been purged @@ -423,3 +422,27 @@ Feature: Email | is | an inactive | 7 months | has NOT | | is | a | 5 months | has NOT | | is | a | 7 months | has | + + Scenario: Not sending HR notification emails too frequently + Given hr notification email is set + And the database has been purged + And a user already exists + And the user has not logged in for "7 months" + And I send abandoned user email + When I send abandoned user email again + Then the abandoned user email has been sent 1 time + + Scenario: Not sending external-group sync-error emails too frequently + Given the database has been purged + And I send an external-groups sync-error email + When I send an external-groups sync-error email again + Then the external-groups sync-error email has been sent 1 time + + Scenario: Ensure no EmailLog is to both a User and a non-user address + Given a user already exists + When I try to log an email as sent to that User and to a non-user address + Then an email log validation error should have occurred + + Scenario: Ensure each EmailLog is to either a User or a non-user address + When I try to log an email as sent to neither a User nor a non-user address + Then an email log validation error should have occurred