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

Support for inline attachments with NewEmailFromReader #119

Merged
merged 4 commits into from
Sep 17, 2020

Conversation

tarmo-randma
Copy link
Contributor

@tarmo-randma tarmo-randma commented Jul 14, 2020

in v4.0.0 when using NewEmailFromReader to create an email, attachments are only created if Content-Disposition is 'attachment'. This PR adds support for inline attachments as well.

Links to issue #82 just like PR #89

The failing test during build was there before this change already, assumed it is a known issue

@jordan-wright
Copy link
Owner

Hi there!

I'll have to investigate this. Your change to:

if cd == "attachment" || cd == "inline" {

is what broke the existing test. I'll need to take a look and see what's the expected behavior there.

@tarmo-randma
Copy link
Contributor Author

Sorry I missed that it worked before.

I looked at the failing test and I must say I am stumped. Sure there is an inline attachment in the test message there but I cannot make out WHY the test needs to behave in the way it behaves. Does not seem logical to me. I hope you will find some time to check it out in detail. Thanks for your work on this lib btw

@tarmo-randma
Copy link
Contributor Author

So the TestMultipartNoContentType contained an inline attachment with no filename part and required this part to be parsed as the actual e-mail body. I do not know if this is expected behavior according to the standards but it does not matter for this pull request I think.

I updated the PR to only parse inline attachments if they also have a filename component defined in their Content-Disposition header. This allows the existing tests to pass and also allows supporting inline attachments. Hopefully it is acceptable now.

@jordan-wright
Copy link
Owner

This LGTM - thanks for sending in and being flexible with adjusting the PR. 💚

@jordan-wright jordan-wright merged commit e1c00e1 into jordan-wright:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants