Closed Bug 1004242 Opened 7 years ago Closed 7 years ago

Expose FxAccounts resendVerificationEmail to Gaia


(Firefox OS Graveyard :: FxA, defect)

Gonk (Firefox OS)
Not set


(Not tracked)

2.0 S1 (9may)


(Reporter: spenrose, Assigned: spenrose)




(1 file, 2 obsolete files)

FxAccountsClient.jsm has a resendVerificationEmail method, which is exposed to FxAccounts.jsm, but needs to be added to FxAccountsManager.jsm and b2g/components/FxAccountsMgtmService.jsm so that Gaia can reach it with a mozFxAccountsContentEvent .
Blocks: 1004251
Attached patch 1004242-expose-resend.patch (obsolete) — Splinter Review
I managed to trigger an appropriate mozFxAccountsContentEvent in Gaia and got the server to respond to a '/recovery_email/status' HTTP request, so this patch "works." I'm a little nervous about the reject() path in FxAccountsManager.jsm, and would love your thoughts on whether it is enough given the throw in its callee. Thanks!
Attachment #8416159 - Flags: feedback?(jparsons)
No longer blocks: 974999
Attached patch 1004242-expose-resend.patch (obsolete) — Splinter Review
Per offline chat, I added a unit test that verifies error handling. Thanks for your consideration!
Attachment #8416159 - Attachment is obsolete: true
Attachment #8416159 - Flags: feedback?(jparsons)
Attachment #8418825 - Flags: review?(jparsons)
Comment on attachment 8418825 [details] [diff] [review]

Review of attachment 8418825 [details] [diff] [review]:

Looks good, Sam!  Thanks for adding the test.

::: services/fxaccounts/FxAccountsManager.jsm
@@ +254,5 @@
> +    return this._fxAccounts.resendVerificationEmail().then(
> +      (result) => {
> +        return result;
> +      },
> +      (error) => {

I realize more and more how many different formatting styles we have in these files for promises.  It's not a problem with this patch, but something to add to the List Of Things To Fix When We Have All The Time In The World.

::: services/fxaccounts/tests/xpcshell/test_manager.js
@@ +608,5 @@
> +    (success) => {
> +      do_throw("Unexpected success");
> +    },
> +    (error) => {
> +      do_check_eq(error.error, ERROR_SERVER_ERROR);

perfect. thank you.
Attachment #8418825 - Flags: review?(jparsons) → review+
Attachment #8418825 - Attachment is obsolete: true
Keywords: checkin-needed
Ryan --

Per your request I ran a try build. The result was a cascade of known issues, mostly timeouts, many of them two years old. I don't see any real problems, but of course I may be missing one. What's the best way forward?
Flags: needinfo?(ryanvm)
That run is looking fine so far. We're just getting more picky about requiring Try runs before landing checkin-needed patches.

BTW, please give a look when you get a chance :)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
You need to log in before you can comment on or make changes to this bug.