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

Status: VERIFIED FIXED

(MailNews Core :: Account Manager, defect)

Description

[WADA:World Anti-bad-Duping Agency]

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]

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:World Anti-bad-Duping Agency]

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]

Attachment: 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]

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]

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]

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]

Attachment: patch v2

Added additonal test according comment #6 to prevent duplicate smtp servers.

Comment 8

[David :Bienvenu]

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]

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]

David, any progress here?

Comment 11

[WADA:World Anti-bad-Duping Agency]

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]

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

Asking Neil for r=

Comment 13

[[e-mail]]

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

[[e-mail]]

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]

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

[[e-mail]]

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]

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

Comment 18

[Mike Cowperthwaite]

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

Comment 19

[Mike Cowperthwaite]

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

Comment 20

[Mike Cowperthwaite]

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]

(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]

(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:World Anti-bad-Duping Agency]

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

Comment 24

[Mike Cowperthwaite]

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

Comment 25

[Mike Cowperthwaite]

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:World Anti-bad-Duping Agency]

(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]

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:World Anti-bad-Duping Agency]

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:World Anti-bad-Duping Agency]

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:World Anti-bad-Duping Agency]

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:World Anti-bad-Duping Agency]

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 33

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Roughly the previous patch + comment 16.

Comment 34

[[e-mail]]

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 [:mkmelin]]

(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 [:mkmelin]]

Attachment: proposed fix, v4

Addressing neils comments

Comment 37

[[e-mail]]

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 [:mkmelin]]

(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

[[e-mail]]

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 [:mkmelin]]

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

[[e-mail]]

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 [:mkmelin]]

Attachment: proposed fix, v5

Ah, carrying fwd sr=neil

Comment 43

[[e-mail]]

Comment on attachment 405896 [details] [diff] [review]
proposed fix, v5

>+    reusingDefaultSmtp = true;
You're not using this any more ;-)

Comment 44

[Magnus Melin [:mkmelin]]

I'll remove it (won't bother uploading a new patch for that).

Comment 45

[Karsten Düsterloh]

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 [:mkmelin]]

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)]

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 [:mkmelin]]

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]]

(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 [:mkmelin]]

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:World Anti-bad-Duping Agency]

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.