Set initial SMTP server choice to "Always Use Default SMTP Server" instead of specific SMTP server which is set as "Default" when account definition

VERIFIED FIXED in Thunderbird 3.0rc1

Status

MailNews Core
Account Manager
VERIFIED FIXED
14 years ago
8 years ago

People

(Reporter: World, Assigned: Magnus Melin)

Tracking

({fixed-seamonkey2.0.1})

Trunk
Thunderbird 3.0rc1
fixed-seamonkey2.0.1
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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:

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

14 years ago
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".
(Reporter)

Comment 2

14 years ago
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

14 years ago
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

Updated

14 years ago
Attachment #133646 - Flags: review?(bienvenu)

Comment 4

14 years ago
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

14 years ago
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.

Updated

14 years ago
Attachment #133646 - Flags: review?(bienvenu)

Comment 6

14 years ago
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.
Assignee: sspitzer → ch.ey

Comment 7

14 years ago
Created attachment 134109 [details] [diff] [review]
patch v2

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

Updated

14 years ago
Attachment #134109 - Flags: review?(bienvenu)

Comment 8

14 years ago
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

14 years ago
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

14 years ago
David, any progress here?
(Reporter)

Comment 11

14 years ago
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.
(Reporter)

Updated

13 years ago
Blocks: 119609
(Reporter)

Updated

13 years ago
Blocks: 202308

Comment 12

13 years ago
Comment on attachment 134109 [details] [diff] [review]
patch v2

Asking Neil for r=
Attachment #134109 - Flags: superreview?(bienvenu)
Attachment #134109 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134109 - Flags: review?(bienvenu)

Comment 13

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
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).
Attachment #134109 - Flags: review?(neil.parkwaycc.co.uk) → review-

Updated

13 years ago
Attachment #134109 - Flags: superreview?(bienvenu)

Comment 17

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

Comment 18

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

Comment 19

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

Comment 20

13 years ago
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

13 years ago
(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

13 years ago
(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.
Product: Browser → Seamonkey
(Reporter)

Comment 23

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

Updated

12 years ago
Summary: Set initial SMTP server setting to "Always Use Deafult SMTP Server" instead of specific SMTP server → Set initial SMTP server setting to "Always Use Default SMTP Server" instead of specific SMTP server

Comment 24

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

Comment 25

12 years ago
Note that bug 170520 has been targeted for TB 1.1.  If that is implemented as 
described, this bug will become moot.
(Reporter)

Updated

12 years ago
Summary: Set initial SMTP server setting to "Always Use Default SMTP Server" instead of specific SMTP server → Set initial SMTP server choice to "Always Use Default SMTP Server" instead of specific SMTP server which is set as "Default" when account definition
(Reporter)

Comment 26

12 years ago
(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

11 years ago
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.
(Reporter)

Comment 28

11 years ago
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.  
(Reporter)

Comment 29

10 years ago
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.
(Reporter)

Comment 30

9 years ago
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. 
Depends on: 170520
QA Contact: nbaca → nobody
(Reporter)

Comment 31

8 years ago
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.

Updated

8 years ago
Duplicate of this bug: 393605
(Assignee)

Updated

8 years ago
QA Contact: nobody → mailnews-account
(Assignee)

Comment 33

8 years ago
Created attachment 404517 [details] [diff] [review]
proposed fix, v3

Roughly the previous patch + comment 16.
Assignee: ch.ey → mkmelin+mozilla
Attachment #133646 - Attachment is obsolete: true
Attachment #134109 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #404517 - Flags: superreview?(neil)
Attachment #404517 - Flags: review?(mnyromyr)
(Assignee)

Updated

8 years ago
Severity: enhancement → normal
Target Milestone: --- → seamonkey2.0
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
(Assignee)

Comment 35

8 years ago
(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.
(Assignee)

Comment 36

8 years ago
Created attachment 404872 [details] [diff] [review]
proposed fix, v4

Addressing neils comments
Attachment #404517 - Attachment is obsolete: true
Attachment #404872 - Flags: superreview?(neil)
Attachment #404872 - Flags: review?(mnyromyr)
Attachment #404517 - Flags: superreview?(neil)
Attachment #404517 - Flags: review?(mnyromyr)
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.
Attachment #404872 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 38

8 years ago
(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.
Sorry, which first smtp server values? If we're forcing the creation of a new server then there won't be any other values.
(Assignee)

Comment 40

8 years ago
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.
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.
(Assignee)

Comment 42

8 years ago
Created attachment 405896 [details] [diff] [review]
proposed fix, v5

Ah, carrying fwd sr=neil
Attachment #404872 - Attachment is obsolete: true
Attachment #405896 - Flags: superreview+
Attachment #405896 - Flags: review?(mnyromyr)
Attachment #404872 - Flags: review?(mnyromyr)
Comment on attachment 405896 [details] [diff] [review]
proposed fix, v5

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

Comment 44

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

Comment 45

8 years ago
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.
Attachment #405896 - Flags: review?(mnyromyr) → review+
(Assignee)

Updated

8 years ago
Component: MailNews: Account Configuration → Account Manager
Product: SeaMonkey → MailNews Core
QA Contact: mailnews-account → account-manager
Target Milestone: seamonkey2.0 → Thunderbird 3.0rc1
(Assignee)

Comment 46

8 years ago
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.
Attachment #405896 - Flags: approval-thunderbird3?
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
Attachment #405896 - Flags: approval-thunderbird3? → approval-thunderbird3+
Flags: in-litmus?
(Assignee)

Comment 48

8 years ago
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).

---
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(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
Flags: in-litmus? → in-litmus+
(Assignee)

Comment 50

8 years ago
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.
(Reporter)

Comment 51

8 years ago
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.
Status: RESOLVED → VERIFIED

Updated

8 years ago
Keywords: fixed-seamonkey2.0.1
You need to log in before you can comment on or make changes to this bug.