From 6b6d7fb6b4313cfa1a6788bb8c6b79f08a8b8958 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Wed, 6 Nov 2024 16:47:15 -0500 Subject: [PATCH 1/8] Add test that the abandoned-user email isn't sent too frequently --- application/features/bootstrap/EmailContext.php | 11 ++++++++++- application/features/email.feature | 10 +++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index 262c28b3..e2bdcf46 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -1243,7 +1243,7 @@ public function theUserHasNotLoggedInFor($months) } /** - * @When I send abandoned user email + * @When I send abandoned user email (again) */ public function iSendAbandonedUserEmail() { @@ -1292,4 +1292,13 @@ public function theDatabaseHasBeenPurged() User::deleteAll(); $this->fakeEmailer->forgetFakeEmailsSent(); } + + /** + * @Then the abandoned user email has been sent :expectedNumber time(s) + */ + public function theAbandonedUserEmailHasBeenSentTime($expectedCount) + { + $actualCount = $this->countEmailsSent(EmailLog::MESSAGE_TYPE_ABANDONED_USERS); + Assert::eq($actualCount, $expectedCount); + } } diff --git a/application/features/email.feature b/application/features/email.feature index b6d21892..140b28bd 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,12 @@ 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 From 1fb630242fa21439c4ea4cdbb7c38c64f179cf5b Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 12 Nov 2024 15:44:54 -0500 Subject: [PATCH 2/8] Add test that the ext-groups-sync-errors email isn't sent too frequently --- .../features/bootstrap/EmailContext.php | 30 +++++++++++++++++++ application/features/email.feature | 6 ++++ 2 files changed, 36 insertions(+) diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index e2bdcf46..149da540 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -59,6 +59,8 @@ class EmailContext extends YiiContext /** @var array */ protected $matchingFakeEmails; + private string $dummyExtGroupsAppPrefix = 'ext-dummy'; + public const METHOD_EMAIL_ADDRESS = 'method@example.com'; public const MANAGER_EMAIL = 'manager@example.com'; @@ -1301,4 +1303,32 @@ 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); + } } diff --git a/application/features/email.feature b/application/features/email.feature index 140b28bd..a5c670d8 100644 --- a/application/features/email.feature +++ b/application/features/email.feature @@ -431,3 +431,9 @@ Feature: Email 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 From 94a1177678527c278a3f768a64c6939b8bab28d6 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 12 Nov 2024 15:45:14 -0500 Subject: [PATCH 3/8] Remove unneeded `use` import in test file --- application/features/bootstrap/EmailContext.php | 1 - 1 file changed, 1 deletion(-) diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index 149da540..973b834f 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 From 7a2f5b1c96ddc889fc08cb6bd19d80bb46048b75 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 12 Nov 2024 15:45:29 -0500 Subject: [PATCH 4/8] Fix parameter name mismatch in doc. block --- application/features/bootstrap/EmailContext.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index 973b834f..e014d3fb 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -1295,7 +1295,7 @@ public function theDatabaseHasBeenPurged() } /** - * @Then the abandoned user email has been sent :expectedNumber time(s) + * @Then the abandoned user email has been sent :expectedCount time(s) */ public function theAbandonedUserEmailHasBeenSentTime($expectedCount) { From feda8c5bb3d1ef9ed5910b99d650cfbe11e80616 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 12 Nov 2024 15:54:44 -0500 Subject: [PATCH 5/8] Avoid sending ext-groups-sync-errors email too frequently --- application/common/components/Emailer.php | 68 ++++++++++++++++------- 1 file changed, 49 insertions(+), 19 deletions(-) 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', + ])); + } } /** From c70b206b7b96474dbaf260bcccd4077d38aa2076 Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 12 Nov 2024 16:18:18 -0500 Subject: [PATCH 6/8] Ensure we don't log an email as to both a User and non-user address --- application/common/models/EmailLog.php | 17 ++++++++++++ .../features/bootstrap/EmailContext.php | 26 +++++++++++++++++++ application/features/email.feature | 5 ++++ 3 files changed, 48 insertions(+) diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index 9b8a98cc..83509b14 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -118,6 +118,23 @@ public function rules() 'default', 'value' => MySqlDateTime::now(), ], + [ + 'non_user_address', + 'emptyIfUserIsSet', + ], ], parent::rules()); } + + 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 + ) + ); + } + } } diff --git a/application/features/bootstrap/EmailContext.php b/application/features/bootstrap/EmailContext.php index e014d3fb..dc331df6 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -60,6 +60,8 @@ class EmailContext extends YiiContext private string $dummyExtGroupsAppPrefix = 'ext-dummy'; + private ?EmailLog $tempEmailLog; + public const METHOD_EMAIL_ADDRESS = 'method@example.com'; public const MANAGER_EMAIL = 'manager@example.com'; @@ -1330,4 +1332,28 @@ public function theExternalGroupsSyncErrorEmailHasBeenSentTime($expectedCount) ); 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' + ); + } } diff --git a/application/features/email.feature b/application/features/email.feature index a5c670d8..582d5573 100644 --- a/application/features/email.feature +++ b/application/features/email.feature @@ -437,3 +437,8 @@ Feature: Email 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 From 75d2513726d494f7798dfa46f348902364bb404b Mon Sep 17 00:00:00 2001 From: "Matt H." Date: Tue, 12 Nov 2024 16:44:30 -0500 Subject: [PATCH 7/8] Ensure every email we log is to either a User or a non-user address --- application/common/models/EmailLog.php | 18 ++++++++++++++++++ .../features/bootstrap/EmailContext.php | 11 +++++++++++ application/features/email.feature | 4 ++++ 3 files changed, 33 insertions(+) diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index 83509b14..39e220f6 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -122,6 +122,11 @@ public function rules() 'non_user_address', 'emptyIfUserIsSet', ], + [ + 'non_user_address', + 'requiredIfUserIsEmpty', + 'skipOnEmpty' => false, + ], ], parent::rules()); } @@ -137,4 +142,17 @@ public function emptyIfUserIsSet($attribute) ); } } + + 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 dc331df6..9f8c3897 100644 --- a/application/features/bootstrap/EmailContext.php +++ b/application/features/bootstrap/EmailContext.php @@ -1356,4 +1356,15 @@ public function anEmailLogValidationErrorShouldHaveOccurred() '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 582d5573..d385669a 100644 --- a/application/features/email.feature +++ b/application/features/email.feature @@ -442,3 +442,7 @@ Feature: Email 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 From a07666377e18f9d6b39853a7e12e5163023f87da Mon Sep 17 00:00:00 2001 From: forevermatt Date: Wed, 13 Nov 2024 15:39:56 -0500 Subject: [PATCH 8/8] Add documentation to new EmailLog validator functions Co-authored-by: briskt <3172830+briskt@users.noreply.github.com> --- application/common/models/EmailLog.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/application/common/models/EmailLog.php b/application/common/models/EmailLog.php index 39e220f6..39feb630 100644 --- a/application/common/models/EmailLog.php +++ b/application/common/models/EmailLog.php @@ -130,6 +130,12 @@ public function rules() ], 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])) { @@ -143,6 +149,12 @@ public function emptyIfUserIsSet($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])) {