661795 - [autoconfig] POP3 Settings are used although IMAP is selected, for ISPs where POP3 is the default and IMAP is optional

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Nitrox]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110602 Firefox/7.0a1
Build Identifier: Thunderbird/5.0b1

Account creation wizard uses POP3 settings even when IMAP is selected for yahoo.com accounts.

Reproducible: Always

Steps to Reproduce:
To reproduce this issue, 
1.Install and Run thunderbird 5 beta 1 with fresh profile
2.In account creation wizard use a yahoo.com email account
3.After filling other details, click continue
4.Now Thunderbird will contact ISP Database and get the details
5.The radio box is selected (By default) as IMAP but the details are shown for POP3
If i continue to click create account, POP3 settings are used.
Note: If i click on IMAP radio box again, the settings are corrected.


Expected Results:  
The expected result is IMAP details should be shown and when i click create account IMAP details should be used.

Comment 1

[Ludovic Hirlimann [:Usul]]

We've disabled imap for yahoo as the yahoo infrastructure aren't ready for desktop client load on them. :-)

Comment 2

[Ben Bucksch (:BenB)]

> uses POP3 settings even when IMAP is selected

That is a bug.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to fix various TODOs.

So, it looked like the bug was because we were setting the value of the radiogroup before we populated the configIncomings of the elements, which led to this.  And then we had another bug where we would never remove elements from the alternatives, so I fixed that, too.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to fix various TODOs, in a better way. :)

Turns out we couldn't filter out the objects because some of them were filled out.  So this patch fixes that.  :)

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Attachment: Further digging revealed more problems… ;)

So, if we leave the big block of code in the displayConfigResult function, then it calls onResultIMAPOrPOP3 (when changing e("result_imappop").value), which then recursively calls displayConfigResult again.  I don't think that we really want set the selected radio button from the callback when the user has set the radio button, so I've moved this code out of displayConfigResult, and into _foundConfig2, but I'm not sure that's the right thing to do.

We should chat about the design tomorrow on IRC, and see if this is okay, or if there's a better way to handle it…

Thanks,
Blake.

Comment 6

[Ben Bucksch (:BenB)]

Comment on attachment 537934 [details] [diff] [review]
Further digging revealed more problems…  ;)

> I've moved this code out of displayConfigResult, and into _foundConfig2

Please leave it where it is, just break that silly cycle you described. (Thanks!)

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: Only call the handler on user action.

Here we go.  Hopefully the last patch.  :)

Comment 8

[Ben Bucksch (:BenB)]

Comment on attachment 538027 [details] [diff] [review]
Only call the handler on user action.

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

Thanks, Blake!

r=BenB with the 2 things fixed.

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +847,5 @@
> +          config.incoming;
> +      e("result_imappop").onselect = null
> +      e("result_imappop").value =
> +          config.incoming.type == "imap" ? 1 : 2;
> +      e("result_imappop").onselect = this.onResultIMAPOrPOP3;

Remove this .onselect=... and the .onselect=null above.

You said on IRC: "Oh, detritus from an earlier version."

@@ +858,5 @@
>  
> +  // This function must only be called by user action, not by setting the
> +  // value or selectedItem or selectedIndex of the radiogroup!  This is why
> +  // we use the oncommand attribute of the radio elements instead of the
> +  // onselect attribute of the radiogroup!

Please make this a JavaDoc comment, compare the other functions

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The final version of the patch.

Carrying forward Ben's r+…

I was hoping to check this in, but then the tree started burning, so I'm posting this so that it doesn't get lost in the shuffle…

Thanks,
Blake.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Committed as http://hg.mozilla.org/comm-central/rev/6d97c01e32a5

Comment 11

[Mark Banner (:standard8)]

Checked into comm-miramar: http://hg.mozilla.org/releases/comm-miramar/rev/73904271e5da

Comment 12

[Mark Banner (:standard8)]

Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/45ba9b39b15c