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)

defect
Not set
normal

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)

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.
Blocks: 904461
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.
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...
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #794833 - Flags: review?(kent)
Blocks: 394983
Attached patch m904458-test.patch (obsolete) — Splinter Review
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.
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.
Attachment #794833 - Flags: review?(kent) → review-
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.
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.
OS: Windows 7 → All
Hardware: x86_64 → All
- 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)
Attached patch proposed fix w/ test (obsolete) — Splinter Review
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)
rkent: review ping
Attachment #8369244 - Flags: review?(irving)
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+
(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.
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.
Attached patch proposed fix (obsolete) — Splinter Review
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 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-
(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)
Attached patch MimeParser version wip (obsolete) — Splinter Review
(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 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).
Thx for the hint, that works!
Attached patch proposed fix, v3 (obsolete) — Splinter Review
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)
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 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+
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
Flags: in-testsuite+
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 → ---
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.
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.
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.
Attached patch 904458_v4.patch (obsolete) — Splinter Review
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
Oh well still crashing.
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
Whiteboard: [patchlove]
Target Milestone: Thunderbird 31.0 → ---
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)
Blocks: 904478
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+
https://hg.mozilla.org/comm-central/rev/a0e462249dc8 -> FIXED
Status: REOPENED → RESOLVED
Closed: 10 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
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)
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)
(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.

Attachment

General

Created:
Updated:
Size: