Add support for Sendgrid unique arguments #609
Conversation
There was a problem hiding this comment.
Hi @jeepers3327 thanks for opening this PR.
I see that you added the with_unique_args/2 function to the SendGridHelper that puts the args under the private map. That's one side of the equation.
For this to work, we also need to grab those unique args and put them in the correct place when sending the email. For example, with categories, we put them in the email's body in https://github.com/thoughtbot/bamboo/blob/6aa74c3ff16604aeba853c34cd525c6b7f46b33f/lib/bamboo/adapters/send_grid_adapter.ex#L126 which calls https://github.com/thoughtbot/bamboo/blob/6aa74c3ff16604aeba853c34cd525c6b7f46b33f/lib/bamboo/adapters/send_grid_adapter.ex#L305-L311
Do you mind including that in this PR?
|
Apologies, I only thought of the SendgridHelper module. I've added Thank you. |
| """ | ||
| def with_unique_args(email, unique_args) when is_map(unique_args) do | ||
| unique_args = | ||
| Map.get(email.private, @unique_args, %{}) |
There was a problem hiding this comment.
What do you think about using Map.update/4 here instead of getting the values and then updating them? I think it reveals intention slightly more clearly.
There was a problem hiding this comment.
Here's my new implementation using Map.update
%{unique_args: unique_args} =
Map.update(
email.private,
@unique_args,
unique_args,
&Map.merge(&1, unique_args)
)
email
|> Email.put_private(@unique_args, unique_args)
I pattern match and only retrieve the value since Map.update/4 also returns the map key. I think its not good, can I ask for your suggestion for improvement?
There was a problem hiding this comment.
If you think Map.update/4 makes it more confusing. I'm okay leaving things as they are. No need to change this. Thanks for looking into it!
| @@ -440,4 +441,12 @@ defmodule Bamboo.SendGridAdapter do | |||
| do: Map.put(body, :ip_pool_name, ip_pool_name) | |||
|
|
|||
| defp put_ip_pool_name(body, _), do: body | |||
|
|
|||
| defp put_unique_args(body, %Email{private: %{unique_args: unique_args}}) | |||
There was a problem hiding this comment.
Thanks for adding this function. Could you add a test to ensure this behaves as expected? I know we tested the with_unique_args function in the helper, but it would be nice to test this one too.
There was a problem hiding this comment.
@jeepers3327 whenever you have time, if we add a test for this, I think the PR is good to go. 👍
There was a problem hiding this comment.
@germsvel I've added a test case for send_grid_adapter. Let me know if I need to a add more tests.
This PR add support for Sendgrid unique_arguments (#592 )