1658781 - Error message "An account with this name already exists..." comes late - two accounts with same name already created (prevent duplicates)

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect, P1)

Description

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: Error message comes too late, two accounts with same name already exist

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.

Comment 1

[Magnus Melin [:mkmelin]]

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

Comment 2

[Thomas D. (:thomas8)]

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

Comment 5

[Magnus Melin [:mkmelin]]

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

Comment 7

[Magnus Melin [:mkmelin]]

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.

Comment 8

[Alessandro Castellani [:aleca]]

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?

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1658781-account-creation.diff

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.

Comment 10

[Alessandro Castellani [:aleca]]

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.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9190694 [details] [diff] [review]
1658781-account-creation.diff

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

::: mail/components/accountcreation/content/createInBackend.js
@@ +388,5 @@
>  /**
> + * Check whether the user's setup already has an account with the same email
> + * address. Even tho we could allow using the same email for different protocols
> + * (eg. IMAP and POP3) it's better to disallow this scenario and prevent errors.
> + *

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.

Comment 12

[Alessandro Castellani [:aleca]]

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.

Comment 13

[Magnus Melin [:mkmelin]]

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

Comment 14

[Alessandro Castellani [:aleca]]

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.

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: 1658781-account-manager.diff

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.

Comment 16

[Magnus Melin [:mkmelin]]

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

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

::: mailnews/base/prefs/content/AccountManager.js
@@ +770,1 @@
>      }

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

::: mailnews/base/prefs/content/amUtils.js
@@ +251,5 @@
>  /**
> + * Check if the currently selected account has an exact duplicate matching the
> + * login+hostname+type+port combination.
> + *
> + * @param {int} accountKey - The unique account key.

The account key is a string.

@@ +268,5 @@
> +      a.incomingServer.hostName == account.incomingServer.hostName &&
> +      a.incomingServer.type == account.incomingServer.type &&
> +      a.incomingServer.port == account.incomingServer.port
> +  );
> +

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

Could just do return MailServices.accounts.accounts.some(a => .......)

Comment 17

[Alessandro Castellani [:aleca]]

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

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1658781-account-manager.diff

Comment 19

[Magnus Melin [:mkmelin]]

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

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

::: mail/components/accountcreation/content/createInBackend.js
@@ +99,5 @@
> +  // protocol type to distinguish them.
> +  inServer.prettyName = checkAccountNameAlreadyExists(
> +    config.identity.emailAddress
> +  )
> +    ? `${config.identity.emailAddress} (${config.incoming.type.toUpperCase()})`

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

@@ +396,5 @@
> + * address. This might happen if the user uses the same email for different
> + * protocols (eg. IMAP and POP3).
> + *
> + * @param {string} emailAddress - The email address of the new account.
> + * @returns {Object|null} The duplicated account, if found.

Some returns a boolean. 
It's also short enough not have it as separate function.

::: mailnews/base/prefs/content/AccountManager.js
@@ +770,1 @@
>      }

We should prevent both cases: duplicate name, and duplicate configuration.
But if the duplicate account already was set up as a duplicate, seems it's too late.

Comment 20

[Alessandro Castellani [:aleca]]

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.

Comment 21

[Thomas D. (:thomas8)]

(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 [e-mail], (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 [e-mail] (POP3) (2) or if that also exists, [e-mail] (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.

Comment 22

[Alessandro Castellani [:aleca]]

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?

Comment 23

[Magnus Melin [:mkmelin]]

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.

Comment 24

[Thomas D. (:thomas8)]

Attachment: Screenshot 2: Account autoconfig for IMAP - [e-mail]

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

Comment 25

[Thomas D. (:thomas8)]

Attachment: obsolete

.

Comment 26

[Thomas D. (:thomas8)]

Attachment: Screenshot 3: Account autoconfig for POP3 - [e-mail]

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.

Comment 27

[Thomas D. (:thomas8)]

Attachment: Screenshot 4: IMAP and POP3 account (both for [e-mail] as intended) with same account name ([e-mail]; this bug) after regular creation via account autoconfig

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

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: 1658781-account-manager.diff

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?

Comment 29

[Thomas D. (:thomas8)]

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

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

Nice work! Some driveby comments...

(In reply to Alessandro Castellani (:aleca) from comment #28)
> Created attachment 9192098 [details] [diff] [review]
> This patch covers all the scenarios and issues reported above.

Sounds and looks great!

::: mail/components/accountcreation/content/createInBackend.js
@@ +399,5 @@
> + * address. This might happen if the user uses the same email for different
> + * protocols (eg. IMAP and POP3).
> + *
> + * @param {string} name - The name or email address of the new account.
> + * @returns {boolean|null} True if an account with the same name is found.

So when does this return null? I think it cannot.

@@ +415,5 @@
> + * @param {AccountConfig} config - The config data of the account being created.
> + * @returns {string} - The unique account name.
> + */
> +function generateUniqueAccountName(config) {
> +  // Generate a potential unique name. Eg. "foo@invalid (POP3)".

invalid is irritating. maybe [e-mail] or [e-mail]? "E. g." is two words (ex gregia).
  // Generate a potentially unique name, e. g. "[e-mail] (POP3)".

@@ +423,5 @@
> +
> +  // If this name already exists, append a counter until we find a unique name.
> +  if (checkAccountNameAlreadyExists(name)) {
> +    let counter = 1;
> +    while (checkAccountNameAlreadyExists(`${name}_${counter}`)) {

Nice!

::: mailnews/base/prefs/content/amUtils.js
@@ +260,5 @@
> +  let account = MailServices.accounts.getAccount(accountKey);
> +
> +  // Check if any of the accounts is a duplicate.
> +  return MailServices.accounts.accounts.some(
> +    a =>

Elegant!

Comment 30

[Thomas D. (:thomas8)]

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

Comment 31

[Thomas D. (:thomas8)]

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

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

One nit, if I'm not mistaken.

::: mailnews/base/prefs/content/AccountManager.js
@@ +764,5 @@
>      let accountName = getFormElementValue(serverNameElem);
>  
>      if (!accountName) {
>        alertText = prefBundle.getString("accountNameEmpty");
> +    } else if (accountServerExists(currentAccount.key)) {

On second thoughts, I'm not sure why you're testing all acount details for duplicates here.
This function is called "checkAccountNameIsValid()" , so it's all about checking account name: why are we checking unrelated properties? If this check is needed, it should probably live somewhere else.

Comment 32

[Alessandro Castellani [:aleca]]

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.

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: 1658781-account-manager.diff

Updated with Thomas' comments.

Comment 34

[Magnus Melin [:mkmelin]]

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

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

::: mail/components/accountcreation/content/createInBackend.js
@@ +422,5 @@
> +  } (${config.incoming.type.toUpperCase()})`;
> +
> +  // If this name already exists, append a counter until we find a unique name.
> +  if (checkAccountNameAlreadyExists(name)) {
> +    let counter = 1;

well, it would start from 2

::: mailnews/base/prefs/content/AccountManager.js
@@ +765,5 @@
>  
>      if (!accountName) {
>        alertText = prefBundle.getString("accountNameEmpty");
>      } else if (accountNameExists(accountName, currentAccount.key)) {
>        alertText = prefBundle.getString("accountNameExists");

So we still alert? Isn't that confusing?

@@ +767,5 @@
>        alertText = prefBundle.getString("accountNameEmpty");
>      } else if (accountNameExists(accountName, currentAccount.key)) {
>        alertText = prefBundle.getString("accountNameExists");
> +      // Change the account name to prevent UI freeze.
> +      let counter = 1;

2

Comment 35

[Alessandro Castellani [:aleca]]

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.

Comment 36

[Alessandro Castellani [:aleca]]

Attachment: 1658781-account-manager.diff

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.

Comment 37

[Magnus Melin [:mkmelin]]

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

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

Great, thx! r=mkmelin

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3719f3600451
Fix the existing account name condition in the account manager. r=mkmelin

Comment 39

[Magnus Melin [: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 40

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for beta

Comment 41

[Rob Lemley [:rjl]]

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

Comment 42

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 43

[Rob Lemley [:rjl]]

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