-
Notifications
You must be signed in to change notification settings - Fork 343
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!: Use interceptor before stripping the recipients with the formatter #682
base: main
Are you sure you want to change the base?
feat!: Use interceptor before stripping the recipients with the formatter #682
Conversation
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.
Thank you @tarzan! I looked this over and left a quick comment on the test changes.
While I can't immediately think of a negative impact here, I do suspect this could have unintended consequences to current users of Bamboo and would require a breaking changes. If so, that would warrant some more conversation 🤔
def call(%{to: recipients} = email) when is_list(recipients) do | ||
if Enum.any?(recipients, &(&1 in @deny_list)) do | ||
Bamboo.Email.block(email) | ||
else | ||
end | ||
end | ||
|
||
def call(%{to: recipient} = email) when recipient in @deny_list do | ||
Bamboo.Email.block(email) | ||
end | ||
|
||
def call(email) do | ||
end |
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.
Hi @tarzan can you walk me through this part of the change? It's not immediately clear why we made these changes other than to use multi-function heads instead of the prior solution.
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.
Of course. Previously this interceptor (created for test scenario's) would expect the to:
field to be populated by a list of tuples: {name, email}
.
The actual test scenario's just put strings containing the email(s) on the to:
field, so I had to check for a single binary that is an email or a list of binaries.
You are absolutely right, this is a breaking change. Any custom interceptors might expect a tuple [{name, email}] as the value of In our case the value on I can totally understand that current users that have spend time in constructing their own interceptors would not be to thrilled about this change. Would it help if I wrote some simple instructings on what to look for and how to apply these changes - ie something like a (mini) migration guide? |
hey @doomspork any updates on how you would propose we move forward with this? or would like to keep it on hold? |
6159e2d
to
37f420c
Compare
…e it gets stripped by the formatter
37f420c
to
3e1ef7b
Compare
still waiting for approval here, @doomspork is there anything I can do to make things easier? |
I would like to use the interceptor to detect the preferences of the rich struct that is set as a receiver on the 'to:'
The (implemented) formatter strips all info from this struct and just set a {name, email} tuple instead which I cannot use at the interceptor level anymore.
I decided to swap them: interceptor first normalization and validation second - can't think of a reason this would result in unwanted behavior.
Could you take this under consideration?