Mailing List does not work when entered in address box

VERIFIED FIXED in Thunderbird 24.0

Status

Thunderbird
Message Compose Window
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: Herm, Assigned: aceman)

Tracking

(Blocks: 1 bug, {regression})

23 Branch
Thunderbird 24.0
regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird22 unaffected, thunderbird23 fixed, thunderbird24 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

5.02 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130409194949

Steps to reproduce:

Typed mailing list name in "to:" field.


Actual results:

Send fails.  Reports email address must be followed by @...


Expected results:

mail should have been sent to addresses in list.  If a messages is created from the address book, individual names populate the "to:" boxes.
(Reporter)

Comment 1

4 years ago
Exact message is:

"XXXX Internal Distribution List <"Internal Email List for Document Distribution"> is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the e-mail."

Problems remains in version 24.  Previously, this feature worked.

Comment 2

4 years ago
Confirmed that this failure happens for me also - version is 23a2 from Aurora channel for linux 64 bit as of yesterday's build. Was working fine in 22a2.

Updated

4 years ago
Keywords: regression, regressionwindow-wanted

Comment 3

4 years ago
Is the problem that the address list does not expand to individual addresses when entered, or does it expand but you nevertheless get the error trying to send it?

Comment 4

4 years ago
It is hard to know since in the compose window you only see the list name and the name in chevrons, but not the list of addresses at that point, but you don't get the chance to proceed further before the popup complains and you can't do anything beyond that. Only once it has been sent can you see the list of addresses for the recipients in the copy of the mail in the Sent folder.

Comment 5

4 years ago
Moving this to the Address Book component as it's definitely related to the autocomplete function somehow.
Component: Message Compose Window → Address Book
(Reporter)

Comment 6

4 years ago
(In reply to rsx11m from comment #3)
> Is the problem that the address list does not expand to individual addresses
> when entered, or does it expand but you nevertheless get the error trying to
> send it?

It appears Thunderbird is interpreting the entry as an individual email address rather than a mailing list.  From the address book, if you click on a list, it opens a compose window with all of the individual addresses shown, but if you attempt to send from Thunderbird's main screen, it auto completes the list name, but appears not to properly link to the list entries.

Comment 7

4 years ago
Not seeing the addresses during compose in the compose window is normal and is the case with 22a2 as well - in other words for a list name, say, mylist, entering that in the To: field gives mylist <mylist>to the right of the To:, and you do not see the expanded list of addresses in the compose window. Once the mail is sent then checking that mail in the Sent folder will then show the expanded list of email addresses corresponding to the list name mylist. That is the behaviour I have seen for quite some time so when I saw that in 23a2 I was not surprised. However you can't get to the point of sending the email in 23a2 so you can't see the expanded list - in 22a2 you could not see the expanded list either but the email would send just fine and then you can see the full list of recipients only after it has been processed and sent on to the MTA.

Comment 8

4 years ago
A recent checkin matching the 23.0 time frame was bug 68784 which moved checking the recipient completeness before other checks, specifically the presence of a subject line.

aceman, can you have a look if maybe that check went too much upfront and prevents the address list from properly expanding prior to sending now? If that's the issue, it should work with a trunk build prior to April 15 and fail with an April 16 or later build.

If I read GenericSendMessage() correctly, it still executes Recipients2CompFields() before CheckValidEmailAddress(), thus msgCompFields should contain the already auto-completed addresses (unless I'm mistaken in assuming that address lists are handled there).
Flags: needinfo?(acelists)

Comment 9

4 years ago
Is it possible that the problem lies in the lines:

-  // crude check that the to, cc, and bcc fields contain at least one '@'.
-  // We could parse each address, but that might be overkill.
-  if (to.length > 0 && (!to.contains("@", 1) && to.toLowerCase() != "postmaster" || to.endsWith("@")))
-    invalidStr = to;
-  else if (cc.length > 0 && (!cc.contains("@", 1) && cc.toLowerCase() != "postmaster" || cc.endsWith("@")))
-    invalidStr = cc;
-  else if (bcc.length > 0 && (!bcc.contains("@", 1) && bcc.toLowerCase() != "postmaster" || bcc.endsWith("@")))
-    invalidStr = bcc;

which comes from https://bug68784.bugzilla.mozilla.org/attachment.cgi?id=733026

i.e. the check seems to "require" that addresses contain "@" which would not apply to a list name?

Comment 10

4 years ago
It would apply to a list /after/ that list was expanded. Thus, expanding the list needs to happen /before/ any of those checks.
(Assignee)

Comment 11

4 years ago
Strange, I am not sure where the list expansion does happen.
I don't think bug 872041 changed the check on what is a valid recipient.

But the bug does exist. I'll check it out.
Assignee: nobody → acelists
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(acelists)
(Assignee)

Comment 12

4 years ago
OK, for now I have at least identified that reverting bug 68784 (moving the checkValidEmailAddress back to where it was) fixes the problem.
Blocks: 68784
Component: Address Book → Message Compose Window
Keywords: regressionwindow-wanted
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 13

4 years ago
Created attachment 751209 [details] [diff] [review]
patch

So DetermineHTMLAction calls checkAndPopulateRecipients and that expands mailinglists per the description at http://mxr.mozilla.org/comm-central/source/mailnews/compose/public/nsIMsgCompose.idl#148 .

So I propose moving the expansion to sooner, before the checkValidEmailAddress. It can also reduce some code duplication.
Attachment #751209 - Flags: review?(mkmelin+mozilla)

Comment 14

4 years ago
Comment on attachment 751209 [details] [diff] [review]
patch

Review of attachment 751209 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with some nits

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4031,5 @@
> +/**
> + * Check what to do with HTML message according to what preference we have
> + * stored for the recipients.
> + *
> + * @param convertible  A nsIMsgCompConvertible constant describing

An

@@ +4046,5 @@
>          var preferFormat;
>  
>          //Check the address book for the HTML property for each recipient
> +        let noHtmlRecipients = expandRecipients(true);
> +        if (noHtmlRecipients === false) {

if (!noHtmlRecipients
with the changes suggested below

@@ +4059,2 @@
>  
>          if (noHtmlRecipients != "" || noHtmlnewsgroups != "")

while here

 if (noHtmlRecipients || noHtmlnewsgroups)

@@ +4061,5 @@
>          {
>              if (convertible == nsIMsgCompConvertible.Plain)
>                return nsIMsgCompSendFormat.PlainText;
>  
>              if (noHtmlnewsgroups == "")

if (!noHtmlnewsgroups)

@@ +4084,5 @@
>              }
>              return nsIMsgCompSendFormat.AskUser;
>          }
>          else
>              return nsIMsgCompSendFormat.HTML;

while here, remove the else. (no else after returns)

@@ +4095,5 @@
> + * Expands mailinglists found in the recipient fields.
> + *
> + * @param aGetNonHtmlRecipients  If this is set to true, the function returns
> + *                               the list of recipients that prefer other format
> + *                               than HTML.

add @return description

@@ +4101,5 @@
> +function expandRecipients(aGetNonHtmlRecipients)
> +{
> +  let obj;
> +  try {
> +    obj = new Object;

you can move the let obj declaration to here. personally i also prefer new Object();

@@ +4106,5 @@
> +    gMsgCompose.checkAndPopulateRecipients(true, aGetNonHtmlRecipients, obj);
> +    return obj.value;
> +  } catch(e) {
> +    Components.utils.reportError("gMsgCompose.checkAndPopulateRecipients failed: " + e);
> +    return false;

null, i guess

@@ +4109,5 @@
> +    Components.utils.reportError("gMsgCompose.checkAndPopulateRecipients failed: " + e);
> +    return false;
> +  }
> +
> +  return true;

unreachable no?
Attachment #751209 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 15

4 years ago
Created attachment 751456 [details] [diff] [review]
patch v2

Thanks, I have reworked the return values of expandRecipients() a bit. Can you please look at it?
Attachment #751209 - Attachment is obsolete: true
Attachment #751456 - Flags: review?(mkmelin+mozilla)

Comment 16

4 years ago
Comment on attachment 751456 [details] [diff] [review]
patch v2

Review of attachment 751456 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4098,5 @@
> + *                               prefer other format than HTML is requested.
> + * @return  If aGetNonHtmlRecipients == true, then returns list of non-HTML
> + *          recipients. If it fails, returns null.
> + *          If aGetNonHtmlRecipients == false, then returns true or false
> + *          depending on whether the expansion of mailinglists succeeded.

Grr, this mixing of return values sure is ugly, like the whole checkAndPopulateRecipients method. 

Could we maybe don't duplicate the ugliness and have one method for expanding and one for getting nonhtml recipients?

@@ +4110,5 @@
> +      return obj.value;
> +
> +    return true;
> +  } catch(e) {
> +    Components.utils.reportError("gMsgCompose.checkAndPopulateRecipients failed: " + e);

i know it was there before but is there really a reason for the try-catch? wouldn't it sooner or later have to go through successful expansion anyway to produce anything useful?
(Assignee)

Comment 17

4 years ago
(In reply to Magnus Melin from comment #16)
> Grr, this mixing of return values sure is ugly, like the whole
> checkAndPopulateRecipients method. 
> 
> Could we maybe don't duplicate the ugliness and have one method for
> expanding and one for getting nonhtml recipients?
Method in nsIMsgCompose.idl ?

> 
> @@ +4110,5 @@
> > +      return obj.value;
> > +
> > +    return true;
> > +  } catch(e) {
> > +    Components.utils.reportError("gMsgCompose.checkAndPopulateRecipients failed: " + e);
> 
> i know it was there before but is there really a reason for the try-catch?
> wouldn't it sooner or later have to go through successful expansion anyway
> to produce anything useful?
But it seems the function can fail. So why is the try-catch bad? What is the other alternative?
Flags: needinfo?(mkmelin+mozilla)

Comment 18

4 years ago
(In reply to :aceman from comment #17)
> (In reply to Magnus Melin from comment #16)
> > Grr, this mixing of return values sure is ugly, like the whole
> > checkAndPopulateRecipients method. 
> > 
> > Could we maybe don't duplicate the ugliness and have one method for
> > expanding and one for getting nonhtml recipients?
> Method in nsIMsgCompose.idl ?

Well, at least your helper js functions. Probably not a bad idea to split out CheckAndPopulateRecipients to not do multiple things, but that's more work for not so high reward.

> > @@ +4110,5 @@
> > > +      return obj.value;
> > > +
> > > +    return true;
> > > +  } catch(e) {
> > > +    Components.utils.reportError("gMsgCompose.checkAndPopulateRecipients failed: " + e);
> > 
> > i know it was there before but is there really a reason for the try-catch?
> > wouldn't it sooner or later have to go through successful expansion anyway
> > to produce anything useful?
> But it seems the function can fail. So why is the try-catch bad? What is the
> other alternative?

Most things can fail, but if it's not an "expected exception" it just unnecessary - and things would fail later anyways - (e.g. the mailing list wasn't expanded). You can keep it if you think that's not the case.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 19

4 years ago
Created attachment 751785 [details] [diff] [review]
patch v3
Attachment #751456 - Attachment is obsolete: true
Attachment #751456 - Flags: review?(mkmelin+mozilla)
Attachment #751785 - Flags: review?(mkmelin+mozilla)

Comment 20

4 years ago
Comment on attachment 751785 [details] [diff] [review]
patch v3

Review of attachment 751785 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't test it, but looks good to me. r=mkmelin

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2684,5 @@
>        msgType == nsIMsgCompDeliverMode.Later ||
>        msgType == nsIMsgCompDeliverMode.Background;
>    if (sending)
>    {
> +    expandRecipients(false);

no arg for this now

@@ +4100,5 @@
> +  gMsgCompose.checkAndPopulateRecipients(true, false, dummyObj);
> +}
> +
> +/**
> + * Returns recipients that prefer other message format than HTML.

that prefer to get messages in plain text.
Attachment #751785 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 21

4 years ago
Created attachment 751798 [details] [diff] [review]
patch v4

Ok, thanks.
Attachment #751785 - Attachment is obsolete: true
Attachment #751798 - Flags: review+
(Assignee)

Comment 22

4 years ago
Herm, rsx11m, are you able to test the patch? Or please comment after it lands in a TB nightly.
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Reporter)

Comment 23

4 years ago
Be happy to test the patch.  Let me know when it is available as part of the nightly build, or send a copy of the modified file, whichever is easier for you.
(Reporter)

Comment 24

4 years ago
(In reply to Herm from comment #23)
> Be happy to test the patch.  Let me know when it is available as part of the
> nightly build, or send a copy of the modified file, whichever is easier for
> you.

Intended to say, or let me know how to apply the patch.  Hit send a little too quickly.
https://hg.mozilla.org/comm-central/rev/4ce23522bc41
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0

Comment 26

4 years ago
Herm, tinderbox builds should be available in 90-120 minutes, you could test those (may be easier than creating your own build or hacking a nightly one).
status-thunderbird22: --- → unaffected
status-thunderbird23: --- → affected
status-thunderbird24: --- → fixed

Updated

4 years ago
Blocks: 360488
(Reporter)

Comment 27

4 years ago
Confirmed. Bug resolved.  Mail list function restored.

Tinderbox build Index of /pub/mozilla.org/thunderbird/tinderbox-builds/comm-central-win32/1369083404 for Win32.

Comment 28

4 years ago
Great, thanks for checking.

aceman, can you please nominate this for comm-aurora?
Status: RESOLVED → VERIFIED
(Assignee)

Comment 29

4 years ago
Comment on attachment 751798 [details] [diff] [review]
patch v4

Thanks for testing.

[Approval Request Comment]
Regression caused by (bug #): bug 68784
User impact if declined: sending mailinglist (group in addressbook) not working
Testing completed (on c-c, etc.): Tinderbox build Index of /pub/mozilla.org/thunderbird/tinderbox-builds/comm-central-win32/1369083404
Risk to taking this patch (and alternatives if risky): unknown, maybe the HTML convertibility could be affected by the patch
Attachment #751798 - Flags: approval-comm-aurora?
Attachment #751798 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/cf6eda1735f7
status-thunderbird23: affected → fixed

Comment 31

4 years ago
Using Thunderbird 17.0.7 on OS X, this bug is NOT fixed.

It still insists on list@host, instead of expanding the addresses within the list.

I use address lists only about once a month and this is the first time it happened.
(Assignee)

Comment 32

4 years ago
That must be a different problem because the one fixed here was only introduced in TB23. You may have similar symptoms on TB17 but caused by a different problem. Please open a separate bug for it.

Comment 33

4 years ago
Since posting the original complaint (#31 above, yesterday) I have been able to test Thunderbird on other systems, in all cases using the current release, 17.0.7.

The (very) brief tests show that Linux (on Ubuntu) and Windows (W 7) both work properly. So the fault seems to be only in the Mac OS X version. However these tests should be verified.
Depends on: 921836

Comment 34

3 years ago
I wonder if it involves the length of the address list. Thunderbird (Mac 24.3.0) failed today (15 Fen 14) with a real-world list about 12 addresses, but worked with a simple test of 3.
(Assignee)

Comment 35

3 years ago
No, 12 addresses should be fine.
However this bug is marked fixed and there were no reports for half a year. Your problem is probably a different bug, please open a new bug. Thanks.

Comment 36

3 years ago
This bug appears to be back in 31.1.  I had trouble that seems to match this bug and others appear to as well: http://forums.mozillazine.org/viewtopic.php?f=39&t=2865353

Should another bug be opened?  A quick search didn't show any, but I may have missed one.
(Assignee)

Comment 37

3 years ago
First make sure you are on TB31.1.1. See bug 1060901. Please do not reopen this one.

Comment 38

a year ago
The behavior I mention below (observed in Thunderbird 38.7.0 of 14 March 2016, as installed on my Ubuntu 14.04LTS Linux system) might be not a bug but merely "how it is".  Also, it may be a consequence of bug 1060901 mentioned in previous comment, 37.

Anyhow, following guidelines in <http://kb.mozillazine.org/Command_line_arguments_(Thunderbird)> for creating an email at the command line, I entered 

thunderbird -compose "to='ttestgroup',subject='test7',body='body7'"

where ttestgroup is a mailing list group with three email addresses on it, a nickname "ttest group", and a description "ttest group w/ 3 me".  When I press return a Thunderbird message box labeled "Write: test7" pops up with subject=test7, body=body7, and the To: field showing as ttestgroup <>.

(Also, an irritating "(process:2531): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed" message appears in the terminal where the command was entered.)

When I click send, a thud sounds, and boxed message appears: "ttestgroup is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the e-mail."  

On the other hand, the "Write: test7" message box popped up by the following command sends ok (but still gets GLib-CRITICAL message):

thunderbird -compose "to='ttestgroup <ttest group w/ 3 me>',subject='test7',body='body7'"


For a different mailing list called ttestgroupN with blank nickname and blank description and three email addresses, the first of the following commands gets a "ttestgroupN is not a valid e-mail address ..." message upon clicking Send, and the second one does not (ie the second form sends ok):

thunderbird -compose "to='ttestgroupN',subject='test7',body='body7'"

thunderbird -compose "to='ttestgroupN <ttestgroupN>',subject='test7',body='body7'"
You need to log in before you can comment on or make changes to this bug.