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

Change Email Attachment Behavior to wrap mail parts in multipart/related #1569

Merged
merged 6 commits into from May 30, 2014

Conversation

Projects
None yet
3 participants
@farmdawgnation
Copy link
Member

farmdawgnation commented May 27, 2014

This is the same work in #1403 by @limansky rebased on top of the current master branch. Going to do some testing here to ensure that email behavior works as expected across various email clients. If this works, then it closes #1197 and we'll have working Outlookyness.

I may take the opportunity to do some spec addition and variable name cleanup as well.

@farmdawgnation farmdawgnation changed the title [WIP] Change Email Attachment Behavior to wrap mail parts in multipart/related Change Email Attachment Behavior to wrap mail parts in multipart/related May 27, 2014

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 27, 2014

Tested this branch using the code at email-multipart-testing. Here are the results:

https://litmus.com/pub/bdf903f/screenshots

So far it looks like Outlook 2000 is the only one to balk. Thunderbird and iOS also look suspicious in this test, but I confirmed locally that they both, in fact, work. Now I'm going to see what the original code did to determine how much of a win this ends up being.

Removing the WIP tag. Not going to be making much in the way of code changes on top of what @limansky did already after all.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 27, 2014

Well, this is strange... here's the test from the current 2.6 master:

https://litmus.com/pub/a15678c

I'm seeing no difference here. @limansky Did my test code (linked above) do something differently than you did?

@limansky

This comment has been minimized.

Copy link
Contributor

limansky commented May 27, 2014

Hi, actually I don't really remember the original case of that fix (it was more than year ago). As I remember it was one of the bugs from our customers, so I just pushed the fix to our product and got the response that the issue is resolved.

The test code looks pretty same: https://gist.github.com/limansky/4603352

But, according to this thread https://groups.google.com/forum/?fromgroups#!topic/liftweb/xIPg2be17Fg the issue can be related not to the client, but to M$ Exchange server or the Exchage and ActiveSync protocols.

Could you clarify which protocols have you used for testing?

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 27, 2014

Ah. I think this would have just been regular SMTP. I'll try on the Exchange connection to my work email and see what happens.

On Tue, May 27, 2014 at 4:39 PM, Mike Limansky notifications@github.com
wrote:

Hi, actually I don't really remember the original case of that fix (it was more than year ago). As I remember it was one of the bugs from our customers, so I just pushed the fix to our product and got the response that the issue is resolved.
The test code looks pretty same: https://gist.github.com/limansky/4603352
But, according to this thread https://groups.google.com/forum/?fromgroups#!topic/liftweb/xIPg2be17Fg the issue can be related not to the client, but to M$ Exchange server or the Exchage and ActiveSync protocols.

Could you clarify which protocols have you used for testing?

Reply to this email directly or view it on GitHub:
#1569 (comment)

@farmdawgnation farmdawgnation added this to the 2.6-M4 milestone May 28, 2014

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 28, 2014

Ok, so I tested the original Lift master with my work email. I can't reproduce the bug you're describing. That said, this change doesn't appear to break anything. :/

What are thoughts around putting this in? We have a report on the ground that it resolves an issue, though we can't confirm the original bug, and I'm not seeing brokenness as a result of the change. Unless there are objections, I'm going to drop this on the 2.6-M4 milestone and recommend its merge.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented May 28, 2014

I think I'd be okay with that.

@limansky

This comment has been minimized.

Copy link
Contributor

limansky commented May 28, 2014

I've checked your "bacon" example with my work exchange 2010 server and was not able to reproduce the issue using Lift from maven. Possible MS changed something, but it's the only server I have to test.
From the other side, the patch is using in our product for a quite long period and there are no issues observed.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 28, 2014

It's also entirely possible this is an issue that only affects older versions of Exchange server. Either way it doesn't appear to break anything for anyone else.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 28, 2014

I may add a comment in the code about this. I hate code comments as documentation but this is the kind of thing I could see someone familiar with the RFC undoing without thinking too much about it...

farmdawgnation added some commits May 28, 2014

Add a comment documenting behavior of Exchange w/ mixed.
We added this code because some Exchange servers don't play nice without
a mixed multipart. This appears to be tied to some versions of the
Exchange server, not neccicarily Outlook or other associated clients.
@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented May 28, 2014

Ok, I made some formatting updates so hopefully the code will be a bit easier to parse out for the next guy who comes along. I also added a spec to cover this behavior.

farmdawgnation added a commit that referenced this pull request May 30, 2014

Merge pull request #1569 from lift/msf_email_attachments
Email attachments are now wrapped in a multipart/mixed segment to satisfy some versions of Exchange server that don't handle attachments correctly without this segment.

@farmdawgnation farmdawgnation merged commit d302153 into master May 30, 2014

@farmdawgnation farmdawgnation deleted the msf_email_attachments branch May 30, 2014

@limansky

This comment has been minimized.

Copy link
Contributor

limansky commented Jun 3, 2014

Should it be also pushed for 3.0?

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Jun 3, 2014

Yep. That'll happen whenever we merge master into the lift_30 branch.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Jun 3, 2014

(This happens regularly, to clarify.)

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