Closed
Bug 904458
Opened 11 years ago
Closed 8 years ago
Filters that "Reply with Template" must add an Auto-Submitted header
Categories
(Thunderbird :: Filters, defect)
Thunderbird
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 45.0
People
(Reporter: dveditz, Assigned: mkmelin)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [patchlove])
Attachments
(1 file, 9 obsolete files)
7.67 KB,
patch
|
rkent
:
review+
|
Details | Diff | Splinter Review |
According to RFC 3834, in order to preven auto-response loops mails that are an automatic response should have an Auto-Submitted header of "auto-replied" added to the mail, so: Auto-Submitted: auto-replied When a user creates a Message Filter with the action "Reply with Template" we need to add this header to the generated reply. Given the amount of ms-exchange out there we may want to throw in an extra header for them: x-auto-response-suppress: OOF, AutoReply Ideally we would not reply to autoresponder mails either. Since users are normally not going to think of this the best option might be to do so automatically, that is, a hidden rule that we only reply if there is no Auto-Submitted header (or if there is one the value must be "no" w/out the quotes). If we go that route we should also automatically obey the common x-auto-response-suppress headers. What if a user really wants to reply to auto-replies? A few choices a) tough luck, that's the spec. b) if the filter includes a rule about Auto-Submitted then ignore the built-in default rule c) Don't do any implicit handling, but if the user selects "Reply with Template" action automatically add a rule Auto-Submitted Doesn't Contain auto- (this will reply if there's no header, or if it's 'no'.) We then allow users to edit or delete this rule if they want. I don't like b), too much hidden behavior. c) wins on user-control and explicitness, a) would be easiest to implement and better usability for the vast majority of our users.
Reporter | ||
Comment 1•11 years ago
|
||
I went ahead and split this bug into two. This bug is only about what it says in the summary: adding the outgoing header automatically for "Reply with Template" filters. bug 904461 is about what to do with incoming auto-submitted headers.
Assignee | ||
Comment 2•11 years ago
|
||
Adding the Auto-Submitted header, and also making the subject of the reply to foo be "Auto: foo" per the rfc suggestion. I don't think the previous behavior of including the template subject is very appealing. I may well have a template subject i don't necessarily want to "share". And no, i don't know how to write a test for this...
Comment 3•11 years ago
|
||
Here's a possible test for this. Somehow this seems like the hard way to do it, but I captured the reply message text that was sent to the SMTP fake server, and checked the header values. It does exercise your code though.
Comment 4•11 years ago
|
||
I'm not very enthusiastic about the change here to the subject handling, removing the subject from the template message. There are two issues here: what to do with the template subject, and what to do with the "subject message" subject (unfortunately confusing terminology from the RFC). 1. The "subject message" subject The discussion of this in bug 394983 is quite confusing. The OP in that bug wants exactly the opposite of what you have done: "Reply includes the subject from the message replied to. Dumb, it shouldn't do that" from bug 394983 comment 2 But the spec that you are trying to implement has this requirement: "3.1.5. Subject field The Subject field SHOULD contain a brief indication that the message is an automatic response, followed by contents of the Subject field (or a portion thereof) from the subject message. The prefix "Auto:" MAY be used as such an indication." So the OP's recommendation of bug 394983 goes against the SHOULD recommendation of the spec. I think that the proper handling of that bug is to mark it WONT FIX, not dependent on this one. I agree with the inclusion of portions of the subject message's subject here. 2. the template message subject Previously, I could do an auto reply whose subject looked like this: I am on vacation (was: test of auto) Now that subject looks like this: Auto: test of auto Personally I do not find that removal of the template subject to be an improvement. For this case (by far the most common use of auto reply) it seems to me the most useful response subject would be: Auto: I am on vacation (was: test of auto) so that you can quickly pick up the gist of this message from the subject. Now Magnus you are free to disagree with me (and you have in comment 2). What I would suggest though is that you reduce the scope of this bug so that the needed new header and Auto: prefix are not held hostage to an unrelated discussion of whether to include the template subject. I also think that the bar is a little higher for a behavior change like this than it might have been if the template subject was never included, as this change will have a significant impact on user's existing templates, where the subject was significant (and I think that impact is negative). We should discuss that change in a separate bug. Otherwise the patch is fine. For the test, I'll tweak it after the main bug is landed and ask for a separate review.
Updated•11 years ago
|
Attachment #794833 -
Flags: review?(kent) → review-
Assignee | ||
Comment 5•11 years ago
|
||
Ok, i guess i can buy that. Perhaps we could offer the (somewhat hidden choice that if the template subject is bracketed like "[no no no]" then we don't include the template subject? What do you think? If not, i'll just go with only adding Auto: like you suggested.
Comment 6•11 years ago
|
||
I could accept a hidden double bracketed subject like "[[no no no]]" since that would be unlikely to be included in any existing templates. Unfortunately I do not feel like I have a good grasp of user interface choices like this, but I guess I have to take a stand. I do feel pretty firmly that removing existing functionality (being able to include a specific subject in an auto-reply template) without a strong motivation is problematic. For reference, the wording in RFC 3834 at issue here is: "The Subject field SHOULD contain a brief indication that the message is an automatic response, followed by contents of the Subject field (or a portion thereof) from the subject message. The prefix "Auto:" MAY be used as such an indication. If used, this prefix SHOULD be followed by an ASCII SPACE character (0x20)." The original patch matched this spec, including the "MAY" portion. An advantage of the recommendation, compared to the existing Thunderbird implementation, is the claim that "Auto" is an internationally understood, Greek-derived universal term, while "was" in our implementation is English. Still, I would argue that being able to include a message-specific subject is an advantage of the existing Thunderbird implementation, and it is not forbidden by the standard (the template subject along with the Auto could be considered part of a "brief indication that the message is an automatic response"). My personal recommendation would be a response subject such as this: "Auto (I am in Hawaii!): this is the original subject" along with the double-bracket convention such that "Auto: this is the original subject" is possible if someone really wanted it.
Assignee | ||
Updated•11 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee | ||
Comment 7•11 years ago
|
||
- Adds the header - If the template subject is [[double bracketed]] the reply subject is e.g. "Auto: meeting on monday?" - If not, the reply subject is like now, except the Auto: prefix is used e.g. "Auto: on holiday (was: meeting on monday?)" FWIW, i believe the (was: ) pattern is a kind of semi-convention often used on mailing lists when changing subject within a thread.
Attachment #794833 -
Attachment is obsolete: true
Attachment #798167 -
Flags: review?(kent)
Assignee | ||
Comment 8•10 years ago
|
||
Updated and including an updated test.
Attachment #795854 -
Attachment is obsolete: true
Attachment #798167 -
Attachment is obsolete: true
Attachment #798167 -
Flags: review?(kent)
Attachment #8369244 -
Flags: review?(kent)
Assignee | ||
Comment 9•10 years ago
|
||
rkent: review ping
Assignee | ||
Updated•10 years ago
|
Attachment #8369244 -
Flags: review?(irving)
Comment 10•10 years ago
|
||
Comment on attachment 8369244 [details] [diff] [review] proposed fix w/ test Review of attachment 8369244 [details] [diff] [review]: ----------------------------------------------------------------- Switching to f+ until we decide on the notation to indicate not to use the template subject. ::: mailnews/compose/src/nsMsgComposeService.cpp @@ +956,5 @@ > compFields->SetTo(NS_ConvertUTF8toUTF16(replyTo)); > > nsString body; > nsString templateSubject, replySubject; > + Please remove space character on blank line @@ +959,5 @@ > nsString templateSubject, replySubject; > + > + mHdrToReplyTo->GetMime2DecodedSubject(replySubject); > + mTemplateHdr->GetMime2DecodedSubject(templateSubject); > + nsString subject(NS_LITERAL_STRING("Auto: ")); // RFC 3834 3.1.5. I suspect we'll have a few users screaming at us for a way to turn this off. I'm not sure we want to *give* them a way, but it's still a concern. Backward compatibility is an issue, but could we support some sort of string substitution so that the default template subject could be: Auto: blah blah blah (was %replySubject) and then the users could modify it if they want something different? @@ +961,5 @@ > + mHdrToReplyTo->GetMime2DecodedSubject(replySubject); > + mTemplateHdr->GetMime2DecodedSubject(templateSubject); > + nsString subject(NS_LITERAL_STRING("Auto: ")); // RFC 3834 3.1.5. > + if (StringBeginsWith(templateSubject, NS_LITERAL_STRING("[[")) && > + StringEndsWith(templateSubject, NS_LITERAL_STRING("]]"))) Can we get the same effect by just allowing and detecting an empty templateSubject? The '[[whatever]]' notation to indicate "don't actually include this" seems odd. Do we use it anywhere else in Thunderbird? ::: mailnews/compose/test/unit/test_autoReply.js @@ +36,5 @@ > +function ParseString(aString) > +{ > + this.string = aString; > +} > + White space ::: mailnews/test/data/autoreply-template @@ +1,4 @@ > +From - Sat Jun 07 04:23:43 2008 > +X-Mozilla-Status: 0000 > +X-Mozilla-Status2: 00000000 > +X-Mozilla-Keys: White space @@ +10,5 @@ > +X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0 > +User-Agent: Thunderbird 3.0a2pre (Windows/2008060416) > +MIME-Version: 1.0 > +To: bugmail@example.org > +Subject: [[autoreply without including original subject]] s/original/template/ (though I'd still prefer to use an empty subject for this, if possible) ::: mailnews/test/resources/mailTestUtils.js @@ +136,5 @@ > + i++; > + enumerator.getNext(); > + continue; > + } > + return enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr); The continue; inside the if block is a bit tricky (I had to read it twice to realize it wasn't a bug); how about: while { let next = enumerator.getNext(); if (i == n) { return next.QI(...); } i++; }
Attachment #8369244 -
Flags: review?(irving) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to :Irving Reid from comment #10) > @@ +961,5 @@ > > + mHdrToReplyTo->GetMime2DecodedSubject(replySubject); > > + mTemplateHdr->GetMime2DecodedSubject(templateSubject); > > + nsString subject(NS_LITERAL_STRING("Auto: ")); // RFC 3834 3.1.5. > > + if (StringBeginsWith(templateSubject, NS_LITERAL_STRING("[[")) && > > + StringEndsWith(templateSubject, NS_LITERAL_STRING("]]"))) > > Can we get the same effect by just allowing and detecting an empty > templateSubject? The '[[whatever]]' notation to indicate "don't actually > include this" seems odd. Do we use it anywhere else in Thunderbird? We don't do it elsewhere afaik. The idea is to give a pref per template since what you want may vary. I don't think an empty template subject is useful - it would be a pain to find what template the filter should use and mess with other template usage - you want a subject to actually find the template you need.
Assignee | ||
Comment 12•10 years ago
|
||
So, should I just make it a normal pref perhaps? As I wrote earlier, i don't see the point of including the template subject. E.g. the "i'm on vacation" surely is the first thing in the email anyway. With the "Auto: " hint in the subject people would just have to peak at the body to realize it's a vacation responder.
Assignee | ||
Comment 13•10 years ago
|
||
Using a normal pref for including template subject or not, since that seems to be the controversial part here.
Attachment #8369244 -
Attachment is obsolete: true
Attachment #8369244 -
Flags: review?(kent)
Attachment #8406789 -
Flags: review?(irving)
Comment 14•10 years ago
|
||
Comment on attachment 8406789 [details] [diff] [review] proposed fix Review of attachment 8406789 [details] [diff] [review]: ----------------------------------------------------------------- I'm not happy with controlling the subject handling with a global preference either. I chatted with bwinton about this for a while; aside from that the subject handling probably belongs as a property of the template, we couldn't come up with a user interface that didn't feel wrong in some way. Can we split this out, and have this patch add the Auto-Submitted header and "Auto: " prefix on the subject, and have a separate bug to discuss changing the way we build the template-reply subject? Patch reviews don't feel like a very productive place to discuss that sort of UI change... ::: mailnews/compose/src/nsMsgComposeService.cpp @@ +954,5 @@ > if (replyTo.IsEmpty()) > mHdrToReplyTo->GetAuthor(getter_Copies(replyTo)); > compFields->SetTo(NS_ConvertUTF8toUTF16(replyTo)); > > + bool replyFilterIincludeTemplateSubject = false; If we keep this code, globally s/replyFilterIincludeTemplateSubject/replyFilterIncludeTemplateSubject/ (remove the extra lower case i from "Include") ::: mailnews/compose/test/unit/test_autoReply.js @@ +100,5 @@ > + > + // Parse the headers from the posted message. > + let foundSubject = false; > + let foundAutoSubmitted = false; > + let parser = new ParseString(server._daemon.post); Please use jcranmer's new jsmime tools to parse these test messages and extract headers. See bug 858337 and bug 959309.
Attachment #8406789 -
Flags: review?(irving) → review-
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to :Irving Reid from comment #14) > Please use jcranmer's new jsmime tools to parse these test messages and > extract headers. See bug 858337 and bug 959309. Should I be able to do just let headers = MimeParser.extractHeaders(server._daemon.post); ... or rather, why does that not work? (Doesn't populate any headers.)
Flags: needinfo?(Pidgeot18)
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
(In reply to Magnus Melin from comment #15) > (In reply to :Irving Reid from comment #14) > > Please use jcranmer's new jsmime tools to parse these test messages and > > extract headers. See bug 858337 and bug 959309. > > Should I be able to do just > let headers = MimeParser.extractHeaders(server._daemon.post); > > ... or rather, why does that not work? (Doesn't populate any headers.) Headers here is basically a funky Map-like object. You'll basically want headers.get("Subject") instead of headers["subject"]. The very long documentation comment at <https://github.com/jcranmer/jsmime/blob/master/mimeparser.js#L62> is the best explanation of how the return object works.
Flags: needinfo?(Pidgeot18)
Comment 18•10 years ago
|
||
Comment on attachment 8406789 [details] [diff] [review] proposed fix Review of attachment 8406789 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgComposeService.cpp @@ +986,5 @@ > + compFields->SetSubject(subject); > + > + // We need to end with \r\n - see bug #195965. > + compFields->SetOtherRandomHeaders( > + NS_LITERAL_STRING("Auto-Submitted: auto-replied\r\n")); // RFC 3834 3.1.7. Random drive-by comment: This ain't right, compfields could theoretically have OtherRandomHeaders already set from the UI or front-end (and I see some add-ons may be doing this).
Assignee | ||
Comment 19•10 years ago
|
||
Thx for the hint, that works!
Assignee | ||
Comment 20•10 years ago
|
||
So this uses the new js mime parsing, adds the header and prepends "Auto: " to the reply subject (getMsgHdrN isn't strictly necessary for this bug, but i'd land it now so followups can use it, it does have general usability)
Attachment #8406789 -
Attachment is obsolete: true
Attachment #8407766 -
Attachment is obsolete: true
Attachment #8408480 -
Flags: review?(irving)
Assignee | ||
Comment 21•10 years ago
|
||
Forgot to qrefresh addressing jcranmer's concern about stomping existing random headers)
Attachment #8408480 -
Attachment is obsolete: true
Attachment #8408480 -
Flags: review?(irving)
Attachment #8408487 -
Flags: review?(irving)
Comment 22•10 years ago
|
||
Comment on attachment 8408487 [details] [diff] [review] bug904458_auto-submitted-header.patch Review of attachment 8408487 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I love how much the new mimeParser cleaned up the test code - much easier to see what's going on. Thanks for separating out the change to control whether the template subject is included - I think it's much easier to think about the two changes on their own. ::: mailnews/compose/src/nsMsgComposeService.cpp @@ +954,5 @@ > if (replyTo.IsEmpty()) > mHdrToReplyTo->GetAuthor(getter_Copies(replyTo)); > compFields->SetTo(NS_ConvertUTF8toUTF16(replyTo)); > > + Please remove the extra blank line.
Attachment #8408487 -
Flags: review?(irving) → review+
Assignee | ||
Comment 23•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/7342df10a60f -> FIXED Thx to all! I suggest we use bug 394983 to discuss what we want the subject to be.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 24•10 years ago
|
||
Grr. Backed out due to OSX 1.8 and linux debug orange - https://hg.mozilla.org/comm-central/rev/a00c8fec3a8a https://tbpl.mozilla.org/php/getParsedLog.php?id=38139153&tree=Thunderbird-Trunk&full=1 https://tbpl.mozilla.org/php/getParsedLog.php?id=38148866&tree=Thunderbird-Trunk&full=1 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/compose/test/unit/test_autoReply.js | Test timed out TEST-UNEXPECTED-FAIL | /builds/slave/test/build/xpcshell/tests/mailnews/compose/test/unit/test_autoReply.js | test failed (with xpcshell return code: -6), see following log: PROCESS-CRASH | /builds/slave/test/build/xpcshell/tests/mailnews/compose/test/unit/test_autoReply.js | application crashed [@ mozilla::ReentrantMonitor::Enter()]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 25•10 years ago
|
||
I'm not able to figure out what's crashing. We're not even getting to the code I'm changing (try with debug printfs https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=6885199e2530). Can anyone tell me if it's actually the take server or something that's crashing? Or someone on OSX 1.8 could look at it closer.
Comment 26•10 years ago
|
||
I believe that the issue with the test is that you have mixed two different auto_sync frameworks. I have a fixed patch on the try server now to see if it works.
Comment 27•10 years ago
|
||
So here's the scoop as I see it. The real point they we need to continue the test, occurring after the sending, is in nsMsgComposeSendListener::OnStopSending That method gets its listener from the compose object. Unfortunately the compose object is created in nsMsgTemplateReplyHelper::OnStopRunningUrl but it is created as a local nsCOMPtr. Because the only reference remaining to that object is a weak reference in the smtp server, that object is destroyed when nsMsgTemplateReplyHelper::OnStopRunningUrl exits. So when nsMsgComposeSendListener::OnStopSending tries to access the compose object to notify state listeners and progress listeners, it is not there. So there is no notification sent at onStopSending that we can grab onto for test management. I suppose it works in some cases with earlier notifications like onStopCopy, but not in all cases. In the long run we need to have some way to get a notification of the sending, if for no reason to report errors in filters, but for now I think that the best thing to doo is to just abandon the unit test and land the patch without it.
Comment 28•10 years ago
|
||
Actually I had another idea. Although we cannot capture the OnStopSending event, we can capture the event of adding an item to the FCC folder which happens after that. Here's a patch that does that. It got through the Linux test at https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d6cd20a6ba52 and I'm testing with all types at https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=d59680183f71
Comment 29•10 years ago
|
||
Oh well still crashing.
Comment 30•10 years ago
|
||
Here is another try server run that just runs the test (with no actual checks, but fixed as best as I can) but does not have any of the changes from the bug. Still crashing in the same place. So these test problems are not dependent on any of the changes from this bug. https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=821cdb3c7dd6
Updated•8 years ago
|
Whiteboard: [patchlove]
Target Milestone: Thunderbird 31.0 → ---
Assignee | ||
Comment 31•8 years ago
|
||
Kent, want to have a look a the test, again. Try should be happy with this, though I'll send it of for one more test. Test disabled on OSX because I can't get that to really work.
Attachment #8408487 -
Attachment is obsolete: true
Attachment #8412376 -
Attachment is obsolete: true
Attachment #8697084 -
Flags: review?(rkent)
Comment 32•8 years ago
|
||
Comment on attachment 8697084 [details] [diff] [review] bug904458_auto-submitted-header.patch Review of attachment 8697084 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8697084 -
Flags: review?(rkent) → review+
Assignee | ||
Comment 33•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/a0e462249dc8 -> FIXED
Status: REOPENED → RESOLVED
Closed: 10 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
Comment 34•7 years ago
|
||
Magnus, could be implement a preference to switch off the "Auto:" in the subject. It's not mandatory according to RFC 3834 3.1.5, see bug 1334980 comment #60.
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 35•7 years ago
|
||
Yes, there are different use cases. I'm not sure if it should be a pref or some runtime parameter. For instance when you run a filter manually, is that necessarily and automatic response?
Flags: needinfo?(mkmelin+mozilla)
Comment 36•7 years ago
|
||
(In reply to Magnus Melin from comment #35) >For instance when you run a filter manually, is that necessarily and automatic response? If the filter is only able to manually run then no its not an auto response and should not be treated as one because there is no chance of an auto cycle. That said, if one of the reply addresses within a template is set up with an auto response then its still possible to trigger an auto cycle just not for the from email, but this could be said for any email whether a filtered template or not. I've read through the thread and from what i can see, another checkbox on the filter UI would not be any great pains to the end user, I would certainly welcome the choice. Bracketing the response subject looks hackish to me, like its an after thought, it certainly does not look professional. Here is how i see it: Maintain Senders Subject Check box. Add the headers when filters are run automagically but not manually. Resulting subject. Auto:new subject || Auto:Old subject. I understand the RFC guidelines for this make sense in a case where a reply is in direct response to the content of said email, but there are use cases where the response may not cover some of it if at all. Out of office. You do not want the email subject to be maintained. send the auto header to alert mail servers, but leave the subject to the template. Just my 200 cents worth hopefully its of some use.
You need to log in
before you can comment on or make changes to this bug.
Description
•