Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat SES event-based email sendouts #1530

Merged
merged 20 commits into from
Oct 1, 2023
Merged

Feat SES event-based email sendouts #1530

merged 20 commits into from
Oct 1, 2023

Conversation

tchalvak
Copy link
Member

@tchalvak tchalvak commented Sep 25, 2023

Purpose of PR:

--- Sending out emails via AWS SES via the conduit of EventBridge

  • Email generated and sent (locally, but hooked into the AWS account eventbridge for testing purposes).
  • It goes from: php code -> NMail php class -> send method -> aws-php-sdk -> eventbridge -> lambda -> SES
  • And then in to the magic of the email system.

Fixes: #1230

End Result in my gmail:
image

Before

We were using a system based on exim4 on the server, and also relying on a phpmailer library in php that became deprecated and then removed.

After

Now we hook right in to SES via events. We could probably eventually use SES templates and other such things.

For Non-Hotfixes:

Attached Screenshot of my change:

image

image

Things that make review take longer:

(remove lines that do not apply to this PR)

  • Changing more than 20 files (much harder to review)
  • Changing more than 5 files (a bit harder to review)
  • Changes to critical code (login, dashboard, etc)
  • No comments on changed files (unknown)
  • Tests do not pass (will get pushed back)

Things that make review faster and easier:

(check box with an x where it applies)

  • I attached a screenshot of the changed part of the app working
  • I added tests
  • This feature is requested specifically by a user
  • This will fix a bug

Preview results in my branch at the url:

  • Add aws access to send through SES? You probably don't want to do that, so just review the tests and check that the new ones make sense, and if you're ambitious, run the unit tests yourself locally via:
  • :/src/ninjawars$ ./vendor/bin/phpunit-watcher watch /src/ninjawars/deploy/lib/events/PutEventTest.php
  • You can run a signup at https://localhost:8765/signup , but of course that'll stop at the attempt to send an event to aws which will fail if you don't have it hooked in to an aws account.

tchalvak and others added 20 commits September 24, 2023 18:50
This commit fixes the style issues introduced in 6276d1c according to the output
from PHP CS Fixer.

Details: #1530
This commit fixes the style issues introduced in 1feb906 according to the output
from PHP CS Fixer.

Details: #1530
This commit fixes the style issues introduced in 9465868 according to the output
from PHP CS Fixer.

Details: #1530
a future note that to get expected overriding, have to use the initial array
as the second entry in the plus operator + chain, strangely.
This commit fixes the style issues introduced in 1501583 according to the output
from PHP CS Fixer.

Details: #1530
@tchalvak
Copy link
Member Author

@Beagle Feel like giving a PR review? : D

@tchalvak tchalvak mentioned this pull request Sep 27, 2023
8 tasks
@tchalvak
Copy link
Member Author

tchalvak commented Oct 1, 2023

Ah, need to run with some of this functionality, but do check through @Beagle and give review and I’ll follow up.

@tchalvak tchalvak merged commit 8517fd9 into master Oct 1, 2023
9 checks passed
@tchalvak tchalvak deleted the feat/email branch October 1, 2023 17:07
list($email, $display) = is_array($email_complex) ?
[array_key_first($email_complex), reset($email_complex)] :
[$email_complex, null];
$sanitized_email = filter_var($email, FILTER_SANITIZE_EMAIL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double check this against valid email addresses that have special characters. As an example, ' is a valid email character so you should be able to send to seamus.o'[email protected]

}

/**
* Email config validation, which should perhaps eventually be moved away from event lib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is config? Saying "validateEmailIncomingConfig provides email config validation" is not a useful comment. Perhaps: "This validates email header information includes from, to, subject, etc"

return null;
}

function generateEmailEvent(array $config): array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Returns a dictionary with the keys and data required to submit an event to the event bus"

@@ -79,47 +96,23 @@ public function setReplyTo($email_or_array)
*
* @return boolean whether the mail function accepted the inputs.
*/
public function send()
public function send($debug_override = false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document the intention of debug_override

@Beagle
Copy link
Member

Beagle commented Oct 4, 2023

Super nice. A great bridgehead into getting an EDA in place. Nice job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Critical! Error with signup system due to dependency upgrade.
2 participants