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: send emails via http api endpoint instead of smtp #3048

Closed
wants to merge 8 commits into from

Conversation

barnarddt
Copy link

@barnarddt barnarddt commented Jan 26, 2023

This change adds a new delivery method to the courier called mailer. Similar to SMS functionality it posts a templated Data model to a API endpoint. This API can then send emails via a CRM or any other mechanism that it wants.

Mailer still uses the existing email data models so any new email added will automatically be sent to the API/CRM as well.

Related issue(s)

Resolves #2825
Also see #1030 and #3008
Documentation PR ory/docs#1298

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Merging #3048 (3290032) into master (27ccecc) will decrease coverage by 1.02%.
The diff coverage is 69.23%.

❗ Current head 3290032 differs from pull request most recent head 8f0ac6a. Consider uploading reports for the commit 8f0ac6a to get more accurate results

@@            Coverage Diff             @@
##           master    #3048      +/-   ##
==========================================
- Coverage   78.12%   77.11%   -1.02%     
==========================================
  Files         324      312      -12     
  Lines       20762    19373    -1389     
==========================================
- Hits        16221    14940    -1281     
+ Misses       3333     3268      -65     
+ Partials     1208     1165      -43     
Impacted Files Coverage Δ
driver/config/config.go 82.37% <58.33%> (-0.57%) ⬇️
courier/mailer.go 66.66% <66.66%> (ø)
courier/courier.go 71.42% <100.00%> (+0.84%) ⬆️
courier/smtp.go 70.00% <100.00%> (+0.43%) ⬆️

... and 128 files with indirect coverage changes

@karaimin
Copy link

Nice work @barnarddt. I was looking for this feature for a long time. Hope it will be reviewed and released soon.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! This is already going in a good direction. Here are some improvement ideas:

  1. Add tests for the new config options (right now the config won't work because it's not in the config JSON Schema)
  2. Add an e2e tests to test/e2e which ensures that this works end-to-end
  3. Make a few changes to the configuration layout
  4. Add documentation :)

@@ -0,0 +1,11 @@
function(ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a good default value for the body payload. Maybe it makes sense to use it as the default?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure how we'd make it the default as the body of the template engine is a file URI not a absolute value?

driver/config/config.go Outdated Show resolved Hide resolved
@barnarddt
Copy link
Author

2. Add an e2e tests to test/e2e which ensures that this works end-to-end

@aeneasr

For the end2end test what would be the preferred setup? Configure kratos to use the http email functionality with the url being a localhost port that the end2end test will listen on and ensure that that gets called when a admin call gets issued to do something that will trigger an email to be sent?

@SoGoDev
Copy link

SoGoDev commented Mar 11, 2023

Guys, any updates on this PR?

Copy link
Contributor

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

This already looks fantastic. And sorry for the delays on our side.

As for the E2E tests, we already have some "mock" services that just provide a target for the webhooks to be tested with, here: https://github.com/ory/kratos/blob/master/test/e2e/mock. That service is started in test/e2e/modd.conf

Maybe you could set up some kind of echo server here, that makes all incoming messages available on an endpoint and assert that the expected messages end up in there?

courier/courier.go Outdated Show resolved Hide resolved
courier/mailer.go Outdated Show resolved Hide resolved
@kmherrmann
Copy link
Contributor

Thanks @barnarddt - this is a great addition. Would you be up to address the requested changes? It'd be fantastic to get http email delivery support merged!

Resolves #2825

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Great job! :)

Could you please add an end-to-end Testcafe / Cypress test (or some other type of integration test in Go) to prove that this does indeed work as intended? Thank you!

@aeneasr
Copy link
Member

aeneasr commented May 25, 2023

  1. Add an e2e tests to test/e2e which ensures that this works end-to-end

@aeneasr

For the end2end test what would be the preferred setup? Configure kratos to use the http email functionality with the url being a localhost port that the end2end test will listen on and ensure that that gets called when a admin call gets issued to do something that will trigger an email to be sent?

Sorry - I missed this comment :(

Yes that could work! We now support PlayWright and there you can set up a mock server easily: https://playwright.dev/docs/test-webserver

So probably the test would be this:

  1. set up web server
  2. trigger registration flow
  3. ensure web server got api request with correct payloads (potentially using a snapshot)

That should be it!

@hperl hperl self-assigned this Jun 22, 2023
@hperl hperl mentioned this pull request Jun 22, 2023
7 tasks
@hperl
Copy link
Contributor

hperl commented Jun 22, 2023

@barnarddt thank you so much for your contribution! I did some clean-ups in #3341 to get this across the finish line:

  • added the config values to the schema, including a default configuration for the request_config
  • added an E2E test
  • added subject and body to the template directly

@hperl
Copy link
Contributor

hperl commented Jun 22, 2023

Closing in favor of #3341. Again, big thanks to @barnarddt for the excellent work!

@hperl hperl closed this Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email rendering & delivery via API / HTTP
8 participants