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: add ability to define interceptors #591

Merged
merged 7 commits into from
Apr 5, 2021

Conversation

StephaneRob
Copy link
Contributor

This PR add ability to define interceptors for a given mailer. An interceptor can modify or intercept email before send.

@germsvel
Copy link
Collaborator

@StephaneRob thanks so much for opening this PR. I'll review it as soon as I can.

In the meantime, I was wondering if you could provide some more information as to the possible use cases for interceptors? I know I've used similar things in the past to prevent staging emails from going out, so I can see how this would be very useful.

But I'd love to have more information so we can make sure to document all of that. Thanks!

@StephaneRob
Copy link
Contributor Author

Hi @germsvel,
As you said, interceptors can either be used to prevent emails from going out or to modify some email data such as subject in a single place.

use cases for interceptions:

  • block outbound emails for a given environment (staging, dev,...)
  • block email in case the recipient is blocked (bounces, complaints, ...)

use cases for modifications:

  • rewrite subject for a given environment (for example I like prefix subject w/ environment ex: [staging] Welcome. the subject is Welcome and the mailer take care of this part, and the interceptor will prefix when needed.
  • rewrite email to make sure no email is sent to real email adress in a dev environment.

Certainly others use cases :)

Copy link
Collaborator

@germsvel germsvel 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 so much for introducing this work. I'm really excited for this. I think this will be very useful! 🥳

I left several comments. If any of them are unclear, please let me know. I'd be happy to clarify them or help as much as I can.

I think the biggest change needed is for us to handle all deliver_x functions. The PR currently handles deliver_now.

Thanks again, and please reach out if you have comments or questions.

mix.exs Outdated Show resolved Hide resolved
lib/bamboo/interceptor.ex Outdated Show resolved Hide resolved
lib/bamboo/interceptor.ex Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/bamboo/mailer.ex Outdated Show resolved Hide resolved
lib/bamboo/mailer.ex Outdated Show resolved Hide resolved
test/lib/bamboo/mailer_test.exs Outdated Show resolved Hide resolved
test/lib/bamboo/mailer_test.exs Outdated Show resolved Hide resolved
test/lib/bamboo/mailer_test.exs Outdated Show resolved Hide resolved
@StephaneRob
Copy link
Contributor Author

@germsvel thank you for your review :)
Happy that this feature can be useful, i'm working on it and let you know as soon as a new version is available

@StephaneRob
Copy link
Contributor Author

Hi @germsvel, I reworked the feature. I also moved apply_interceptors after validate_and_normalize. It allows to work with a single format of from, to, cc and bcc.

Let me know if it's more like you imagined it

@davidlibrera
Copy link

davidlibrera commented Mar 30, 2021

Nice work 👍
I was looking exactly for this feature and I've created a branch with a similiar feature (master...leanpanda-com:master).

@StephaneRob what do you think about adding some headers with original addresses? That way it's possible to see which addresses should have received emails without interception.
I've imaged something like

%{
  "X-Real-To" => list_of_real_to_recipients,
  "X-Real-Cc" => list_of_real_cc_recipients,
  "X-Real-Bcc" => list_of_real_bcc_recipients
}

@StephaneRob
Copy link
Contributor Author

Thx @davidlibrera :)
Interceptors is something generic, Bamboo doesn't do anything by default (logic will be on application side). In your use case you could create a RecipientReplacerInterceptor which replace recipients and add headers w/ original recipients.

@davidlibrera
Copy link

Ok, I'm understand what's happening. This PR was notified to me as the same feature I was working by @maartenvanvliet but it seems that provides a different feature.

@StephaneRob
Copy link
Contributor Author

@davidlibrera At first sight, your use case fits with interceptors feature.

@davidlibrera
Copy link

Ok, my fault.
I didn't notice that the DenyListInterceptor is a test module. Ok, now I figured out how it works and I imagine that in order to have what I was trying to achieve I simply need to create an Interceptor that replaces recipients. Am I right?

@StephaneRob
Copy link
Contributor Author

Yes exactly :)

@davidlibrera
Copy link

It sounds good! Many thanks!

Copy link
Collaborator

@germsvel germsvel 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 all the improvements! 🥳

I left a few more comments, because I think we might not be handling two cases that we need to handle.

README.md Show resolved Hide resolved
lib/bamboo/email.ex Outdated Show resolved Hide resolved
lib/bamboo/email.ex Outdated Show resolved Hide resolved
lib/bamboo/interceptor.ex Show resolved Hide resolved
lib/bamboo/mailer.ex Outdated Show resolved Hide resolved
test/lib/bamboo/mailer_test.exs Outdated Show resolved Hide resolved
@germsvel
Copy link
Collaborator

As I've read through the code, one thing that stood out to me is that we're using the term intercept for two different (but related) concepts. And that can make it confusing.

One the one hand, we can define an interceptor module that modifies the email on its way out. A good example of that is the EnvInterceptor you've defined in the tests — it adds the environment to the subject.

On the other hand, we can define interceptor modules that block an email from going out. That's when we use the Email.intercept/1 function to mark the %Email{intercepted: true}.

So, I think the part that could cause confusion is that you can have an email that has been "intercepted" in the sense that it's gone through an interceptor but that hasn't been blocked — and thus has intercepted: false.

So, here's my question. What do you think about changing the term we use for one of those? I think the term interceptor implies blocking or obstructing, so it makes sense that an email is intercepted. So perhaps we can find a better name for the modules?

But please feel free to disagree with this. It's possible that the term interceptor is good enough for both the modules and the boolean field. I'd love to hear your thoughts and see if we can find other terms. If we can't find other terms that better express that, I think I'm okay with using intercept for both.

@StephaneRob
Copy link
Contributor Author

I understand what you mean. The term interceptor comes originally from Action Mailer (rails). They use Interceptor for the class but there is not intercept notion (only prevent delivery via mail.perform_deliveries = false.

Maybe we can change this term to something closer than a Hook ?

config :my_app, MyApp.Mailer,
  adapter: MyAdapter,
  before_send: [EnvHook, DenyListHook]
  
# with 
defmodule Bamboo.Hook do
  @callback call(email :: Bamboo.Email.t()) :: Bamboo.Email.t()
end

before_send would receive a list of adopted Hook behaviours.

@germsvel
Copy link
Collaborator

germsvel commented Apr 3, 2021

@StephaneRob thanks for mentioning that. I took a look at some of the interceptor libraries in Ruby, and I also tried to look for similar language used by other projects (for example, intercepting outgoing events in Phx Channels). It seems that the concept of intercept is used for modifying and blocking, like you have used it here. 👍

So, now I think we should keep the interceptors config key and the module names as Interceptor. But what do you think about changing the %Email{intercepted: true} key to something like blocked? (e.g.%Email{blocked: true}). That way there's a distinction between emails being intercepted (and modified) from those that are blocked.

I think that also lines up with the language you used in the docs for the README:

It's possible to configure per Mailer interceptors. Interceptors allow
to modify / intercept (block) email on the fly.

I think that's a great description of interceptors: "Interceptors allow you to modify / block email on the fly.

What do you think?

@StephaneRob
Copy link
Contributor Author

@germsvel good idea! I update the pull request and let you know when ready for review.

@StephaneRob
Copy link
Contributor Author

@germsvel I updated the PR.

Copy link
Collaborator

@germsvel germsvel left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you so much for all the work and back and forth @StephaneRob! 🥳

Very excited for this feature.

assert %Bamboo.Email{blocked: true} = Mailer.deliver_later!(email)
refute_receive {:deliver, %Bamboo.Email{to: [{nil, "blocked@blocked.com"}]}, _config}
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the change to these tests. They are far easier to read 🎉

@germsvel germsvel merged commit 733b9e5 into beam-community:master Apr 5, 2021
@StephaneRob
Copy link
Contributor Author

@germsvel thank you for your help!

@StephaneRob StephaneRob deleted the feat-interceptors branch April 5, 2021 16:30
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.

3 participants