1571772 - [autoconfig] Support Exchange as an alternative account type with other autodetection methods

Status: VERIFIED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[[e-mail]]

Currently the account creation wizard only supports Exchange accounts through the Exchange autodiscover routine.

• The code for reading config XML doesn't support Exchange
• The code for supporting alternatives assumes exactly one alternative
• The code for fetching addons assumes that Exchange is the default config
• The code for installing addons assumes that Exchange is the only config

Comment 1

[[e-mail]]

Attachment: Initial pass

The code for fetching addons when a found config includes Exchange is probably all wrong, but the rest probably isn't that bad.

Comment 2

[[e-mail]]

Attachment: v0.2

Comment 3

[Ben Bucksch (:BenB)]

Comment on attachment 9083682 [details] [diff] [review]
v0.2

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

Looks good overall.
2 small comments.

Please list which cases you tested. Think about whether you forgot some.

::: mail/components/accountcreation/content/emailWizard.js
@@ +718,5 @@
> +    }, e => {
> +      this._abortable = null;
> +      config.addons = [];
> +      this.displayConfigResult(config);
> +    });

set `config.addons` before. Then you can do the same for both success and error.
In the error case, please log the error.

@@ +879,5 @@
> +          alt.type != config.incoming.type &&
> +          !alternatives.some(duplicate => duplicate.type == alt.type)) {
> +        alternatives.push(alt);
> +      }
> +    }

Isn't there an useful array function to remove duplicates?

Comment 4

[[e-mail]]

(In reply to Ben Bucksch from comment #3)

Please list which cases you tested. Think about whether you forgot some.

Tested:

• Autoconfig finds IMAP via ISP XML is unaffected
• Autoconfig finds Exchange via autodiscover is unaffected
• Autoconfig finds both IMAP and Exchange via XML
  • Tested choosing between IMAP and Exchange
  • Tested changing choice because account verification failed
  • Tested creating account of the other type after first choice failed

set config.addons before. Then you can do the same for both success and error.
In the error case, please log the error.

(... in which case it's no longer the same...)

Isn't there an useful array function to remove duplicates?

They're not duplicates in the normal sense of the word, they're distinct objects that duplicate a certain property value. Otherwise you could stick them into a Set.

Comment 5

[[e-mail]]

Attachment: v0.3

Comment 6

[[e-mail]]

Attachment: v0.4

Comment 7

[[e-mail]]

Attachment: Weird stack

Is PriorityOrderAbortable notifying too much?

Comment 8

[Ben Bucksch (:BenB)]

Comment on attachment 9083742 [details] [diff] [review]
v0.4

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

2 small (but significant) problems. Once fixed, r=BenB

::: mail/components/accountcreation/content/emailWizard.js
@@ +877,4 @@
>      var configFilledIn = this.getConcreteConfig();
>  
>      // IMAP / POP3 server type radio buttons
> +    let alternatives = config.incomingAlternatives.filter(alt => alt.type ==  "iamp" || alt.type == "pop3" || alt.type == "exchange");

"iamp"? Please copy: `alt.type == "imap" || alt.type == "pop3" || alt.type == "exchange"`

::: mail/components/accountcreation/content/util.js
@@ +673,5 @@
> +      found.push(nextEl);
> +    }
> +    return found;
> +  }, []);
> +};

Please move this to a logical place in the file, close to other array and similar functions, not at the end.

Comment 9

[Ben Bucksch (:BenB)]

Comment on attachment 9083742 [details] [diff] [review]
v0.4

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

r- due to bad error handling.

::: mail/components/accountcreation/content/emailWizard.js
@@ +720,5 @@
> +    this._abortable = getAddonsList(config, successCallback, ex => {
> +      // This call often times out, but that doesn't provide a message.
> +      // For now, just show the warning icon and continue.
> +      gEmailWizardLogger.error("Error getting addons list.  stack=\n" + ex.stack);
> +      e("status_area").setAttribute("status", "error");

eh, no. You're just setting the error mode, but never actually show the error message to the user. That's wrong.

(Just because you don't see an error string in the error case that you ran into, that doesn't mean that `ex` will never have a message. In fact, in most cases, it will have one.)

Please use the existing function `this.showErrorMsg(ex);`, as used elsewhere in the code, and as I had specifically said. This is a 1-liner, really.

That means you can remove the logging (which is also lacking the error message), because showErrorMsg() will already do that. And drop the comment. That's a server bug and a FetchHTTP bug to fix.

Comment 10

[[e-mail]]

Attachment: v0.5

The least dissimilar function I could find was one that at least returned an array, so I put it there.

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 9083966 [details] [diff] [review]
v0.5

Thank you!

r+ BenB

Comment 12

[Jorg K (CEST = GMT+2)]

Target Milestone: The version when it lands.

Comment 13

[[e-mail]]

(Need to figure out an issue where we're making multiple requests for the addons for some reason.)

Comment 14

[[e-mail]]

Attachment: Turn spinner on while getting addons list

Is this what you were looking for?

• The existing spinner functions all want to change the displayed string at the same time
• You can't remove the spinner after showing an error, because the attribute conflicts

Comment 15

[[e-mail]]

Attachment: v0.6

(Previous two patches folded together)

Comment 16

[Ben Bucksch (:BenB)]

Comment on attachment 9084283 [details] [diff] [review]
v0.6

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

r+ with this little change.

::: mail/components/accountcreation/content/emailWizard.js
@@ +706,5 @@
> +      this._abortable = null;
> +      e("status_area").setAttribute("status", "result");
> +      this.displayConfigResult(config);
> +    };
> +    e("status_area").setAttribute("status", "loading");

NIT: To maintain order of execution = order of code as best as possible, please move this line up, before `config.addons = [];`. I expect the set loading to be before set result, also in code order.

Comment 17

[Ben Bucksch (:BenB)]

@Neil: Please add HG header

Comment 18

[[e-mail]]

Attachment: Addressed review comments

Comment 19

[Jorg K (CEST = GMT+2)]

Checkin needed?

Comment 20

[[e-mail]]

(In reply to Jorg K from comment #19)

Checkin needed?

Just checking on whether we need 1572467 first.

Comment 21

[[e-mail]]

Attachment: Tweaked commit message

... to avoid confusion with the existing support as an alternative discovery option.

Comment 22

[[e-mail]]

Attachment: Properly rebased to trunk

Comment 23

[Jorg K (CEST = GMT+2)]

You guys are giving me a head-ache (every time) :-(
hg qimport bz:1571772 spits the dummy on that patch. You have a line that's 135 characters long and three linting issues:

$ ../mach eslint mail/components/
c:/mozilla-source/comm-central/comm/mail/components/accountcreation/content/emailWizard.js
854:79 error Multiple spaces found before '"imap"'. no-multi-spaces (eslint)

c:/mozilla-source/comm-central/comm/mail/components/accountcreation/content/exchangeAutoDiscover.js
313:5 error Function 'getAddonsList' expected no return value. consistent-return (eslint)
335:3 error Function 'getAddonsList' expected no return value. consistent-return (eslint)

? 3 problems (3 errors, 0 warnings)

Comment 24

[Jorg K (CEST = GMT+2)]

It's so bad, I can't fix the inconsistent return values in getAddonsList(). While you're at it, do fix the long line with the double spaces before "imap".

Comment 25

[Ben Bucksch (:BenB)]

  if (!incoming) {
    successCallback();
    return new Abortable();
  }

See a few lines down in the file (not visible in the patch), after calling errorCallback().

@Neil: Please provide diffs with 8 lines of context. Also, please run eslint before attaching patches.

Comment 26

[Ben Bucksch (:BenB)]

@Neil: please check whether we're tripping the caller by calling successCallback/errorCallback before returning, instead of being async. Compare bug 1573564.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1571772.diff

This should do then.

Comment 28

[Jorg K (CEST = GMT+2)]

Oh, I see comment #26: More checking needed for the patch.

Comment 29

[[e-mail]]

Attachment: Linted

If you don't mind I'd prefer this version as it avoids unnecessarily changing line 841 of emailWizard.js.

./mach eslint errors out when I try it... I figured out I needed to limit it so as not to try to lint common/src/viewSource.xul.

An error occurred running eslint. Please check the following error messages:

Error: ENOENT: no such file or directory, open '/home/neil/mozilla-central/comm/common/src/ubst @TOOLKIT_DIR@/content/editMenuKeys.inc.xul'

Comment 30

[[e-mail]]

(In reply to Ben Bucksch from comment #26)

@Neil: please check whether we're tripping the caller by calling successCallback/errorCallback before returning, instead of being async. Compare bug 1573564.

In this case, the call is being moved to a new call site that used to be synchronous, so it doesn't mind, although I don't think the old site minds either.

Comment 31

[Ben Bucksch (:BenB)]

Comment on attachment 9085373 [details] [diff] [review]
1571772.diff

Thanks, Jörg

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f32c71c705f6
Support Exchange as an alternative account type for ISPDB. r=BenB

Comment 33

[[e-mail]]

Comment on attachment 9085417 [details] [diff] [review]
Linted

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

Note that bug 1551133 renamed `status_area` to `status-area` on trunk.

Comment 34

[Jorg K (CEST = GMT+2)]

So that means that it needs a special beta patch, right? Can you supply it?

Comment 35

[Jorg K (CEST = GMT+2)]

Hmm, I tried editing the patch and it still doesn't apply in mail/components/accountcreation/content/exchangeAutoDiscover.js. Maybe best left to the authors to make it fit.

BTW, we shipped TB 69 beta 3 today, so there won't be another beta for a few days.

Comment 36

[Jorg K (CEST = GMT+2)]

Can you supply a patch that applies to beta please.

Comment 37

[Ben Bucksch (:BenB)]

I've pinged "NeilAway" to awake him. :-P

@Jörg: We've requested ESR 68, not beta. Just to be sure we understand correctly: You need a beta patch first? For TB 69, not TB 70?

Comment 38

[Jorg K (CEST = GMT+2)]

Umm, you requested beta: neil: approval-comm-beta?

As you know: Unless it's a) totally trivial, b) super-urgent (like a security fix) or c) and ugly regression where fixing it outweighs the risk, we run all uplifts on beta first before putting them into ESR.

Your patch here isn't one of a) b) or c) (same applies to bug 1572418 and bug 1572467) so according to current planning, they will take a spin on TB 69 beta 4 first before going onto TB 68.1 in September.

Of course it would be nice of the patches applied to beta and ESR and if they don't, please supply some that do.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 9085417 [details] [diff] [review]
Linted

Please supply a patch that applies cleanly.

Comment 40

[[e-mail]]

Attachment: 1571772.diff

(In reply to Jorg K (GMT+2) from comment #35)

Hmm, I tried editing the patch and it still doesn't apply in mail/components/accountcreation/content/exchangeAutoDiscover.js.

It turns out that the id change I mentioned also applied to some context. With that, the patch actually applies cleanly, although this is a rebased patch anyway because I was expecting a merge conflict.

Comment 41

[[e-mail]]

(Ironically hg qpush would have worked if the patch had only had 3 lines of context...)

Comment 42

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086621 [details] [diff] [review]
1571772.diff

Thanks.

Comment 43

[Jorg K (CEST = GMT+2)]

TB 69 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/ea35b787e55b464c5617f4f412a57dd78d801660

You'll request ESR approval here and in the other two bugs, right?

Comment 44

[Ben Bucksch (:BenB)]

Hi Jörg, I guess given that TB 68 isn't released yet, I considered TB 68 to be the "beta". But:

• trunk = 70
• beta = 69
• RC = 68
• release = 60
  Is that right?

We definitely need all these patches in TB 68.0, and would like to backport them to 60, too. Yes, we'll request ESR approval.

Comment 45

[Jorg K (CEST = GMT+2)]

I think the TB 60 ship has sailed. Do these patches even vaguely apply? I haven't tried. I'm not so keen pushing something like this into the very last TB 60.x which is TB 60.9.

Comment 46

[[e-mail]]

(In reply to Jorg K (GMT+2) from comment #45)

I think the TB 60 ship has sailed. Do these patches even vaguely apply? I haven't tried. I'm not so keen pushing something like this into the very last TB 60.x which is TB 60.9.

It applies for a large enough value of vaguely i.e. a diff -U 3 patch applies with fuzz.

Comment 47

[[e-mail]]

Attachment: Applies cleanly to TB60

Comment 48

[Jorg K (CEST = GMT+2)]

I'm willing to uplift this to TB 60.9 if you promise to test the hell out of TB 69 beta 4:
http://ftp.mozilla.org/pub/thunderbird/candidates/69.0b4-candidates/build1/
I don't want any trouble in account creation in the last TB 60.x in the cycle.

Comment 49

[[e-mail]]

(In reply to Jorg K from comment #48)

I'm willing to uplift this to TB 60.9 if you promise to test the hell out of TB 69 beta 4:
http://ftp.mozilla.org/pub/thunderbird/candidates/69.0b4-candidates/build1/
I don't want any trouble in account creation in the last TB 60.x in the cycle.

Couldn't find anything wrong in the Thunderbird end, although Owl doesn't like the fact that Microsoft has two OWA servers.

Comment 50

[[e-mail]]

(The distinction doesn't matter when using Exchange autodiscover, but other methods can't readily distinguish between the two.)

Comment 51

[Jorg K (CEST = GMT+2)]

TB 68.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/2f10eda9d3a70a2d15a19ce80a066c7e7a0e3459

Comment 52

[Jorg K (CEST = GMT+2)]

TB 60.9 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/4e2ee80b276f82780e2650fe7b08b06491344182

Comment 53

[Ben Bucksch (:BenB)]

Neil verified with a local config file:

• TB 68.1 candidate build 1 (with this change): works as expected and offers Exchange, IMAP and POP3 configs.
• TB 60.8 (without this change): ignores the new option and shows IMAP and POP3 configs as before.
• TB 60.9 (with this change): still building