560051 - [autoconfig] Make guessConfig() return a POP3 config, too

Status: VERIFIED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[Ben Bucksch (:BenB)]

Currently, our guess algo aborts as soon as it found a high-preference server. Whereby IMAP is more preferred than POP3. And only the most preferred server is returned. That means we won't return a POP3 config unless there's no corresponding IMAP config.
For bug 532590, we need both, though. So, return the POP3 config as alternative, and only abort when we have a server for each protocol.

(The corresponing bug for ISP configs DB is bug 556729.)

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix, v2

First cut

Comment 2

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

Comment on attachment 439734 [details] [diff] [review]
Fix, v2

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>@@ -147,60 +147,72 @@ function guessConfig(domain, progressCal
>-  var incomingSuccess = function(thisTry)
>+  var incomingSuccess = function(thisTry, alternativeTry)

I think we should just pass an array of HostTrys as the single argument.
At the very least, we should change the alternativeTry to an array.

Other than that, I'm generally happy with the code, but…
Per https://wiki.mozilla.org/Thunderbird/Landing_Patches_on_Thunderbird3.1 this is unlikely to land without any tests.  So, uh, r+ as long as you address the comment above and add tests in a separate patch before this lands.

Thanks,
Blake.

Comment 3

[Ben Bucksch (:BenB)]

I'm not sure I'll continue this bug.

Comment 4

[Ben Bucksch (:BenB)]

I.e. bwinton, if you want to adopt the bug and patch, feel free.

Comment 5

[rsx11m]

This introduces an inconsistency in ISPDB-listed vs. unlisted ("guessed") configurations, see bug 549045 comment #180; so, fixing this in the backend
would be valuable for UX purposes as well when servers provide a choice.

Comment 6

[Ben Bucksch (:BenB)]

The patch is there, working, since 8 months...

Comment 7

[rsx11m]

... yes, except that it won't go in without comment #2 addressed.  :-\

Comment 8

[Ben Bucksch (:BenB)]

Yeah, fine, but I'm totally burned out. Feel free to adopt it, if you want.

Comment 9

[rsx11m]

I figured that following bug 549045, so hoping that Blake may take it from here or at least has some suggestion who could come up with a test (I can't). This sure would be a nice incremental improvement to the main bug 549045 rewrite.

Comment 10

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

I'm kind of busy with beta-blockers for the next while.  Standard8, sid0, asuth, protz, and squib are the other people who've committed stuff in the mozmill directory, so perhaps one of them could pick this up?

Comment 11

[Ben Bucksch (:BenB)]

protz, could you help out with tests, here, maybe?
I think this would be important for users to get in.

Comment 12

[Ben Bucksch (:BenB)]

Attachment: Fix, v3, ported after bug 549045

Bug 549045 changes the surrounding code, causing the patch Fix v2 here to no longer apply. The actual code lines changed here are not affected, so this should work the same as before.

Comment 13

[Jonathan Protzenko [:protz]]

So if I understand correctly I need to:
- address the review comments from comment #2 and,
- write tests.

Is that correct?

Comment 14

[Ben Bucksch (:BenB)]

> I think we should just pass an array of HostTrys as the single argument.
> At the very least, we should change the alternativeTry to an array.

Then we should do the same for outgoing. I considered that during implementation, but thought that was unnecessary. bwinton, do you think that change is necessary?

protz, I was only referring to the tests.

Comment 15

[Ben Bucksch (:BenB)]

If bwinton says the change should be done, I'll just do it myself.

Comment 16

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

(In reply to comment #14)
> > I think we should just pass an array of HostTrys as the single argument.
> > At the very least, we should change the alternativeTry to an array.
> Then we should do the same for outgoing. I considered that during
> implementation, but thought that was unnecessary. bwinton, do you think that
> change is necessary?

Yeah, I think that it would end up with a cleaner API, so I think I'ld like to see the change made.  (But feel free to let protz make it, if you're still burnt out…)

Thanks,
Blake.

Comment 17

[Ben Bucksch (:BenB)]

ok, I'll change it.

Comment 18

[Ben Bucksch (:BenB)]

Attachment: Fix, v4, assumes bug 549045 - with review fix

Comment 19

[Ben Bucksch (:BenB)]

bwinton: ping

Comment 20

[Ben Bucksch (:BenB)]

<http://getsatisfaction.com/mozilla_messaging/topics/why_does_thunderbird_ignore_my_settings?utm_content=reply_link&utm_medium=email&utm_source=reply_notification#reply_5051552>

Comment 21

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

Comment on attachment 517534 [details] [diff] [review]
Fix, v4, assumes bug 549045 - with review fix

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>@@ -315,18 +336,21 @@ HostTry.prototype =
>  *    Called when the config is OK
>+ *    |result| is the most preferred server.
>+ *    |alts| currently exists only for |IncomingHostDetector| and contains a
>+ *    server of the other type (POP3 instead of IMAP), if available.

How about:
 *    |alts| currently exists only for |IncomingHostDetector| and contains SOME
 *    SERVERS of the other type (POP3 instead of IMAP), if available.
?  (Without the scary caps, of course. ;)


Aside from that, I like it.  r=me.

Of course, I would prefer if we waited to land it until the tests were done.  ;)

Comment 22

[Ben Bucksch (:BenB)]

bwinton, sure, I'll adapt the comment.

protz, when do you think can you do the tests?
bwinton, based on the answer, please let me know whether I should commit now or wait for protz. I *would* like to get this patch in 3.3b4, though.

Comment 23

[Jonathan Protzenko [:protz]]

Let's say I'll fix the tests by the end of the week. If you can wait, then I think it's best to ship everything together.

Comment 24

[Jonathan Protzenko [:protz]]

So after discussing the matter on IRC with :bwinton and :bienvenu, it turns out we can't test the guessed config with mozmill because we have no fake server setup yet. We can't use xpcshell to test that either [1], so our best bet is to have people manually test this. :bienvenu said we could use an ispdb server that's empty to test for the "no config at all case", and then use gmail for the "we have both configs available" case. The drawback being that we have no control of what gmail offers, and this might require us to change things later, but that looks like a good compromise for now on.

[1] needs to be verified though. If there's already an XPCShell running for guessConfig, I'll add a test to it making sure it takes into account this new patch

Comment 25

[Jonathan Protzenko [:protz]]

http://mxr.mozilla.org/comm-central/ident?i=guessConfig returns nothing in test directories, so I think we can go for litmus tests for that one :-)

Comment 26

[Ben Bucksch (:BenB)]

Commited as <http://hg.mozilla.org/comm-central/rev/14bd77c0cfea>
FIXED

Comment 27

[Ben Bucksch (:BenB)]

Litmus tests:
A) Test: IMAP/POP selection for ISPDB-delivered configs
Tests: bug 532590 and bug 556729
1. File | New | Mail Account...
2. Enter name "foo", email address "[e-mail]", password "foo"
3. Click [Continue], wait
Expected result:
"Configuration found in Mozilla ISP database"
(o) IMAP    ( ) POP3
IMAP, imap.googlemail.com, SSL
SMTP, smtp.googlemail.com, SSL
4. Click (o) POP3 radio button
Expected result:
"Configuration found in Mozilla ISP database"
( ) IMAP    (o) POP3
POP3, pop.googlemail.com, SSL
SMTP, smtp.googlemail.com, SSL

B) Test: IMAP/POP selection for guessed configs
Tests: bug 560051
1. File | New | Mail Account...
2. Enter name "foo", email address "[e-mail]", password "foo"
3. Click [Continue], wait
Expected result:
"Configuration found by trying common server names"
(o) IMAP    ( ) POP3
IMAP, mail.sdf.com, STARTTLS
SMTP, mail.sdf.com, STARTTLS
4. Click (o) POP3 radio button
Expected result:
"Configuration found by trying common server names"
( ) IMAP    (o) POP3
POP3, mail.sdf.com, SSL
SMTP, mail.sdf.com, STARTTLS

Alternative to B:
C) Test: IMAP/POP selection for guessed configs
Tests: bug 560051
1. in <about:config> (= [Config Editor]), set pref
   "mailnews.auto_config_url" to "http://localhost/"
2. File | New | Mail Account...
3. Enter name "foo", email address "[e-mail]", password "foo"
4. Click [Continue], wait
Expected result:
"Configuration found by trying common server names"
(o) IMAP    ( ) POP3
IMAP, imap.gmail.com, SSL
SMTP, smtp.gmail.com, STARTTLS
4. Click (o) POP3 radio button
Expected result:
"Configuration found by trying common server names"
( ) IMAP    (o) POP3
POP3, pop.gmail.com, SSL
SMTP, smtp.gmail.com, STARTTLS
5. Reset pref "mailnews.auto_config_url"
   (otherwise, test A above will not work)

Comment 28

[rsx11m]

Verified fixed, works fine with an unlisted but guessable server [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0pre) Gecko/20110330 Thunderbird/3.3a4pre].

Comment 29

[Ludovic Hirlimann [:Usul]]

https://litmus.mozilla.org/show_test.cgi?id=15382
https://litmus.mozilla.org/show_test.cgi?id=15383
https://litmus.mozilla.org/show_test.cgi?id=15384