1153543 - when adding a new identity, the smtp server menulist is collapsed with no default item selected

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

When adding a new identity, the smtp server menulist is collapsed with no default item selected. The code intends to select the default SMTP server, but it didn't pan out correctly.

The error can be seen only when the accounts default identity has SMTP server set to "use default server", which means the .smtpServerKey returns null.

Comment 1

[:aceman]

Attachment: patch

So the default menuitem in the menulist has a value of "" but the default server key is null. That is already solved for existing identities, but not in the code branch for a new identity.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8591224 [details] [diff] [review]
patch

Review of attachment 8591224 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/am-identity-edit.js
@@ +41,5 @@
>      document.getElementById('identity.attachVCard').checked = identity.attachVCard;
>      document.getElementById('identity.escapedVCard').value = identity.escapedVCard;
>  
>      document.getElementById('identity.smtpServerKey').value =
> +      identity.smtpServerKey || ""; // useDefaultItem.value is "" but key can be null.

the "but key can be null." addition is confusing IMHO

Comment 3

[:aceman]

Attachment: patch v1.1

OK.

Comment 4

[Ian Neal]

Comment on attachment 8591743 [details] [diff] [review]
patch v1.1

>+++ b/mailnews/base/prefs/content/am-identity-edit.js
>+    // useDefaultItem.value is "" but smtpServerKey returns null for that case
>     document.getElementById('identity.smtpServerKey').value =
>-      identity.smtpServerKey || ""; // useDefaultItem.value is ""
>+      identity.smtpServerKey || "";
>   }
>   else
>   {
>     // We're adding an identity, use the best default we have.
I would prefer something is added to this comment (similar to the one at the start of this patch) rather than the one below.
>     document.getElementById('identity.smtpServerKey').value =
>-      gAccount.defaultIdentity.smtpServerKey;
>+      gAccount.defaultIdentity.smtpServerKey || ""; // useDefaultItem.value is ""
r=me with that addressed.

Comment 5

[:aceman]

Attachment: patch v1.2

What about this?

Comment 6

[Ian Neal]

Comment on attachment 8592447 [details] [diff] [review]
patch v1.2

>+  function initSmtpServer(aServerKey) {
>+    // Select a server in the SMTP server menulist by its key.
>+    // But the value of the identity.smtpServerKey is null
>+    // when the "use default server" option is used. If we get that passed in,
>+    // select the useDefaultItem representing this option, with value of "".
The following seems to flow better:
// Select a server in the SMTP server menulist by its key.
// The value of the identity.smtpServerKey is null when the
// "use default server" option is used so, if we get that passed in, select
// the useDefaultItem representing this option by using the value of "".

r=me with that or something similar

Comment 7

[:aceman]

Attachment: patch v1.3

I am no native English speaker so I am fine with whatever you propose :) If it contains the wanted idea.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/comm-central/rev/3677ad2ad8c0

Comment 9

[Kent James (:rkent)]

Is TB 38 affected by this?

Comment 10

[:aceman]

Yes, it is. Using mxr diff on the file it seems the code missing '|| ""' was there forever, at least since TB 1.9.1. So it is not a recent regression. But it is nice polish, possibly unrisky.

Comment 11

[Kent James (:rkent)]

Comment on attachment 8592465 [details] [diff] [review]
patch v1.3

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 12

[Kent James (:rkent)]

Comment on attachment 8592465 [details] [diff] [review]
patch v1.3

http://hg.mozilla.org/releases/comm-aurora/rev/02317417c674

Comment 13

[Kent James (:rkent)]

Comment on attachment 8592465 [details] [diff] [review]
patch v1.3

https://hg.mozilla.org/releases/comm-beta/rev/73b80ca7e276