Closed Bug 40012 Opened 21 years ago Closed 7 years ago

Two identical Account Names are allowed to exist

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 28.0

People

(Reporter: bugzilla, Assigned: aceman)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
I think I have a dupe on this somewhere.
Status: NEW → ASSIGNED
Target Milestone: --- → M20
QA Contact: lchiang → nbaca
Appears similar to bug# 29579 which should be changed so that anyone can view 
the bug.
massive reassign of account manager bugs -> sspitzer
please feel free to put me back on the CC if you have any questions/comments
Assignee: alecf → sspitzer
Status: ASSIGNED → NEW
Working this one. Taking this one from Seth.
Assignee: sspitzer → racham
Status: NEW → ASSIGNED
Keywords: patch
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.
Keywords: nsCatFood
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);
Keywords: nsCatFoodnsCatFood+
Target Milestone: --- → Future
> +  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.
mass re-assign.
Assignee: racham → sspitzer
Status: ASSIGNED → NEW
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Priority: P3 → --
QA Contact: nbaca
Target Milestone: Future → ---
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
Status: NEW → UNCONFIRMED
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
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
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
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
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
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
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
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → EXPIRED
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!)...
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: EXPIRED → ---
Assignee: mail → nobody
Status: REOPENED → NEW
Component: MailNews: Account Configuration → Account Manager
Product: SeaMonkey → MailNews Core
QA Contact: account-manager
So is this a problem in the Account manager (advanced) or the Account wizard?
Summary: Two identical Account Names → Two identical Account Names are allowed to exist
Still happens at least in the Account manager. I'll try it.
Assignee: nobody → acelists
OS: Windows 2000 → All
Hardware: x86 → All
Bwinton, is this worth an alert (or a sliding alert via bug 725615)? Or just rename the duplicate name silently?
Keywords: uiwanted
I like MPT's view in comment 8.  Can we go with that instead of the sliding alert or the silent rename?
That one seems to only cover the Account Wizard. What about the Account manager?
Put an error message beside the account name, and disable the OK button?
That would fit into the Wizard, there is already UI like that. But not in the Manager.
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?
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.
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.
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?
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.
Flags: needinfo?(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.
Flags: needinfo?(bwinton)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #20368 - Attachment is obsolete: true
Attachment #812257 - Flags: ui-review?(bwinton)
Status: NEW → ASSIGNED
Keywords: uiwanted
Blocks: 922735
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.
Attachment #812257 - Flags: ui-review?(bwinton) → ui-review+
(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 :)
Attachment #812257 - Flags: review?(mkmelin+mozilla)
Attachment #812257 - Flags: review?(iann_bugzilla)
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
Attachment #812257 - Flags: review?(mkmelin+mozilla) → review+
(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.
Attached patch patch v3 (obsolete) — Splinter Review
I've fixed all the other nits, thanks.
Attachment #812257 - Attachment is obsolete: true
Attachment #812257 - Flags: review?(iann_bugzilla)
Attachment #822383 - Flags: review?(neil)
Attachment #822383 - Flags: review?(iann_bugzilla)
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 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.
Attachment #822383 - Flags: review?(iann_bugzilla) → review+
Attached patch patch v4Splinter Review
Thanks, done.
Attachment #822383 - Attachment is obsolete: true
Attachment #822383 - Flags: review?(neil)
Attachment #8338764 - Flags: review+
Keywords: checkin-needed
(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.
It appears to me it is actually not used in the function. Am I missing something?
https://hg.mozilla.org/comm-central/rev/dcce7d1da770
Status: ASSIGNED → RESOLVED
Closed: 11 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
(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?
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.
You need to log in before you can comment on or make changes to this bug.