40012 - Two identical Account Names are allowed to exist

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Henrik Gemal]

I dont know if this is intended, but you can have to Mail Accounts with the same 
Account Name.
It does make it a bit confusing when you move or copy mails around since there 
are two identical account names.

Comment 1

[Alec Flett]

I think I have a dupe on this somewhere.

Comment 2

[Ninoschka Baca]

Appears similar to bug# 29579 which should be changed so that anyone can view 
the bug.

Comment 3

[Alec Flett]

massive reassign of account manager bugs -> sspitzer
please feel free to put me back on the CC if you have any questions/comments

Comment 4

[racham]

Working this one. Taking this one from Seth.

Comment 5

[racham]

Attachment: Patch to alert user if the account name already exists..

Comment 6

[racham]

The patch posted will prevent all the users who create their account goign
through account wizard and eventually reach the screen where they need to set
the account name. If the user accidentally enters an existing account name for a
new account, an alert will come up and the user will be asked to enter a
different account name.

We still have a situation with ISPs who want provide a rdf file with all their
settings. Pretty Name is one of those settings. AOL and Webmail  are some of the
examples we see today. So, when an account is created with any of these ISPs,
we do not take the user to the Account Name screen as we get that information
from rdf file.

We should either change the format in in which we generate account names or add
additional attribute to the rdf file to solve ISPs problem.

Adding Jennifer, Ninoschka to the cc list.

Comment 7

[timeless]

RCS file: 
/cvsroot/mozilla/mailnews/base/prefs/resources/content/AccountManager.js,v

please don't use travis style indentation, either 2 spaces or 4 spaces, not 2 
then 4.

RCS file: 
/cvsroot/mozilla/mailnews/base/prefs/resources/content/aw-accname.xul,v
+  <script language="JavaScript" 
src="chrome://messenger/content/AccountManager.js"/>
this is my fault, i should have fixed that file to use 
<script type="application/x-javascript" />
so that you wouldn't perpetuate this, please use the type syntax instead of 
language.

+var accountManagerContractID   = "@mozilla.org/messenger/account-manager;1";
why is there excess whitespace before the equal sign?

supportsArray.GetElementAt(i).QueryInterface(iid);
i seem to remember a function that would get an array element and QI at once, i 
don't remember if that's from nsisupportsarray ...

reguardless, if you insist on using separate things, this syntax is bad:
+      var serverSupports = accountManager.GetServersForIdentity(identity);
+      if(serverSupports.GetElementAt(0))
+      {
+          var result = 
serverSupports.GetElementAt(0).QueryInterface(Components.interfaces.nsIMsgIncom
ingServer);

instead use:

var interim=accountManager.GetServersForIdentity(identity).GetElementAt(0);
if (interim) {
var result = 
interim.QueryInterface(Components.interfaces.nsIMsgIncomingServer);

Comment 8

[Matthew T (active 1999-2002)]

> +  if (accountNameExists(accountname)) {
> +      var alertText = Bundle.GetStringFromName("accountNameExists");
> +      window.alert(alertText);

Please don't use an alert for this. In general, alerts should be the method of 
last resort for solving UI problems. And in particular, using an alert to trap 
bad input for an assistant goes against the UI guidelines for both Windows and 
Mac OS. Here's the relevant guideline for Windows:
|
| Minimize the number of pages that require the display of a secondary window.
| Novice users are often confused by the additional complexity of secondary
| windows.
|
<http://msdn.microsoft.com/library/default.asp?URL=/library/books/winguide/
ch13h.htm>

And for Mac OS:
|
| When a user types an answer that's clearly wrong (such as an e-mail address
| that doesn't include the @ character), we recommend that you integrate the
| error trapping into the flow of the interview questions, rather than
| presenting an alert box. For example, when the user clicks the right arrow
| button and there are invalid values in the current panel, the next panel
| should point out the error and restate the question. The goal is to preserve
| the question-and-answer, conversational characteristics of the interview.
|
<http://developer.apple.com/dev/techsupport/develop/issue27/arcellana.html>

Instead of showing an alert, when the user clicks `Next' the next page should 
be blank, except for text saying
|
| You already have an account called "Mail for racham". Please go back and
| choose a different name.
|,
and the `Next' button should be disabled.

Comment 9

[Matthew T (active 1999-2002)]

Attachment: Screenshot of an assistant's typical response to bad input

Comment 10

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 11

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 12

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 13

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 14

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 15

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 16

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 17

[Robert Kaiser]

This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 18

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago.

Because of this, we're resolving the bug as EXPIRED.

If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component.

Query tag for this change: EXPIRED-20100420

Comment 19

[Karsten Düsterloh]

This bug is actually an indirect result of relying upon the server names to be distinct for distinct accounts - which is a big problem by itself (lots of bugs filed!)...

Comment 20

[:aceman]

So is this a problem in the Account manager (advanced) or the Account wizard?

Comment 21

[:aceman]

Still happens at least in the Account manager. I'll try it.

Comment 22

[:aceman]

Bwinton, is this worth an alert (or a sliding alert via bug 725615)? Or just rename the duplicate name silently?

Comment 23

[Blake Winton (:bwinton) (:☕️)]

I like MPT's view in comment 8.  Can we go with that instead of the sliding alert or the silent rename?

Comment 24

[:aceman]

That one seems to only cover the Account Wizard. What about the Account manager?

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Put an error message beside the account name, and disable the OK button?

Comment 26

[:aceman]

That would fit into the Wizard, there is already UI like that. But not in the Manager.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Maybe we're talking about two different things.  By "Account Manager", I mean http://dl.dropbox.com/u/2301433/Screenshots/AccountManager.png

and by "Account Wizard", I mean http://dl.dropbox.com/u/2301433/Screenshots/Which%20Incoming%20Server.png

Does that match what you were thinking?

Comment 28

[:aceman]

Yes, that is it.
I have not yet seen an error popping out besides a field in the manager. But I remember it being in wizard.

Comment 29

[Matthew T (active 1999-2002)]

I apologize on behalf of my eleven-years-ago self for not thinking of an inline error message; I didn't start using those until the Ubuntu installer in 2005. (And in retrospect, preventing the error using an alert would have been better than leaving it completely unprevented for so long.)

The reason you see inline error messages more commonly in assistants than in dialogs is that dialogs are much more compact -- it would be weirder for them to leave room after every field just in case they needed to explain that the field was invalid.

It's a bit surprising that common toolkits still don't have a standard solution for this problem -- a standard way of conveying that, and why, a field is invalid. A reusable widget for this would be interesting -- it could be used for the e-mail address field here, for example. But in the meantime, *this particular* bug could be solved using an inline error message in both the assistant and the dialog, because there's room in both places.

Comment 30

[:aceman]

Even though I do not like the new UI element being introduced, I can try to look at this soon.

Also disabling the OK button is strange. What happens if the user selects another pane in the tree? Should it still stay disabled and let the user hunt in which pane the problem stayed?

Comment 31

[:aceman]

Bwinton, can you please check this out again?
Also consider we already have an alert when the server name or username is unusable. We could use the same method for this new alert. I'm not keen on implementing a new UI widget with unknown semantics just for this one field. But if you intend to convert the existing alerts to this new widget ("error message beside the field, and disable the OK button?") then we could flesh that feature out.

Comment 32

[Blake Winton (:bwinton) (:☕️)]

Wow, do we ever do terrible things there.  :P
(If we delete the server name, and then switch servers it silently uses the previous server name, for instance.)

So, I would still like the warning to show up in the dialog itself, so that the user is less surprised by what happens, but if you wanted to leave the OK button enabled and pop up a dialog, because it was easier, then feel free to do that as well.

Comment 33

[:aceman]

Attachment: patch v2

OK, I'll reuse the alert UI as we have now and I'll file a new bug to rework all the alerts in there.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 812257 [details] [diff] [review]
patch v2

Okay, it's taken a while, but ui-r=me.

So, how do you feel now about trying to add a new UI element to show the error?  ;)

Thanks,
Blake.

Comment 35

[:aceman]

(In reply to Blake Winton (:bwinton) from comment #34)
> So, how do you feel now about trying to add a new UI element to show the
> error?  ;)
Spun off to bug 922735 so that whoever feels like doing it can :)

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 812257 [details] [diff] [review]
patch v2

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

::: mail/locales/en-US/chrome/messenger/prefs.properties
@@ +5,5 @@
>  # The following are used by the Account Wizard
>  #
>  enterValidEmail=Please enter a valid email address.
> +accountNameExists=An account with this name already exists. Please enter a different account name.
> +accountNameEmpty=The account name can not be empty.

too much programmer jargon maybe?
"You have to enter an account name." perhaps

::: mailnews/base/prefs/content/AccountManager.js
@@ +652,5 @@
> +  const prefBundle = document.getElementById("bundle_prefs");
> +  let alertText = null;
> +
> +  for (let i = 0; i < pageElements.length; i++) {
> +    if (pageElements[i].id) {

favour bailing early, makes it less bulky and easier to read

if (!pageElements[i].id)
  continue;

@@ +655,5 @@
> +  for (let i = 0; i < pageElements.length; i++) {
> +    if (pageElements[i].id) {
> +      if (pageElements[i].id == "server.prettyName") {
> +        let accountName = getFormElementValue(pageElements[i]);
> +        if (!accountName || accountName == "")

"" is already covered by the falsy check

::: mailnews/base/prefs/content/amUtils.js
@@ +187,5 @@
> + * Check if the given account name already exists in any account.
> + *
> + * @param aAccountName  The account name string to look for.
> + * @param aAccountKey   The key of an account that is skipped when searching the name.
> + *                      Set to null to not skip any account.

it's usually called an optional parameter. javascript isn't fussy about missing arguments

Comment 37

[:aceman]

(In reply to Magnus Melin from comment #36)
> > +accountNameEmpty=The account name can not be empty.
> 
> too much programmer jargon maybe?
> "You have to enter an account name." perhaps
There is already a similar string at "userNameEmpty=The user name can not be empty."
If you feel strongly about changing it, please file it as a new bug so that we can fix all such occurrences uniformly. So I will keep the string as is for now as it also passed ui-r.

Comment 38

[:aceman]

Attachment: patch v3

I've fixed all the other nits, thanks.

Comment 39

[[e-mail]]

Comment on attachment 822383 [details] [diff] [review]
patch v3

>-accountExists=A mail or newsgroup account with the same user name and server name already exists. Click Back and enter a different server name, or click Cancel.
Huh, nobody uses this.

> function onAccept(aDoChecks) {
>-  if (aDoChecks) {
>-    // Check if user/host have been modified correctly.
>-    if (!checkUserServerChanges(true))
>-      return false;
>-  }
>+  if (aDoChecks && !checkAccountDataIsValid())
>+    return false;
Why not just add the new check here instead of writing a whole new function?

>+    if (!accountName)
>+      alertText = prefBundle.getString("accountNameEmpty");
>+
>+    if (accountNameExists(accountName, currentAccount.key))
>+      alertText = prefBundle.getString("accountNameExists");
Nit: use else if

>+    if ((!aAccountKey || account.key != aAccountKey) && account.incomingServer &&
>+        aAccountName == account.incomingServer.prettyName) {
No account's key is going to be empty, so the !accountKey test doesn't give you anything.

Comment 40

[Ian Neal]

Comment on attachment 822383 [details] [diff] [review]
patch v3

>+++ b/mailnews/base/prefs/content/AccountManager.js
>+function checkAccountDataIsValid() {
As Neil said, no need to add a new function, just inline it into the onAccept function.

>@@ -576,16 +584,17 @@ function checkUserServerChanges(showAler
>       if (oldUser != newUser)
>         changeText = changeText + "\n\n" + prefBundle.getString("userNameChanged");
> 
>       if (changeText != "")
>         Services.prompt.alert(window, alertTitle, changeText.trim());
>     }
>   }
> 
>+
Not sure why you have an extra empty line added here.

>+/**
>+ * If account name is not valid, alert the user.
>+ */
>+function checkAccountNameIsValid() {
>+  if (!currentAccount)
>+    return true;
>+
>+  let accountValues = getValueArrayFor(currentAccount);
As this is only used the once, you might as well inline it.
>+  if (!accountValues)
>+    return true;

r=me with the points addressed/fixed.

Comment 41

[:aceman]

Attachment: patch v4

Thanks, done.

Comment 42

[Ian Neal]

(In reply to Ian Neal from comment #40)
> Comment on attachment 822383 [details] [diff] [review]
> patch v3
> 
> >+function checkAccountNameIsValid() {
> >+  if (!currentAccount)
> >+    return true;
> >+
> >+  let accountValues = getValueArrayFor(currentAccount);
> As this is only used the once, you might as well inline it.
> >+  if (!accountValues)
> >+    return true;
You seem to have removed this completely rather than inlining it.

Comment 43

[:aceman]

It appears to me it is actually not used in the function. Am I missing something?

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/dcce7d1da770

Comment 45

[Ian Neal]

(In reply to :aceman from comment #43)
> It appears to me it is actually not used in the function. Am I missing
> something?

Other than checking the account has a value array, when does getValueArrayFor(currentAccount) not return a value array?

Comment 46

[:aceman]

No idea, from the definition of it it looks like never (returns null). It looks like the check was copied from somewhere. But I removed it as accountValues does not seem to be used in the function.