216245 - subscribe/account wizard for newsgroups forgets account name

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Patrick]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030813
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030813

Couldn't find this one.

The wizard for subscribing the newsgroups lets you choose an account name.
However this name is forgotten after pressing NEXT.

Reproducible: Always

Steps to Reproduce:
1. Subscribe to a newsgroup on a server to which you're not yet subscribed, for
instance in a new profile news://news.mozilla.org/netscape.public.mozilla.general
2. go to the wizards steps. After filing out some other info you get to pick
account name. Delete the default name and type "Mozilla" instead [screenshot 1]
3. Click next
4. The Account wizard congratulates you, but shows replaced the user provided
account name with the default name again [screenshot 2]
3.

Actual Results:  
information entered would be remembered

Expected Results:  
information entered was forgotten

Tested with clean profile as well. 
Screenshots coming up.

Comment 1

[Patrick]

Attachment: The wizard allows you to enter an Account name for this server

Comment 2

[Patrick]

Attachment: ...But after pressing NEXT, the account name is reset to its default value

Comment 3

[Patrick]

Still present in 2004041208

Comment 4

[Ian Neal]

Attachment: Patch v0.1 - simple fix

This fixes the problem but not sure if it breaks ISPs stuff.

Comment 5

[Patrick]

Ian, i'll try your patch later.
 
Is this something that would reside in CORE nowadays? 

Also, can you fix the typo in 
  // If the AccountData exists, tha means we have values read from rdf file.

-> "tha" -> "that"

What ISP stuff could be broken by this? Anything special i should keep an eye
out for?

Comment 6

[Ian Neal]

Attachment: Tweaked simple patch v0.1a

Changes since v0.1a
* Fixed typo in comment

Comment 7

[Karsten Düsterloh]

Comment on attachment 187987 [details] [diff] [review]
Tweaked simple patch v0.1a

>     var accountName="";
>-    if (pageData.accname && pageData.accname.prettyName) {
>+    if (pageData.accname && pageData.accname.prettyName)
>         accountName = pageData.accname.prettyName.value;
>-
>-        // If the AccountData exists, tha means we have values read from rdf file.
>+    else {
>+        // If the AccountData exists, that means we have values read from rdf file.

The checking mechanism of the previous wizard page (in aw-accname.js) ensures
that pageData.accname.prettyName is *never* empty (except in edge cases), so
the else clause effectively cuts off that part.
See the original version of this block in
<http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&
root=/cvsroot&subdir=mozilla/mailnews/base/prefs/resources/content&command=DIFF
_FRAMESET&file=aw-done.js&rev2=1.10&rev1=1.9> for what most probably was
meant...

Comment 8

[Mark Banner (:standard8)]

Attachment: Fixes the account name setting.

This fix takes a slightly different approach. Basically, if the user hasn't
entered a value for account name, and a name is specified in the current
account details, then we will use it, otherwise we use what they entered. The
"otheraccount" additional line also fixes a js problem which I believe was
fallout from bug 253519.

I've tested on seamonkey for: news link, mail account, news account, movemail &
isp specific data.
thunderbird: mail account, news account, movemail, rss & isp specific data.

I couldn't work out how to get a news link into thunderbird, but I don't see
any problems with that if you can.

Comment 9

[Mark Banner (:standard8)]

*** Bug 141852 has been marked as a duplicate of this bug. ***

Comment 10

[Karsten Düsterloh]

Comment on attachment 191795 [details] [diff] [review]
Fixes the account name setting.

Looks good, just a few unrelated ;-) comments.

>   setPageData(pageData, "accname", "prettyName", accountname);
>+  // Set this to true so we know the user has set the name.
>+  setPageData(pageData, "accname", "userset", true);

This is quite a strange function, as it creates lots of normal arrays that
aren't actually used as normal arrays, but as associative arrays only (AFAICT!)
- and these are actually just objects in JS. Judging by the comments there, the
author probably didn't exactly knew the difference...


BTW: I get some JS strict errors in that dialog, partly even without this
patch:

JavaScript strict warning: chrome://messenger/content/AccountWizard.js, line
631: function setupCopiesAndFoldersServer does not always return a value
JavaScript strict warning: chrome://messenger/content/AccountWizard.js, line
642: function setupCopiesAndFoldersServer does not always return a value
JavaScript strict warning: chrome://messenger/content/AccountWizard.js, line
644: function setupCopiesAndFoldersServer does not always return a value

JavaScript strict warning: chrome://messenger/content/aw-accounttype.js, line
78: reference to undefined property gCurrentAccountData.wizardSkipPanels
JavaScript strict warning: chrome://messenger/content/aw-accounttype.js, line
93: assignment to undeclared variable skipArray
JavaScript strict warning: chrome://messenger/content/aw-done.js, line 72:
reference to undefined property currentAccountData.emailIDFieldTitle

Your patch fixes these:
JavaScript strict warning: chrome://messenger/content/aw-server.js, line 133:
reference to undefined property pageData.accounttype.otheraccount
JavaScript error: chrome://messenger/content/aw-server.js, line 133:
pageData.accounttype.otheraccount has no properties

Maybe you could at least fix the strict warnings in those files you're touching
anyway?

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> JavaScript strict warning: chrome://messenger/content/aw-accounttype.js, line
> 78: reference to undefined property gCurrentAccountData.wizardSkipPanels
> JavaScript strict warning: chrome://messenger/content/aw-accounttype.js, line
> 93: assignment to undeclared variable skipArray
> JavaScript strict warning: chrome://messenger/content/aw-done.js, line 72:
> reference to undefined property currentAccountData.emailIDFieldTitle

Karsten, I fixed the others, but can't replicate these - I'd like to be able to
before I try and "fix" them. Can you give me some pointers please?

Comment 12

[Mark Banner (:standard8)]

Attachment: Fix the account name setting v2 (checked in to trunk + branch)

I've address Mnyromyr's comments and removed all the strict warnings. Carrying
forward his r. Requesting sr. (btw this fixes both SeaMonkey & Thunderbird)

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 194016 [details] [diff] [review]
Fix the account name setting v2 (checked in to trunk + branch)

I just checked this into trunk. I'm leaving open whilst requesting approval for
1.8 branch.

This patch fixes the account name setting when performed via the account wizard
for both SeaMonkey and Thunderbird.

Comment 14

[[e-mail]]

Comment on attachment 194016 [details] [diff] [review]
Fix the account name setting v2 (checked in to trunk + branch)

>     if (pageData.accname && pageData.accname.prettyName) {
>         accountName = pageData.accname.prettyName.value;
> 
>-        // If the AccountData exists, tha means we have values read from rdf file.
>+        // Only if the user hasn't set the account name should we use the
>+        // values from the rdf file - if the account data exists.
>         // Get the pretty name and polish the account name
>-        if ( currentAccountData && 
>-             currentAccountData.incomingServer.prettyName)
>+        if (pageData.accname.userset &&
>+            !pageData.accname.userset.value &&
>+            currentAccountData && 
>+            currentAccountData.incomingServer.prettyName)
>         {
>             var prettyName = currentAccountData.incomingServer.prettyName; 
>             // Get the polished account name 
>             accountName = gPrefsBundle.getFormattedString("accountName",
>                                                           [prettyName,
>                                                            userName]);
>             // Set that to be the name in the pagedata 
>             pageData.accname.prettyName.value = accountName;
Would you mind confirming/explaining what used to happen in the following
cases:
a) new account - page data holds user entered name, account data holds no name
b) news server - page data holds user entered name, account data holds news
server name
c) isp account - account data holds isp entered name, page data holds what?
I can think of two possibilities:
i) c) page data is blank, but that would seem to mean that the code never used
to work
ii) c) page data holds isp entered name, so that the override code is
unnecessary

Comment 15

[Mark Banner (:standard8)]

> Would you mind confirming/explaining what used to happen in the following
> cases:

> a) new account - page data holds user entered name, account data holds no name

Correct.

> b) news server - page data holds user entered name, account data holds news
> server name

Correct as long as you take news accounts selected for creation in the wizard as
case a). 

In full: If going via a news:// link then b) applies. If creating a new account
via selecting on the wizard, then a) applies.

> c) isp account - account data holds isp entered name, page data holds what?
> I can think of two possibilities:
> i) c) page data is blank, but that would seem to mean that the code never used
> to work
> ii) c) page data holds isp entered name, so that the override code is
> unnecessary

ok, I'm still looking at this, it depends on the exact isp data. One case is
example.rdf where we allow the user to set the name (page data) but then use the
account data to modify it.  So now I've fixed the news group one, but broken
some cases of isp data :-(

All of the isp data account name settings are a bit strange anyway, because the
user can modify them afterwards regardless of what the wizard created them as.

I'll see if I can come up with an improved fix later.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 194016 [details] [diff] [review]
Fix the account name setting v2 (checked in to trunk + branch)

Removing approval request as it breaks isp data.

Comment 17

[Mark Banner (:standard8)]

Attachment: Fix the ispdata regression/problem.

Ok, this patch fixes the ispdata regression - we now check to see if the data was isp supplied, and if it was we also add in a check to see if we should be using the user supplied accountName, or just the first part of the email address.

This seems to work correctly for all the rdf examples of ispdata that we have in the tree and for creating new newsgroups.

Comment 18

[David :Bienvenu]

Comment on attachment 201991 [details] [diff] [review]
Fix the ispdata regression/problem.

+            if (pageData.accname.userset && pageData.accname.userset.value)
+            {
+              accountName = gPrefsBundle.getFormattedString("accountName",
+                                                            [prettyName,
+                                                             accountName]);
+            }
+            else
+            {
+              accountName = gPrefsBundle.getFormattedString("accountName",
+                                                            [prettyName,
+                                                             userName]);
+            }


you can use the ? operator here, I would think to choose between userName and accountName based on the if criteria.

And, do we even need the accountName variable at all? Can't we set pageData.accname.prettyName.value directly to the result of gPrefsBundle.getFormattedString...?

Comment 19

[Mark Banner (:standard8)]

Attachment: Fix the ispdata regression/problem v2 (checked in to trunk + branch)

Revised to remove the use of the extra variables that are essentially redundant.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 203155 [details] [diff] [review]
Fix the ispdata regression/problem v2 (checked in to trunk + branch)

This patch is now in on trunk:
new revision: 1.129; previous revision: 1.128
done
Checking in mailnews/base/prefs/resources/content/aw-done.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/aw-done.js,v  <--  aw-done.js
new revision: 1.23; previous revision: 1.22

Comment 21

[Mark Banner (:standard8)]

Regression fixed -> bug fixed :-)

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 194016 [details] [diff] [review]
Fix the account name setting v2 (checked in to trunk + branch)

Requesting 1.8.1 branch approval for this patch that has been on the trunk for quite a while now. It'll need the second patch on this bug as well to avoid the regression.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 203155 [details] [diff] [review]
Fix the ispdata regression/problem v2 (checked in to trunk + branch)

The second patch on this bug that needs to go in with the first...