164678 - Cannot set Outgoing SMTP Server in Account Settings

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[jhp (no longer active)]

Found using builds from 20020826 on Win2000 and OS/2.

Steps to recreate:
1) Create new profile
2) Open MailNews
3) In Account Wizard, create a Newsgroup account.
4) When the MailNews window appears, open Edit->Mail & Newsgroups Account
Settings...
5) Go to "Outgoing Server (SMTP)" panel
6) Enter smtp server and click OK to dismiss dialog
7) Reopen "Mail & Newsgroups Account Settings..." dialog, and notice that smtp
server is not set in SMTP panel

Notes:
a) When ever I bring up the "Outgoing Server (SMTP)" panel, I get this error in
the console:
   Error: uncaught exception: [Exception... "Component returned failure code:
0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult:
"0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame ::
chrome://messenger/content/smtpEditOverlay.js :: onLockPreference :: line 95" 
data: no]

Comment 1

[varada]

nominating nsbeta1 - reassigning to varada

Comment 2

[varada]

Attachment: Patch V1.0

When we create a news account- we dont have an smtp server. If the user enters
the info in the smtp page we do not save it because of faulty logic. This fix
will make it so that we save the info and make it the default smtp server as
well will not let the hostname be blank once the default smtp server is
created. If the user deletes it we replace it with the old hostname.Changes to
the hostname are accepted. This patch also contains the fix for 169366 (do not
create emtpy smtp servers).

Comment 3

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

1)

+    //if we have a defaultSmtpServer and user deletes the hostname 
+    //we have to retain that value - otherwise we will have an empty server slot.
+    if(defaultSmtpServer && hostnameIsIllegal(gSmtpHostname.value))
+      gSmtpHostname.value = defaultSmtpServer.hostname;

So if I am editing an SMTP server, and I set the hostname to something illegal, 
it sets it to the default smtp server hostname?  Why not just allow your other
code, the one
that won't let you close the dialog window if hostname is illegal, come into play?
Or am I misunderstanding?

2) 

+    
+    //if we have a null defaultSmtpServer and if the hostname field has valid 
+    //values then we create a server and make it as the default.
+    if((defaultSmtpServer == null) && (!hostnameIsIllegal(gSmtpHostname.value))) {
+      defaultSmtpServer = smtpService.createSmtpServer();
+      defaultSmtpServer = smtpService.defaultServer;
+    }

Why is the second line needed?  if I didn't have an smtp server before, creating
one is enough.
See AccountWizard.js(450), for example.

3)

make sure you code does the right thing from the main SMTP panel, and the
advanced SMTP edit dialog.

4)

where do we alert the user and prevent them from continuing if the hostname is
illegal?

5)

A style nit:

follow the normal if () convention, instead of if()

Comment 4

[varada]

Attachment: Patch V 1.1

>>1)
>>So if I am editing an SMTP server, and I set the hostname to something
illegal, 
>>it sets it to the default smtp server hostname?  Why not just allow your
other
>>code, the one
>>that won't let you close the dialog window if hostname is illegal, come into
>>play?
>>Or am I misunderstanding?

Changed it so that if there is an illegal name we throw up an alert(as jglick
had suggested).

>>2) 
>>
>>Why is the second line needed?  if I didn't have an smtp server before,
>.creating
>>one is enough.
>>See AccountWizard.js(450), for example.

Got rid of the redundant setting.
3)

>>make sure you code does the right thing from the main SMTP panel, and the
>>advanced SMTP edit dialog.

Tested to make sure that they both behave as they should

4)

>>where do we alert the user and prevent them from continuing if the hostname
is
>>illegal?

As in question 1- We now alert them in three places - one - when they have an
illegal name in the Smtp panel and try clicking elsewhere; two - when they try
to click Ok and three when they try to click Advanced.
Similarly for the Advanced button - when they click OK with an illegal name.

5)

>>A style nit:

>>follow the normal if () convention, instead of if()

Took care of this.

Comment 5

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

Comment on attachment 99806 [details] [diff] [review]
Patch V 1.1

needs work. 

don't use window.alert() [or window.confirm()] from chrome. 

you should be getting an assertion when you try to do this in a debug build.

Comment 6

[varada]

Attachment: Patch V1.2

Converted the window.alerts to using PromptService as requested in previous
patch and moved common functions to a new utility file amUtils.js (to be shared
between account manager and account wizard xul files)

Comment 7

[varada]

Attachment: New file amUtils.js

New utility file amUtils.js

Comment 8

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

Comment on attachment 100640 [details] [diff] [review]
Patch V1.2

1) should the code you added to function checkUserServerChanges(showAlert)  
heed showAlert?

2)

we already have a property for this string:

enterValidHostname=Please enter a valid hostname.

Comment 9

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

Comment on attachment 100641 [details]
New file amUtils.js

sr=sspitzer

is there any other common code we should move here?

for completeness, can you log a bug on the XXX todo item?  (there might be one
already, query first)

Comment 10

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

About using window.alert() as a back up, are you seeing that in other code or
are you trying to be extra safe?

If we don't get the prompt service, we're in a world of hurt.  So I'm not sure
if we want to add the extra safe code, as it's just bloat.

Comment 11

[varada]

>>1) should the code you added to function checkUserServerChanges(showAlert)  
>>heed showAlert?
we need to show this alert regardless of whether the showAlert is true or not
becuase lack of hostname for smtp servers cause a host of problems and the user
has to be alerted to this.
>>2)
>>we already have a property for this string:
>>enterValidHostname=Please enter a valid hostname.

I can use this instead of the one I added 
validServerName=Please enter a valid server name.

>>About using window.alert() as a back up, are you seeing that in other code or
>>are you trying to be extra safe?
I have seen this be used as a backup in other places in the mail.

>>for completeness, can you log a bug on the XXX todo item?  (there might be one
>>already, query first)

http://bugzilla.mozilla.org/show_bug.cgi?id=80855
fix hostnameIsIllegal() in aw-server.js to be more robust

Comment 12

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

ok, thanks for addressing my questions.

can you attach a final patch?

Comment 13

[varada]

Attachment: Patch V1.3

Got rid of the new properties entry validServerName and used the existing
enterValidHostname.

Comment 14

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

Comment on attachment 100774 [details] [diff] [review]
Patch V1.3

sr=sspitzer

Comment 15

[varada]

Marking Fixed.

Comment 16

[Ninoschka Baca]

Buffy Trunk build 2002-10-29: WinMe, Linux 8, Mac 10.1.3
Verified Fixed.(New profile, configured news, added an smtp server, successfully
sent a message)