Last Comment Bug 222388 - Set initial SMTP server choice to "Always Use Default SMTP Server" instead of specific SMTP server which is set as "Default" when account definition
: Set initial SMTP server choice to "Always Use Default SMTP Server" instead of...
Status: VERIFIED FIXED
: fixed-seamonkey2.0.1
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: Thunderbird 3.0rc1
Assigned To: Magnus Melin
:
Mentors:
: 170089 254385 255585 258861 393605 (view as bug list)
Depends on: 170520
Blocks: 119609 202308
  Show dependency treegraph
 
Reported: 2003-10-15 23:52 PDT by WADA
Modified: 2009-12-13 06:05 PST (History)
11 users (show)
ludovic: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (3.08 KB, patch)
2003-10-19 15:05 PDT, Christian Eyrich
no flags Details | Diff | Review
patch v2 (3.30 KB, patch)
2003-10-25 07:41 PDT, Christian Eyrich
neil: review-
Details | Diff | Review
proposed fix, v3 (9.48 KB, patch)
2009-10-04 10:29 PDT, Magnus Melin
no flags Details | Diff | Review
proposed fix, v4 (10.67 KB, patch)
2009-10-06 11:49 PDT, Magnus Melin
neil: superreview+
Details | Diff | Review
proposed fix, v5 (10.74 KB, patch)
2009-10-12 12:15 PDT, Magnus Melin
mnyromyr: review+
mkmelin+mozilla: superreview+
standard8: approval‑thunderbird3+
Details | Diff | Review

Description WADA 2003-10-15 23:52:44 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.6a) Gecko/20031015
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.6a) Gecko/20031015

Mozilla Version : Mozilla 1.5 release build, Comfirmed on Win-Me.

When mail/news account is defined, SMTP server setting in "Server
Settings"/"Advanced" becomes specific SMTP server which is currently set as
default SMTP server instead of "Always Use Default SMTP Server".
This has been confused not a little number of users.
I could find at least next 3 bugs.
  Bug 220761 (Already closed as invalid by reporter)
  Bug 170089 (Still UNCONFIRMED)
  Bug 222064 (Still UNCONFIRMED)

I belive Mozilla should have set SMTP setting for mail/news account to "Always
Use Default (SMTP) Server" on account definition.

 

Reproducible: Always

Steps to Reproduce:
Comment 1 Christian Eyrich 2003-10-16 05:42:00 PDT
Same when deleting a SMTP server which is used in an identity. Currently the
identities SMTP server will be the default SMTP server instead of "" (or pref
not set at all) to become it "Always Use Default SMTP Server".
Comment 2 WADA 2003-10-16 11:24:53 PDT
I think cause of Bug 161117 is also "Specific SMTP server(Which is default SMTP
Server)" instead of "Always Use Default (SMTP) Server".
Comment 3 Christian Eyrich 2003-10-19 15:05:51 PDT
Created attachment 133646 [details] [diff] [review]
proposed patch

This one should do the job.
It sets the SMTP server (mail.identity.id*.smtpServer) to "" for a new identity
and for a existing identity if its SMTP server gets deleted.

The only case for which I'm not sure is if one creates the first account (and
first SMTP server) using the wizard. It works for normal use, but I don't know
for all cases because of AccountWizard.js#489:
// refer bug#141314
// since we clone the default smtpserver with the new account's username
// force every account to use the smtp server that was created or assigned
// to it in the case of isps using rdf files
Comment 4 David :Bienvenu 2003-10-20 20:11:05 PDT
Christian, so, setting the server to the emtpy string will make us use the
default server? that seems fine...

here,

+            if(smtpServer == smtpService.defaultServer)
+              destIdentity.smtpServerKey = '';
+            else
+              destIdentity.smtpServerKey = smtpServer.key;

destIdentity.smtpServerKey = (smtpServer == smtpService.defaultServer) ? '' :
smtpServer.key;
Comment 5 Christian Eyrich 2003-10-21 05:42:32 PDT
Yep, empty means default. As does a not existing mail.identity.id*.smtpServer
pref, but I guess a undefined key in the js code can be dangerous.

> destIdentity.smtpServerKey = (smtpServer == smtpService.defaultServer) ? '' :
> smtpServer.key;

Er, yes, ok.
Comment 6 Christian Eyrich 2003-10-21 17:46:25 PDT
Huh, I just discovered a problem with the patch. It generates dupes of the
default SMTP server when creating an account.

That's because of AccountWizard.js#462
  if (!destIdentity.smtpServerKey)
It looks as this is also true if destIdentity.smtpServerKey == "". Is this
normal in JS?

I've to do 
  if (!destIdentity.smtpServerKey && destIdentity.smtpServerKey != "")
to get it work. That looks crazy to me.
Comment 7 Christian Eyrich 2003-10-25 07:41:21 PDT
Created attachment 134109 [details] [diff] [review]
patch v2

Added additonal test according comment #6 to prevent duplicate smtp servers.
Comment 8 David :Bienvenu 2003-10-29 09:37:41 PST
Christian, does checking for a 0 length string work? e.g.,
!destIdentity.smtpServerKey.length()  ? that seems marginally nicer looking code
if it works.
Comment 9 Christian Eyrich 2003-10-30 06:13:41 PST
No it doesn't work. Using length() results in not creating any SMTP server.
Maybe because if the first server is about to be created
destIdentity.smtpServerKey is null and using length() on a null pointer ...
Comment 10 Christian Eyrich 2004-01-19 13:47:58 PST
David, any progress here?
Comment 11 WADA 2004-01-20 23:36:35 PST
Bug 119609 seems to be a victim of combination of Bug 96207 (multiple SMTP
server entries were defined for same SMTP server address in the past) and this
bug (SMTP server is not "Always Use Deafult (SMTP) Server). 
Bug 218518 and Bug 222064 seem to be victims of this bug.
Comment 12 David :Bienvenu 2004-01-27 13:12:51 PST
Comment on attachment 134109 [details] [diff] [review]
patch v2

Asking Neil for r=
Comment 13 neil@parkwaycc.co.uk 2004-01-27 16:57:36 PST
Comment on attachment 134109 [details] [diff] [review]
patch v2

There's something wrong here but I can't quite put my finger on it...
Comment 14 neil@parkwaycc.co.uk 2004-02-01 06:09:53 PST
I think the wizard code would be much simpler if it was possible to make some
assumptions which I believe are true in the mozilla core but I don't know about
interaction with migration, ispdata, etc.
I'd like to be able to assume that we never prefill an existing smtp server,
i.e. that all new accounts either create a new server or use the default server.
Failing that, I'd like to be able to assume that we only prefill the key and not
the hostname for an existing smtp server.
Failing that, I'd like to be able to assume that we don't need to prefill the
hostname when reusing the default smtp server.
David, can you let me know which one is correct and I'll explain my idea.
Comment 15 David :Bienvenu 2004-02-04 14:40:31 PST
Neil, I think the first two assumptions are false, and the last one may be OK,
"assume that we don't need to prefill the hostname when reusing the default smtp
server", but I don't know for sure.
Comment 16 neil@parkwaycc.co.uk 2004-02-04 16:31:17 PST
Comment on attachment 134109 [details] [diff] [review]
patch v2

OK, I'm going to mark this as minus, because my idea is that we should not set
the smtp hostname (in aw-server.js, I think) if we are reusing the default
server.

>-    if (!destIdentity.smtpServerKey)
>+    if (!destIdentity.smtpServerKey && destIdentity.smtpServerKey != '')

So here we would check for a smtp hostname without a key, which indicates that
we're creating a new smtp server; only if our new server is a private smtp
server should we associate it by key (the code already in the patch might still
be suitable for that).
Comment 17 Mike Cowperthwaite 2004-10-20 11:18:00 PDT
*** Bug 258861 has been marked as a duplicate of this bug. ***
Comment 18 Mike Cowperthwaite 2004-10-20 11:18:42 PDT
*** Bug 255585 has been marked as a duplicate of this bug. ***
Comment 19 Mike Cowperthwaite 2004-10-20 11:27:27 PDT
*** Bug 170089 has been marked as a duplicate of this bug. ***
Comment 20 Mike Cowperthwaite 2004-10-20 11:29:50 PDT
I definitely encountered this problem when I first set up TB (0.7) but I just 
tested creating a new POP account with TB 0.8, and also with Moz 1.8a5-1019, and 
no new SMTP server entry was created.  Has this been fixed elsewhere?
Comment 21 Christian Eyrich 2004-10-20 12:22:43 PDT
(In reply to comment #20)
> I definitely encountered this problem when I first set up TB (0.7) but I just 
> tested creating a new POP account with TB 0.8, and also with Moz 1.8a5-1019,
> and no new SMTP server entry was created.  Has this been fixed elsewhere?

As I understand this bug thes problem isn't that a new SMTP server is created
upon creating a POP account. It is, that the SMTP server to use for the newly
created account is a specific (the existing, currently default) server instead
of "Always Use Default SMTP Server".
Or don't I understand your comment?
Comment 22 Mike Cowperthwaite 2004-10-20 12:50:04 PDT
(In reply to comment #21)
> It is, that the SMTP server to use for the newly
> created account is a specific (the existing, currently default) server instead
> of "Always Use Default SMTP Server".
> Or don't I understand your comment?

You understand my comment, and I was mistaken.  Your description of the bug is 
correct, and that symptom is still present in TB 0.8.  Sorry for the noise.

I did, at one point recently, notice that I had multiple entries for the same 
SMTP server, and deleted them; but now I'm not sure if that was in TB or 
Mozilla; if it was in the suite, there's no saying how far back those entries 
dated from.
Comment 23 WADA 2005-01-29 18:02:27 PST
*** Bug 170089 has been marked as a duplicate of this bug. ***
Comment 24 Mike Cowperthwaite 2005-03-09 14:11:18 PST
*** Bug 254385 has been marked as a duplicate of this bug. ***
Comment 25 Mike Cowperthwaite 2005-03-09 14:17:08 PST
Note that bug 170520 has been targeted for TB 1.1.  If that is implemented as 
described, this bug will become moot.
Comment 26 WADA 2005-10-29 22:11:56 PDT
(In reply to comment #25)
> Note that bug 170520 has been targeted for TB 1.1.  If that is implemented as 
> described, this bug will become moot.

Even when bug 170520 will be fixed(then SMTP definition on account definition will revive), this issue remains if user doesn't create new SMTP server entry on new account definition.
SMTP choice for the newly created account should be "Always use default SMTP" instead of "SMTP which is set as default on account definition", when user doesn't create new SMTP on account definition. 
Comment 27 Mike Cowperthwaite 2006-01-12 09:37:28 PST
Note that when creating a new identity for an existing account, the value is initialized to "Use default server" (Seamonkey 1.0/1.5 and TB 1.5/1.6) -- but this might be a side-effect of bug 313987.
Comment 28 WADA 2006-02-01 17:13:49 PST
Additional description in order to avoid confusion like Bug 299495 Comment #3.

After great improvement of Bug 202468, "SMTP choice" isn't hidden under "Advanced" button any more.
Go Tools/Account Settings/Manage Identities, and see each identity's settings thru "Edit" button, and go "settings" tab, then see "Outgoing Server(SMTP)" choice.
"Always Use Default SMTP Server" is slightly modified, and "Use Default Server" when Thunderbird 1.5.
To use SMTP server which is currently defaulted SMTP(default SMTP when mail composing) always, "Use Default Server" should be choosed.  
Comment 29 WADA 2007-08-25 16:49:05 PDT
Very important/good MozillaZine Knowledge Base article has been made recently.
  http://kb.mozillazine.org/Mail_concepts
  SMTP section : http://kb.mozillazine.org/Mail_concepts#SMTP_servers
Thanks a lot to MozillaZine Knowledge Base team.
Comment 30 WADA 2008-07-19 23:57:07 PDT
Setting dependency to Bug 170520, because Bug 170520 is trying to provide option to choose "Always use the default outgoing server" during Account Wizard.
See Bug 170520 Comment #61 for it. 
Comment 31 WADA 2009-08-14 16:24:16 PDT
Bug 170520 has been fixed, and new File/New/mail Account(Quick Setup) offers SMTP server setting upon account creation. However, SMTP setting is still (a) new one, or (b) one of existent SMTPs including currently set as default. There is still no way to set "Use Default Server" even after fix of Bug 170520.

This bug's request is now:
 (1) For ordinal account definition
     Set initial choice of SMTP to "Use Default Server"
     instead of "SMTP currently set as default SMP server".
 (2) For File/New/mail Account(Quick Setup)
     Offer "Use Default Server" option, if already defined SMTP exists.
Comment 32 [:Aureliano Buendía] 2009-09-23 03:30:52 PDT
*** Bug 393605 has been marked as a duplicate of this bug. ***
Comment 33 Magnus Melin 2009-10-04 10:29:40 PDT
Created attachment 404517 [details] [diff] [review]
proposed fix, v3

Roughly the previous patch + comment 16.
Comment 34 neil@parkwaycc.co.uk 2009-10-05 04:21:13 PDT
Comment on attachment 404517 [details] [diff] [review]
proposed fix, v3

Thanks for working on this!

>+            // If smtpCreateNewServer pref is set then createSmtpServer
>+            // routine() will create one. Otherwise, default server is returned
>+            // which is also set to create a new smtp server
>+            // (via GetDefaultServer()) if no default server is found.
I don't think this comment is relevant any more.

>+              destIdentity.smtpServerKey =
>+                (smtpServer == smtpService.defaultServer) ? "" : smtpServer.key;
I'm not sure what the point of this is?

>+    var usingDefaultSMTP = document.getElementById("noSmtp").hidden;
Ugly, but I don't have any better ideas at the moment.

>-  if (pageData.server && pageData.server.smtphostname && smtpCreateNewServer) {
>+  if (pageData.server && pageData.server.smtphostname && smtpCreateNewServer &&
>+      !reusingDefaultSmtp) {
resuingDefaultSmtp can never be set if smtpCreateNewServer is set.

>   }
> 
>   if (smtpServer && smtpServer.hostname) {
I wonder whether we would be better off with several else if conditions to separate out the various possibilities i.e.
a) existing smtp server (only modify static text)
b) ispdata provides smtp server (smtphostname prefilled)
c) manual new smtp server
Comment 35 Magnus Melin 2009-10-06 11:49:04 PDT
(In reply to comment #34)
> >+            // If smtpCreateNewServer pref is set then createSmtpServer
> >+            // routine() will create one. Otherwise, default server is returned
> >+            // which is also set to create a new smtp server
> >+            // (via GetDefaultServer()) if no default server is found.
> I don't think this comment is relevant any more.

Indeed, there were a lot of old garbage here.

> >   if (smtpServer && smtpServer.hostname) {
> I wonder whether we would be better off with several else if conditions to
> separate out the various possibilities i.e.
> a) existing smtp server (only modify static text)
> b) ispdata provides smtp server (smtphostname prefilled)
> c) manual new smtp server

I'm not sure, i think that would be more code duplication, i left it as is to cause least disruption.
Comment 36 Magnus Melin 2009-10-06 11:49:54 PDT
Created attachment 404872 [details] [diff] [review]
proposed fix, v4

Addressing neils comments
Comment 37 neil@parkwaycc.co.uk 2009-10-07 05:44:06 PDT
Comment on attachment 404872 [details] [diff] [review]
proposed fix, v4

>+            if (!smtpService.defaultServer.hostname) {
>+              smtpService.defaultServer = smtpServer;
>+              isDefaultSmtpServer = true;
>+            }
> 
>+            copyObjectToInterface(smtpServer, accountData.smtp, false);
> 
>+            // If it's the default server we created, make the identity use
>+            // "Use Default" by default.
>+            destIdentity.smtpServerKey =
>+              (isDefaultSmtpServer) ? "" : smtpServer.key;
[You could possibly write
if (!smtpService.defaultServer.hostname)
  smtpService.defaultServer = smtpServer;
else
  destIdentity.smtpServerKey = smtpServer.key;
but ideally you would choose the key based on whether the smtpCreateNewServer flag was set (i.e. for a manually entered server you would suggest adding it as use default server but for an ispdata entered server you would always want that identity to use that server).]

>   if (pageData.server && pageData.server.smtphostname && smtpCreateNewServer) {
>     var smtpTextBox = document.getElementById("smtphostname");
>     if (smtpTextBox && smtpTextBox.value == "")
>       smtpTextBox.value = pageData.server.smtphostname.value;
>-    boxToShow = haveSmtpBox;
>-    boxToHide = noSmtpBox;
>   }
> 
>   if (smtpServer && smtpServer.hostname) {
>     // we have a hostname, so modify and show the static text and 
>     // store the value of the default smtp server in the textbox.
>     modifyStaticText(smtpServer.hostname, "1")
>-    var smtpTextBox = document.getElementById("smtphostname");
>-    if (smtpTextBox && smtpTextBox.value == "")
>-      smtpTextBox.value = smtpServer.hostname;
>+    if (!reusingDefaultSmtp) {
>+      var smtpTextBox = document.getElementById("smtphostname");
>+      if (smtpTextBox && smtpTextBox.value == "")
>+        smtpTextBox.value = smtpServer.hostname;
I just realised we don't need to set this here, because in the case of a new smtp server forced by the ispdata we already set the smtpTextBox above, so this assignment and the reusingDefaultSmtp variable can be eliminated.
Comment 38 Magnus Melin 2009-10-08 10:11:20 PDT
(In reply to comment #37)
> >   if (pageData.server && pageData.server.smtphostname && smtpCreateNewServer) {
> >     var smtpTextBox = document.getElementById("smtphostname");
> >     if (smtpTextBox && smtpTextBox.value == "")
> >       smtpTextBox.value = pageData.server.smtphostname.value;
> >-    boxToShow = haveSmtpBox;
> >-    boxToHide = noSmtpBox;
> >   }
> > 
> >   if (smtpServer && smtpServer.hostname) {
> >     // we have a hostname, so modify and show the static text and 
> >     // store the value of the default smtp server in the textbox.
> >     modifyStaticText(smtpServer.hostname, "1")
> >-    var smtpTextBox = document.getElementById("smtphostname");
> >-    if (smtpTextBox && smtpTextBox.value == "")
> >-      smtpTextBox.value = smtpServer.hostname;
> >+    if (!reusingDefaultSmtp) {
> >+      var smtpTextBox = document.getElementById("smtphostname");
> >+      if (smtpTextBox && smtpTextBox.value == "")
> >+        smtpTextBox.value = smtpServer.hostname;
> I just realised we don't need to set this here, because in the case of a new
> smtp server forced by the ispdata we already set the smtpTextBox above, so this
> assignment and the reusingDefaultSmtp variable can be eliminated.

Actually no, if i get rid of this change it won't work properly. A second smtp server with part of the first smtp server values would get created.
Comment 39 neil@parkwaycc.co.uk 2009-10-08 13:08:22 PDT
Sorry, which first smtp server values? If we're forcing the creation of a new server then there won't be any other values.
Comment 40 Magnus Melin 2009-10-08 22:16:32 PDT
The test i did (when i'd removed the reusingDefaultSmtp part of the patch) is that i created a gmail imap account (from the rdf file) using the old account wiz. That created an smtp server correctly as needed - all well. Then i created second normal email account, the wiz said it would reuse the smtp server, but in the end it ended up creating another smtp.gmail.com server with the wrong port.
Comment 41 neil@parkwaycc.co.uk 2009-10-11 16:21:19 PDT
Sorry, I wasn't quite clear: when I said "we don't need to do this", I was referring to the existing code, not your change, i.e. rather than changing the code you should simply be able to remove it instead.
Comment 42 Magnus Melin 2009-10-12 12:15:51 PDT
Created attachment 405896 [details] [diff] [review]
proposed fix, v5

Ah, carrying fwd sr=neil
Comment 43 neil@parkwaycc.co.uk 2009-10-12 15:56:47 PDT
Comment on attachment 405896 [details] [diff] [review]
proposed fix, v5

>+    reusingDefaultSmtp = true;
You're not using this any more ;-)
Comment 44 Magnus Melin 2009-10-13 11:32:00 PDT
I'll remove it (won't bother uploading a new patch for that).
Comment 45 Karsten Düsterloh 2009-10-19 13:46:28 PDT
Comment on attachment 405896 [details] [diff] [review]
proposed fix, v5

>         if (destIdentity.attachSignature)
>         {
>             var sigFileName = accountData.signatureFileName;
>-      
>+
...
> nsSmtpService::CreateSmtpServer(nsISmtpServer **aResult)
> {
>     if (!aResult) return NS_ERROR_NULL_POINTER;
> 
>     loadSmtpServers();
>     nsresult rv;
>-    
>+
>     PRInt32 i = 0;
>     PRBool unique = PR_FALSE;
> 
>     findServerByKeyEntry entry;
>     nsCAutoString key;
>-    
>+
>     do {
>         key = "smtp";
>         key.AppendInt(++i);
>-        
>+
>         entry.key = key.get();
>         entry.server = nsnull;
> 
>         mSmtpServers.EnumerateForwards(findServerByKey, (void *)&entry);
>         if (!entry.server) unique=PR_TRUE;
>-        
>+
>     } while (!unique);

You might as well just drop all those empty lines.
Comment 46 Magnus Melin 2009-10-19 23:21:07 PDT
Comment on attachment 405896 [details] [diff] [review]
proposed fix, v5

Risk assesment: low to moderate. Moderate mainly because the relative complexity of the account wiz code.
Comment 47 Mark Banner (:standard8) 2009-10-20 01:53:13 PDT
Comment on attachment 405896 [details] [diff] [review]
proposed fix, v5

Having known similar issues to this before, I think it is worth taking. It would be good if you could come up with a few litmus tests or basic procedure so that testers can check this does what it is meant to and we don't get strange effects like changing/adding an SMTP server sets all the options wrongly.

a=Standard8
Comment 48 Magnus Melin 2009-10-20 12:14:41 PDT
changeset:   4204:0bc211f9879d
http://hg.mozilla.org/comm-central/rev/0bc211f9879d

->FIXED

The litmus tests would be 

---

Using the "old" account wizard, set up an account, the SMTP server for the new account should be set to "Use Default Server", not the specific server that is currently default.

---

Set up two SMTP servers, and set an identity to use the second SMTP server. Delete the second SMTP server, and check that the SMTP of the identity that used it is now set to "Use Default Server" (not the specific server that is currently default).

---
Comment 49 Ludovic Hirlimann [:Usul] 2009-10-21 04:18:37 PDT
(In reply to comment #48)

> 
> Using the "old" account wizard, set up an account, the SMTP server for the new
> account should be set to "Use Default Server", not the specific server that is
> currently default.
> 

9593

> 
> Set up two SMTP servers, and set an identity to use the second SMTP server.
> Delete the second SMTP server, and check that the SMTP of the identity that
> used it is now set to "Use Default Server" (not the specific server that is
> currently default).

9594
Comment 50 Magnus Melin 2009-10-28 10:36:49 PDT
Thx Ludo. Is there somewhere i could have added those myself? Didn't find it, or maybe i don't have correct bits for it to show modification actions.
Comment 51 WADA 2009-11-25 18:46:57 PST
This bug was changed to FIXED by resolving problem of "old" account wizard part in Comment 31 only(because initial request was so). And "old" account wizard was removed from Thunderbird 3. Seamonkey keeps the "old" account wizard, and initial problem is resolved by Sm 2.1(current latest-trunk) => Verified.
 
For new auto-config part of Tb 3, Bug 529905 is opened.

Note You need to log in before you can comment on or make changes to this bug.