Closed Bug 1628800 Opened 10 months ago Closed 9 months ago

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"

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68 wontfix, thunderbird76 wontfix)

RESOLVED FIXED
Thunderbird 77.0
Tracking Status
thunderbird_esr68 --- wontfix
thunderbird76 --- wontfix

People

(Reporter: regner, Assigned: mkmelin)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [bug 1629842 fixed the major issue])

Attachments

(1 file)

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.

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

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

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

Duplicate of this bug: 1629385

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

Duplicate of this bug: 1629725
Duplicate of this bug: 1629469
Duplicate of this bug: 1629656
Duplicate of this bug: 1629103

surprising this didn't get found in beta?

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(geoff)
Summary: After update to TB 68.7.0 (32 bit), mail cannot be sent with distribution lists → After update to TB 68.7.0 (32 bit), mail cannot be sent with distribution list with spaces, errors "not valid email address, not in form of user@host"

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

Flags: needinfo?(geoff) → needinfo?(mkmelin+mozilla)
Summary: After update to TB 68.7.0 (32 bit), mail cannot be sent with distribution list with spaces, errors "not valid email address, not in form of user@host" → After update to TB 68.7.0, mail cannot be sent with distribution list with spaces, errors "not valid email address, not in form of user@host"

(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.

Component: Untriaged → Message Compose Window
Duplicate of this bug: 1629799

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.

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(gilles.lelong)

(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.

Flags: needinfo?(gilles.lelong)

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.)

(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

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.

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

Assignee: nobody → mkmelin+mozilla

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

Status: NEW → ASSIGNED
Summary: After update to TB 68.7.0, mail cannot be sent with distribution list with spaces, errors "not valid email address, not in form of user@host" → 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"

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

Attachment #9140576 - Flags: review?(paul)
Regressed by: 1526456
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(...).
Attachment #9140576 - Flags: review?(paul) → review+

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.

(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

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.

Pushed by mkmelin@iki.fi:
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

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 77.0
Duplicate of this bug: 1631606

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.

Duplicate of this bug: 1633559

Please adjust patch if needed for esr and request uplift

Flags: needinfo?(mkmelin+mozilla)
Duplicate of this bug: 1633793

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.

Flags: needinfo?(mkmelin+mozilla)
Duplicate of this bug: 1634818
Duplicate of this bug: 1635411

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?

Flags: needinfo?(mkmelin+mozilla)

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.

Flags: needinfo?(mkmelin+mozilla)
Whiteboard: [bug 1629842 fixed the major issue]

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

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!

You need to log in before you can comment on or make changes to this bug.