394216 - opening draft selects different account/identity (IMAP, multi-clients with different/undefined identity number)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Josef Vacatko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: version 2.0.0.6 (20070728)

I have 5 accounts and when I save draft in one account and then open it opens in different account so I often send it from other e-mail than i intend to

Reproducible: Always

Steps to Reproduce:
1. save draft
2. open draft
3.


Expected Results:  
open draft in the same account from which it has been saved

Comment 1

[Magnus Melin [:mkmelin]]

POP or IMAP? At least IMAP seems fine to me...

Comment 2

[Josef Vacatko]

I use IMAP accounts, some of them are at the same IMAP server, but different users.

Comment 3

[Magnus Melin [:mkmelin]]

Do you access the mails from several clients? With accounts in different order perhaps? Check the X-Identity-Key: header of the drafts. That's used to see which identity to use.

Comment 4

[Josef Vacatko]

Yes, I use e-mail client at home and at work - and that's probably the problem, because accounts have different X-Identity-Key on each client. Is it possible to solve this somehow?

But one more thing - as I can see, I have two accounts at home with the same X-Identity-Key and that probably shouldn't happen.

Comment 5

[Magnus Melin [:mkmelin]]

Well, you can find out which identities are the same and change the identities to always correspond to each other. Of course, it can be a bit of work if you have many identities.

Comment 6

[Josef Vacatko]

OK, please can you tell me how to change the identities? May I change the number (X-Identity-Key) somehow or I have to define all identities once again and in the same order on both clients?

Comment 7

[WADA:World Anti-bad-Duping Agency]

Dup of Bug 264626.

You can change internal numbering by manual edit of prefs.js.
mail.account.accountN.identities = idXA,idXB,... points next entries.
 - mail.identity.idXA.xxx
 - mail.identity.idXB.xxx
So following changes can alter identity numbering, from XA/XB to YA/YB.
   mail.identity.idXA.xxx => mail.identity.idYA.xxx
   mail.identity.idXB.xxx => mail.identity.idYB.xxx
   mail.account.accountN.identities = idXA,idXB,...
   => mail.account.accountN.identities = idYA,idYB,...

Comment 8

[M]

This looks like the same problem as the (expired) bug 291452. It is indeed a problem with shared IMAP accounts, and needs a fix.

Changing the ids in the prefs file is not a workable solution if there are several people involved. It is a huge work, prone to errors, and will break every time any of the users reconfigures or changes his/her accounts.

What about a preference to NOT store X-Identity-Key in Drafts?

Comment 9

[David]

SeaMonkey (1.1.13) mail composition bug.
I think this bug in SeaMonkey is common to bugs 264626, 79455, 122352, 462589, 279846, 394216 that I have found. 
The mail in Netscape 7.2 works as expected. 
In a SeaMonkey mail program with multiple accounts, quite often the wrong account and "From" address is used when a message is replied to or forwarded on. 
For example:
An email is recieved in account1's inbox.
It is moved to account2's inbox
It is selected and replied to (or forwarded).
Netscape correctly uses account2's "compose in html" setting and the reply-to address. 
SeaMonkey incorrectly uses account1's settings and uses account1's reply-to address. The "from address" can be over-ridden with the pull down box, but the incorrect formatting etc is used when the reply (or forwarded) 
message is initially generated. 
Selecting a new composition from each account works correctly.
I think that the account settings, from address and formatting etc used when the reply or forwarded message is created, should be from the account where the initial message is currently stored (inbox, draft, template, sent etc), not some obscure address in the header.

Comment 10

[WADA:World Anti-bad-Duping Agency]

Same phenomenon due to same cause(X-Identity-Key:) as Bug 264626. But this bug is IMAP's Drafts unique issue. So setting dependency to Bug 264626.

Comment 11

[somatic]

I can confirm this bug tonight in version 2.0.0.21.

1) Saved draft to account via IMAP.
2) Opened draft on different computer using IMAP.
3) Revised and sent draft.
4) Draft was sent via the default identity account in thunderbird, not the account that the draft had been created and saved within.  Email was also as saved to "sent" folder of default identity account, not "sent" folder of the account the draft had been created in.
5) This is repeatable for other drafts in the IMAP account:  the "From:" field changes silently to the default identity account.

This is an extremely painful bug.

Comment 12

[Jan Peter Stotz]

Is it necesary that the identity references are of the form "id[0-9]" or can I rename them in prefs.js to e.g. "idWork" and "idHome"?

Comment 13

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #12)
Id should be : "id" + (1)*[1-9] + (0 to N)*[0-9] (leading ZERO is not used by Tb)
It's applicable to account number and server number in prefs.js.
I don't know about max N(signed 32 bits or unsigned 32 bits? Or else?).
I don't know what happens if leading ZERO is used. (ignored?)

I used 99999999 for account, server(Local Folders), and Tb worked with no problem.
I'm using 999999 as account/server number for "Local Folders" for ease of editing of account/id/server(to use same number for account/id/server, number+suffix if multiple identities) with SeaMonkey 1.1.17, and I don't have experience of problem due to the manual number change.

Comment 14

[Jan Peter Stotz]

I am sorry, but looking close onto the problem I have to say that I don't understand why the X-Identity-Key entry is needed.

Usually the list of identities managed in one Thunderbird profile is a list of E-Mail addresses (and an assigned name). I assume that in more than 90% of all Thunderbird installations the E-Mail Address uniquely identifies the used identity. 
In such a case there is no need to use the X-Identity-Key, you only have to enumerate through all identities and compare the E-Mail address from the identity with the one from the draft e-mail. If you find only one match you got the identity/profile by that it was created.
Only if there are multiple matches the X-Identity-Key as it is used now should be used.

Comment 15

[David]

Yes, get rid of the X-Identity-Key altogether. There doesn't seem to be any need for it, and all it does is cause problems.

Comment 17

[David]

This problem still exists in Seamonkey 2.0 
AIt is a Major problem/bug/annoyance that needs to be sorted out. 
If you click on an email in an inbox (however it got there and whatever To
address it has), and click reply or forward, the composition window should use
the composition settings, return address, signature file and settings for the
account that the inbox was in. It's that simple.

Comment 18

[Han Hwei Woo]

This has caused some embarassing issues for me, as I represent different organizations using different e-mail accounts. Having the wrong e-mail address/organization info when sending e-mails looks extremely unprofessional, and confusing to clients.

Comment 20

[David]

Bug still there in Seamonkey 2.0.10 !!!

Comment 21

[Jeff W]

I have to agree with Comment 18.  This has caused embarrassment for me.  Is there at least a work-around?  I know you guys are very busy, but this could (sigh) force me out of the SM camp.

/j (SM/Mozilla/NS user since 1995)

Comment 22

[Jeff W]

Still broken in SM 2.2.  Please understand the issue here:

1) at my work computer, I draft an Email in an IMAP account.
2) I go home.  At my home computer, I go to drafts folder.  Email to/from look correct.
3) I open the draft to send it.  The From address has changed to whatever the next one down is on the drop-down associated with the from address)
4) I don't notice the change (hard to see - small black type on blue background).
5) the work email goes to 15 people from a personal Email address.

Please give this some attention when you can?  It's killin' me 8-}

Thanks!
/j

Comment 23

[Jeff W]

No offense guys, but some of the bugs in 2.5 (like "fixing" mail icons so that they reflect message status) seem pretty trivial compared to this one. Is there anything I can do to get this fixed????

Pretty please????

/j

Comment 24

[David]

Exactly. It is a major bug that has been there for almost 5 years!!!

Comment 25

[Eringath]

(In reply to WADA from comment #7)
> Dup of Bug 264626.
> 
> You can change internal numbering by manual edit of prefs.js.
> mail.account.accountN.identities = idXA,idXB,... points next entries.
>  - mail.identity.idXA.xxx
>  - mail.identity.idXB.xxx
> So following changes can alter identity numbering, from XA/XB to YA/YB.
>    mail.identity.idXA.xxx => mail.identity.idYA.xxx
>    mail.identity.idXB.xxx => mail.identity.idYB.xxx
>    mail.account.accountN.identities = idXA,idXB,...
>    => mail.account.accountN.identities = idYA,idYB,...

As for several other users, this bug is a SERIOUS problem -- I wish I could use all 1000 of my votes for this bug!  That said, it doesn't look like it's going to be fixed anytime soon :(

So, I want to try the work around described in comment #7.  However, I have limited experience with editing prefs.js and don't want to make things worse.  So I'd like clarification of the following questions before I go forward:

1.  Can I make the necessary edits to prefs.js from in the Thunderbird Config editor?  Or do I have to make them in a text editor?  I believe I have to make the changes to the preference names (mail.identity.idXA.*) in a text editor (plus since there's ~20 per id this seems like the better way to go).  Is this correct?  (Yes, I'll definitely close Thunderbird and make a backup before making changes.)

2.  In the config editor, when I filter for "mail.account" there is no preference for "mail.account.account2.identities".  There are preferences for
    -mail.account.account1.identities  value=id1
    -mail.account.account3.identities  value=id2
    -mail.account.account4.identities  value=id3
    -mail.account.account5.identities  value=id4
    -mail.account.account6.identities  value=id5
This worries me.

Can I safely add a preference for mail.account.account2.identities  value=id2  (and renumber the other identities and renumber the mail.account preferences)?

3.  I have checked the prefs.js file in 4 different profiles (there are more that I'll have to edit to do this workaround, but I can't access them right now...) and ALL of them are missing mail.account.account2.identities?  Is this another bug?  Or what?  Any ideas?

4.  Are there any other preferences/settings that will be affected by this id renumbering?  For example where is the setting that tells Thunderbird in which folder it should save an account's messages?  Is this setting independent of the account id?

5.  In at least one of my profiles there are id's assigned as follows:
    -mail.account.account1.identities  value=id1
    -mail.account.account2.identities  no such preference exists
    -mail.account.account3.identities  value=id6
    -mail.account.account4.identities  value=id3
    -mail.account.account6.identities  value=id5
    -mail.account.account7.identities  value=id2
    (note the lack of an account5)
(This profile has had email accounts deleted and re-added so that they're listed in a new order).
Can I safely renumber these to *.account1.* id1  through *.account6.* id6?  Or should I keep it as account1, 2, 3, 4, (don't use 5), 6, 7 and assign the id#'s as needed to match up to my other profiles?

5.  Can I simply set the order of my accounts by modifying the mail.accountmanager.accounts preference?

Thanks for any help anyone can provide!

Comment 26

[Eringath]

RE my comment #25:

I think I've found the answer to my question #3:  it seems that by default account2 is for "Local Folders" so it doesn't have a corresponding mail account id.  (I also understand Comment #13 better now, thanks WADA.)

Comment 27

[Eringath]

Sorry about the wordy comments.  I think I've figured it out.  Here's my answers to my q's, in case it helps someone else.

1. Used a text editor.  I ended up building a new prefs.js from the old one (this way I knew which settings I'd already renumbered -- I copied in the settings one "section" at a time and edited them as necessary.)  This way I also knew that I'd "caught" all relevant settings -- i.e. answered my Q#4.

2.  As mentioned in Comment #25 account2 is for local folders and doesn't have an account id.  According to comment #13 you can probably renumber this to account99999 (I left it as account2).

3.  n/a see 2.

4.  n/a see 1.

5.  Yes this worked perfectly.

Comment 28

[Johannes Linke]

this problem still exists in thunderbird 13.

Comment 29

[Azrael AOD]

This has affected me twice in the past few weeks. Solution would be fantastic.

Comment 30

[FlashMartian]

We have a number of people using shared IMAP accounts, it seems that when a template is created using a different machine connected to the same account, on opening the template the "mail from" account is set to some default. 

When I create a template on my own Thunderbird and use it on the same instance, there is no problem and the correct account is selected. It seems that the way this account is selected is not a sensible one, it would be better if the from account was either selected by lookup from the From address or not set at all to avoid the mistake.

This issue has caused our company major embarrassment and really should be resolved.

Comment 31

[David.P]

Same here with TB 24.

And it happens also with drafts that are saved to a local folder (thus not saved to the IMAP server).

Pls. fix

Comment 32

[thenterprises2]

Still seeing this with the latest update to Thunderbird, 31.6.0.

This is a real embarrassment, just as comment 18 states. It may not affect every user of Thunderbird, but it does affect a lot of folks who have multiple e-mail accounts managed by Thunderbird, personal, business, and volunteer-organizational. Inadvertently sending an e-mail using the wrong identity is extremely embarrassing and appears very unprofessional, particularly when the user's expectation of Thunderbird behavior is that a message composed and saved as a draft under a particular identity SHOULD have the default identity be the one under which the draft was originally saved, and not some other e-mail identity.

Comment 33

[u539207]

i have the same problem. my boss is now very angry that my email sent to a business partner has been sent from another account

Comment 34

[MP]

Same problem here. Decided to create an account because I was so annoyed by it happening again... Please fix this.

Comment 35

[Johannes]

Is there any chance this will be fixed? I would really appreciate it!

Comment 36

[wrighrc]

I can confirm that this problem still exists.

I'm using Debian 8.3 x86_64 and thunderbird 38.5.1
(I downloaded thunderbird manually directly)

Can some please update the version number to be at least 38.5.1 and effected platform of this bug?

Any idea when it could be assigned to a human being to fix?

Thanks.

Comment 37

[jmnoelle]

Same problem here. 

Saving on one pc and opening in the drafts folder on a second pc results in changing the "from" account. 
Very annoying, as both accounts are different companies.

Everyone vote for it!

Comment 39

[supertankeren]

Same problem here.

When using for different companies it is a CRUCIAL BUG!

VOTED!

Comment 40

[kadah.coba]

One of my users showed me this happening on their accounts. Someone else would draft the email replies and they would finish it before sending. The "reply-to" account is correct when opening the draft and sending the message, but the received email is addressed from a completely different account and has been causing issues for the company. The only way I can see for her to truly send the email from the desired account is to copy and paste the draft in to a new reply...

I'm rather surprised and annoyed that this bug is apparently 11 years old. I expect this type of thing from Outlook...

Comment 41

[jhataka]

I am glad to see that I am not alone ! I have the same problem as well.
No help so far

Comment 42

[:aceman]

(In reply to Jan Peter Stotz from comment #14)
> I am sorry, but looking close onto the problem I have to say that I don't
> understand why the X-Identity-Key entry is needed.

Because not all settings are stored in the draft itself.
The identity also contains settings like the Sent folders, signature, even certificate information). So we need to know which identity this draft belongs to. Yes, some of those settings are only needed before starting the draft. But some are also needed while editing it or sending it out.

> In such a case there is no need to use the X-Identity-Key, you only have to
> enumerate through all identities and compare the E-Mail address from the
> identity with the one from the draft e-mail. If you find only one match you
> got the identity/profile by that it was created.
> Only if there are multiple matches the X-Identity-Key as it is used now
> should be used.

I would propose the other way round. Check what X-Identity-Key is in the draft, look up that identity and check if the email in it matches the From email that is also stored in that draft. If it matches, use the identity key.

But what to do if the comparison fails? Fall back to the default identity? The bug here is exactly about the fact the identity (sending address) changed silently when opening a draft and the user didn't notice.
So I'd propose warning the user loudly that we couldn't match the identity and let the user choose another one (from those available on the current machine/TB install).

Comment 43

[David]

I agree with Jan Peter Stotz from comment #14, get rid of the X-Identity-Key altogether. There doesn't seem to be any need for it, and all it does is cause problems.

Comment 45

[Uniline IT Support]

Exists in TB 45.2.0

Causing problems for 200 of my clients.

Comment 46

[Robert]

Confirmed in 45.3.0

Comment 47

[Jan Peter Stotz]

The situation in 45.4 is even worse than the years before. Yesterday I opened a draft saved by a TB installation with different account/identity ordering. Therefore I expected problems and was surprised that after opening the draft for editing the correct sender identity was selected. 

However when I pressed "send" the wrong SMTP server (hence the wrong identity) was used! The identity shown in the "from" combo was not the one internally selected and used. But that could only be seen after I opened the dropdown menu of the from-combo.

Comment 48

[Jorg K (CEST = GMT+2)]

Developer's note just for the record:
In bug 1287268 we implemented an attribute creatorIdentityKey
https://hg.mozilla.org/comm-central/rev/1b6364120f38#l2.12
which may (or may not) help addressing this bug.

Comment 49

[Aaron B]

Hello, I've tested the fix mentioned in comment #48 via 52.0a2 and it does not resolve the issue, it only slightly improves things when a draft from IMAP does not have a X-Identity-Key at all (for example created in another client like gmail.com), in which case it will now use the default identity instead of the behavior mentioned comment #47. But the default identity is not always the correct one of course.

That behavior from comment #47 is still present in all other cases which makes this bug worse then originally reported as Thunderbird still uses the wrong identity, but now automatically and silently activates the 'Customize From Address' feature displaying the correct From: header address but with the wrong identity, deceiving the user and ultimately sending from the wrong address.

The core of this issue is that the X-Identity-Key is specific to a given computer/profile. A different computer/profile connected to the same IMAP account could have the identities in a different order, or even have totally different identities. So it is in general not valid to use the keys across different computer/profiles. But apparently from the discussion here there are some advanced uses where a user would want to keep their identities in sync.

After investigating I believe the best fix is described in comment #42, except only warn when the From: email matches a different identity. (otherwise the warning would come everytime the 'Customize From Address' is used on purpose)  Such a warning UI should have the identity matching the From: header highlighted so it can be selected in a single click. Also I think the majority of Thunderbird users have fairly simple setups even when using multiple identities and would want a checkbox in the UI to 'Automatically switch drafts From: [e-mail] to [the selected identity]' in order to never be asked again for that identity. Perhaps it should even be checked by default.

If this is problematic in some way it could be restricted to only running on IMAP accounts, or placed behind a pref.

At this point some technical direction is needed from whoever is in charge. Once provided I would be happy to help write some code for this 9 year old bug!

Also in order to fully support advanced usage (ie 'Customize From Address' with IMAP) and avoid the steps taken in comment #7, the Manage Identities window should sort by identity key and have Move up/Move down buttons to swap keys.

Comment 50

[David]

About time someone did something about this. 
How about putting an option to disable X-Identity-Key. 
It would probably correctly then.

Comment 51

[harry_cube]

I stumbled over this bug and have a few thougths I'd like to share.

What is known is:    The problem does only happen if (by way of imap) a draft is moved to another installation of thunderbird (or rather a different desktop/computer). 

In other words:  Thunderbird handles sender adress correctly as long as "X-Identity-Key", "X-Account-Key", and "from:  of the header match with the corresponding info in the "pref.js", that is
user_pref("mail.identity.idxx.useremail", ....   and  user_pref("mail.account.accountxx.identities", "idxx");

So my approach is:  
- check if the info in the draft header and in the pref.js matches, and if not, retrieve, out of the "pref.js" for the e-mail-address in "from",  the correct "X-Identity-Key" and "X-Account-Key";
- replace  "X-Identity-Key" and "X-Account-Key" in the draft headers by the correct values found

If the sender address does not exist on the instalation (as may easily happen with 2 different installations), then prompt the user to make a choice of an address that is available.




So the first step should be:
1) when a draft or template is moved from the original folder, this should be marked e.g. in the "X-Mozilla-Draft-Info", say e.g. by "forwarded-inline"


2) When opening a draft or template marked with such a  X-Mozilla-Draft-Info:  "forwarded-inline":
   - read   X-Identity-Key   and X-Account-Key of the header of the draft  (in this case (see below):  id4, account 6);
   - read "pref.js", get info about id- and account number (see below respectives lines in pref.js)

     Now, if the info from the draft headers and from the pref.js matches (as is the case in my example here), everything is o.k.

     If not matching:  - read line in pref.js: "mail.identity.id_xx..."  for the given e-mail-address
			- read line in pref.js  "mail.account.account_yy.identities", "id_xx" for the found id_xx to find corresponding account
			and 
			- replace  X-Identity-Key   and X-Account-Key in the draft header

				- if not matching, and if address in "from:"  is not available on this machine, promt the user to make choice




Below I copied an example of a header info of a draft header, and the relavant lines of the pref.js  --as can be seen here, the info of the draft header and the pref.js are matching,  X-Identity-Key and X-Account-Key are already correct, and nothing needs to be corrected in this case.




------draft headers------

X-Identity-Key: id4
X-Account-Key: account6
Subject: .....
To: ....
References: ...
..
From: [e-mail]
.
.
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1


-----pref.js-------

user_pref("mail.identity.id4.useremail", "[e-mail]");
.
.
user_pref("mail.account.account6.identities", "id4");

Comment 52

[harry_cube]

Correction to my above post:  since the problem can happen (in imap) without moving the draft out of the original folder (just by opening it in a different installation/desktop),  every draft (in imap) must be checked for matching of "X-Identity-Key" and "X-Account-Key"; so step 1) needs to be deleted.

Comment 53

[harry_cube]

And one more:

Obviously, there is already something similar in place:  
When I take a draft and manually change the sender address (by drop down menu), and then save and close the changed draft, the draft is correspondingly moved to the draft folder of the address (that is, the account of the address) I just manually entered . When checking the mail headers, I can see that the "X-Identity-Key" and the "X-Account-Key" have been changed to match the entered sender address. (here on the same desktop)

So, as I see it, only the mechanism to check for differring entries in the "pref.js"-file (on a different desktop) as to the allocation of a given sender address to the "X-Identity-Key" and the "X-Account-Key" needs to be added. And the complete procedure then be performed whenever opening a draft (in imap).

Comment 54

[harry_cube]

O.K. - one more try from my side:

The following scenario: Somebody (let’s say Pete) has one computer at work and one at home, on each one Thunderbird is running as e-mail client. Pete has the 3 mail addresses (A, B and C) and uses imap throughout. 

At work, the installation order in the Thunderbird is: (Address, ID#, Account#):
(A, id1, account 1), (B, id2, account 2), (C, id3, account 3),

At home, the installation order in the Thunderbird is: (Address, ID#, Account#):
(A, id3, account 3), (B, id2, account 2), (C, id1, account 1),

Now, Pete is at work and writes an e-mail message from his address A. He doesn’t send the message, so the message gets stored in the draft folder of the account of address A.

Pete goes home, start his computer and wants to send the message he has already written at work. He opens the draft folder of address A: the message has in its headers (A, id1, account 1), [or rather From: address A, X-Identity-Key: id1, X-Account-Key: account1]. But at home on the local computer, the correct allocation for address A would be: (A, id3, account 3). When Pete sends the message, Thunderbird internally uses the address C and the SMPT-server for address C, since on this machine, the “allocation” is: (address C, id1, account 1). But Pete’s intention was to send it from address A….

Now, the solution to the problem could be (as stated here in comment #5) to change the identities to correspond to each other. This could be done, as is stated here in comment #7,  by changing internal numbering by manual edit of prefs.js, so that the set-up at home corresponds to the set-up at work. Obviously, as stated here in comment #8, this is not a viable solution.

So, the solution at hand seems to me:  each time a draft message is opened, Thunderbird needs to check if a given address is “in sync” with the ID# and account# on the respective local machine. To this end, Thunderbird has to read from pref.js the respective combinations (Address, ID#, Account#) of the local machine

This check has to be done every time that a draft is opened, because Thunderbird can never know if the draft has been produced on the local machine or on the remote machine. So, to avoid this, Thunderbird has to make sure that the address is “in sync” with its ID# and Account# on the local machine. 

For this check, it might be necessary for Thunderbird to each time read the pref.js. But as well, Thunderbird could write a little “allocation table” (Address, ID#, Account#), which has to be rewritten only when an e-mail-account is added to or deleted from the local installation. And then just compare this “allocation table” with the draft message at hand, and correct the message headers (X-Identity-Key, X-Account-Key) if necessary.

Comment 55

[harry_cube]

It is me again....sorry for the many words....

I have done some testing with 2 desktop computers...as follows: 
On each computer, I installed thunderbird (“TB”) with 2 IMAP accounts, with the accounts set up in "reversed order" -  the idea being that the reversed chronological order of installing on the computers brings about 
address A/address B -> id1/id2 on first computer (PC)
address A/address B  -> id2/id1 on second computer (netbook)
                                    address A = [e-mail]
                                     address B = [e-mail]

Now, I wrote a new message with sender address B on the netbook (second computer) and saved it without sending it. Then I opened this message in the draft folder on the PC (first computer). As expected, this message had id1/account1 in its headers, whereas the same address on the PC should read id2/account3.  (account 2 seems to be reserved by TB for some local folder, as is mentioned here in some earlier comment).
Headers at this state, see below --->  Example 1

Then I again hit save (on the PC), and the message mysteriously disappeared from this account’s draft folder, only to reappear in the draft folder of the account of address A. In the first line in the headers, it can be seen that the IMAP-server is changed from “imap.gmx… “ to “imap.mnet…”
Headers at this state, see below --->  Example 2.   

Now, I sent the message - and the message now moved into the "sent"-folder of the account of address A. As can be seen in the headers in example 3, the sender address is, as originally intended, address B ("gmx"), but obviously, the SMTP-server of mnet was used, and "return-path" and "message-ID" is address A "mnet". When this message is replied to by hitting "reply", the correct address (address A) is filled in.
Headers at this state, see below --->  Example 3

I also tested how TB handles draft messages when written on an K9 e-mail-client. Actually, TB does everything perfect (headers see examples 7, 8, and 9. As in the examples before:  after opening it in TB (ex. 7), after hitting “save” (ex. 8), and after sending (ex. 9). Looking at the headers example 9, it can be observed that everything about the sending process took place as intended.

Examples 4, 5 and 6 provide further evidence that TB can already handle draft messages perfectly well without “X-Identity-Key” and “X-Account-Key”. Again, I opened on the PC a message that I had written before on the netbook in TB, that was only to check the headers (headers Example 4), but then closed TB for editing the “draft”-folder in TB’s profile folder and deleting the lines “X-Identity-Key:..” and “X-Account-Key:..”. Now again I opened the draft message on the PC and hit “save” (ex. 5).  This time again, everything about the sending is handled as intended (ex. 6).

In conclusion, it could be said that the only positive thing of TB’s current way of handling integrated IMAP-accounts is that TB does at least not screw up the sender address. But the user experience of messages that mysteriously move from one account to another is certainly not very positive.  Also, the headers of the message sent by this procedure do certainly not comply to the usual rules (see example 3), might cause the SMTP-server to refuse sending and might cause spam filters to block the message.

Also in my understanding, the original idea behind the "X-Identity-Key" and "X-Account-Key" probable was for TB to be able to find the folder where to save messages to.  And TB internally probable uses id- and account-numbers instead of mail-addresses for ease of handling. But why not only find out this id- and account-number right before it is needed?  In my view, this moment should be right before TB needs to save a message to a folder and to decide which account/id to use for this.  All info needed is the sender address and the content of the “pref.js”-file.  Obviously, TB is already perfectly able to open a draft which does not have an "X-Identity-Key" and "X-Account-Key" in it (at least when only 2 account are involved, as was also mentined here in comment #49), and then send it (e.g. example 7,8, 9, and examples 4,5,6 for draft messages written on a K9-mail client.

In my understanding, TB anyway uses this "X-Identity-Key" and "X-Account-Key" only for drafts and templates - and when IMAP is concerned, these are more often than not just false and misleading. So I would go with comment # 50, but I would go further and say:  throw out altogether the "X-Identity-Key" and "X-Account-Key".




---------------------------------------------------------------------------------------------

Examples 1, 2, 3:  Headers to show Thunderbird's behaviour when handling draft messages (coming from remote computer) on integrated/connected IMAP-accounts on different computers of the current version 45.5.1:   When opening a draft message (ex. 1), after hitting the "save"-button (ex. 2), and after sending (ex. 3) (remote computer has different "account installation order")

Examples 4,5,6:  same as for example 1,2,3,  but this time with editing the "draft"-file in TB's profile folder before hitting "save"-button. Editing, that is deleting the 2 lines "X-Identity-Key: ..."  and "X-Account-Key: ..."   (intention is to show that TB does perfectly well without X-Identity-and X-Account-Key).

Examples 7,8,9:  same as for example 1,2,3,  but this time the remote computer for writing the draft message was e-mail-client K9  (K9 doen't use X-Identity- and X-Account-Key)

Please check corresponding lines "Subject", "Message-ID" and "Date", especially  between example 7 --> 8, and between example 4 --> 5 to see that I'm not making anything up. It seems to be the usual behaviour, though, that the Message-ID# changes after sending (example 9 and ex. 6 and also ex. 3). 


-----headers example  1  -----

FCC: imap://[e-mail]/Sent
X-Identity-Key: id1
X-Account-Key: account1
From: steffan <[e-mail]>
Subject: test--netbook--gmx
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:27:17 +0100
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0;
 attachmentreminder=0
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 7bit


-----headers example  2  -----

FCC: imap://[e-mail]/INBOX/Sent
X-Identity-Key: id1
X-Account-Key: account1
From: steffan <[e-mail]>
Subject: test--netbook--gmx
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:27:40 +0100
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0;
 attachmentreminder=0
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/html; charset=windows-1252
Content-Transfer-Encoding: 7bit


-----headers example  3  -----

Return-Path: <[e-mail]>
Received: from mail-out.m-online.net ([212.18.0.9]) by mx-ha.gmx.net (mxgmx105
 [212.227.17.5]) with ESMTPS (Nemesis) id 0MAByn-1cN9i004wD-00BLBE for
 <[e-mail]>; Mon, 12 Dec 2016 06:28:29 +0100
Received: from frontend01.mail.m-online.net (unknown [192.168.8.182])
	by mail-out.m-online.net (Postfix) with ESMTP id 3tcWb05Gx3z3hj25
	for <[e-mail]>; Mon, 12 Dec 2016 06:28:28 +0100 (CET)
Received: from localhost (dynscan1.mnet-online.de [192.168.6.68])
	by mail.m-online.net (Postfix) with ESMTP id 3tcWb0590RzvkcC
	for <[e-mail]>; Mon, 12 Dec 2016 06:28:28 +0100 (CET)
X-Virus-Scanned: amavisd-new at mnet-online.de
Received: from mail.mnet-online.de ([192.168.8.182])
	by localhost (dynscan1.mail.m-online.net [192.168.6.68]) (amavisd-new, port 10024)
	with ESMTP id ZaH6yEq_S0U9 for <[e-mail]>;
	Mon, 12 Dec 2016 06:28:27 +0100 (CET)
X-Auth-Info: Atfu62SYyG0MdylnEYx7ebOoGeKSp+ob6FUrBPjy8GHwUMT0T5zTz2/OpJabn5jI
Received: from [192.168.178.37] (ppp-188-174-83-206.dynamic.mnet-online.de [188.174.83.206])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mail.mnet-online.de (Postfix) with ESMTPSA
	for <[e-mail]>; Mon, 12 Dec 2016 06:28:27 +0100 (CET)
From: steffan <[e-mail]>
Subject: test--netbook--gmx
To: [e-mail]
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:28:23 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Envelope-To: <[e-mail]>


-----headers example  4  -----

FCC: imap://[e-mail]/Sent
X-Identity-Key: id1
X-Account-Key: account1
From: steffan <[e-mail]>
Subject: test 2 netbook gmx
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:34:46 +0100
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0;
 attachmentreminder=0
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 7bit


-----headers example  5  -----

FCC: imap://[e-mail]/Sent
X-Identity-Key: id2
X-Account-Key: account3
From: steffan <[e-mail]>
Subject: test 2 netbook gmx
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:39:16 +0100
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0;
 attachmentreminder=0
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 7bit


-----headers example  6  -----

Return-Path: <[e-mail]>
Received: from mout.gmx.net ([212.227.17.22]) by mx-ha.gmx.net (mxgmx011
 [212.227.15.9]) with ESMTPS (Nemesis) id 0MciBt-1bz3br1Fjd-00Hy88 for
 <[e-mail]>; Mon, 12 Dec 2016 06:40:14 +0100
Received: from [192.168.178.37] ([188.174.83.206]) by mail.gmx.com (mrgmx103
 [212.227.17.168]) with ESMTPSA (Nemesis) id 0MHH3P-1cTBdN01PX-00E4KV for
 <[e-mail]>; Mon, 12 Dec 2016 06:40:14 +0100
From: steffan <[e-mail]>
Subject: test 2 netbook gmx
To: [e-mail]
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:40:09 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
....
Envelope-To: <[e-mail]>


-----headers example  7  -----

User-Agent: K-9 Mail for Android
X-K9mail-Identity: !l=0&o=0&pl=0&po=0&qs=PREFIX&f=HTML&s=--%20%0D%0ADiese%20Nachricht%20wurde%20von%20meinem%20Android-Mobiltelefon%20mit%20K-9%20Mail%20gesendet.&p=0&q=NONE
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 8bit
Subject: K9. Test
From: Harry <[e-mail]>
Date: Mon, 12 Dec 2016 06:51:32 +0100
Message-ID: <[e-mail]>


-----headers example  8  -----

FCC: imap://[e-mail]/Sent
X-Identity-Key: id2
X-Account-Key: account3
From: Harry <[e-mail]>
Subject: K9. Test
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:52:53 +0100
X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0;
 attachmentreminder=0
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit


-----headers example  9  -----

Return-Path: <[e-mail]>
Received: from mout.gmx.net ([212.227.15.15]) by mx-ha.gmx.net (mxgmx107
 [212.227.17.5]) with ESMTPS (Nemesis) id 0LxMHu-1ciVTt49ve-01701z for
 <[e-mail]>; Mon, 12 Dec 2016 06:53:39 +0100
Received: from [192.168.178.37] ([188.174.83.206]) by mail.gmx.com (mrgmx003
 [212.227.17.190]) with ESMTPSA (Nemesis) id 0MAgzj-1cMaoC2t9n-00BsTo for
 <[e-mail]>; Mon, 12 Dec 2016 06:53:39 +0100
From: Harry <[e-mail]>
Subject: K9. Test
To: [e-mail]
Message-ID: <[e-mail]>
Date: Mon, 12 Dec 2016 06:53:35 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
.
Envelope-To: <[e-mail]>

Comment 56

[David]

Agree totally with "throw out altogether the "X-Identity-Key" and "X-Account-Key".
"
Also in Seamonkey.

Comment 57

[Aaron B]

Although removing the identity keys would also meet my own needs, there are apparently some advanced uses where a user needs to manually specify the identity, as mentioned in comment #42. For example 2 identities with the same email address (meaning From: header cannot determine which one), one with a digital signing certificate and one without, or even simply having 2 different email signatures.

The other complicating factor is the Customize from address feature which changes the From: header, so we're even more dependent on the identity headers.

Hence the suggestion of detecting and having a warning and option to automatically switch identities when From: does not match the current identity headers, but does match a different identity.

At this point some direction and confirmations of this and what I wrote in comment #49 is needed, before getting to writing some code. I'm new here and not exactly sure how to get the attention of someone in charge? I've set needinfo to a dummy address, my apologies if I didn't do it right. Or should I use the dev mailing list?

Comment 58

[Jorg K (CEST = GMT+2)]

Who is [e-mail]?

Comment 59

[Jorg K (CEST = GMT+2)]

Sorry, I missed the last paragraph of comment #57. Aceman, Magnus and I are Thunderbird peers and can answer questions. Aceman already made a suggestion in comment #42:

Check that the e-mail that belongs to the identity stored in the draft matches the From e-mail stored in the draft. If not, show a warning.

One could add a step where in case it doesn't match, TB will try to find an identity which matches the From e-mail in the draft. It could use this identity if only one matches.

Examples:
Draft: id4, From:[e-mail]
id4 has [e-mail] --> matches.

Draft: id4, From:[e-mail]
id3 has [e-mail] --> matches uniquely, use id3.
id4 has [e-mail]

Draft: id4, From:[e-mail]
id3 has [e-mail] with signature 1
id4 has [e-mail] with signature 2
No unique match --> Warning.

Draft: id4, From:[e-mail]
id3 has [e-mail]
id4 has [e-mail]
No match at all --> Warning.

This shouldn't be too hard to implement in mail/components/compose/content/MsgComposeCommands.js given that the "creator identity key" is already available in creatorIdentityKey.

Do you agree, Aceman?

Comment 60

[Jorg K (CEST = GMT+2)]

Oh, reading comment #55: Drafts from other clients should also be used.

Well, if no creator identity key is set, then just match on the From: address.

Comment 61

[:aceman]

Yes, I think that would be the algorithm.

Comment 62

[Aaron B]

Thanks for the input. Jorg K, just want to make sure, in your examples were you taking into account the 'Customize From Address' feature? In this case your example #4 will happen even when the id's are correct, meaning that feature would always have a warning, even when reopening on the same computer. I think if its a non intrusive notificaton bar this is fine. Drafts from other clients with no From: match would be the same but with default identity.

I agree with your example #2, no warning is needed. Are we sure #3 should warn? Seems that it might be an annoyance for users with that setup, as it would warn even when reopening on the same computer.

However in this case:

Draft: id4, From:[e-mail]
id2 has [e-mail] with signature 1
id3 has [e-mail] with signature 2
id4 has [e-mail]
--> Warning is needed. Should it still switch to the first matching id, or stay with id4 until the user selects?

Comment 63

[Jorg K (CEST = GMT+2)]

(In reply to Aaron B from comment #62)
> ... in your examples were
> you taking into account the 'Customize From Address' feature?
I'm afraid I wasn't :-(
In fact, I came to this bug very late and can admit being wrong on most of what I said ;-)

> In this case
> your example #4 will happen even when the id's are correct, meaning that
> feature would always have a warning, even when reopening on the same
> computer.
Well, I think the "edit From" is meant to allow editing from
[e-mail] to "Mr Right <[e-mail]>" but not change the domain.
So perhaps we need to fine tune this warning business a bit. In case #4 it would warn, but in my example above it shouldn't.  

> I think if its a non intrusive notification bar this is fine.
Good idea.

> Are we sure #3 should
> warn? Seems that it might be an annoyance for users with that setup, as it
> would warn even when reopening on the same computer.
Right, you're right, case #3 shouldn't warn, just pick the identity match.

> However in this case:
> Draft: id4, From:[e-mail]
> id2 has [e-mail] with signature 1
> id3 has [e-mail] with signature 2
> id4 has [e-mail]
> --> Warning is needed.
Yes.

> Should it still switch to the first matching id, or stay with id4 until the user selects?
Good question. I'd go with id2.

Look, we won't be able to offer a solution that fixes 100% of these cases, so a solution that is 80% right and asks in 20% is OK.

For a use of a notification bar take a look at the attachment reminder or perhaps easier the blocked content notification:
Search for |setBlockedContent| in MsgComposeCommands.js.

Comment 64

[harry_cube]

> I agree with your example #2, no warning is needed. Are we sure #3 should
> warn? Seems that it might be an annoyance for users with that setup, as it
> would warn even when reopening on the same computer.
> 

In Comment 62, as to "would warn even when reopening on the same computer", I would suggest (as was mentioned here before by somebody) that the user can set preferences (which would have an effect only on the local machine).  So the user would set options e.g. "for [e-mail] always use id2 without notification".  Another approach could be to make the notification "intrusive" when it is displayed for the first time (the user must make a choice then), and later this notication will be non-intrusive just to keep the user informed. This way it would also be easily accessible for the user in case he wants to set a different option later.

Comment 65

[harry_cube]

I hoped that there was finally some momentum going on for this bug ....obviously not really.

I have another idea for pushing the cause:  Since for this bug, reluctance as to "take it and work on it" seems to be very hard to overcome, I'd like to initiate a "work-on-incentive".  How about if everybody on the "CC-List"  for this bug (38) would give 5 bucks? - I myself would pledge to give at least 10.  Even when subtracting from these 38  the people who work at mozilla, we should be able to raise 150 bucks, I'd like to think. The money could be submitted e.g. with the reference: incentive Bug 394216.

The idea would then be that the courageous coder who decides to work on this bug would receive half of the incentive money at the time he assigns himself to "take it and work on it", and the other half when a working bugfix is submitted. 

I don't know if this collides with any regulations of Thunderbird/Mozilla - I would say this could be seen as a variation of the usual donation system where the donator has a say what his/her donation is used for.

So, let's do this (before the next 9 years have passed by and the bug still has status "new")...

Comment 66

[Jorg K (CEST = GMT+2)]

You need to understand that TB is almost exclusively run by volunteers, we have one part-time paid staff who's looking after trunk builds and the various release channels (Aurora, Beta, ESR). This person doesn't have time for bugs like this one. The volunteers are free to chose what they want to work on.

I had the impression that Aaron was defining the solution and that at some stage we'd receive a patch.

I'd estimate the work here between one and two days with various review loops. I don't think it's doable in three hours which is what you could be paying with $150. So unless someone shows some goodwill here, it's not going to happen.

Comment 67

[harry_cube]

I didn't know that there are almost only volunteers. And I sure do agree that this "incentive" I mentioned doesn't make up for the coding work needed here.  

But yes, I heard that the Thunderbird project is kind of abandoned by Mozilla. And I think it would be really sad if the project would die because of unsufficient funding. But I guess this isn't the proper thread here to discuss these matters in detail.

Comment 68

[Jorg K (CEST = GMT+2)]

You should follow the mailing list TB planning: https://mail.mozilla.org/listinfo/tb-planning
There you can also browse the archives. The Thunderbird project lead by the Thunderbird Council is currently raising around $50K per month. There will be a new financial, legal and organisational home (outside Mozilla) soon, and there will be more people employed, but none of them will be employed to fix bugs like this one. The project still lives from volunteers fixing their most hated bugs. That's how I started.

Comment 69

[Aaron B]

Yes I do plan to work on this eventually, but no guarantees on timeline. My initial investigations show that this may be a very old regression as there is basic unused implementation at MsgComposeCommands.js:2469 but it will need moving and lots of changes to meet the use cases described here.

If someone wishes to financially support this bug that would enable me to get to it more quickly. I have seen some Mozilla bugs that had bountysource.com tasks (or other similar sites) associated, so I don't think there's any rule against doing that.

Comment 70

[harry_cube]

@Aaaron B
Just checked the FAQ in bountysource.com - 
and I made a start and put $30 bounty onto the web address of this bug here.

Comment 71

[David]

If I contribute to this, will it also fix the same problem in Seamonkey using POP?

Comment 72

[:aceman]

I think POP was never mentioned yet in this bug. On IMAP it is understandable why people get into the buggy state.

Can you elaborate how you get the problem with POP? Then we can see if it needs the same fix or a different solution.

Comment 73

[David]

See list of the similar bug reports at the end:
The original code from Netscape worked correctly if someone can find it. 
I currently use "Identity Chooser by speedball2001" to get around the bug. 

A previous post of mine:
Surely the idea of multiple accounts is that an account is setup for each 
email identity that the user has, ie Home, work, charity etc. 
Then it make sense to keep the mail relating to that identity in folders 
in the relevant account, ie any work related emails would be kept in the 
work\inbox, or work\sent etc. 
Emails could get there by either downloading from different pop servers, 
or by dragging from another account if all incoming mails are consolidated to one server.

Composing a new message when a certain account is highlighted works correctly, 
ie the correct formatting is selected (text/html) and the correct from address 
is selected. 

When it comes to replying or forwarding a message that is highlighted, 
it should work the same way, BUT IT DOESNT !!

Please correct me if I am wrong, but I believe the solution is blatantly obvious and very simple:
1 Ignore the X-Account-Key, or add an option to ignore the X-Account-Key
2 When replying to a message, or forwarding a message, use the same subroutine that creates a new message. 
   That correctly Opens a new edit window
   Uses the correct formatting 
   Uses the correct signature, CC and Bcc addresses
   Uses the correct From address 

To cater for different setups/users then it would be sensible to have some options 
that the user could select in their preferences.


466537 	Thunderbird 	Mail Window Front En 	[e-mail] 	UNCO 	--- 	Reply settings should be from the account where the message is filed, not where it was originally sent 	2014-07-07

499786 	MailNews Core 	Composition 	[e-mail] 	UNCO 	--- 	save reply to current folder but if it is the INBOX to Sent 	2012-03-14

751461 	MailNews Core 	Composition 	[e-mail] 	UNCO 	--- 	Sent messages are saved in inbox when "Place replies in the folder.." is UNticked 	2015-09-25

200872 	MailNews Core 	Composition 	[e-mail] 	NEW 	--- 	Pick identity for reply per folder or sender of original message (like mutt) 	2015-09-25

264626 	MailNews Core 	Composition 	[e-mail] 	NEW 	--- 	From address changes to different mail account when forwarding mail containing X-Identity-Key header from sender 	2015-07-09

279846 	MailNews Core 	Composition 	[e-mail] 	NEW 	--- 	With multiple identities, drafts cannot be opened/sent with wrong address (IMAP access from multiple Tb. X-Identity-Key: is used in Drafts, but Identity number is not always same on multiple Tb) 	2016-10-25

327713 	MailNews Core 	Composition 	[e-mail] 	NEW 	--- 	From: address in reply is always set using X-Account-Key: header, even though mail folder owner account does not use "Global Inbox" 	2015-11-04

394216 	Thunderbird 	Message Compose Wind 	[e-mail] 	NEW 	--- 	opening draft selects different account/identity (IMAP, multi-clients with different/undefined identity number) 	11:38:05

474725 	Thunderbird 	Message Reader UI 	[e-mail] 	NEW 	--- 	messagereader: message header / body can get out of sync, don't match 	2012-08-23

515004 	Thunderbird 	Mail Window Front En 	[e-mail] 	NEW 	--- 	Identity guessing relies on X-Account-Key header (If folder owner is not Global Inbox owner, X-Account-Key: shouldn't be used for identity selection) 	2011-11-06

520812 	Thunderbird 	Message Compose Wind 	[e-mail] 	NEW 	--- 	clicking on mail links opens Compose window with wrong identity 	2014-07-06

525787 	SeaMonkey 	MailNews: Compositio 	[e-mail] 	NEW 	--- 	Seamonkey 2.0 POP3 mail: signature file not attached on reply or forward except on first account 	2011-11-03

592935 	Thunderbird 	Mail Window Front En 	[e-mail] 	NEW 	--- 	When forwarding emails from the Sent folder the program sends it from a different account POP/Global inbox. 	2015-11-08

699681 	Thunderbird 	General 	[e-mail] 	NEW 	--- 	[Meta] Preset From: upon Reply/Forward is frequently far different from user's expectation, due to forcing X-Account-Key: based identity selection or "Reply-to-self" on any user, by inappropriate mail addr matching/guessing or Reply-To handling) 	2016-12-31

704613 	Thunderbird 	Message Compose Wind 	[e-mail] 	NEW 	--- 	Email replies are send under wrong identity 	2016-08-26

36482 	MailNews Core 	Composition 	[e-mail] 	NEW 	--- 	Initial identity based on rules 	2014-07-06

122352 	MailNews Core 	Composition 	[e-mail] 	REOP 	--- 	E-mail address incorrectly modified when replying 	2014-07-06

111298 	SeaMonkey 	MailNews: Message Di 	[e-mail] 	RESO 	WONT 	[RFE] Shift + Forward should "Forward As" 	2013-11-11

228562 	Thunderbird 	Mail Window Front En 	[e-mail] 	RESO 	FIXE 	shift+forward should toggle html/plain text 	2013-11-11

522633 	MailNews Core 	Networking: SMTP 	[e-mail] 	RESO 	FIXE 	Migration results in useSecAuth set for SMTP servers even though secure connection is selected 	2012-06-20

233412 	MailNews Core 	Composition 	[e-mail] 	RESO 	WONT 	Prompt before converting HTML original to plain text on Reply, Forward Inline 	2013-12-28

598334 	MailNews Core 	Attachments 	[e-mail] 	RESO 	INVA 	pdf attachment won't open with Adobe Acrobat Reader if its filename has spaces 	2016-10-23

621139 	Thunderbird 	Message Compose Wind 	[e-mail] 	RESO 	DUPL 	Clicking email link in body uses wrong account for new message 	2012-08-23

684259 	Thunderbird 	Untriaged 	[e-mail] 	RESO 	DUPL 	TB6 uses wrong "From:" address in Reply All to Sent message 	2016-08-26

686968 	Thunderbird 	Folder and Message L 	[e-mail] 	RESO 	INCO 	Thunderbird 6.0.2 Sent Mail keeps appearing at Inbox 	2015-12-14

795844 	Thunderbird 	General 	[e-mail] 	RESO 	DUPL 	sender address of draft message changed when opening it with different profile 	2012-10-01

937467 	SeaMonkey 	MailNews: Compositio 	[e-mail] 	RESO 	DUPL 	Forward changes html formatting to plain text in 2.22 	2013-12-28

1271156 	SeaMonkey 	MailNews: Compositio 	[e-mail] 	RESO 	INCO 	From shows idx when email composed from clicking on an email address link 	2016-09-04

79455 	MailNews Core 	Composition 	[e-mail] 	RESO 	WONT 	Reply-To address gets picked up in every compose window 	2015-08-13

Comment 74

[Jorg K (CEST = GMT+2)]

I don't think posting long lists of (unrelated) bugs helps here. It just wastes the already very scarce time of the developers. Please also refer to the BMO etiquette:
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 75

[David]

Well I think they are all related and probably from the same cause.

Comment 76

[harry_cube]

@ Aaron B.
Bountysource.com reported: bounty on this bug has increased to $60.

(Contributions ? anybody else  ?)

Comment 77

[Aaron B]

Attachment: Initial patch for 294216

I've had some time to work on a patch. This is my first Mozilla contribution so I apologize in advance for any review process or coding standards violations. I've tested all the usecases recently described in this bug.

I didn't end up using creatorIdentityKey because in my testing it seems like params.identity is preloaded with it, although I didn't look into that code.

Not entirely sure if I've placed my function definitions in the preferred locations. I couldn't find a standard function to split up email addresses so I made one. I've made use of the existing notification bar at the bottom of the window.

The main other thing I don't know is if there are any encoding or IDN  complications here, or if they're taken of automatically.

Comment 78

[:aceman]

Comment on attachment 8827723 [details] [diff] [review]
Initial patch for 294216

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

Thanks for the patch! This looks neat, nicely short, if it does all that is needed. But does follow the algorithm we proposed in this bug.

I haven't tested it yet (surely Jorg will), I've just found some style nits for now.

The patch seems to follow the needed HG format. Please add more lines of context before and after your changes. We use 8 lines. You can set it in the HG configuration file.

You do not need to include the "changed mail/components/compose/content/MsgComposeCommands.js" lines in the commit message.
Please fold the commit message into a single line if possible, like "Bug 394216 - Search for correct identity when draft header keys do not match From: address."

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2466,5 @@
>          params.originalMsgURI = args.originalMsgURI;
>        if (args.preselectid)
>          params.identity = getIdentityForKey(args.preselectid);
> +      if (args.from)
> +        composeFields.from = args.from

We do preferer to write semicolons where they belong.

@@ +2597,5 @@
> +  if (!params.identity || !params.identity.email || (from && from != params.identity.email.toLowerCase())) {
> +    let identities = MailServices.accounts.allIdentities;
> +    let enumerator = identities.enumerate();
> +    let ident = null;
> +    let fromIdentity = null

Trailing semicolon.

@@ +2599,5 @@
> +    let enumerator = identities.enumerate();
> +    let ident = null;
> +    let fromIdentity = null
> +    let needIdentityWarning = false;
> +    

You added whitespace here.

@@ +2614,5 @@
> +          }
> +        }
> +      }
> +    }
> +    

You added whitespace here.

@@ +2617,5 @@
> +    }
> +    
> +    if (fromIdentity)
> +      params.identity = fromIdentity;
> +    else if(!params.identity || !params.identity.email) {

Space after 'if'.

@@ +2624,5 @@
> +      if (identities.length == 0)
> +        identities = MailServices.accounts.allIdentities;
> +      params.identity = identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);
> +    }
> +    

You added whitespace here.

@@ +2625,5 @@
> +        identities = MailServices.accounts.allIdentities;
> +      params.identity = identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);
> +    }
> +    
> +    // Warn if no or 2+ matches found

We always add a dot after comments when they are full sentences.

@@ +2761,4 @@
>    gAutoSaveKickedIn = false;
>  }
>  
> +function splitEmailAddress(email) {

We should probably have a function for this somewhere.

@@ +5611,5 @@
>  
>    clearNotifications: function(aValue) {
>      this.notificationBar.removeAllNotifications(true);
> +  },
> +  

You added whitespace.

@@ +5612,5 @@
>    clearNotifications: function(aValue) {
>      this.notificationBar.removeAllNotifications(true);
> +  },
> +  
> +  setIdentityWarning: function(identity) {

Please call the argument "aIdentityName" as we prepend 'a' for function arguments and the argument contains just the name string, not the whole identity object.

@@ +5613,5 @@
>      this.notificationBar.removeAllNotifications(true);
> +  },
> +  
> +  setIdentityWarning: function(identity) {
> +    if(!this.notificationBar.getNotificationWithValue('identityWarning')) {

Space after 'if'. Please use double quotes for all strings ("identityWarning").

@@ +5616,5 @@
> +  setIdentityWarning: function(identity) {
> +    if(!this.notificationBar.getNotificationWithValue('identityWarning')) {
> +      this.notificationBar.appendNotification(
> +        this.stringBundle.getFormattedString('identityWarning', [identity]),
> +        'identityWarning', null, this.notificationBar.PRIORITY_WARNING_MEDIUM, null

Will the notification contain a button to dismiss it? Users may check the identity (in From menulist) and fix it if needed and then they would not like to have the notification annoy them for the whole duration of the composition.

@@ +5617,5 @@
> +    if(!this.notificationBar.getNotificationWithValue('identityWarning')) {
> +      this.notificationBar.appendNotification(
> +        this.stringBundle.getFormattedString('identityWarning', [identity]),
> +        'identityWarning', null, this.notificationBar.PRIORITY_WARNING_MEDIUM, null
> +      )

Trailing semicolon missing. Also the closing bracket should be on the previous line.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +420,4 @@
>  blockedContentPrefLabelUnix=Preferences
>  blockedContentPrefAccesskeyUnix=P
>  
> +## Identity matching bug 394216

I think we do not reference bugs in .properties file.

Comment 79

[Jorg K (CEST = GMT+2)]

Comment on attachment 8827723 [details] [diff] [review]
Initial patch for 294216

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

Hey, that's great that we finally get a patch ;-) You'll be in the exclusive club of fixing a bug with a low six-digit number.

Aceman, also a Thunderbird peer, came the review first and found some nits, an I found some more. Could you please correct those and submit a new patch. And please use the commit message Aceman suggested.

I'm clearing the review for now (since I get and ugly red reminder pressuring me), but I'll get right to it when the new patch comes.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2591,4 @@
>  
>    gComposeType = params.type;
>  
> +  // Bug 394216 detect correct identity when missing or mismatched. An identity with no email is likely not valid.

Please no bug numbers is code. We have "blame" to tell us where/why lines were added.

@@ +2607,5 @@
> +        ident = enumerator.getNext();
> +        if (from == ident.email.toLowerCase()) {
> +          if (!fromIdentity)
> +            fromIdentity = ident;
> +          else { //at least 2 matching identities

Nit.
// At least two matching identities found.

@@ +2625,5 @@
> +        identities = MailServices.accounts.allIdentities;
> +      params.identity = identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);
> +    }
> +    
> +    // Warn if no or 2+ matches found

And we try to make the comments English and not a mix of abbreviations:
// Warn if no or more than two matches were found.

@@ +2627,5 @@
> +    }
> +    
> +    // Warn if no or 2+ matches found
> +    if (!fromIdentity || needIdentityWarning) {
> +      // But don't warn for +suffix additions

// But don't warn for +suffix additions ([e-mail]).

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +420,5 @@
>  blockedContentPrefLabelUnix=Preferences
>  blockedContentPrefAccesskeyUnix=P
>  
> +## Identity matching bug 394216
> +identityWarning=A unique identity matching the from address was not found. Message will be sent using identity %S

Maybe a full-stop after the message since you have one inside the message.

Comment 80

[Aaron B]

Attachment: 294216v2

In my testing a close button (x) is automatically added to the notification, I'm presuming that's sufficient.

I searched mail/ mailnews/ for '@' and "@" and found a couple dozen places where an email address is split, all of them doing it inline (and in a variety of different ways :s)

Comment 81

[:aceman]

Comment on attachment 8828428 [details] [diff] [review]
294216v2

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

Thanks, the default closing button on a notification should be fine.
I wonder if we should automatically close the notification if the user operates the identity picker in the From field (like when he changes the identity, then we are sure we can dismiss the notification).

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2591,5 @@
>  
>    gComposeType = params.type;
>  
> +  // Detect correct identity when missing or mismatched. An identity with no email is likely not valid.
> +  let from = MailServices.headerParser.parseEncodedHeader(params.composeFields.from, null)[0]

Missing semicolon. Is it sure that the returned array will have at least one member so that you can safely get [0] ? Either check the array length or it seems there is an out argument to parseEncodedHeader that tell you the count of returned members.

@@ +2617,5 @@
> +    }
> +
> +    if (fromIdentity)
> +      params.identity = fromIdentity;
> +    else if (!params.identity || !params.identity.email) {

We do add {} to all branches of if() if at least one branch needs them.

@@ +2763,5 @@
>  
> +function splitEmailAddress(email) {
> +  let at = email.lastIndexOf('@');
> +  return (at != -1) ? [email.slice(0, at), email.slice(at + 1)] : [email, ''];
> +}

Would  makeFromDisplayAddress() be of use here? 
https://dxr.mozilla.org/comm-central/rev/cd83e52fc61642c9e8f0d7853fe04ffe3fe4c458/mailnews/mime/public/nsIMsgHeaderParser.idl#185
https://dxr.mozilla.org/comm-central/rev/cd83e52fc61642c9e8f0d7853fe04ffe3fe4c458/mailnews/mime/src/mimeJSComponents.js#349

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +419,5 @@
>  
>  blockedContentPrefLabelUnix=Preferences
>  blockedContentPrefAccesskeyUnix=P
>  
> +## Identity matching warning notification bar.

I think you need to add "## LOCALIZATION NOTE(identityWarning): %S contains identity name." here as the next line so translators understand what will be put instead of the placeholder.

Comment 82

[Richard Marti (:Paenglab)]

Comment on attachment 8828428 [details] [diff] [review]
294216v2

Looks good.

Two things:
- Is it possible to make the identity in the notification bold to enhance it a bit?
- Because the notification bar was only used for attachment related things the attachment icon is always shown in the back of the warning icon. I'll attach your patch together with some CSS changes to show the icon only for attachment things.

Comment 83

[Richard Marti (:Paenglab)]

Attachment: patch with CSS changes

Comment 84

[Magnus Melin [:mkmelin]]

Comment on attachment 8828428 [details] [diff] [review]
294216v2

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2627,5 @@
> +    }
> +
> +    // Warn if no or more than one match was found.
> +    if (!fromIdentity || needIdentityWarning) {
> +      // But don't warn for +suffix additions ([e-mail]).

Why do you not want to warn about +addresses? I know I certainly have a bunch of them set up with different identities.

Comment 85

[Magnus Melin [:mkmelin]]

(In reply to Richard Marti (:Paenglab) from comment #82)
> Comment on attachment 8828428 [details] [diff] [review]
> - Is it possible to make the identity in the notification bold to enhance it
> a bit?

While that's possible to do if you touch the innards of the notification manually, notifications are not "supposed to" do such things.

Comment 86

[Aaron B]

Attachment: 294216v4

Thanks for all the help everyone.

If you have +suffix identities they will continue to work as before. That code is simply to avoid the warning when using the Customize From Address feature to add a +suffix on the fly.

Good idea on hiding the notification, I've added that. I also noticed that the content blocking code was clearing all notifications so I added a separate function for it to only clear the right one. Kind of weird because the bar is already used for the attachment warning as well, I'm presuming it wasn't trying to clear both types at once.

makeFromDisplayAddress splits a list of addresses instead of a single address.

I made the identity notification bold before the previous comment was posted, it can be removed if preferred. I used its support for DocumentFragment so did not need to touch its innards, just had to change the translation to use a placeholder. Wasn't sure what element is preferred, if there should be a class, or where to put the CSS.

Comment 87

[:aceman]

(In reply to Aaron B from comment #86)
> Good idea on hiding the notification, I've added that. I also noticed that
> the content blocking code was clearing all notifications so I added a
> separate function for it to only clear the right one. Kind of weird because
> the bar is already used for the attachment warning as well, I'm presuming it
> wasn't trying to clear both types at once.

Great, thanks.

> makeFromDisplayAddress splits a list of addresses instead of a single
> address.

Reading its code it seems it would work on a single address as well.

Comment 88

[Magnus Melin [:mkmelin]]

(In reply to Aaron B from comment #86)
> If you have +suffix identities they will continue to work as before. That
> code is simply to avoid the warning when using the Customize From Address
> feature to add a +suffix on the fly.

You could loop through identities and check that there isn't an identity set up for the + address though.

Comment 89

[Aaron B]

Attachment: 294216v5

(In reply to Magnus Melin from comment #88)
> You could loop through identities and check that there isn't an identity set
> up for the + address though.

The main search loop before that will already find the identity with the +suffix ("if (from == ident.email"...) so that case is already handled with no error. Unless you're referring to removing the +suffix to search a 2nd time, but then keep the suffix in the header? I considered that but wasn't sure if it's worthwhile or if there would be any use cases where that would be invalid since it's a more aggressive matching assumption.

I did notice a minor inconsistency as the multiple matching identities case should always warn regardless of suffix, so I made that change. Also tested makeFromDisplayAddress and didn't see the result needed.

Comment 90

[Magnus Melin [:mkmelin]]

Right, good if it was handled already.

Comment 91

[Jorg K (CEST = GMT+2)]

When you're happy with the solution, please ask for review again.

Comment 92

[Aaron B]

Comment on attachment 8830530 [details] [diff] [review]
294216v5

Thanks was about to ask if I should do that. I'm satisfied with it.

Comment 93

[Jorg K (CEST = GMT+2)]

Comment on attachment 8830530 [details] [diff] [review]
294216v5

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

Thanks, looking good, but needs another round. You can simplify the logic by not having two variables decide whether you need a warning at the end. There are other style nits and trailing spaces.

Most importantly, I need to know what happens to (params.)composeFields.from during your processing.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2466,5 @@
>          params.originalMsgURI = args.originalMsgURI;
>        if (args.preselectid)
>          params.identity = getIdentityForKey(args.preselectid);
> +      if (args.from)
> +        composeFields.from = args.from;

Is it safe to store the from in the compose fields? Existing code doesn't do that. As far as I can tell, this value is not removed, even if no suitable identity is found. Am I missing something?

@@ +2591,5 @@
>  
>    gComposeType = params.type;
>  
> +  // Detect correct identity when missing or mismatched. An identity with no email is likely not valid.
> +  let from = MailServices.headerParser.parseEncodedHeader(params.composeFields.from, null);

I assume that composeFields.from got copied to params.composeFields somewhere, right?

@@ +2592,5 @@
>    gComposeType = params.type;
>  
> +  // Detect correct identity when missing or mismatched. An identity with no email is likely not valid.
> +  let from = MailServices.headerParser.parseEncodedHeader(params.composeFields.from, null);
> +  from = (0 < from.length && from[0] && from[0].email) ? from[0].email.toLowerCase().trim() : null;

Nit: This looks funny. I prefer: from.length > 0, or just from.length.

@@ +2596,5 @@
> +  from = (0 < from.length && from[0] && from[0].email) ? from[0].email.toLowerCase().trim() : null;
> +  if (!params.identity || !params.identity.email || (from && from != params.identity.email.toLowerCase())) {
> +    let identities = MailServices.accounts.allIdentities;
> +    let enumerator = identities.enumerate();
> +    let ident = null;

Move this into the while loop.

@@ +2597,5 @@
> +  if (!params.identity || !params.identity.email || (from && from != params.identity.email.toLowerCase())) {
> +    let identities = MailServices.accounts.allIdentities;
> +    let enumerator = identities.enumerate();
> +    let ident = null;
> +    let fromIdentity = null;

You can lose this variable.

@@ +2598,5 @@
> +    let identities = MailServices.accounts.allIdentities;
> +    let enumerator = identities.enumerate();
> +    let ident = null;
> +    let fromIdentity = null;
> +    let needIdentityWarning = false;

Intitalise to true.

@@ +2603,5 @@
> +
> +    // Search for a matching identity.
> +    if (from) {
> +      while (enumerator.hasMoreElements()) {
> +        ident = enumerator.getNext();

let ident = ...

@@ +2606,5 @@
> +      while (enumerator.hasMoreElements()) {
> +        ident = enumerator.getNext();
> +        if (from == ident.email.toLowerCase()) {
> +          if (!fromIdentity)
> +            fromIdentity = ident;

needIdentityWarning = false;
params.identity = ident;

@@ +2607,5 @@
> +        ident = enumerator.getNext();
> +        if (from == ident.email.toLowerCase()) {
> +          if (!fromIdentity)
> +            fromIdentity = ident;
> +          else { // At least two matching identities found.

Nit: If one part of the if/else has braces, all of them need braces according to our coding style.

@@ +2615,5 @@
> +        }
> +      }
> +    }
> +
> +    if (fromIdentity) {

With the changes above, you can lose the variable.

@@ +2626,5 @@
> +      params.identity = identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);
> +    }
> +
> +    // Warn if no or more than one match was found.
> +    if (!fromIdentity || needIdentityWarning) {

Lose fromIdentity.

@@ +2632,5 @@
> +      let fromParts = splitEmailAddress(from);
> +      let identityParts = splitEmailAddress(params.identity.email.toLowerCase());
> +      if (needIdentityWarning || !(fromParts[1] == identityParts[1] && fromParts[0].startsWith(identityParts[0] + '+')))
> +        gComposeNotificationBar.setIdentityWarning(params.identity.identityName);
> +    }

In all this processing, you never set params.composeFields.from any more. What if the from was originally invalid and your algorithm decided to use the default entity? Then the .from has to change, or am I missing something?

@@ +5609,5 @@
>  
>    isShowingBlockedContentNotification: function() {
>      return !!this.notificationBar.getNotificationWithValue("blockedContent");
>    },
> +  

Nit: trailing spaces.

@@ +5629,5 @@
> +      label.appendChild(document.createTextNode(text[1]));
> +      this.notificationBar.appendNotification(label, "identityWarning", null, this.notificationBar.PRIORITY_WARNING_MEDIUM, null);
> +    }
> +  },
> +  

Nit: And here.

Comment 94

[Jorg K (CEST = GMT+2)]

Comment on attachment 8830530 [details] [diff] [review]
294216v5

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2598,5 @@
> +    let identities = MailServices.accounts.allIdentities;
> +    let enumerator = identities.enumerate();
> +    let ident = null;
> +    let fromIdentity = null;
> +    let needIdentityWarning = false;

Actually, looking at this again, maybe best to make this a number:
suitableIdentities = 0;

@@ +2605,5 @@
> +    if (from) {
> +      while (enumerator.hasMoreElements()) {
> +        ident = enumerator.getNext();
> +        if (from == ident.email.toLowerCase()) {
> +          if (!fromIdentity)

This then becomes suitableIdentities == 0

@@ +2607,5 @@
> +        ident = enumerator.getNext();
> +        if (from == ident.email.toLowerCase()) {
> +          if (!fromIdentity)
> +            fromIdentity = ident;
> +          else { // At least two matching identities found.

You can just lose the else part and just go:
suitableIdentities++;

@@ +2626,5 @@
> +      params.identity = identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);
> +    }
> +
> +    // Warn if no or more than one match was found.
> +    if (!fromIdentity || needIdentityWarning) {

This then becomes: suitableIdentities == 1

Comment 95

[Jorg K (CEST = GMT+2)]

Attachment: 294216export5.patch (JK)

Untested!! This is what I mean. Can you see whether it still works? Also, we have an old-fashioned line-length restriction of 80 characters, yes, it's a pain. So I shortened your lines and also removed the trailing spaces.

I'm still not sure about the (params.)composeFields.from issue.

Comment 96

[Jorg K (CEST = GMT+2)]

Comment on attachment 8830530 [details] [diff] [review]
294216v5

I'm afraid a gave the positive feedback here too soon :-(

I've thought about this a little more away from the screen and this occurred to me. Forget mobile devices, forget using the same IMAP account from two different machines with different account numbering and just look at a very simple case.

One user, one PC, one IMAP account, and now it gets a little difficult, three identities.

1) [e-mail], no signature, id1.
2) [e-mail], signature 1, id2.
3) [e-mail], signature 2, id3.

User composes an e-mail with id3 and changes the from to [e-mail].

That already throws your implementation, doesn't it?

You will enter this if block:
  if (!params.identity || !params.identity.email ||
    (from && from != params.identity.email.toLowerCase())) {
since the from doesn't match the email in the identity. And then in the loop you won't find a match and consequently use the default, id1. Do I read that correctly? If in the loop you discarded the "+hihi", you'd match the address to id1, when it was id3.

I also think you should create a function emailEqual() which tests the equality of two e-mail addresses while disregarding the +* on both. That will make your code more elegant and I believe you need that function more than once. Your test only goes one way, but the identity can also be defined with a + which the user might have removed from the From in a one-off case.

In summary, I think you need to change your condition to:

  if (!params.identity || !params.identity.email ||
    (from && !emailEqual(from, params.identity.email))) {

I think you observed correctly that 'params.identity.key' comes pre-populated with 'creatorIdentityKey', but only if TB created that draft. Otherwise 'params.identity' will be any old identity, most likely the first one of all accounts.

Comment 97

[Jorg K (CEST = GMT+2)]

Attachment: 294216export5.patch (JK v2).

Maybe something like this. You need to implement emailEqual().

Comment 98

[Jorg K (CEST = GMT+2)]

Comment on attachment 8833311 [details] [diff] [review]
294216export5.patch (JK v2).

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ -2473,5 @@
> -
> -        while (enumerator.hasMoreElements()) {
> -          ident = enumerator.getNext();
> -          if (args.from.toLowerCase().trim() == ident.email.toLowerCase()) {
> -            params.identity = ident;

Actually, this *is* the code that pre-populates params.identity. If you remove that, you'll need to use 'creatorIdentityKey' in some way.

@@ +2595,5 @@
>    // An identity with no email is likely not valid.
> +  let from = MailServices.headerParser
> +                         .parseEncodedHeader(params.composeFields.from, null);
> +  from = (from.length > 0 && from[0] && from[0].email) ?
> +    from[0].email.toLowerCase().trim() : null;

Maybe add here:
if (creatorIdentityKey)
  params.identity = ... some expression with creatorIdentityKey ... ;-)

Comment 99

[Aaron B]

Attachment: 294216v8

Thanks for the feedback and updated patch! I've tested this.

(In reply to Jorg K (GMT+1) from comment #93)
> > +      if (args.from)
> > +        composeFields.from = args.from;
> 
> Is it safe to store the from in the compose fields? Existing code doesn't do
> that. As far as I can tell, this value is not removed, even if no suitable
> identity is found. Am I missing something?

This is actually legacy code that never runs, I just updated it for completeness sake, it's inside of this condition:

> if (!params) {
>     // This code will go away soon as now arguments are passed to the window using a object of type nsMsgComposeParams instead of a string

I verified that .from is defined in the composeFields data structure and actually is passed into this function already. params.composeFields.from is used in the code immediately below this patch's main block.

> In all this processing, you never set params.composeFields.from any more.
> What if the from was originally invalid and your algorithm decided to use
> the default entity? Then the .from has to change, or am I missing something?

Identity is less reliable with this bug's key ordering issue, but from is stored directly in the email so it's more authoritative in a sense, we know for sure it's what the user actually wants. For example when 'Customize From Address...' feature is used they are supposed to be different, and the from header takes priority. So we update identity based on from header, but not vice versa.

(In reply to Jorg K (GMT+1) from comment #96)
> since the from doesn't match the email in the identity. And then in the loop
> you won't find a match and consequently use the default, id1. Do I read that
> correctly? If in the loop you discarded the "+hihi", you'd match the address
> to id1, when it was id3.

I've verified that params.identity is also passed into this function, pre-filled based on creatorIdentityKey if available, otherwise the default identity. I'm assuming this can be relied on. So in your example params.identity already has the correct id3, and since the search never finds a match it isn't changed, so that case is handled. 

However you are correct that this does not work in the case of mismatched identities across multiple machines. As I mentioned in a previous comment I debated making the search more agressive as you have done, but I'm not convinced that it's a good idea. I'm not sure if there's any use cases where that would be invalid since it's a more aggressive matching assumption. This whole thing is a bit of a work around after all :)

So I haven't included that in this patch, but if you're confident that we should that's fine with me. However it would need to be done differently then your patch, because if a +suffix partial match identity is found before an exact match, then the exact match will not be used. So you would need to keep going until an exact match is found, keeping track of the best partial match along the way, and then only if no exact match is found use the partial match.

> I also think you should create a function emailEqual() which tests the
> equality of two e-mail addresses while disregarding the +* on both. That
> will make your code more elegant and I believe you need that function more
> than once. Your test only goes one way, but the identity can also be defined
> with a + which the user might have removed from the From in a one-off case.
> 
> In summary, I think you need to change your condition to:
> 
>   if (!params.identity || !params.identity.email ||
>     (from && !emailEqual(from, params.identity.email))) {

Good catch, I've implemented it as emailSimilar for clarity and used it in the warning condition. I don't think it should be used in this initial condition though because then when an email differs only by +suffix it won't benefit from the search for an exactly matching identity that may exist. Note that even if you add the emailSimilar() matching to the search, we still want to avoid warning on +suffix changes with customized from address.

thanks for bearing with me on this bug!

Comment 100

[Jorg K (CEST = GMT+2)]

(In reply to Aaron B from comment #99)
> This is actually legacy code that never runs, I just updated it for
> completeness sake, it's inside of this condition:
> > if (!params) {
> >     // This code will go away soon as now arguments are passed to the window ...

Well, no. This code runs at the moment. Also see bug 454064.

Thunderbird has command line options which are (poorly) documented here:
http://kb.mozillazine.org/Command_line_arguments_(Thunderbird)
You can run: thunderbird.exe -p testing -no-remote -compose "from=[e-mail]" and that code *will* execute.
Or use -h to see all the options.

Would you mind testing that. The |composeFields.from = args.from;| you already have might be all that's required.

I hope to find time to look at the rest of the patch over the weekend.

Comment 101

[Jorg K (CEST = GMT+2)]

Comment on attachment 8836297 [details] [diff] [review]
294216v8

OK, I tried:
thunderbird.exe -p testing -no-remote -compose "from=[e-mail]"

I got:
A unique identity matching the from address was not found. Message will be sent using identity *JK <[e-mail]>*. Nice, but incorrect ;-)

I have these identities in my test setup:
[e-mail] and [e-mail].

So that looks pretty unique to me ;-)

Also, when you submit a new patch, please keep our old-fashioned line-length restriction in mind. I know it sucks, but
+    if (suitableIdentities > 1 || (suitableIdentities == 0 && !emailSimilar(from, params.identity.email)))
and a few lines further down are way too long.

Clearing the review for now until this is fixed.

Comment 102

[Benjamin]

Stumbled over the problem again a few minutes ago... I doubled the amount at bountysource to 120$. 

We have not been that close to a solution for nearly 10 years. Keep up the good work. Thanks everyone who contributed.

Comment 103

[Aaron B]

Attachment: 294216v9

Ah, ok that code comment threw me off. I wasn't able to reproduce your case exactly but fixed a few similar cases, I noticed that even just a plain -compose was throwing it off :s So I think this should do it, only warn if from header exists. Fixed line endings too.

Comment 104

[Jorg K (CEST = GMT+2)]

(In reply to Aaron B from comment #103)
> I wasn't able to reproduce your case exactly but fixed a few similar
> cases, I noticed that even just a plain -compose was throwing it off
Great. I'm really sorry for sending you on a wild goose chase. "My case" was just plain wrong, I had overlooked that I had a news account set up in my test environment with the same e-mail address, hence the warning.

You might not we aware of our extensive test suite. So before we accept anything, we see whether it makes any tests fail. I've done this for you here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=247ce5f06c94dde483fd5bd790896eb84ae9d3c2

In a while you should see a green B and a Z which should also be green. If not, you can click on it, and you will see the test failures your patch caused.

Speaking of tests. We really need the changed behaviour covered by a test. So the test needs to prepare a few identities and then answer a few e-mails. Those e-mails could be pre-canned messages which can just be opened and answered to check the resulting From used. You'd also have to check the notification displayed.

Writing a new Mozmill test is a difficult task for a newcomer. Even I tear my hair out sometimes over those tests. Maybe we can recruit our test expert Aceman who seems to churn them out in his sleep ;-)

Comment 105

[Jorg K (CEST = GMT+2)]

Comment on attachment 8838263 [details] [diff] [review]
294216v9

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

More thoughts.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2597,5 @@
> +                         .parseEncodedHeader(params.composeFields.from, null);
> +  from = (from.length && from[0] && from[0].email) ?
> +    from[0].email.toLowerCase().trim() : null;
> +  if (!params.identity || !params.identity.email ||
> +      (from && from != params.identity.email.toLowerCase())) {

Why don't you use emailSimilar() here? You don't need the whole search if the From already matches the identity's e-mail. What am I missing?

@@ +2606,5 @@
> +    // Search for a matching identity.
> +    if (from) {
> +      while (enumerator.hasMoreElements()) {
> +        let ident = enumerator.getNext();
> +        if (from == ident.email.toLowerCase()) {

I can understand that you don't want to use emailSimilar() here since you'd have to do a "best match" as you explained in comment #99.

@@ +2765,5 @@
> +
> +// Emails are equal ignoring +suffixes ([e-mail]).
> +function emailSimilar(a, b) {
> +  if (!a || !b)
> +    return a == b

Well, as far as I understand, !a is fulfilled by null, 0, false, "" and undefined. I'm not very comfortable that this will do the right thing. I think what you want to say is:
if (!a && !b) return true;
if (!a || !b) return false;
Right?

I'm not sure how useful the result will be if either of the parameters are falsy. In fact, you assure that from is set and identity.email should be set, too. But no harm in catering for bad parameters.

Comment 106

[Aaron B]

(In reply to Jorg K (GMT+1) from comment #105)
> > +  if (!params.identity || !params.identity.email ||
> > +      (from && from != params.identity.email.toLowerCase())) {
> 
> Why don't you use emailSimilar() here? You don't need the whole search if
> the From already matches the identity's e-mail. What am I missing?

My initial reasoning was a draft with +suffix added via 'Customize from Address' synced to a machine that actually has an identity exactly matching the +suffix, so it would be useful to search for it. But I'm having second thoughts now, I suppose its possible someone might want that on purpose to use differing settings in the identity that doesn't match exactly. I'm undecided now.

> Well, as far as I understand, !a is fulfilled by null, 0, false, "" and
> undefined. I'm not very comfortable that this will do the right thing. I
> think what you want to say is:
> if (!a && !b) return true;
> if (!a || !b) return false;
> Right?

Yes that is my intention, as far I can tell it does the same thing as that. |thunderbird -compose| was originally causing a TypeError here, but you're right that doesn't matter now that we assure from is set. dang it I missed a ; again >_<

(In reply to Jorg K (GMT+1) from comment #104)
> Writing a new Mozmill test is a difficult task for a newcomer. Even I tear
> my hair out sometimes over those tests. Maybe we can recruit our test expert
> Aceman who seems to churn them out in his sleep ;-)

I'll take your word for it :) but I'm still willing to contribute somehow. Maybe if someone can get it started, I can help provide different cases, or maybe even add some in myself.

Comment 107

[Jorg K (CEST = GMT+2)]

(In reply to Aaron B from comment #106)
> My initial reasoning was a draft with +suffix added via 'Customize from
> Address' synced to a machine that actually has an identity exactly matching
> the +suffix, so it would be useful to search for it. But I'm having second
> thoughts now, I suppose its possible someone might want that on purpose to
> use differing settings in the identity that doesn't match exactly.
Hmm, I think I lost you here. Can you please give an example.

My case would be:
[e-mail] was changed to [e-mail] and saved as a draft. When you come to reuse that draft, the identity recorded is correct and the e-mail matches, if you disregard the +xx. So why search?

> I'll take your word for it :) but I'm still willing to contribute somehow.
> Maybe if someone can get it started, I can help provide different cases, or
> maybe even add some in myself.
Yes, we'll make a start. It would be good if you could write up your test cases.

Comment 108

[Jorg K (CEST = GMT+2)]

What happens in this example:
id1: [e-mail] (default)
id2: [e-mail]

User saves draft with [e-mail] and re-opens it. Without trying the example and just reading the code, wouldn't it go through the loop, not find an exact match and end up using the default?

Comment 109

[Aaron B]

(In reply to Jorg K (GMT+1) from comment #107)
> Hmm, I think I lost you here. Can you please give an example.
> 
> My case would be:
> [e-mail] was changed to [e-mail] and saved as a draft. When you
> come to reuse that draft, the identity recorded is correct and the e-mail
> matches, if you disregard the +xx. So why search?

In your example I was thinking if there was also another identity for [e-mail], then we could switch to it. The most likely scenario would be with an IMAP sync to another machine with the additional identity before reusing the draft. I'm starting to think its better to not do that switch anyways though.

(In reply to Jorg K (GMT+1) from comment #108)
> What happens in this example:
> id1: [e-mail] (default)
> id2: [e-mail]
> 
> User saves draft with [e-mail] and re-opens it. Without trying the
> example and just reading the code, wouldn't it go through the loop, not find
> an exact match and end up using the default?

params.identity is already populated based on X-Identity-Key if it exists, so since it doesn't find anything in the search it stays unchanged. I had previously only verified this by testing, so I finally did some digging to find where this happens.

MailServices.compose.OpenComposeWindowWithParams get called from different places, sometimes via .OpenComposeWindow, passing an identity if needed, otherwise it falls back to the default identity. In the case of a draft it eventually gets to mailnews/mime/src/mimedrft.cpp:mime_parse_stream_complete() which overrides based on the identityKey. If you think we shouldn't rely on this we can add the code to do the same thing based on creatorIdentityKey in this patch, it's up to you.

Comment 110

[Jorg K (CEST = GMT+2)]

Thanks for the explanation, I finally got it ;-)

I think it would be best to have a couple of test cases that define the desired behaviour.

My preference would be: If there is an identity stored in the draft and its e-mail matches the From using the "similar" function, then accept it don't try to switch to a better choice.

The aim should be not to change current behaviour in case there is only one IMAP access and no mobile devices at play. So if a user has [e-mail] (id1) and [e-mail] (id2) and he uses a draft created with id1 but with From changed to [e-mail], IMHO he/she should still get id1.

This bug is about sending e-mail with a completely undesired e-mail address since different clients accessing the same IMAP data have different account setups. And we will surely fix that, no matter which solution we implement.

As for using creatorIdentityKey: If I read my change here correctly:
https://hg.mozilla.org/comm-central/rev/1b6364120f38#l5.12
https://dxr.mozilla.org/comm-central/rev/6ee27a7af6cd3e7bd25f392048fd40160910275a/mailnews/mime/src/mimedrft.cpp#1308
the identity will always be set from the identityKey if one is found, and so will creatorIdentityKey. So you can use either.

When no identityKey is found in the draft, what will the identity be set to then? The default? Where does that happen? Or maybe the MIME fields are pre-populated with the default and overwritten where indicated. A simple print before the |mdd->identity = overrulingIdentity;| would show the initial value.

Maybe using creatorIdentityKey is somewhat clearer, no? So the "regular" case where the e-mail of the draft creator entity matches the From can be tested first.

Comment 111

[Jorg K (CEST = GMT+2)]

In fact, I did that:

            if (NS_SUCCEEDED(rv) && overrulingIdentity) {
                nsAutoCString k;
                mdd->identity->GetKey(k);
                mdd->identity = overrulingIdentity;
                fields->SetCreatorIdentityKey(identityKey);
                printf ("=== %s %s\n", k.get(), identityKey);
            }

In my test case that prints |=== id1 id5|, so the identity was initialised to id1, but since an identity key was found in the draft, id1 gets overwritten with id5 and that is also stored in creatorIdentityKey.

So I leave it to you which way you want to do it. We introduced creatorIdentityKey to be able to check whether the draft was created by TB and with this bug here in mind.

Comment 112

[harry_cube]

I have another suggestion - especially as to Comment 110  "The aim should be not to change current behaviour in case there is only one IMAP access and no mobile devices at play. "

The suggestion is:  it should be checked ahead of everything else if the draft message was produced on the same local machine 
If it is recognized that draft was produced on same machine then nothing else needs to be checked or changed. And it wouldn't be neccessary to bother a user with any warning/setting window. This would be especially advantageous, since it is very counter-intuitive for a not-so-experienced user if he gets this kind of warnings for a draft he wrote on the same machine.

It would surely be neccessary to also check if - between writing the draft and re-opening it - the setup of the local machine was changed (i.e. an account/id was added/deleted in the meantime). 

Most probably a timestamp of the time of last change of setup would be sufficient to identify that the same TB installation was used and that the installation hasn't been changed in the meantime. This timestamp ("time of last change of account setup") would then have to be written into the pref.js.  This timestamp will then be used when producing a new draft, e.g. a hash value of this timestamp is written in the draft header (suggested name e.g.  "TB account setup hash")


So a possible algorithm for checking if a draft was written on this local machine, plus the account setup remains unchanged since then, could be:  

- check if value of "TB setup time hash" in the draft headers equals a hash value computed from the timestamp ("time of last change of account setup") in the pref.js
- if this is the case, no other checks have to be done  (use id numer and account number as is)


This way, the majority of the cases could be handled with 100% accuracy and without any hassle for the user (this is for all drafts written on the same local machine).

Comment 113

[Jorg K (CEST = GMT+2)]

Comment on attachment 8838263 [details] [diff] [review]
294216v9

(In reply to Jorg K (GMT+1) from comment #111)
> So I leave it to you which way you want to do it. We introduced
> creatorIdentityKey to be able to check whether the draft was created by TB
> and with this bug here in mind.
Maybe params.identity is easier to use since it already contains the identity and not just a key. A comment explaining what's going on would be nice, like:
// When editing a draft, 'params.identity' is pre-polulated with the identity
// that created the draft or the default identity for a "foreign" draft.

I'll clear the review for now until we finalised where with want to go with regards to comment #109, that is, the "switch anyway".

(In reply to harry_cube from comment #112)
Nice idea, but I think that goes beyond the scope of this bug. I think checking that the e-mail matches the identity's e-mail (ignoring +xyz) gets us close.

Comment 115

[Aaron B]

Attachment: 294216v10

Ok I've switched to using emailSimilar in the initial condition.

(In reply to Jorg K (GMT+1) from comment #110)
> When no identityKey is found in the draft, what will the identity be set to
> then? The default? Where does that happen? Or maybe the MIME fields are
> pre-populated with the default and overwritten where indicated. A simple
> print before the |mdd->identity = overrulingIdentity;| would show the
> initial value.

OpenComposeWindowWithParams() uses the default identity if it didn't receive any, but looks like in most cases it does receive one, in the case of a saved draft from mail/base/content/mailCommands.js:ComposeMessage() which I think usually ends up passing the default anyways.

Test cases: not sure how this is typically done but here's what could be tested. This may be too detailed :)

Configuration: several identities, 2 with same email addresses, some with +suffix added.

X-Identity-Key header exists:

1. From header matches X-Identity-Key identity exactly
2. From header similar to X-Identity-Key identity with +suffix appended
3. X-Identity-Key identity similar to From header with +suffix appended
4. command line with preselectid=id2 but no from address
=> identity unchanged from X-Identity-Key, no warning

From header not similar to existing X-Identity-Key identity:

5. no matching identity exists
=> identity unchanged from X-Identity-Key, warning
6. 1 matching identity exists
=> identity changed, no warning
7. 2 or more matching identities exist
=> identity changed to first match, with warning

No X-Identity-Key header:

8. no matching identity exists
=> default identity, warning
9. From header matches default identity
10. command line with no from=
=> unchanged default identity, no warning
11. command line with from= has 1 match
=> identity changed, no warning

Comment 116

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841158 [details] [diff] [review]
294216v10

I think this is OK now, thanks for writing down the tests.

Aceman, can you look at
+function emailSimilar(a, b) {
+  if (!a || !b)
+    return a == b;
I'm not convinced that this is the best way to do it as I said in comment #105 right at the end.

The next step would be to cast those test cases into a test. My suggestions would be to write a test that sets up two identities, and then loads a few messages from file and edits those a draft.

Comment 117

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841158 [details] [diff] [review]
294216v10

Typo:
... and then loads a few messages from file and edits those *as* draft.

Comment 118

[:aceman]

Comment on attachment 8841158 [details] [diff] [review]
294216v10

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2602,5 @@
> +  if (!params.identity || !params.identity.email ||
> +      (from && !emailSimilar(from, params.identity.email))) {
> +    let identities = MailServices.accounts.allIdentities;
> +    let enumerator = identities.enumerate();
> +    let suitableIdentities = 0;

When 'identities' var contains the identity objects, but 'suitableIdentities' does not contain a subset of them, just a count, it could use a better name, like suitableCount.

@@ +2606,5 @@
> +    let suitableIdentities = 0;
> +
> +    // Search for a matching identity.
> +    if (from) {
> +      while (enumerator.hasMoreElements()) {

Here you could use:
for (let ident of fixIterator(identities.enumerate())) {}

@@ +2624,5 @@
> +      identities = MailServices.accounts.defaultAccount.identities;
> +      if (identities.length == 0)
> +        identities = MailServices.accounts.allIdentities;
> +      params.identity =
> +        identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);

There exist states in TB where there exist no identities (e.g. a profile with a RSS account only). So fetching the first identity could fail here. Of course, it is questionable if in such a profile you can get into the composition and run this code you add.

@@ +2759,5 @@
>  
>    gAutoSaveKickedIn = false;
>  }
>  
> +function splitEmailAddress(email) {

Please call it aEmail.

@@ +2760,5 @@
>    gAutoSaveKickedIn = false;
>  }
>  
> +function splitEmailAddress(email) {
> +  let at = email.lastIndexOf('@');

You do not use quotes on single character strings consistently. I think we prefer double quotes.

@@ +2765,5 @@
> +  return (at != -1) ? [email.slice(0, at), email.slice(at + 1)] : [email, ''];
> +}
> +
> +// Emails are equal ignoring +suffixes ([e-mail]).
> +function emailSimilar(a, b) {

If there are 'a' and 'b' in the example, do not name the arguments the same.

@@ +2767,5 @@
> +
> +// Emails are equal ignoring +suffixes ([e-mail]).
> +function emailSimilar(a, b) {
> +  if (!a || !b)
> +    return a == b;

So to address Jorg's question, what do we want to do here if a or b is 0, null, undefined, false or some other falsy value? Do we want '0 == null' to return true? If one of the emails is proper, this will return false, which is probably what you wanted.

@@ +5635,5 @@
> +      let label = new DocumentFragment();
> +      label.appendChild(document.createTextNode(text[0]));
> +      label.appendChild(document.createElement("b"));
> +      label.lastChild.appendChild(document.createTextNode(aIdentityName));
> +      label.appendChild(document.createTextNode(text[1]));

This seems overcomplicated to me. You can't assume where the {{identityName}} placeholder will be in the text when it gets translated into various languages. It may even end up at the end of the string so text[1] could be empty. Anyway, you do not need to care.
Something like this may work for you:
label.innerHTML = this.stringBundle.getFormattedString("identityWarning", ["<b>"+aIdentityName+"</b>"]);

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +420,5 @@
>  blockedContentPrefLabelUnix=Preferences
>  blockedContentPrefAccesskeyUnix=P
>  
> +## Identity matching warning notification bar.
> +## LOCALIZATION NOTE(identityWarning): {{identityName}} will be replaced at run time with the identity name.

We use %S as placeholders (see even in the lines above). Then the bundle.getFormattedString() I proposed above will work.

Comment 119

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #118)
> When 'identities' var contains the identity objects, but
> 'suitableIdentities' does not contain a subset of them, just a count, it
> could use a better name, like suitableCount.
Sorry, my bad choice of name :-(

Thanks for looking at it thoroughly, Aceman, much appreciated.

Comment 120

[Aaron B]

(In reply to :aceman from comment #118)
> @@ +5635,5 @@
> > +      let label = new DocumentFragment();
> > +      label.appendChild(document.createTextNode(text[0]));
> > +      label.appendChild(document.createElement("b"));
> > +      label.lastChild.appendChild(document.createTextNode(aIdentityName));
> > +      label.appendChild(document.createTextNode(text[1]));
> 
> This seems overcomplicated to me. You can't assume where the
> {{identityName}} placeholder will be in the text when it gets translated
> into various languages. It may even end up at the end of the string so
> text[1] could be empty. Anyway, you do not need to care.
> Something like this may work for you:
> label.innerHTML = this.stringBundle.getFormattedString("identityWarning",
> ["<b>"+aIdentityName+"</b>"]);

I would then have to deal with escaping, especially because the identityName always has < > in it. Could do that of course but in my opinion its cleaner to let gecko handle that automatically. Same reason why I switched to using a {{}} placeholder. Up to you.

Comment 121

[Aaron B]

Attachment: 294216export11.patch

Addressed the rest of the review feedback.

I could write the tests but to minimize the learning curve it would be great if someone could do the initial layout for the test patch, such as what files and functions are to be created with what names, etc.

Comment 122

[Jorg K (CEST = GMT+2)]

Comment on attachment 8845616 [details] [diff] [review]
294216export11.patch

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

Sorry, we're a terribly picky bunch, and having to please two people makes it even harder ;-( Thanks for your persistence. As for the test: I was hoping that Aceman would make a start. But currently we're having other fires to extinguish.

Since this has string changes, we won't ever reintegrate this into TB 52 out next week. So the next major release is TB 59 in early 2018.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2607,5 @@
> +    // Search for a matching identity.
> +    if (from) {
> +      for (let ident of fixIterator(identities.enumerate())) {
> +        if (from == ident.email.toLowerCase()) {
> +          if (suitableCount == 0)

Look == 0.

@@ +2619,5 @@
> +
> +    if (!params.identity || !params.identity.email) {
> +      // No preset identity and no match, so use the default account.
> +      identities = MailServices.accounts.defaultAccount.identities;
> +      if (!identities.length)

I don't think anyone asked to change == 0 to !. Please revert that. It makes it inconsistent.

@@ +2629,5 @@
> +
> +    // Warn if no or more than one match was found.
> +    // But don't warn for +suffix additions ([e-mail]).
> +    if (from && (suitableCount > 1 ||
> +        (suitableCount == 0 && !emailSimilar(from, params.identity.email))))

== 0.

Comment 123

[:aceman]

Yes, I can start the tests.

I still need to solve comment 120.

Comment 124

[:aceman]

(In reply to Aaron B from comment #115)
> No X-Identity-Key header:
> 
> 8. no matching identity exists
> => default identity, warning
> 9. From header matches default identity

How can there be no X-Identity-Key in a draft? Or what is meant here?

> 10. command line with no from=
> => unchanged default identity, no warning
> 11. command line with from= has 1 match
> => identity changed, no warning
Also, I don't know how to pass command line arguments in mozmill tests so I would skip 4., 10. and 11.

Comment 125

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #124)
> > No X-Identity-Key header:
> How can there be no X-Identity-Key in a draft? Or what is meant here?
That's the point, they use a phone app to store the draft.

> Also, I don't know how to pass command line arguments in mozmill tests so I
> would skip 4., 10. and 11.
Yep, maybe test that manually.

Comment 126

[Jorg K (CEST = GMT+2)]

Since Aceman is getting ready to write some tests, he asked me to test this manually, so I did a few cases.

Just as a clarification, this is about selecting the right/best identity to send the message, not to adjust the From: of the message.

Aceman pointed out the following issue to me: Say you customise the from address further then just adding +xyz at the end of the user name, so change |Mr User <[e-mail]>| to |Mr User <[e-mail]>|. That gets you the notification when re-editing that draft. It's basically case 5 from comment #115:
  5. no matching identity exists
  => identity unchanged from X-Identity-Key, warning

I'm not sure how to address this issue.

Comment 127

[Jorg K (CEST = GMT+2)]

OK, Aceman had another weird case, a From: like |From: aceman|. He noticed that the patch did nothing in that case. I think that's OK, it just uses whatever identity was in the draft.

The code is:
  from = (from.length && from[0] && from[0].email) ?
    from[0].email.toLowerCase().trim() : null;
So since there is no email, from is null. And the next if-block isn't entered. I don't see a problem with that.

Comment 128

[Aaron B]

(In reply to Jorg K (GMT+1) from comment #126)
> Aceman pointed out the following issue to me: Say you customise the from
> address further then just adding +xyz at the end of the user name, so change
> |Mr User <[e-mail]>| to |Mr User <[e-mail]>|. That gets
> you the notification when re-editing that draft. It's basically case 5 from
> comment #115:
>   5. no matching identity exists
>   => identity unchanged from X-Identity-Key, warning

It's also the same as your previous example:

(In reply to Jorg K (GMT+1) from comment #59)
> Draft: id4, From:[e-mail]
> id3 has [e-mail]
> id4 has [e-mail]
> No match at all --> Warning.

While in your new example the user did customize the address on purpose, in my opinion you were right in originally suggesting a warning, as it reminds the user they're doing something unusual and should be mindful of the identity used, even when they customized on purpose. If a user doesn't like the warning, they already have a way to easily avoid it: just create the missing identity :)

Comment 129

[:aceman]

(In reply to Aaron B from comment #115)
> Test cases: not sure how this is typically done but here's what could be
> tested. This may be too detailed :)
> 
> Configuration: several identities, 2 with same email addresses, some with
> +suffix added.

The proposed cases can be mapped to tests very well and I am progressing on them.
Thanks for the detailed description.(In reply to Aaron B from comment #120)

> (In reply to :aceman from comment #118)
> > This seems overcomplicated to me. You can't assume where the
> > {{identityName}} placeholder will be in the text when it gets translated
> > into various languages. It may even end up at the end of the string so
> > text[1] could be empty. Anyway, you do not need to care.
> > Something like this may work for you:
> > label.innerHTML = this.stringBundle.getFormattedString("identityWarning",
> > ["<b>"+aIdentityName+"</b>"]);
> 
> I would then have to deal with escaping, especially because the identityName
> always has < > in it. Could do that of course but in my opinion its cleaner
> to let gecko handle that automatically. Same reason why I switched to using
> a {{}} placeholder. Up to you.

Ok, I understand the benefit of createTextNode() ignoring any HTML entities so you do not need to care about escaping. I didn't think up a more clever way to construct this, if you want to avoid the escaping. But please still change the placeholder to something more common so it is immediately apparent it is a placeholder. I would propose %S again.

Comment 130

[:aceman]

Comment on attachment 8845616 [details] [diff] [review]
294216export11.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5636,5 @@
> +      label.appendChild(document.createElement("b"));
> +      label.lastChild.appendChild(document.createTextNode(aIdentityName));
> +      label.appendChild(document.createTextNode(text[1]));
> +      this.notificationBar.appendNotification(label, "identityWarning", null,
> +        this.notificationBar.PRIORITY_WARNING_MEDIUM, null);

This uses the same notification priority as the attachment notifier. I wonder if we should use a different one so that we can be sure of the order of the notifications (which one comes on top if both are shown).
If the identity one is more important, I would propose PRIORITY_WARNING_HIGH .

Comment 131

[:aceman]

Comment on attachment 8845616 [details] [diff] [review]
294216export11.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4652,5 @@
>            var event = document.createEvent('Events');
>            event.initEvent('compose-from-changed', false, true);
>            document.getElementById("msgcomposeWindow").dispatchEvent(event);
> +
> +          gComposeNotificationBar.clearIdentityWarning();

If you open a draft with an identity that needs warning and then switch identity the notification goes away. If you switch again, there is an error:
TypeError: aNotification is null 1 notification.xml:270:17
	_showNotification chrome://global/content/bindings/notification.xml:270:17
	removeCurrentNotification chrome://global/content/bindings/notification.xml:222:13
	removeNotification chrome://global/content/bindings/notification.xml:193:15
	clearIdentityWarning chrome://messenger/content/messengercompose/MsgComposeCommands.js:5656:5
	LoadIdentity chrome://messenger/content/messengercompose/MsgComposeCommands.js:4667:11
	oncommand chrome://messenger/content/messengercompose/messengercompose.xul:1:1

You probably can't remove the notification if it is not there. Please check for it first.

Comment 132

[:aceman]

When the identity is xxx@yyy and I edit it to yyy@zzz, then reopen the draft, I get the warning that message will be sent using identity xxx@yyy, but the From field contains the yyy@zzz address. I know understand you only show the identity name, but it happens to contain the email address. I'm sure most users (including me) will think the message will also be sent using the xxx@yyy address as From.
Can you somehow improve the text to avoid this meaning?

Something like:
identityWarning=A unique identity matching the 'From' address was not found. Message will be sent with the current value of From field but using settings from identity {{identityName}}.

Comment 133

[:aceman]

Comment on attachment 8845616 [details] [diff] [review]
294216export11.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2605,5 @@
> +    let suitableCount = 0;
> +
> +    // Search for a matching identity.
> +    if (from) {
> +      for (let ident of fixIterator(identities.enumerate())) {

fixIterator(identities, Components.interfaces.nsIMsgIdentity) here. I wonder why just identities.enumerate() appears to work here.

@@ +2616,5 @@
> +        }
> +      }
> +    }
> +
> +    if (!params.identity || !params.identity.email) {

While playing with the tests, I found out this is never hit. The params.identity is set from the beginning of this function so this block never gets executed. It is set to some identity, which is NOT always the defaultAccount.defaultIdentity (to which we would like to set it here).

@@ +2623,5 @@
> +      if (!identities.length)
> +        identities = MailServices.accounts.allIdentities;
> +      if (identities.length)
> +        params.identity =
> +          identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);

For MailServices.accounts.allIdentities getting the first item as a fallback is fine. But for a single account, let's not assume default identity is the first one. Change the block to:

 // No preset identity and no match, so use the default account.
    let identity = MailServices.accounts.defaultAccount.defaultIdentity;
if (!identity) {
  let identities = MailServices.accounts.allIdentities;
  if (identities.length > 0)
    identity = identities.queryElementAt(0, Components.interfaces.nsIMsgIdentity);
}
 params.identity = identity;

Comment 134

[:aceman]

Attachment: tests

So these are some tests according to comment 115.

Test 8 is disabled for now as it doesn't pass. I expected the From identity be set to the defaultIdentity of the default account (my created one), but it isn't. Is is set to the first identity from all accounts, which is a different one as mozmill sets up some accounts automatically. Maybe it is due to the problem from my comment 133. Please check if I set correct assumptions in the tests.

I have also left in all my debugging dumps so I could see which paths are taken.

Aaron, please update your patch for all the problems I have found so far.

Comment 135

[Jorg K (CEST = GMT+2)]

Comment on attachment 8851273 [details] [diff] [review]
tests

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

I'm not sure which sort of feedback you're after. I haven't looked at the various cases, maybe Aaron can do that, but overall, this is very very nice indeed! I can see this bug land now ;-) Many thanks for moving this forward.

If you want more detailed feedback, I'll do that on the plane tomorrow ;-)

One thing I don't like is example.invalid. example.com is a valid test domain we use all over the place, so I'd use it here, too.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3837,5 @@
>  
>  function getCurrentAccountKey()
>  {
>      // get the accounts key
> +  let identityList = GetMsgIdentityElement();

Uneven indentation here.

Comment 136

[:aceman]

Comment on attachment 8845616 [details] [diff] [review]
294216export11.patch

This needs a new patch anyway.

Comment 137

[Jorg K (CEST = GMT+2)]

Any news here? Let's finish this off.

Comment 138

[Jorg K (CEST = GMT+2)]

Aceman, how long do we want/need to wait before we take over the bug and get it finished?

Comment 139

[:aceman]

There is already a meaty bounty accumulated on this bug :) Let's give Aaron one more month to finish this.
It seems he needs to fix the problems I found and then I have to update the test.

Comment 141

[Aaron B]

Sorry for delay I've been busy at work lately but I plan to look at this in the next week or two.

Comment 142

[Wayne Mery (:wsmwk)]

https://www.bountysource.com/issues/3489444-opening-draft-selects-different-account-identity-imap-multi-clients-with-different-undefined-identity-number

Comment 144

[Jorg K (CEST = GMT+2)]

(In reply to Aaron B from comment #141)
> Sorry for delay I've been busy at work lately but I plan to look at this in
> the next week or two.
That hasn't happened. The TB team will finish this bug starting in early August.

Comment 145

[Jorg K (CEST = GMT+2)]

Attachment: 394216-multiple-identities.patch (v11) - refreshed

I'm taking over the bug now after five months of inactivity.

Richard, can you please check that I refreshed this correctly.

Comment 146

[Jorg K (CEST = GMT+2)]

Looks like interdiff is getting a little confused when comparing Aaron's v11 with mine although I only corrected one hunk:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8845616&action=interdiff&newid=8896561&headers=1

Once Richard confirms, I'll address the review issues from comment #133.

Comment 147

[Richard Marti (:Paenglab)]

This patch doesn't work for me. Opening a draft with a not owned from address/identity with "Edit" show a empty composer like a  completely new message. In the console I get:
13:16:31.038 ReferenceError: reference to undefined property "stringBundle"[Learn More]  MsgComposeCommands.js:5664:11
13:16:31.038 this.stringBundle is undefined  MsgComposeCommands.js:5664
	setIdentityWarning chrome://messenger/content/messengercompose/MsgComposeCommands.js:5664:11
	ComposeStartup chrome://messenger/content/messengercompose/MsgComposeCommands.js:2632:7
	ComposeLoad chrome://messenger/content/messengercompose/MsgComposeCommands.js:2849:7
	onload chrome://messenger/content/messengercompose/messengercompose.xul:1:1
13:16:31.039 NS_ERROR_NOT_AVAILABLE: Cannot call openModalWindow on a hidden window nsPrompter.js:350
13:17:04.070 TypeError: parentDocShell.getDocShellEnumerator is not a function[Learn More]  tab.js:62:23
13:17:04.070 NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsICommandManager.removeCommandObserver]  MsgComposeCommands.js:2878

Maybe it's because M-C changed the stringbundle handling.

Comment 148

[Jorg K (CEST = GMT+2)]

Attachment: 394216-multiple-identities.patch (v12)

It's due to bug 1383569 where I removed 'this.stringBundle'. Try this patch. I'd still like to know whether I got the CSS right. If not, please attach a fixed patch.

Comment 149

[Richard Marti (:Paenglab)]

Attachment: 394216-multiple-identities.patch (v13)

I moved the common messenger.css code to the messengercompose.css. It's not needed to set this rule global. On mesengercompose is enough.

Good, it also exchanges the address to one of mine when I edit a message from an other person (a message I received) and I choose "Edit as New Message".

Comment 150

[Jorg K (CEST = GMT+2)]

Thanks, I'll address the review issues from comment #133 later today.

Comment 151

[Jorg K (CEST = GMT+2)]

Attachment: 394216-multiple-identities.patch (v14)

I've updated the patch according to comment #133.

I don't quite understand:
+    if (!params.identity || !params.identity.email) {
> While playing with the tests, I found out this is never hit. ...
Yet, you suggested to change the content of the block, which I did.
So is the block hit, or not? Looking at comment #134, you seem to suggest that your test 8 should execute that block.

Please refresh the test patch. I've run the test
mozmake SOLO_TEST=composition/test-draft-identity.js mozmill-one
(please add to the top of the file) and it passes. I didn't try test 8:
  // 8. no matching identity exists
  { idIndex: 0, warning: true,
    draftFrom: "Unknown <[e-mail]>" },
since I don't understand it.

> Test 8 is disabled for now as it doesn't pass. I expected the 
> From identity be set to the defaultIdentity of the default
> account (my created one), but it isn't. 
Why would that address match anything? Your debug dumps out:
ACE:nobody on Draft Identity Testing
ACE:id3:[e-mail]
ACE:id1:Tinderbox <[e-mail]>
ACE:id2:Tinderboxpushlog <[e-mail]>
ACE:id4:[e-mail]
ACE:id5:User Y <[e-mail]>
ACE:id6:User YY <[e-mail]> (YY)
ACE:id7:User Y <[e-mail]>
ACE:id8:User Z <[e-mail]> (Label Z)
ACE:id9:User A <[e-mail]>
Where do you set the default account?

Comment 152

[:aceman]

Have you also fixed comments 130, 131, 132?

Comment 153

[Jorg K (CEST = GMT+2)]

No, be my guest. We're leaving the original author, so it doesn't matter who does the tweaking.

Comment 154

[:aceman]

(In reply to Jorg K (GMT+2) from comment #151)
> I've updated the patch according to comment #133.

Thanks.
 
> I don't quite understand:
> +    if (!params.identity || !params.identity.email) {
> > While playing with the tests, I found out this is never hit. ...
> Yet, you suggested to change the content of the block, which I did.
> So is the block hit, or not? Looking at comment #134, you seem to suggest
> that your test 8 should execute that block.

Yes, I expect the test 8 to execute that block (because no matching identity is found). The block is NOT hit yet, so there is some logic error sooner in the code. But once we fix that error and the block is hit, I suggested to fix also the code in this block so it selects the default identity (I think also the code of this block wasn't the best).

I expect to get id4 in test 8 but I still get id1.

The logic error sooner in the code is that params.identity is always initialized to something (maybe id1) so the check in this block for (!params.identity) is never true. I would expect that if we didn't find a matching identity then params.identity would be empty which is also what this block assumes.
 
> Please refresh the test patch. I've run the test
> mozmake SOLO_TEST=composition/test-draft-identity.js mozmill-one
> (please add to the top of the file) and it passes. I didn't try test 8:
>   // 8. no matching identity exists
>   { idIndex: 0, warning: true,
>     draftFrom: "Unknown <[e-mail]>" },
> since I don't understand it.

We have a draft with an identity (name, email) that matches nothing in our list of identities. And there is no identity key in the draft. So the user may be trying to use a foreign message as a draft. I'd assume we want to fallback to the default identity here, use its settings but keep this From (as if the user edited it).
 
> > Test 8 is disabled for now as it doesn't pass. I expected the 
> > From identity be set to the defaultIdentity of the default
> > account (my created one), but it isn't. 
> Why would that address match anything? Your debug dumps out:
> ACE:nobody on Draft Identity Testing
> ACE:id3:[e-mail]
> ACE:id1:Tinderbox <[e-mail]>
> ACE:id2:Tinderboxpushlog <[e-mail]>
> ACE:id4:[e-mail]
> ACE:id5:User Y <[e-mail]>
> ACE:id6:User YY <[e-mail]> (YY)
> ACE:id7:User Y <[e-mail]>
> ACE:id8:User Z <[e-mail]> (Label Z)
> ACE:id9:User A <[e-mail]>
> Where do you set the default account?

I set the default account in setupModule() to the newly created account named "Draft Identity Testing". I expect its default (first) identity is id4 ([e-mail]). The identities id1-id3 are setup by the test infrastructure and I try to ignore those.

Comment 155

[Jorg K (CEST = GMT+2)]

Attachment 8819691 [details] is incomplete but good reading, it gives the answer you're looking for.

The whole story begins in mailCommands.js - function ComposeMessage(type, format, folder, messageArray)
and that is where your identity is determined, the one that owns the draft folder, in the test setup most likely id3.
It is handed all the way down unless overwritten at mimedrft.cpp#1463.

So as you observed params.identity will always be set to something at the start of ComposeStartup().

I had suggested to base the whole thing on creatorIdentityKey which will only be set if found in the draft.

IRC: "in the test run for test 8, params.identity becomes id3, but our algo would pick id4 (default identity of the default account) if it got the chance"

No surprise.

Comment 156

[Jorg K (CEST = GMT+2)]

Attachment: 394216-multiple-identities.patch (v15)

Maybe rather like this.

I tried running the test but I get:
  EXCEPTION: identityList.selectedItem is null
    at: MsgComposeCommands.js line 3841
       getCurrentAccountKey MsgComposeCommands.js:3841 3
       checkCompIdentity test-draft-identity.js:107 17
       test_draft_identity_selection test-draft-identity.js:182 5
and it's too late not to analyse this.

Also the patch doesn't address the issues in comment #130 to comment #132.

Comment 157

[Jorg K (CEST = GMT+2)]

Typo: ... so use the creator identity which *could* be null.

Comment 158

[:aceman]

(In reply to Jorg K (GMT+2) from comment #155)
> So as you observed params.identity will always be set to something at the
> start of ComposeStartup().
> 
> I had suggested to base the whole thing on creatorIdentityKey which will
> only be set if found in the draft.
> 
> IRC: "in the test run for test 8, params.identity becomes id3, but our algo
> would pick id4 (default identity of the default account) if it got the
> chance"
> 
> No surprise.

It is a surprise for the new code which expects params.identity be empty if no good match is found for the identity in the draft. So we need to decide if id3 is good enough and use it, or let the new code pick up id4.

If in the new patch you use params.identity = params.composeFields.creatorIdentityKey; which ma be null (if no identity-key is in the draft?), that could be the solution. I'll play with it in the evening.

Comment 159

[:aceman]

Comment on attachment 8901621 [details] [diff] [review]
394216-multiple-identities.patch (v15)

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2596,5 @@
> +  // that created the draft or the identity owning the draft folder for a "foreign",
> +  // draft, see ComposeMessage() in mailCommands.js. We don't want the latter,
> +  // so use the creator identity which would be null.
> +  if (gComposeType == nsIMsgCompType.Draft)
> +    params.identity = params.composeFields.creatorIdentityKey;

You need to make this:
 let creatorKey = params.composeFields.creatorIdentityKey;
 params.identity = creatorKey ? getIdentityForKey(creatorKey) : null;

@@ +2642,5 @@
>      identityList.getElementsByAttribute("identitykey", params.identity.key)[0];
>  
>    // Here we set the From from the original message, be it a draft or another
>    // message, for example a template, we want to "edit as new".
>    // Only do this the message is our own draft or template.

Here is another problem. If params.composeFields.creatorIdentityKey was null above and we have chosen some identity, choosing that identity in the From picker overwrites the From value.
The block here is intended to fix it, but it contains:
if (params.composeFields.creatorIdentityKey && params.composeFields.from) . Thus, only if params.composeFields.creatorIdentityKey is not null the From address is repaired.

Comment 160

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #159)
> You need to make this:
Thanks, be my guest ;-)

>  let creatorKey = params.composeFields.creatorIdentityKey;
>  params.identity = creatorKey ? getIdentityForKey(creatorKey) : null;
> 
> @@ +2642,5 @@
> >      identityList.getElementsByAttribute("identitykey", params.identity.key)[0];
> >  
> >    // Here we set the From from the original message, be it a draft or another
> >    // message, for example a template, we want to "edit as new".
> >    // Only do this the message is our own draft or template.
> 
> Here is another problem. If params.composeFields.creatorIdentityKey was null
> above and we have chosen some identity, choosing that identity in the From
> picker overwrites the From value.
> The block here is intended to fix it, but it contains:
> if (params.composeFields.creatorIdentityKey && params.composeFields.from) .
> Thus, only if params.composeFields.creatorIdentityKey is not null the From
> address is repaired.
Hmm, where is the problem? You know why that block is there, right? Without the block "edit as new" will always use the From of the re-used message. So if I "edit as new" of a message from Lufthansa, the message will be sent as coming from Lufthansa. This is undesired. It is only desired, if the user edited the From to be Lufthansa and saved the draft.

If we edit our own draft, the From will be restored (or repaired, as you say). If we edit a foreign message, then the From will reflect the chosen identity. That's desired. Where is the problem?

Comment 161

[:aceman]

(In reply to Jorg K (GMT+2) from comment #160)
> > Here is another problem. If params.composeFields.creatorIdentityKey was null
> > above and we have chosen some identity, choosing that identity in the From
> > picker overwrites the From value.
> > The block here is intended to fix it, but it contains:
> > if (params.composeFields.creatorIdentityKey && params.composeFields.from) .
> > Thus, only if params.composeFields.creatorIdentityKey is not null the From
> > address is repaired.
> Hmm, where is the problem? You know why that block is there, right? Without
> the block "edit as new" will always use the From of the re-used message. So
> if I "edit as new" of a message from Lufthansa, the message will be sent as
> coming from Lufthansa. This is undesired. It is only desired, if the user
> edited the From to be Lufthansa and saved the draft.
> 
> If we edit our own draft, the From will be restored (or repaired, as you
> say). If we edit a foreign message, then the From will reflect the chosen
> identity. That's desired. Where is the problem?

OK, then the fundamental question arises: If there is no X-Identity-Key in the draft, is it our draft or not? It wasn't created by TB in the first place, the user just somehow sneaked a msg into TB to be used as a draft (e.g. a TB developer:)).
So is test 8 real and do we want to support the scenario? The test was suggested in comment 115 (the "no X-Identity-Key" block) so I created it.

So what now?
Notice in tests 9-12 at least the From in the drafts matches on of our identities. But in test 8 nothing matches, not the from address and no identity-key.

Comment 162

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #161)
> OK, then the fundamental question arises: If there is no X-Identity-Key in
> the draft, is it our draft or not?
It is not. It is a "foreign" draft. Someone put it there, maybe Android's K9 on an IMAP account.

> It wasn't created by TB in the first
> place, the user just somehow sneaked a msg into TB to be used as a draft
Well, we are here to fix the problem where many clients access an IMAP drafts folder.

> So is test 8 real and do we want to support the scenario? The test was
> suggested in comment 115 (the "no X-Identity-Key" block) so I created it.
Yes. Good. You could do 8a with no X-Identity-Key but nicely matching e-mail address.

> Notice in tests 9-12 at least the From in the drafts matches on of our
> identities. But in test 8 nothing matches, not the from address and no
> identity-key.
Exactly. So we use the default and show a warning. In the case of the Android usage, the e-mail address will hopefully match and we can select a good identity.

Comment 163

[:aceman]

(In reply to Jorg K (GMT+2) from comment #162)
> > So is test 8 real and do we want to support the scenario? The test was
> > suggested in comment 115 (the "no X-Identity-Key" block) so I created it.
> Yes. Good. You could do 8a with no X-Identity-Key but nicely matching e-mail
> address.

I have that already in tests 9-10.
 
> > Notice in tests 9-12 at least the From in the drafts matches on of our
> > identities. But in test 8 nothing matches, not the from address and no
> > identity-key.
> Exactly. So we use the default and show a warning. In the case of the
> Android usage, the e-mail address will hopefully match and we can select a
> good identity.

Ok, so if there is no key in the draft and whatever From:, we treat it as foreign draft and overwrite it with the default identity?

Comment 164

[:aceman]

Attachment: 394216-multiple-identities.patch (v16)

This fixes all the problems I have mentioned so far (comments 130-133 and test 8, assuming it is fine to not preserve the From address in a 'foreign draft').

I see one remaining issue. There is a possibility for the caller to specify which identity should be used, via the code
if (args.preselectid)
  params.identity = getIdentityForKey(args.preselectid);

I think the new added algorithm just discards this selection. This needs decision which identity should win, either the caller specified one, or one automatically determined.

Comment 165

[:aceman]

Attachment: tests v2

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d5862b435132b896379e2512510efcf2968ef664

Part of the test (where I switch identities and expect the notification to vanish) is disabled as the switching doesn't work reliably, as in bug 1238264. I shall make a helper for this once a reliable way is found and use it at both tests.

Comment 166

[Jorg K (CEST = GMT+2)]

Comment on attachment 8902060 [details] [diff] [review]
394216-multiple-identities.patch (v16)

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

Very nice!

(In reply to :aceman from comment #164)
> I see one remaining issue. There is a possibility for the caller to specify
> which identity should be used, via the code
> if (args.preselectid)
>   params.identity = getIdentityForKey(args.preselectid);
If it were my decision, I'd remove the option altogether:
http://kb.mozillazine.org/Command_line_arguments_(Thunderbird)
But then, you couldn't distinguish between two identities using the same e-mail address but different signatures/servers, etc.
I can't recall why this was declared deprecated.

Anyway, if the user uses this obscure advanced option, we should respect it if possible, so if such an identity exists. Of course, if they specify from and preselectid and they don't match, we should apply the new precessing here.

> I think the new added algorithm just discards this selection.
Does it? This case there is no from and the identity is set, so we don't even enter this block:
  if (!params.identity || !params.identity.email ||
    (from && !emailSimilar(from, params.identity.email))) {
So it's not discarded, as far as I can see.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2594,5 @@
>    // An identity with no email is likely not valid.
> +  // When editing a draft, 'params.identity' is pre-populated with the identity
> +  // that created the draft or the identity owning the draft folder for a "foreign",
> +  // draft, see ComposeMessage() in mailCommands.js. We don't want the latter,
> +  // so use the creator identity which would be null.

You forgot to fix the typo I made. s/would/could/.

@@ +2600,5 @@
> +    let creatorKey = params.composeFields.creatorIdentityKey;
> +    params.identity = creatorKey ? getIdentityForKey(creatorKey) : null;
> +  }
> +  let from = MailServices.headerParser
> +                         .parseEncodedHeader(params.composeFields.from, null);

I think we need to protect this with
if (params.composeFields.from)
in case the command line was used and no from was given.

Comment 167

[Jorg K (CEST = GMT+2)]

Comment on attachment 8902061 [details] [diff] [review]
tests v2

Thanks.

Maybe enhance the comment for test 8 and say:
  For a "foreign" draft, we ignore the From and use the identities From
or words to that extent.

If you ask why part of your try run failed, here's she answer. There was an M-C merge at the time of your push, so Windows picked up the new and failed.

Comment 168

[Jorg K (CEST = GMT+2)]

Comment on attachment 8902060 [details] [diff] [review]
394216-multiple-identities.patch (v16)

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

Maybe we should address my comment below and some issues from comment #166.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +445,5 @@
>  blockedContentPrefAccesskeyUnix=P
>  
> +## Identity matching warning notification bar.
> +## LOCALIZATION NOTE(identityWarning): %S will be replaced with the identity name.
> +identityWarning=A unique identity matching the 'From' address was not found. Message will be sent with the current value of From field but using settings from identity %S.

Hmm, thinking about case 8, that's actually not true. Also, don't know why you added 'From' with single quotes once and not the second time. I think putting this in upper-case is enough. Also you lost all articles, which is a little ugly.

So how about:
A unique identity matching the From address was not found. The message will be sent with the From field as shown using settings from identity %S.

Comment 169

[Jorg K (CEST = GMT+2)]

Attachment: 394216-multiple-identities.patch (v17)

Since I'm assigned here, I did some nit-fixing myself.

Comment 170

[:aceman]

Comment on attachment 8902213 [details] [diff] [review]
394216-multiple-identities.patch (v17)

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

Yes, this is fine.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +445,5 @@
>  blockedContentPrefAccesskeyUnix=P
>  
> +## Identity matching warning notification bar.
> +## LOCALIZATION NOTE(identityWarning): %S will be replaced with the identity name.
> +identityWarning=A unique identity matching the From address was not found. The message will be sent with the From field as shown using settings from identity %S.

I find this a bit ambiguous, there should be something after the 'shown' word.

Comment 171

[Jorg K (CEST = GMT+2)]

Capturing some IRC discussion:

The message was originally:
Message will be sent using identity XYZ.

In comment #132 remarked that the From doesn't necessarily match the e-mail address of XYZ, for example when the From was edited before saving the draft in a way that it doesn't match any identity any more.

Aceman's suggestion was:
Message will be sent with the current value of From field but using settings from identity %S.

I suggested:
The message will be sent using the From field as shown and settings from identity XYZ.

On IRC we had:
The message will be sent using the current From field and settings from identity XYZ. (*)
The message will be sent using the shown From field and settings from identity XYZ.

We settled for the version with the (*) which was also approved by a native English speaker.

So I can fix this when landing the patches as long as everything else is OK.

Comment 172

[:aceman]

Attachment: tests v2.1

Thanks, added the comment.

Comment 173

[Jorg K (CEST = GMT+2)]

Attachment: 394216-multiple-identities.patch (v18)

Changed |from = null| to |from = []| and updated notification. Finally ready to go.

Comment 174

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/76ceed2ef472
Search for correct identity when draft identity key does not match From: address. r=jorgk,aceman
https://hg.mozilla.org/comm-central/rev/bc609af633fb
test automatic identity selection depending on X-Identity-Key in draft. r=jorgk

Comment 176

[Samuel Åslund]

I installed the Thunderbird 60.0b9 (Could not find 57.0 on the website) right now and it solved my issue with drafts jumping back to the previous account when saving after having moved/copied them to another accounts drafts-folder.

Thanks :-)

Comment 179

[thenterprises2]

I just had the unhappy occurrence, which I cannot replicate, of selecting "reply-all" on an e-mail in one account, and having the wrong e-mail address load as the send-from address.

One of the recipients was another of the accounts in my Thunderbird profile. Somehow, that recipient got "picked" to be the sender, even though I was in the correct sender e-mail folder.

The other recipient was a business client. Very embarrassing.

Once I realized I had sent the e-mail and it had gone from the wrong sender, I tried again, and the sender was again incorrect. I changed it to the correct sender, and sent the e-mail, and now I can't replicate the problem.

Comment 180

[thenterprises2]

I am running 68.12.1.

Comment 181

[thenterprises2]

Ok, it's only doing this for one particular e-mail, that I can determine. I was able to try reply and reply-all on the same e-mail that had the error before, and it pops up with the wrong sender, again. However, I have not seen this same behavior on any other e-mails.