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

Add support for Sendgrid unique arguments #609

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/bamboo/adapters/send_grid_adapter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ defmodule Bamboo.SendGridAdapter do
|> put_bypass_list_management(email)
|> put_google_analytics(email)
|> put_ip_pool_name(email)
|> put_unique_args(email)
end

defp put_from(body, %Email{from: from}) do
Expand Down Expand Up @@ -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}})
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jeepers3327 whenever you have time, if we add a test for this, I think the PR is good to go. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@germsvel I've added a test case for send_grid_adapter. Let me know if I need to a add more tests.

when is_map(unique_args) do
body
|> Map.put(:unique_args, unique_args)
end

defp put_unique_args(body, _), do: body
end
22 changes: 22 additions & 0 deletions lib/bamboo/adapters/send_grid_helper.ex
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ defmodule Bamboo.SendGridHelper do
@allowed_google_analytics_utm_params ~w(utm_source utm_medium utm_campaign utm_term utm_content)a
@send_at_field :sendgrid_send_at
@ip_pool_name_field :ip_pool_name
@unique_args :unique_args

@doc """
Specify the template for SendGrid to use for the context of the substitution
Expand Down Expand Up @@ -294,4 +295,25 @@ defmodule Bamboo.SendGridHelper do
email
|> Email.put_private(@ip_pool_name_field, ip_pool_name)
end

@doc """
A map of unique arguments for this email. This will override any existing unique arguments.

## Example

email
|> with_unique_args(%{new_arg_1: "new arg 1", new_arg_2: "new arg 2"})
"""
def with_unique_args(email, unique_args) when is_map(unique_args) do
unique_args =
Map.get(email.private, @unique_args, %{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jeepers3327 jeepers3327 Jun 5, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

|> Map.merge(unique_args)

email
|> Email.put_private(@unique_args, unique_args)
end

def with_unique_args(_email, _unique_args) do
raise "expected a map of unique arguments"
end
end
19 changes: 19 additions & 0 deletions test/lib/bamboo/adapters/send_grid_adapter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,25 @@ defmodule Bamboo.SendGridAdapterTest do
assert msg =~ ~r/'email' field/
end

test "deliver/2 correctly handles with_unique_args" do
email = new_email()

unique_args = %{
new_arg1: "new arg 1",
new_arg2: "new arg 2",
new_arg3: "new arg 3"
}

email
|> Bamboo.SendGridHelper.with_unique_args(unique_args)
|> SendGridAdapter.deliver(@config)

assert_receive {:fake_sendgrid, %{params: params}}
assert params["unique_args"]["new_arg1"] == "new arg 1"
assert params["unique_args"]["new_arg2"] == "new arg 2"
assert params["unique_args"]["new_arg3"] == "new arg 3"
end

test "deliver/2 will set sandbox mode correctly" do
email = new_email()
email |> SendGridAdapter.deliver(@config_with_sandbox_enabled)
Expand Down
24 changes: 24 additions & 0 deletions test/lib/bamboo/adapters/send_grid_helper_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -207,4 +207,28 @@ defmodule Bamboo.SendGridHelperTest do
email = email |> with_ip_pool_name(@ip_pool_name)
assert email.private[:ip_pool_name] == @ip_pool_name
end

test "with_unique_args/2 merges multiple maps", %{email: email} do
email =
email
|> with_unique_args(%{new_arg_1: "new arg 1", new_arg_2: "new arg 2"})
|> with_unique_args(%{new_arg_3: "new arg 3"})

assert map_size(email.private[:unique_args]) == 3
end

test "with_unique_args/2 overrides duplicate entries", %{email: email} do
email =
email
|> with_unique_args(%{new_arg_1: "new arg 1"})
|> with_unique_args(%{new_arg_1: "latest new arg 1", new_arg_2: "new arg 2"})

assert map_size(email.private[:unique_args]) == 2
end

test "with_unique_args/2 raises on non-map parameter", %{email: email} do
assert_raise RuntimeError, fn ->
email |> with_unique_args(["new arg"])
end
end
end