Closed
Bug 1008718
Opened 11 years ago
Closed 10 years ago
sending to wrong email (list) if "name" is in address book twice and one of them is a mailing list
Categories
(MailNews Core :: Composition, defect)
Tracking
(thunderbird32 fixed, thunderbird33 fixed, thunderbird34 fixed, thunderbird_esr24 unaffected, thunderbird_esr3133+ fixed)
RESOLVED
FIXED
Thunderbird 34.0
People
(Reporter: craig, Assigned: mkmelin)
References
Details
(Keywords: regression, Whiteboard: [needs documentation])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jcranmer
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
standard8
:
approval-comm-esr31+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.99 Safari/537.36
Steps to reproduce:
In my Personal Address book - I have a mailing list called - Internet Team
I received an email from one of my team, in my address book named as 'Molly Internet Team', I've clicked reply.
The message compose window appears normal, however the 'to' says 'Internet Team' <her email>
Actual results:
When sending, the 'Internet Team' changes to the Mailing List in my address book.
Expected results:
Expected to reply to 'Molly Internet Team'
Reporter | ||
Updated•10 years ago
|
Severity: normal → major
I have a very similar situation. I have several email accounts and when I send a test message between accounts, when received the email has the wrong "From" account. When I click reply it also replies to the wrong account (not the one I sent it from originally). I just noticed this recently but in the past I have always wondered why, when the recipient of an email replies, it doesn't always reply to the account I sent it from.. This seems to be a serious problem as far as I am concerned.
Assignee | ||
Comment 2•10 years ago
|
||
I don't seem able to reproduce.
Assignee | ||
Comment 3•10 years ago
|
||
Craig, did you have any further info that could help track this down. xref/dupe bug 1044559
Keywords: regression
Assignee | ||
Comment 5•10 years ago
|
||
eunall: could you attach the problem mail (as .eml).
Bug 1044559 have sample mails in hebrew, but if you have a more readable testcase that could be useful.
Assignee | ||
Updated•10 years ago
|
Keywords: regressionwindow-wanted
Assignee | ||
Comment 6•10 years ago
|
||
If I had to suspect a regression it would be bug 970118, bug 842632 or a combination of those.
Those who can reproduce: could you try a nightly build from before bug 970118 landed, and report back on the results?
2014-03-03: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/03/2014-03-03-03-02-04-comm-central/
2014-03-05: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2014/03/2014-03-05-03-02-02-comm-central/
Flags: needinfo?(galpeaceman)
Flags: needinfo?(eunall)
Flags: needinfo?(craig)
Comment 7•10 years ago
|
||
Don't know why this is a duplicate of 1044559
Original problem refers mainly to people that don't appear in the address book
Furthermore, original problem sends emails to non-existing Mailing Lists...
But, if this could help resolve the issue, I didn't understand your more info request
What should I do exactly ?
Flags: needinfo?(galpeaceman)
Assignee | ||
Comment 8•10 years ago
|
||
Ok, let's reopen bug 1044559 then, so put the info there.
If you can reproduce, what I'd like you to do is to check it happened to regress around 2014-03-04 by first trying the
2014-03-03 build (download and install), then the 2014-03-05 build and see if there one reproduces the problem and the other not. (If not you could of course go back way further and find the regression range.)
Assignee | ||
Comment 10•10 years ago
|
||
Alexander confirmed in bug 1047305 that it indeed regressed in the range I guessed. So I think it's save to say this is fallout from bug 970118.
I couldn't reproduce in an old profile (not sure why yet), but in a clean profile I did the following, and can now reproduce (on trunk).
1) open the Address Book, select the personal address book
2) add two contacts:
* contact1: display name: m2, email: magnus@netti.fi
* contatc2: display name: Magnus, email: magnus.melin@hut.fi
3) add a list, with list name: magnus
* add contact1 to the list
4) open a new compose window, enter this in the To-line:
Magnus <magnus.melin@hut.fi>
5) add some subject, and message, hit Send Later
6) check Outbox, the source of the message has (only)
To: m2 <magnus@netti.fi>
... which is the address(es) on the list, not the address entered on the To line
Blocks: 970118
Status: UNCONFIRMED → NEW
status-thunderbird31:
--- → affected
status-thunderbird32:
--- → affected
status-thunderbird33:
--- → affected
status-thunderbird34:
--- → affected
status-thunderbird_esr31:
--- → affected
tracking-thunderbird31:
--- → ?
Component: Untriaged → Composition
Ever confirmed: true
Flags: needinfo?(eunall)
Flags: needinfo?(craig)
Keywords: regressionwindow-wanted
OS: Windows 8.1 → All
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All
Summary: Reply-to is sending to wrong email if "name" is in address book twice → sending to wrong email (list) if "name" is in address book twice and one of them is a list
Updated•10 years ago
|
status-thunderbird31:
affected → ---
tracking-thunderbird31:
? → ---
tracking-thunderbird_esr31:
--- → ?
Assignee | ||
Comment 13•10 years ago
|
||
Looks like I have a patch, looking into tests now.
Assignee: nobody → mkmelin+mozilla
Assignee | ||
Comment 14•10 years ago
|
||
So basically the nsMsgMailListComparator is just wrong, and since it's used to differentiate mail addresses from mail lists...
Attachment #8470467 -
Flags: review?(Pidgeot18)
Assignee | ||
Comment 15•10 years ago
|
||
The try run looked ok I think, given the state of the tree - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b2ca7fc2787d
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•10 years ago
|
||
Joshua: ping on the review, we should make sure to get this in soon so it can go into 31.1.
Comment 20•10 years ago
|
||
Comment on attachment 8470467 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch
Review of attachment 8470467 [details] [diff] [review]:
-----------------------------------------------------------------
A comment about the test database:
If feels more appropriate to me under compose/test/unit/data than a unified mailnews/test/data. Even better, though, I think would be to build the database manually in the test code (it's not too complex and it's better documentation within the test file itself).
::: mailnews/compose/src/nsMsgCompose.cpp
@@ +4604,5 @@
> }
> }
> return rv;
> }
> +/**
Nit: add a blank line, please.
@@ +4605,5 @@
> }
> return rv;
> }
> +/**
> + * Comparator; with this we can use IndexOf to find a recipient in an
Nit: "Comparator for use with nsTArray::IndexOf to find a recipient" or something else flows better.
@@ +4622,5 @@
> const nsMsgRecipient &recipient) const {
> + if (!mailList.mName.Equals(recipient.mName,
> + nsCaseInsensitiveStringComparator()))
> + return false;
> + return mailList.mDescription.IsEmpty() ?
Nit: trailing whitespace
@@ +4624,5 @@
> + nsCaseInsensitiveStringComparator()))
> + return false;
> + return mailList.mDescription.IsEmpty() ?
> + mailList.mName.Equals(recipient.mEmail, nsCaseInsensitiveStringComparator()) :
> + mailList.mDescription.Equals(recipient.mEmail, nsCaseInsensitiveStringComparator());
(/me wonders why the name for case-insensitive string comparison is sooo bloody long).
Assignee | ||
Comment 21•10 years ago
|
||
Updated location and nits.
I didn't make it add the list dynamically. It shouldn't be that hard, but I don't have time to do it right now, and with an actual file we can be sure it's a "real" situation we're testing.
Attachment #8470467 -
Attachment is obsolete: true
Attachment #8470467 -
Flags: review?(Pidgeot18)
Attachment #8479316 -
Flags: review?(Pidgeot18)
Updated•10 years ago
|
Attachment #8479316 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Comment 22•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8479316 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch
[Approval Request Comment]
Regression caused by (bug #): 970118
User impact if declined: thunderbird sends mail to a mail list if a displayname happens to be the list name, or if the list name is empty and recipeint display name is empty
Testing completed (on c-c, etc.): works locally and on try, just landed on trunk
Risk to taking this patch (and alternatives if risky): not very risky
Attachment #8479316 -
Flags: approval-comm-esr31?
Assignee | ||
Updated•10 years ago
|
Attachment #8479316 -
Flags: approval-comm-beta?
Attachment #8479316 -
Flags: approval-comm-aurora?
Updated•10 years ago
|
Attachment #8479316 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Comment 24•10 years ago
|
||
status-thunderbird34:
affected → ---
Target Milestone: --- → Thunderbird 34.0
Updated•10 years ago
|
Attachment #8479316 -
Flags: approval-comm-esr31?
Attachment #8479316 -
Flags: approval-comm-esr31+
Attachment #8479316 -
Flags: approval-comm-beta?
Attachment #8479316 -
Flags: approval-comm-beta+
Comment 25•10 years ago
|
||
Updated•10 years ago
|
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
I had to back this out from esr due to test bustage:
https://hg.mozilla.org/releases/comm-esr31/rev/1eeeb979e2b3
The builders are showing:
TEST-UNEXPECTED-FAIL | /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js | ReferenceError: equal is not defined at /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js:42 - See following stack:
JS frame :: /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js:42 :: checkPopulate :: line 3
JS frame :: run_test@/Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js:57:3
JS frame :: /Users/mark/tb/esr31/mozilla/testing/xpcshell/head.js:387 :: _execute_test :: line 5
JS frame :: @-e:1:1
However, even when I change the 'equal' to 'Assert.equal', this fails with:
TEST-UNEXPECTED-FAIL | /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js | "marge <\\"marges own list\\">" == "Simpson <homer@example.com>,Marge <marge@example.com>" - See following stack:
JS frame :: /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js :: checkPopulate :: line 42
JS frame :: /Users/mark/tb/esr31/obj-x86_64-apple-darwin12.5.0/mozilla/_tests/xpcshell/mailnews/compose/test/unit/test_expandMailingLists.js :: run_test :: line 60
JS frame :: /Users/mark/tb/esr31/mozilla/testing/xpcshell/head.js :: _execute_test :: line 387
JS frame :: -e :: <TOP_LEVEL> :: line 1
Updated•10 years ago
|
Attachment #8479316 -
Flags: approval-comm-esr31+ → approval-comm-esr31-
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8479316 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch
Given the severity of this bug, may I suggest we just land the fix without the test for ESR (or disable the test). I have a hard time thinking the code wouldn't work as that code is same on trunk, and there try was ok with it at some point, so it's probably a test issue of some sort.
Attachment #8479316 -
Flags: approval-comm-esr31- → approval-comm-esr31?
Assignee | ||
Comment 29•10 years ago
|
||
Tested a build from https://ftp-ssl.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/comm-esr31-linux64/1409226347/ (which had the fix) and works using manual testing.
Comment 30•10 years ago
|
||
Comment on attachment 8479316 [details] [diff] [review]
bug1008718_sends_to_wrong_addr.patch
Ok, given a manual test works, we'll re-land with the test disabled.
Attachment #8479316 -
Flags: approval-comm-esr31? → approval-comm-esr31+
Comment 31•10 years ago
|
||
Updated•10 years ago
|
Comment 33•10 years ago
|
||
FTR: How recipient autocomplete deals with mailing list names (or not) should be documented on TB's support pages. Reminiscent of Nicknames, where preferential treatment is explicitly wanted and required.
Summary: sending to wrong email (list) if "name" is in address book twice and one of them is a list → sending to wrong email (list) if "name" is in address book twice and one of them is a mailing list
Whiteboard: [needs documentation]
Updated•10 years ago
|
status-thunderbird34:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•