Skip to content
This repository has been archived by the owner on Dec 31, 2024. It is now read-only.

added check to see if body part has a name and disposition is unknown, b... #106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zkestenbaum
Copy link

...efore putting it in the body. This fixes a problem where an email that has an empty body and an attachment (where the content disposition is not set) would have the attachment appear in the message body. With this change, the attachment correctly appears as an attachment.

@NiKiZe
Copy link
Contributor

NiKiZe commented Feb 12, 2015

+1 the only thing I'm missing is unit tests for this. (and also results for if everything passes as it is now)

Would it be reasonable to assume attachment if it has a name? Or does bodyparts come with names sometime as well?

@zkestenbaum
Copy link
Author

Just added a unit test for this. All unit tests pass currently. if I revert the change I made, then this new unit test fails.

I think it is a reasonable assumption, especially since the code is already making that assumption (a few lines below this spot). I think it was an oversight to not include this check in this particular spot.
// If the part has a name it most likely is an attachment and it should go into the
// Attachments collection.
bool hasName = part.Parameters.ContainsKey("name");

@jstedfast
Copy link

The name/filename parameters can be set on any MIME part, whether it is meant to be shown inline as the body of the message or as an attachment.

The only way to tell if a MIME part is meant to be shown as an attachment or not is to look at the Content-Disposition value ("attachment" vs "inline").

It seems that this is what his patch does, so it looks correct to me, although it could probably be simplified to just part.Disposition.Type == ContentDispositionType.Inline instead of !(part.Disposition.Type == ContentDispositionType.Attachment || part.Disposition.Type == ContentDispositionType.Unknown)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants