Seen on 78.1.1 (64-bit)

As seen in screenshot (and in 3-pane folder list), I already succeeded to create two accounts with exactly the same name (POP & IMAP, both test accounts with disfunctional fake servers). At some random point in time, much later, when going through account settings, TB suddenly starts complaining after the fact. Too late, and very irritating.

Expected: warn immediately when creating / renaming an account with a duplicate account name.

I don't think this is a duplicate of some similar bugs, which seem to be about the technical impossibility of creating similar accounts.

Mostly interested in how you managed to set up the two same accounts. That's prevented through the wizard.

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

Mostly interested in how you managed to set up the two same accounts. That's prevented through the wizard.

I used "Configure manually" from wizard to get into account manager directly. I did not use any special trick, it just let me do it.

One way to get into this situation is also creating both an imap and a pop account for the same address (which should be allowed).

It's possible we just check the wrong thing in the account manager. But pretty bad as hard to get out of the situation without deleting the whole profile.

It seems that we're checking for the incoming server and not the actual email/name written.

One way to get into this situation is also creating both an imap and a pop account for the same address (which should be allowed).

I think this is the only way to get into this situation.
The only way I'm able to reproduce the creation of an account with the same name is with the following steps:

• Assume you already have an account setup as IMAP.
• Create a new account with the same email address.
• Select manual configuration and switch from IMAP to POP.

Is this what you did Thomas?

This patch prevents this scenario during account creation.
I had to create some new strings since we don't have some specific.

If we want this uplifted to 78 we could potentially use the same alert prompt of the incoming server.

I forgot to add, if this is a good solution and is approved, please hold on with the check-in as I'd like to cover this scenario with a test.

We want to (and I'm pretty sure we do) support same email but different protocol.
What is unique is the hostname+type/port+login.

All right, I'll check why we prompt that alert on the account manager and be sure the condition used is correct.

https://searchfox.org/comm-central/rev/f1a1f0d96a329b9814dda2aa77aad620018121fe/mailnews/base/public/nsIMsgAccountManager.idl#123

Thanks for the link.
I was using the findRealServer() method like we do in the account creation wizard in order to match the exact same condition we use upon creation.

This fixes the problem for me.

I didn't update the existing accountNameExists() method since that's used in the IM Account Wizard.

I also didn't use the FindServer or FindRealServer methods since those will always return the first server found without the possibility of excluding a server based on the key.

I also replaces the alert message to use the modifiedAccountExists string since that mentions both username and server name, which is more accurate in this situation. A better string might be worth adding for a non-78 patch.

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

Well, we shouldn't allow duplicate names either, it's very confusing in the
UI. The account setup should be fixed to not create such things as well.
Maybe add e.g. " (POP3)" to the name if an imap account is already there

Okay, I will implement a name validation check in the email wizard and add the server type in the name in case of exact match

FindServer doesn't look at port, so we probably don't support that distinction.

We're using FindRealServer in the account creation wizard, which looks at port.
https://searchfox.org/comm-central/rev/f1a1f0d96a329b9814dda2aa77aad620018121fe/mailnews/base/public/nsIMsgAccountManager.idl#139

Thanks for the rest of the review, I'll update accordingly.

All right, let's step back a little bit here because I'm getting confused.
What is the objective of this bug?

For what I understood, and what was discussed before, we want to:

• Prevent the Alert dialog in the Account Manager if the account name is identical to another, but the server settings (eg. protocol) are different.
• Allow having multiple accounts with the same name, but append something to the name upon creation to distinguish them.

Should we append the " (protocol)" to the name automatically upon creation, or prevent the email wizard to complete and prompt the user with an alert asking them to use a unique name?
I don't think we can ask that to the user since in the email wizard we don't have a field for the account name, as we use the email for it.

But, that account name may also exist... (though not as likely, we should still prevent it)

I think this is very unlikely since the user can't create a new account with the same email address + protocol.
A duplication in this case might happen if the user manually updated an existing account name, but who would name an account "email (POP3)" if it's not a pop account?
I think that's extremely remote.

I need a bit of direction here regarding the scope and what are our objective.
Sorry for the confusion.

(In reply to Alessandro Castellani (:aleca) from comment #20)

All right, let's step back a little bit here because I'm getting confused.
What is the objective of this bug?

For what I understood, and what was discussed before, we want to:

• Prevent the Alert dialog in the Account Manager if the account name is identical to another, but the server settings (eg. protocol) are different.

Well, that's oddly phrased. We want to prevent creating two accounts with identical names in the first place, because enforcing unique names only when renaming is too late and nonsensical. There's nothing wrong with the alert in itself, only the scenario.

• Allow having multiple accounts with the same name, but append something to the name upon creation to distinguish them.

Again, oddly phrased. When we're trying to create an (pop/imap) account with the name foo@bar.com, (also) check if that account name already exists and if it does, yes, appending the protocol (POP) or (IMAP) to the name sounds like a good way out. And please do check if the extended name already exists, users can do anything they want to account names, so they might have an account having (POP) where POP has an entirely different private meaning to them, or some other circumstances of stale accounts, redirections, whatever.
Please use "POP" and not "POP3" which is even more technical, and we have "POP Mail Server" in account settings UI.

Should we append the " (protocol)" to the name automatically upon creation, or prevent the email wizard to complete and prompt the user with an alert asking them to use a unique name?

Appending protocol sounds good to me with the above caveat, you need another way out if for whatever reason, the extended name already exists (just add the first available number >=2 in brackets after checking if that the new name with that number doesn't exist), so you'd get something like foo@bar.com (POP3) (2) or if that also exists, foo@bar.com (POP3) (3) (increase number until success, no risks because that won't have more than one or two iterations).

I don't think we can ask that to the user since in the email wizard we don't have a field for the account name, as we use the email for it.

But, that account name may also exist... (though not as likely, we should still prevent it)

+1

I think this is very unlikely since the user can't create a new account with the same email address + protocol.
A duplication in this case might happen if the user manually updated an existing account name, but who would name an account "email (POP3)" if it's not a pop account?
I think that's extremely remote.

It's a matter of coding principle. Better safe than sorry. I actually rename test accounts to anything for creating screenshots. Someone might have an email redirection so the email of the account might not be the one we know. Whatever. We don't want to create loopholes for edge cases which might lateron give us a support and triage nightmare trying to figure out why uniqueness of account name is ensured for all cases except one.

Well, that's oddly phrased. We want to prevent creating two accounts with identical names in the first place, because enforcing unique names only when renaming is too late and nonsensical. There's nothing wrong with the alert in itself, only the scenario.

Sure, I agree, but that alert trigger needs fixing as we're running a different condition compared to what we run upon creation.

email creation: Check if hostname + protocol + port + email already exists.
account manager: Check if account name already exists.

These 2 conditions should run both upon creation and in the account manager, right?

Yes, both checks should be performed in both places. For the wizard, just automatically adjust the name if needed - users can change it later should they feel the need to.

For simplicity, and also since we do use "POP3" for type in the UI e.g. during setup, I'd use that and not POP.

(In reply to Alessandro Castellani (:aleca) from comment #8)

The only way I'm able to reproduce the creation of an account with the same name is with the following steps:

• Assume you already have an account setup as IMAP.
• Create a new account with the same email address.
• Select manual configuration and switch from IMAP to POP.

Is this what you did Thomas?

Not exactly. Using the [Configure manually...] button is not required, and I didn't use that.
Just enter Name, Email, password, then after checking that, account autoconfig offers an option group

(o) IMAP ...
( ) POP3 ...

on the primary autoconfig dialog. So using exactly the same data, I first created the IMAP flavor and then the POP3 flavor for the same email address, resulting in two accounts with same account name (this bug), but different types (as expected).

.

Step 2: Creating the POP account for same email address as the IMAP account from step 1.
Creating both accounts works flawlessly without any hassles.

After regular creation of IMAP and POP3 accounts for same email address, they have the same account name (this bug).

This patch covers all the scenarios and issues reported above.

During the account creation this patch will:

• Alert the user if an account with the same exact configuration already exists.
• Create a unique account name in case of duplicates (no alert here since we don't have a field to allow users to change the account name).

In the Account Manager this patch will:

• Alert the user if an account with the same exact configuration already exists (extremely unlikely, borderline impossible).
• Alert the user if an account with the same name already exists, and append a counter to prevent UI freeze.

These checks are basically identical for both creation workflow and AM interaction.
Unfortunately, the methods are in completely different files that don't need to interact with each other.

Maybe we should consider moving these methods into the accountUtils.js which is used both in the account wizard (until this is moved into the main tab) and the account manager.
Maybe in a follow up patch?

Pls check all instances of "eg. / Eg." in the patch and replace them with "e. g. / E. g."

On second thoughts, I'm not sure why you're testing all account details for duplicates here.

Indeed, I think that extra addition is not really necessary.

Updated with Thomas' comments.

So we still alert? Isn't that confusing?

We need to let the user know that the chosen name is already in use.
The alert also helps to understand why the name automatically changes to *_2.

Counter updated.
I really think the alert should stay as it's super odd to see your account name suddenly change with an additional _2 without any explanation.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/3719f3600451
Fix the existing account name condition in the account manager. r=mkmelin

Comment on attachment 9193590 [details] [diff] [review]
1658781-account-manager.diff

[Approval Request Comment]
User impact if declined: users may locked into an infinite loop, under certain (unusual) circumstances
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): not terribly risky, but should of course go through beta

Comment on attachment 9193590 [details] [diff] [review]
1658781-account-manager.diff

[Triage Comment]
Approved for beta

Thunderbird 85.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/642c65e5ed89

Comment on attachment 9193590 [details] [diff] [review]
1658781-account-manager.diff

[Triage Comment]
Approved for esr78

Thunderbird 78.6.1:
https://hg.mozilla.org/releases/comm-esr78/rev/e1a317fcdee4