1628800 - After update to TB 68.7.0, mail cannot be sent with distribution list with spaces from Contacts Sidebar - not valid email address, not in form of user@host"

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[BirdieBob]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

Addressed message with a distribution list that has functioned for years and attempted to send. Repeated attempts with each of 9 distribution lists.

Actual results:

Received this error message:

Distribution 1 <"Distribution 1"> <> 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.

Note in the error message that the "<>" following the distribution list name does not appear in the list name or on the message when addressing it prior to attempting to send.

This occurs when addressing a message with each of 9 distribution lists.

Expected results:

Message should have been sent to addresses in a given distribution list.

Comment 1

[Gil-s]

Hi,

I also noticed this bug.

When composing a message, when selecting a list as recipient
I have an error message :
"" List1 <"list1"> <> is not a valid e-mail address because it is not of the form user@host. You must correct it before sending the email. ""
This operation worked perfectly with this list in version 68.6.0 and earlier for months.
You can notice the <> alone next to the name of the list.
I exported, deleted and re-imported this list: The problem persists.

Can you correct this problem please ? I work with lists everyday.

Best regards

Comment 2

[Gil-s]

Hello,

After some thorough research, it turns out that the workaround is to delete the spaces in the name of the list (I forgot to transcribe the space in the error message)

The fact remains that this is a bug to fix (Thunderbird really has problems with spaces ... a few months ago it was in the names of attachments)

Best regards

Comment 3

[BirdieBob]

Thanks Gil-s. The workaround works but I agree that this is nevertheless a bug that needs to be fixed.

Comment 5

[Francesc Busquets]

Thanks Gil-s for the workaround.
You saved my day!

Comment 10

[Wayne Mery (:wsmwk)]

surprising this didn't get found in beta?

Comment 11

[Geoff Lankow (:darktrojan)]

Looks to me like bug 1526456 is the most likely culprit.

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to Wayne Mery (:wsmwk) from comment #10)

surprising this didn't get found in beta?

If bug 1526456 is the culprit, did I screw up and not make the second patch go through beta?

Regardless of the regressor, let's plan to quick uplift solutions for beta and esr, and adjust tests if needed to catch this edge case.

Comment 14

[Magnus Melin [:mkmelin] (away until Aug 10)]

I would indeed suspect bug 1526456. But I'm not able to reproduce the error. Is there perhaps some space in the list names somehow? (I can't reproduce with that either.) People who reported this, please see what's different with these lists.

Comment 15

[Magnus Melin [:mkmelin] (away until Aug 10)]

Ah, I see Gil-s noted about spaces. Can you provide an example? Does the UI allow creating such a list now?

Comment 16

[Gil-s]

(In reply to Magnus Melin [:mkmelin] from comment #15)

Ah, I see Gil-s noted about spaces. Can you provide an example? Does the UI allow creating such a list now?

OK,
If your list name is "List 1", that provides the error ####List 1 <"List 1"> <> is not a valid e-mail address because it is not of the form user@host###
If you rename "List 1" in "List1" (without space), all works like a charm ...
Regards.

Comment 17

[Magnus Melin [:mkmelin] (away until Aug 10)]

That works for me... Does it work in a new list? Is it possible the list name originally has some spaces in front or after? (The UI do not allow that at least anymore.)

Comment 18

[Gil-s]

(In reply to Magnus Melin [:mkmelin] from comment #14)

I would indeed suspect bug 1526456. But I'm not able to reproduce the error. Is there perhaps some space in the list names somehow? (I can't reproduce with that either.) People who reported this, please see what's different with these lists.
Anglais

I reproduced the error simply by creating a list named "List 1" (with a space between Lant and 1) by placing it as sender from the "Contacts" pane (F9 from the editor) with a double-click on "List 1". This causes an error.
I renamed "List 1" to "List1" (without the space), I reproduced the same operation and the message went to the recipients of my list.

The problem appears in version 68.7.0 and my list containing a space has been working for months with previous versions

Comment 19

[tonychandler7975]

If you have historically had spaces in Mailing List names that worked before 68.7, they do not work now. Upgrade has introduced the error. This applies to selecting a Mailing List name from the left side pane by double clicking on it.
Select the Mailing List name directly into the To: field and it works fine.

Comment 20

[Magnus Melin [:mkmelin] (away until Aug 10)]

I can reproduce that from the contacts sidebar (F9) yes! Did anybody have a problem with lists elsewhere?

Comment 21

[BirdieBob]

Just to confirm what Gil-S has said: Eliminating spaces in the list name resolves the error. I.e., changing "List 1" to "List1"

Comment 22

[Magnus Melin [:mkmelin] (away until Aug 10)]

Attachment: bug1628800_maillist_f9_regression.patch

This code path was using the makeMimeAddress, not the "pretty version".
For ESR we could take the minimal fix and change only that line.

Comment 23

[Paul Morris [:pmorris]]

Comment on attachment 9140576 [details] [diff] [review]
bug1628800_maillist_f9_regression.patch

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

Looks good overall, and great to have more thorough tests.  A few nits, and some questions about mozmill idioms vs mochitest, and about helper functions for double-clicking a tree.  r+ with those addressed.

::: mail/test/browser/composition/browser_contactsSidebar.js
@@ +41,5 @@
> +  const LISTNAME = "Testing List 3";
> +  let list = create_mailing_list(LISTNAME);
> +  addrBook.addMailList(list);
> +
> +  let cwc = open_compose_new_mail();

Can we use a name that makes it easier to tell what it is?  I'm not sure what 'cwc' is abbreviating. "compose window ...something?"

@@ +55,5 @@
> +
> +  let abTree = sidebar.contentDocument.getElementById("abResultsTree");
> +
> +  // The results are loaded async so wait for the population of the tree.
> +  sidebarController.waitFor(

I think this is a mozmill idiom, right?  If so, I think it would be better to use mochitest idioms instead, since we're moving away from mozmill.

@@ +64,5 @@
> +
> +  let pill = cwc.window.document.querySelector(
> +    "#toAddrContainer > mail-address-pill"
> +  );
> +  Assert.equal(pill.getAttribute("label"), `${LISTNAME} <${LISTNAME}>`);

Would be better to use mochitest style assertions instead of mozmill ones, like `is(...)` here.

::: mail/test/browser/shared-modules/FolderDisplayHelpers.jsm
@@ +1053,5 @@
>  
>  /**
> + * Generic method to simulate a double click on a row in a <tree> element.
> + *
> + * @param aTree        The the element.

"The tree element."  Would be nice to have types for these args too, but I won't insist.

@@ +1055,5 @@
> + * Generic method to simulate a double click on a row in a <tree> element.
> + *
> + * @param aTree        The the element.
> + * @param aRowIndex    Index of a row in the tree to dblclick on.
> + * @param aController  Controller object

Nit: missing "." at end of line.

@@ +1057,5 @@
> + * @param aTree        The the element.
> + * @param aRowIndex    Index of a row in the tree to dblclick on.
> + * @param aController  Controller object
> + */
> +function dblclick_tree_row(aTree, aRowIndex, aController) {

Hm, we already have a tree click helper function that will let you do double clicks:  mailTestUtils.treeClick(...)  
See the end of this page: https://developer.thunderbird.net/thunderbird-development/testing/writing-mochitest-tests
Or if this one is the better approach we should use it and drop the other one.

@@ +1073,5 @@
> +  let column = aTree.columns[0];
> +  let coords = aTree.getCoordsForCellItem(aRowIndex, column, "text");
> +
> +  aController.sleep(0);
> +  EventUtils.synthesizeMouse(

In cases like this you should be able to use EventUtils.synthesizeMouseAtCenter(...).

Comment 24

[Geoff Lankow (:darktrojan)]

Some drive-by answers:

(In reply to Paul Morris [:pmorris] from comment #23)

Can we use a name that makes it easier to tell what it is? I'm not sure
what 'cwc' is abbreviating. "compose window ...something?"

cwc is compose window controller, and it's quite commonly used in these tests.

• Assert.equal(pill.getAttribute("label"), ${LISTNAME} <${LISTNAME}>);

Would be better to use mochitest style assertions instead of mozmill ones,
like is(...) here.

That is a mochitest assertion. (Well, technically it isn't as it can be used in other suites, but it's certainly no a mozmill one.) It's actually better than is in that it logs the values being tested instead of just the assertion label.

I think it's reasonable to keep doing things (like waitFor) the mozmill way in what is mostly a mozmill test. We need to overhaul the whole thing but that's not going to happen quickly, so for the most part I'm not replacing one idiom with another unless I can do so for all of the tests.

Comment 25

[Magnus Melin [:mkmelin] (away until Aug 10)]

(In reply to Paul Morris [:pmorris] from comment #23)

Can we use a name that makes it easier to tell what it is? I'm not sure
what 'cwc' is abbreviating. "compose window ...something?"

Like Geoff said, this is used pretty much everywhere in these tests. mc, cwc, and some variations. c is for controller.

Would be better to use mochitest style assertions instead of mozmill ones,
like is(...) here.

mozmill didn't really have assertions. mozmill just relied on no error occurring, so in case of error you'd just throw. If the test run to completion, all was fine. Mochitests want you to check stuff with assertions instead.

Hm, we already have a tree click helper function that will let you do double
clicks: mailTestUtils.treeClick(...)

Great! I'll use that instead.

See the end of this page:
https://developer.thunderbird.net/thunderbird-development/testing/writing-
mochitest-tests

... which could need some update .... as it's now MailTestUtils.jsm

Comment 26

[Paul Morris [:pmorris]]

Thanks for explaining about assertions, cwc, etc. Good to know. Makes sense to keep doing mozmill things in mozmill tests until a larger migration can be done. Maybe we should be using Assert.equal instead of is since it logs the values being tested?

One of my concerns with 'cwc' and similar abbreviations is that as an open source project we ideally want the code to be easily accessible and comprehensible for casual contributors, who might only be working on one test as a one-off contribution. It also helps with getting new full-time contributors up to speed. Of course, that has to be weighed against other factors. If a longer name is too unwieldy, it might be worth adding a comment where 'cwc' is first declared saying "cwc is compose window controller".

I'll update that doc with MailTestUtils.jsm as a .jsm.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/89315675d1ce
clicking mail lists in the contacts sidebar should use mailbox display string, not the mime encoded address. r=pmorris

Comment 28

[JG Berson]

I too started to have this problem after the last TB update.
Address lists with spaces and other non-alpha characters worked for years with no problem until last update. Somebody made a change and did not properly test it.
It needs to be fixed ASAP as I have a dozen lists, all with same naming format with spaces in them.
To edit out all the spaces would make the list names unintelligible.
The fix should be simple if change logs are properly maintained.

Comment 30

[Wayne Mery (:wsmwk)]

With comment 27 the patch is available nightly builds.

The progression from here is it will appear in the next beta build next week, and in roughly 14 days from now in 68.8.0. Until it is fixed in beta or 68.8.0, you should be able to use such an address list from the compose address line or from address book, i.e. not from the contact sidebar.

Comment 32

[Wayne Mery (:wsmwk)]

Please adjust patch if needed for esr and request uplift

Comment 34

[Magnus Melin [:mkmelin] (away until Aug 10)]

Actually this should not be needed on 68 after all. Bug 1629842 fixed the underlying cause. The fix here is correct, just not needed - cosmetic only.

Comment 37

[JG Berson]

Could you please explain what is happening to this?

I read "Won't Fix" then "Fixed" then "Milestone 77.0" in tracking status.

I'm confused. Does this mean it is fixed; but won't be released till Version 77.0 TEN versions from now? Why wait that long?
Or is it not going to be fixed?

Many others have reported this bug, so it is common.

WHEN will we see a version update with a fix?

Comment 38

[Magnus Melin [:mkmelin] (away until Aug 10)]

Turned out this particular patch wasn't needed for 68. The bug was still fixed through bug 1629842 included in Thunderbird 68.8, which was just released.

Comment 39

[JG Berson]

Update to 68 works!!!
Thanks.
I consider this bug closed.

Comment 40

[BirdieBob]

AS JG Berson said, this is resolved and working correctly in 68.8. Thanks from all of us to everyone who got involved over the past month to correct this issue!