Closed Bug 1571772 Opened 2 months ago Closed 2 months ago

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

Categories

(Thunderbird :: Account Manager, enhancement)

enhancement
Not set

Tracking

(thunderbird_esr6069+ fixed, thunderbird_esr6869+ fixed, thunderbird69 fixed, thunderbird70 fixed)

VERIFIED FIXED
Thunderbird 70.0
Tracking Status
thunderbird_esr60 69+ fixed
thunderbird_esr68 69+ fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 12 obsolete files)

11.72 KB, patch
BenB
: review+
Details | Diff | Splinter Review
11.78 KB, patch
Details | Diff | Splinter Review
8.10 KB, patch
Details | Diff | Splinter Review

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
Attached patch Initial pass (obsolete) — Splinter Review

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

Assignee: nobody → neil
Attachment #9083381 - Flags: review?(ben.bucksch)
Attached patch v0.2 (obsolete) — Splinter Review
Attachment #9083381 - Attachment is obsolete: true
Attachment #9083381 - Flags: review?(ben.bucksch)
Attachment #9083682 - Flags: review?(ben.bucksch)
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?
Attachment #9083682 - Flags: review?(ben.bucksch)
Attachment #9083682 - Flags: review-
Attachment #9083682 - Flags: feedback+

(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.

Attached patch v0.3 (obsolete) — Splinter Review
Attachment #9083682 - Attachment is obsolete: true
Attachment #9083708 - Flags: feedback?(ben.bucksch)
Attached patch v0.4 (obsolete) — Splinter Review
Attachment #9083708 - Attachment is obsolete: true
Attachment #9083708 - Flags: feedback?(ben.bucksch)
Attachment #9083742 - Flags: feedback?(ben.bucksch)
Attached file Weird stack (obsolete) —

Is PriorityOrderAbortable notifying too much?

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.
Attachment #9083742 - Flags: feedback?(ben.bucksch) → review+
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.
Attachment #9083742 - Flags: review+ → review-
Attached patch v0.5 (obsolete) — Splinter Review

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

Attachment #9083742 - Attachment is obsolete: true
Attachment #9083966 - Flags: review?(ben.bucksch)
Comment on attachment 9083966 [details] [diff] [review]
v0.5

Thank you!

r+ BenB
Attachment #9083966 - Flags: review?(ben.bucksch) → review+
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 68.0
Version: unspecified → 60

Target Milestone: The version when it lands.

Target Milestone: Thunderbird 68.0 → Thunderbird 70.0

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

Keywords: checkin-needed
Depends on: 1572467

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
Attachment #9083746 - Attachment is obsolete: true
Attachment #9084045 - Flags: feedback?(ben.bucksch)
Attachment #9084045 - Flags: feedback?(ben.bucksch) → review+
Attached patch v0.6 (obsolete) — Splinter Review

(Previous two patches folded together)

Attachment #9083966 - Attachment is obsolete: true
Attachment #9084045 - Attachment is obsolete: true
Attachment #9084283 - Flags: review+
Summary: Support Exchange as an alternative account type with other autodetection methods → [autoconfig] Support Exchange as an alternative account type with other autodetection methods
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.

@Neil: Please add HG header

Attached patch Addressed review comments (obsolete) — Splinter Review
Attachment #9084283 - Attachment is obsolete: true
Attachment #9085021 - Flags: review+

Checkin needed?

(In reply to Jorg K from comment #19)

Checkin needed?

Just checking on whether we need 1572467 first.

Attached patch Tweaked commit message (obsolete) — Splinter Review

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

Attachment #9085021 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch Properly rebased to trunk (obsolete) — Splinter Review
Attachment #9085065 - Attachment is obsolete: true
Keywords: checkin-needed

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)

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".

Keywords: checkin-needed
  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.

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

Attached patch 1571772.diff (obsolete) — Splinter Review

This should do then.

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

Attached patch LintedSplinter Review

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'

Attachment #9085088 - Attachment is obsolete: true

(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.

Keywords: checkin-needed
Attachment #9085417 - Flags: review+
Comment on attachment 9085373 [details] [diff] [review]
1571772.diff

Thanks, Jörg
Attachment #9085373 - Attachment is obsolete: true

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f32c71c705f6
Support Exchange as an alternative account type for ISPDB. r=BenB

Status: NEW → RESOLVED
Closed: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Attachment #9085417 - Flags: approval-comm-beta?

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

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.

Can you supply a patch that applies to beta please.

Flags: needinfo?(neil)
Flags: needinfo?(ben.bucksch)

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?

Flags: needinfo?(ben.bucksch)

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 on attachment 9085417 [details] [diff] [review]
Linted

Please supply a patch that applies cleanly.
Attachment #9085417 - Flags: approval-comm-beta?
Attached patch 1571772.diffSplinter Review

(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.

Flags: needinfo?(neil)
Attachment #9086621 - Flags: approval-comm-beta?

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

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

Thanks.
Attachment #9086621 - Flags: approval-comm-beta? → approval-comm-beta+

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?

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.

Attachment #9086621 - Flags: approval-comm-esr68?
Attachment #9086621 - Flags: approval-comm-esr60?

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.

(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.

Attachment #9086678 - Flags: approval-comm-esr60?
Attachment #9086621 - Flags: approval-comm-esr60?

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.

(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.

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

Attachment #9086621 - Flags: approval-comm-esr68? → approval-comm-esr68+
Attachment #9086678 - Flags: approval-comm-esr60? → approval-comm-esr60+

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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.