Issue#4 full url linking from emails

[23inhouse]

Hi Ben

https://github.com/bmabey/email-spec/issues/#issue/4

I found I also had issue #4, I read through and saw you liked the approach of passing an options hash.
I've implemented this and added some tests.

Regards
Ben

PS thanks for the great GEM!

[bmabey]

It looks good though. Thanks for the pull request! I'll merge it in and make a release (probably tomorrow).

[23inhouse]

Ben, i changed the commit as we discussed.
I also added a link to the google cache of the old ticket.

[jeroenvandijk]

Before I saw this pull request I monkey patched email-spec in my own application to use the full urls. So I would like to see this merged as well :)

[bmabey]

I've been hesitating on this patch becuase I'm not sure that I like the option :path_only. I like the functionallity provided, but am unsure at how intuitive the API is. I suppose any API is better than none at this point. What are your thoughts on it and would you prefer a different way of using it?

[jeroenvandijk]

I agree that the API might be a bit counterintuitive, maybe change it to relative: true?

Btw, is there a good reason to use a relative path? The only reason I can think of for using the relative path is that Capybara didn't support full urls properly in versions before 1.0.0. Not sure about webrat and other libraries. In issue #4 I saw something being mentioned about Paypal, but I would assume that can be solved in other ways too.

WDYT?

[23inhouse]

From memory i choose :path_only to match the option in url_for.
I just checked the docs and it's actually :only_path.

I agree there could be a better name.

[bmabey]

Ah, I didn't know that was the option for url_for. That makes more sense now. I do like the suggestion of relative: true better though. I suppose another option would be absolute: true. That way we could change opts[:path_only] != false ? path : url.to_s to opts[:absolute] ? url.to_s : path and let :absolute be nil by default. Any preference between the two?

Yeah, I think using relataive paths was due to a limitation of webrat initially. Are you suggesting that we make absolute paths the default now? Staying with relative as the default seems like a safer choice since not everyone will be upgraded to capybara 1.0.0 (i.e. I'd rather not do a major release for this change). That being said, we could probably remove the extra steps and the option for relative links all together if capybara works well with absolute paths now. I think I'd still like to get this change in to a smaller release first thought.

I'll make the changes sometime this week but would gladly merge is a pull request if you beat me to it. (If you do the patch adding some documenation about the option would be appreciated as well.)

[jeroenvandijk]

I agree with backwards compatibility. Maybe still update the emails_steps with the option in place, because people are likely to use newer libraries when they generate them, right?

And I think in that case absolute: true sounds as the best option.

Btw, I also said this in another pull request about DelayedJob, but the test suite seems to have failures. Is that correct? I would prefer to fix that first before I update this pull request.

[ramontayag]

Hey folks! Any news on this? Was scratching my head for a while there. The right URL was generated in the email but it was as if the subdomains were completely disregarded.

[23inhouse]

I think we should clean up this patch before it is merged into master. I was very tentative about it so i just commented out the original line.

[23inhouse]

ramontayag

I'm not 100% on this, but I think that is a separate issue with rails not being full subdomain aware.
When you run your spec does the email contain the correct url including subdomain?

It follows full subdomain links for me, but i do have a subdomain routing hack*, it might be fixing this issue for me.

Here is a link: https://gist.github.com/1193992

• I dont think it's a good idea to use this hack.

[ramontayag]

Hmm... yeah I use a different hack (I set default_url_options). I'm using my branch now that's a merge of yours and @bmabey's. I got it working fine. But yeah - this is also a rack-test issue, so I'm not sure about the "correct" way to apply this.

[jeroenvandijk]

@23inhouse @ramontayag although subdomains didn't work well with pre-1.0 capybara versions (I'm not sure whether it was Rack-Test or not) now subdomains work correctly for me using capybara-mechanize (based on capybara 1+) and Rails 3+