thoughtbot / griddler

Simplify receiving email in Rails (deprecated)
http://griddler.io/
MIT License
1.38k stars 199 forks source link

Body regex is removing blockquotes #76

Closed r38y closed 11 years ago

r38y commented 11 years ago

Hey there, I was having trouble with blockquotes being removed from email bodies. I think it has to do with https://github.com/thoughtbot/griddler/blob/master/lib/griddler/email_parser.rb#L34. I am thinking of changing that to \s+ so blockquotes stick around.

What do you think? Or does this break something else?

calebhearth commented 11 years ago

Hmm, I see the issue. We're explicitly removing markdown style blockquotes (>) as they are an email paradigm for quoted text, which we wouldn't want in the emails. Consider the following:

Yeah, that's reasonable.

> Want to do that thing?

We don't actually want that blockquote.

r38y commented 11 years ago

The second one?

This is text before the sig and "on so and so did whatever" so anything in there if it has a > should be a quote.

theycallmeswift commented 11 years ago

Can you provide an example? I know > is a super common reply delimiter. Not removing it could be risky business, no?

r38y commented 11 years ago

An example? I replied to an email quoting someone's text and it got stripped. I wanted it to come through as a quote just like any other email.

I submitted a PR that got accepted. All other tests passed when I did it and this regex is being used after the reply body has been pulled from the body.

theycallmeswift commented 11 years ago

Just because the tests pass doesn't mean it's right :) My concern is that we're obviously still pulling out the replies where you changed the regex based on the presence of this line right after the one you changed:

 line =~ /^\s*Sent from my /

If it's somehow possible that we haven't entirely stripped out the replies delimited by >, then we may miss them. Can anyone think of a case where that might be true?

r38y commented 11 years ago

I guess I meant at least we know the known cases before this change all still work. If we come across a future case that doesn't behave as desired/expected, a test could be written for it and the code fixed, right?

I could be misunderstanding what you are asking for. I'll shut up until I hear more :smile:

jayroh commented 11 years ago

Soooo ... in my opinion this is an expected behavior thing. Because I know that > is the common, accepted way of quoting text in email and markdown I'm often apt to use it as a means to do so. I'll write emails quoting something outside the context of a reply - therefore I expect that it show up.

However - if it's AFTER the "so and so replied with" then I definitely think it should be stripped.

It should go without saying, but it’s absolutely critical that your user experience reflects natural and expected behaviors when its being used. These behaviors include supporting common shortcuts that allow your users to easily navigate your software. This includes supporting mouse buttons that allow a user to navigate backward and forward in your application.

That is, of course, a trite example of the sort of thing I'm used to. If I replied with this to a griddler-backed app then that quote should/would be stripped. I don't think it should be stripped.

theycallmeswift commented 11 years ago

Ok, I'll trust you guys' judgement here. My new question is: Can we remove this line entirely now that we know we don't care about >'s above the reply delimiter:

line =~ /^\s+>/ ||
r38y commented 11 years ago

Unrelated to that comment, what will happen to the > in front of the reply delimiter?

On Thursday, August 22, 2013, Swift wrote:

Ok, I'll trust you guys' judgement here. My new question is: Can we remove this line entirely now that we know we don't care about >'s above the reply delimiter:

line =~ /^\s+>/ ||

— Reply to this email directly or view it on GitHubhttps://github.com/thoughtbot/griddler/issues/76#issuecomment-23103917 .

Randy Schmidt