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 Postmark email opens and links tracking options #8

Closed
wants to merge 2 commits into from
Closed

Add Postmark email opens and links tracking options #8

wants to merge 2 commits into from

Conversation

whodidthis
Copy link
Contributor

Hopefully the api is fine.

Do you think the adapter should throw if TrackLinks option is not one of the possible values "None", "HtmlOnly", "TextOnly" or "HtmlAndText"?

http://developer.postmarkapp.com/developer-api-email.html

test "deliver/2 puts tracking params" do
email = new_email() |> PostmarkHelper.tracking(%{opens: true, links: "HtmlOnly"})

email |> PostmarkAdapter.deliver(@config)
Copy link
Owner

Choose a reason for hiding this comment

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

Use a function call when a pipeline is only one function long

@@ -165,6 +165,14 @@ defmodule Bamboo.PostmarkAdapterTest do
assert_receive {:fake_postmark, %{params: %{"Tag" => "some_tag"}}}
end

test "deliver/2 puts tracking params" do
email = new_email() |> PostmarkHelper.tracking(%{opens: true, links: "HtmlOnly"})
Copy link
Owner

Choose a reason for hiding this comment

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

Use a function call when a pipeline is only one function long

@@ -103,6 +104,17 @@ defmodule Bamboo.PostmarkAdapter do
params
end

defp maybe_put_tracking_params(params, %{private:
Copy link
Owner

Choose a reason for hiding this comment

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

There should be no trailing white-space at the end of a line.

@pablo-co
Copy link
Owner

pablo-co commented Feb 6, 2017

Ebert has finished reviewing this Pull Request and has found:

  • 1 possible new issue (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/pablo-co/bamboo_postmark/pulls/8.

@pablo-co
Copy link
Owner

pablo-co commented Feb 6, 2017

Thanks for your work!

To better reflect the act of tracking I would rename the tracking function to the imperative track as in giving an order.

For the link tracking I think it would be cleaner to declare them through symbols: :none, :html, :text, :html_and_text. The advantage is that we give a cleaner more abstract interface to Postmark; the drawback is that we loose transparency towards which values are valid and the equivalence between the symbols and string value they get converted to. What do you think?

@whodidthis
Copy link
Contributor Author

whodidthis commented Feb 7, 2017

Actually now that I look at the official adapters it looks like we should just provide a put_param for the additional parameters: https://github.com/thoughtbot/bamboo/blob/master/lib/bamboo/adapters/mandrill_helper.ex#L15

I can redo this pr more in line with the official ones if you want?

@pablo-co
Copy link
Owner

Sorry, the last 2 weeks have been really busy. It actually sounds like a great idea to use a put_param function instead.

@pablo-co pablo-co closed this Mar 7, 2017
@pablo-co
Copy link
Owner

pablo-co commented Mar 7, 2017

Closed in favor of #9.

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.

2 participants