pablo-co / bamboo_postmark

A Bamboo adapter for Postmark
MIT License
39 stars 29 forks source link

Add Postmark email opens and links tracking options #8

Closed whodidthis closed 7 years ago

whodidthis commented 7 years ago

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

pablo-co commented 7 years ago

Ebert has finished reviewing this Pull Request and has found:

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

pablo-co commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Closed in favor of #9.