Google Analytics settings for SendGridAdapter #462
Conversation
Added the ability to enable/disable Google Analytics settings for an individual email in SendGrid.
| |> with_google_analytics(false) | ||
| """ | ||
| def with_google_analytics(email, enabled, utm_params \\ %{}) | ||
| def with_google_analytics(email, enabled, utm_params) when is_boolean(enabled) and is_map(utm_params) do |
There was a problem hiding this comment.
What would you think about adding a couple of unit tests for this function that verify for example the right types being passed in and that UTM params not in white list would get filtered out?
tomtaylor
force-pushed
the
sendgrid-google-analytics
branch
from
afa51c1
to
2f33b5f
Compare
|
@maymillerricci all done! I also added the ability to set the UTM params using a Keyword list, since it seemed like the flexibility would be helpful. |
tomtaylor
force-pushed
the
sendgrid-google-analytics
branch
from
2f33b5f
to
2487267
Compare
| @@ -149,4 +149,59 @@ defmodule Bamboo.SendGridHelperTest do | |||
| email |> with_bypass_list_management(1) | |||
| end | |||
| end | |||
|
|
|||
| test "with_google_analytics/2 with Map utm_params", %{email: email} do | |||
There was a problem hiding this comment.
Should these be with_google_analytics/3?
|
Thanks @tomtaylor! Although the flexibility for being able to pass in the params as a map or keyword list might be nice, I wonder in general whether for simplicity on this end, we might want to restrict it to only pass them as a map, and then people on their end can update what's being passed in into a map? I think in most places here similar params are expected to be maps. What do you think? Do you think it's important to have the flexibility of being able to also pass in a keyword list? |
|
If there's precedent for just accepting Maps, that make sense. The flexibility isn't a major issue here. Will update the test descriptions when I get a moment! |
Based on feedback from @maymillerricci, drop the ability to accept Keywords.
|
@maymillerricci I think this is ready to go. |
| |> with_google_analytics(true, %{utm_source: "email", utm_campaign: "campaign"}) | ||
|
|
||
| |> with_google_analytics(true, utm_source: "email", utm_campaign: "campaign") |
There was a problem hiding this comment.
Thanks for updating this!! Does this example now need be removed?
| end | ||
|
|
||
| def with_google_analytics(_email, _enabled, _utm_params) do | ||
| raise "expected with_google_analytics enabled parameters to be a boolean" |
There was a problem hiding this comment.
| raise "expected with_google_analytics enabled parameters to be a boolean" | |
| raise "expected with_google_analytics enabled parameter to be a boolean" |
|
Good spot! Fixed. |
Added the ability to enable/disable Google Analytics settings for an individual email in SendGrid.