-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add API endpoints to SentEmailViewerPlug #456
Conversation
@tomtaylor Interesting! Can you share an example of how you are thinking to use this? Is this for integration testing email sending? Would it be possible to achieve this using helpers from Bamboo.Test? |
That’s right. I use Cypress for our E2E tests, which can only interact with my Phoenix app over HTTP routes. I expose a couple of test only controller actions for setting up database scenarios, but had no good way of testing journeys with an email step in the middle. With this I can write a function that tells Cypress to reach into the Bamboo sent emails API to assert an email was received. This isn’t happening in process in Elixir, so the |
ae0ea4f
to
7fa7b8b
Compare
Ah, okay, that makes sense. What would you think about making these API endpoints in a separate plug that people could then expose in their apps only if they want to? I feel like the use case for these might be different enough to warrant it being separate, and many people would not need these exposed, although I'm not sure and would love to hear your thoughts. In any case it'd be great to have some documentation on here about what these are for. Thanks for your work on this! |
Thanks @maymillerricci. Happy to do that if you feel it's clearer - I think it's likely that they wouldn't both be used at the same time, so that makes sense. And I'll add some documentation at the same time. |
@@ -190,6 +190,44 @@ defmodule Bamboo.SentEmailViewerPlugTest do | |||
assert conn.resp_body =~ "Email not found" | |||
end | |||
|
|||
test "list emails over API" do | |||
normalize_and_push_pair(:html_email, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on using normalize_and_push
to create a single email since the assertions are all about the first one? Or alternatively doing assertions on both of the emails?
assert first_email["to"] == [[nil, "to@example.com"], ["Alice", "alice@example.com"]] | ||
assert first_email["cc"] == [[nil, "cc@example.com"]] | ||
assert first_email["bcc"] == [["Bob", "bob@example.com"]] | ||
assert first_email["html_body"] == "<p>ohai!</p>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is coming from the factory, I think it feels like a "magic string" and it's not clear just looking at the test itself where it's coming from. (And like if someone were to change the factory, this test would fail.) Thoughts on doing something to make it more clear?
assert {"content-type", "application/json; charset=utf-8"} in conn.resp_headers | ||
|
||
json = Bamboo.json_library().decode!(conn.resp_body) | ||
assert json == %{"ok" => true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense in here to have an assertion that the emails got cleared out?
Thanks @tomtaylor! I can see arguments for both sides, but I do think having them separate would be slightly clearer, and people can decide which endpoints they want to expose. I also left you some minor comments about the tests. Thanks for your work on this! |
2743941
to
3a82218
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple tiny copy changes to the documentation, but this looks really great! 💯
3a82218
to
f4c95f1
Compare
@maymillerricci just pushed a replacement commit with the final tweaks! |
This adds support for exposing the sent emails over an HTTP API for making assertions about delivered emails from an integration test runner.
f4c95f1
to
7ce6dd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 Looks great - thanks for your work on this!
Thanks! |
Added two endpoints to the
SentEmailViewerPlug
to list and reset the sent emails programatically. These are useful for integration test runners, so it's possible to write tests that can check and respond to emailed content.