Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ActionMailer::InlineCssHook.delivering_email trashes attachments #9

Merged
merged 1 commit into from Jan 6, 2012

Conversation

Projects
None yet
3 participants
Contributor

leehambley commented Oct 24, 2011

The line message.body = nil trashes all message attachments, which is a problem!

I'd propose that in the same way that one maintain the exiting_text_part, one aught to preserve attachments too.

This also introduces the issue I raised in #8, which aught to be fixed separately, I think it may also break the untested code for "fallback the text segment to a text version of the HTML component", which was not important for my purposes.

Contributor

ndbroadbent commented Oct 25, 2011

Hi, thanks for this. The problem with

 message.html_part do

is that it doesn't override the existing html part, it simply appends a new one. So message.body = nil is necessary, but I agree that we should definitely preserve any attachements! I'll look into fixing this

Contributor

leehambley commented Oct 25, 2011

My preliminary experimentation seemed to indicate that messge.parts.length wasn't affected by my patch, but I can gladly test again, I was working late at night (GMT+0200) - so I might be mistaken, either way for the time being my fork appears to work for me, but I'd love to contribute back whatever I can.

Perhaps you can tell me, is my test sane, I seemed to have to have explicitly called the hook, perhaps that logic in the hook should be extracted to a class, which is then used by the hooking code? That would decouple the AM Interceptor code implementation from the behaviour of this gem, which i believe is universally useful.

Contributor

ndbroadbent commented Oct 25, 2011

Well, if that's the case, then that would be great (about message.parts.length not changing).

You seem to have removed the auto-generated text part:

-        # Add a text part with either the pre-existing text part, or one generated with premailer.
-        message.text_part do
-          content_type "text/plain; charset=utf-8"
-          body existing_text_part || premailer.to_plain_text
-        end

which is quite handy if you don't want to write the text part manually. I always do, but some people might not care.

If we can overwrite parts by calling message.html_part or message.text_part, then we could leave that code in there.

I think your tests are sane enough, apart from silencing the warnings with Kernel.silence_warnings. We can specify the charset instead.

I don't have too much time to invest in this project, so I'd like to add you as a collaborator and give you more freedom to work on it and fix it up. As long as the code is all tested and the current features keep working as expected (e.g. auto-generating text parts)... I'll give you permission to push new versions to rubygems too :)

Cheers

Contributor

leehambley commented Nov 3, 2011

Thanks @ndbroadbent - I removed the auto-generated message text part partly because it was un-tested, and partly because it was standing between me and my pull request, but roger to all of the above points you raised. I'll overwork some of this stuff soon enough when I have some time to improve the tests.

Any word on this pull request? I am unable to attach a pdf to my html emails because of this issue -- is there a workaround?

Contributor

leehambley commented Dec 22, 2011

Try my fork.

On 22 December 2011 02:48, David Kariuki <
reply@reply.github.com

wrote:

Any word on this pull request? I am unable to attach a pdf to my html
emails because of this issue -- is there a workaround?


Reply to this email directly or view it on GitHub:

ndbroadbent#9 (comment)

Contributor

ndbroadbent commented Dec 22, 2011

Hi, I'll try to find some time to merge this pull request, while writing tests for the auto-generated text part. Until then, please do try @leehambley's fork

Cheers

@ndbroadbent ndbroadbent added a commit that referenced this pull request Jan 6, 2012

@ndbroadbent ndbroadbent Merge pull request #9 from leehambley/master
ActionMailer::InlineCssHook.delivering_email trashes attachments
71294ac

@ndbroadbent ndbroadbent merged commit 71294ac into premailer:master Jan 6, 2012

Contributor

ndbroadbent commented Jan 6, 2012

Have fixed to preserve / generate text parts, while preserving attachments.

Contributor

leehambley commented Jan 6, 2012

@ndbroadbent Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment