496439 - in-message reply-menu should not include "reply list" when List-Post: value is NO

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

As per section 3.4 of RFC 2369.  The value of NO indicates that the list doesn't allow posting.

This is in use in the wild, though I don't know with what frequency.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

Defaulting to reply all also seems like the wrong choice here.  Assuming this List-Post value is correct and you really can't send mail to it.  I'm also not sure that we want to be clever and do a reply all that doesn't include the list.  Situation: unsure.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #1)
> Defaulting to reply all also seems like the wrong choice here.  Assuming this
> List-Post value is correct and you really can't send mail to it.

The intent, at least, is that that header is an expression by the list owner that they do not allow posting to the list.  It's conceivable, I suppose, that there are major email list hosting sites that improperly apply that header to all of their lists.  I'm not aware of any such, however.

> I'm also not sure that we want to be clever and do a reply all that doesn't
> include the list.  Situation: unsure.

I'd suggest landing a change that only offers "reply" in this situation, and see if anybody screams (which I suspect would only happen if there are major list hosting sites or software that have the problem I described above).

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

If we take that path, I'd suggest pulling it back in to at least b4 so that it gets wider testing.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

When I implement this, I plan on checking that the value starts with "no" (case insensitive), because of a comment on this thread http://www.nabble.com/List-Post:-NO-td22378947.html which said:
> Yep, I know it's valid, although on VALID lists I've only seen it as
> List-Post: NO (explanatory text here)

and the example message here:
http://forums.msexchange.org/m_1800454272/mpage_1/key_/tm.htm#1800454272
which says:
> List-Post: NO (posting not allowed on this list)

(Perhaps I'll check that it starts with "no ".  Anyone have an opinion on which would be better?)

Thanks,
Blake.

Comment 5

[Phil Ringnalda (:philor)]

Wow, that's not the most crystal-clear RFC. Wasn't there a single implementer involved in writing it?

The most important question is "are you going to treat 'List-Post: NO' differently than the absence of a List-Post header?"

If not, don't check for NO at all - check for "^<mailto:.*>$", after filing a bug on the compose backend asking for an actual parser that follows the RFC (as far as it can be followed), so that you can put in an XXX comment saying that once bug nnnnnn provides a parser, we should use it instead. There's no point in having the frontend do anything except in cases when the backend will actually work, and the backend appears to only work with "<mailto:(.*)>" (and to treat what it captures there as an address, even if it's "[e-mail]> (comment about the mail), <http://a.b.com/post").

Even when we have a parser, you and/or the parser don't need to look for NO unless it's treated differently than no List-Post header at all (at least until it becomes a parser for the RFC that obsoletes 2369), because if the field starts with anything other than a "<" following leading whitespace and comments, it and everything following it should be ignored, so unless NO has some unmentioned (and unimaginable) side-effect, "NO" and "YES" and "ILIKEGOATS" and "REPLYTOMYLIST, <mailto:[e-mail]>" are all exactly equivalent to no header at all.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Just to add the section of the RFC Phil is talking about:
> To allow for future extension, client applications MUST follow the
> following guidelines for handling the contents of the header fields
> described in this document:
> 1) Except where noted for specific fields, if the content of the
>    field (following any leading whitespace, including comments)
>    begins with any character other than the opening angle bracket
>    '<', the field SHOULD be ignored.

So a List-Post header with the value of "NO" should be the same as no List-Post header, because it doesn't start with a '<', and so should be ignored...

> 2) Any characters following an angle bracket enclosed URL SHOULD be
>    ignored, unless a comma is the first non-whitespace/comment
>    character after the closing angle bracket.

This indicates that we might want to tighten up our backend's regex, or switch to an address parser, so that we don't try to send to an address of "[e-mail]> (comment about the mail), <http://a.b.com/post".


And to summarize the discussion on IRC, while I would like to treat 'List-Post: NO' differently from the absence of a List-Post header, by not trying to reply to the list, in practice, I don't believe I can tell what the list's address is (precisely because it's not in the List-Post header), and so I have no way of filtering it out.

Comment 7

[Phil Ringnalda (:philor)]

(In reply to comment #6)
> > 1) Except where noted for specific fields, if the content of the
> >    field (following any leading whitespace, including comments)
> >    begins with any character other than the opening angle bracket
> >    '<', the field SHOULD be ignored.
> 
> So a List-Post header with the value of "NO" should be the same as no List-Post
> header, because it doesn't start with a '<', and so should be ignored...

A tiny difference with no practical effect, but "NO" is the "Except where noted" - it shouldn't be ignored because it's an illegal value; it should be ignored because the spec failed to realize that it provides no value and says nothing that isn't also said by no header or a header with a random illegal value, and so there's no point in parsing for it separate from parsing for illegal values.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to disable the Reply List option when the List-Post header is invalid or "NO".

Comment 9

[Phil Ringnalda (:philor)]

What's that patch on top of? There doesn't seem to be any |function isReplyListEnabled()| in my world :)

Comment 10

[Blake Winton (:bwinton) (:☕️)]

D'oh!  I've based it on top of https://bugzilla.mozilla.org/attachment.cgi?id=382141

I did that because I needed to split the detection logic out of the UpdateReplyButtons function so that I could call them to figure out whether to enable the menu items and toolbar buttons, and this patch became much simpler when all the logic was nicely packaged off into a function of its own.

Sorry about that,
Blake.

Comment 11

[Phil Ringnalda (:philor)]

Comment on attachment 382156 [details] [diff] [review]
A patch to disable the Reply List option when the List-Post header is invalid or "NO".

Do you actually need to explicitly test listPost != null, instead of just if (listPost)?

You'll keep hearing this until you can barely stand to type .exec(, but, never use exec unless you actually want to use the matches: if you just want a boolean for matched-or-not (which you very much do, here), use test instead.

There's no really good robust way to point at a bit of code from the comments in another bit, but "in nsMsgCompose.cpp's QuotingOutputStreamListener::OnStopRequest" is more likely to survive refactoring and rewriting while still pointing in the general neighborhood of what needs to stay in sync.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with philor's suggestions.

(In reply to comment #11)
> (From update of attachment 382156 [details] [diff] [review])
> Do you actually need to explicitly test listPost != null, instead of just if
> (listPost)?

Not really.  Fixed.

> You'll keep hearing this until you can barely stand to type .exec(, but, never
> use exec unless you actually want to use the matches: if you just want a
> boolean for matched-or-not (which you very much do, here), use test instead.

Fixed.

> There's no really good robust way to point at a bit of code from the comments
> in another bit, but "in nsMsgCompose.cpp's
> QuotingOutputStreamListener::OnStopRequest" is more likely to survive
> refactoring and rewriting while still pointing in the general neighborhood of
> what needs to stay in sync.

Fixed.

Thanks,
Blake.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with all of philor's suggestions.

D'oh, the match->test change slipped out of my patch.  Here's the full set of changes.

Thanks,
Blake.

Comment 14

[Phil Ringnalda (:philor)]

Comment on attachment 385848 [details] [diff] [review]
The previous patch, with all of philor's suggestions.

Looks good to me. The comment lost the thread of its sentence, but I fixed that before pushing.

Comment 15

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/rev/25e853a01b4c