Closed
Bug 1004242
Opened 11 years ago
Closed 11 years ago
Expose FxAccounts resendVerificationEmail to Gaia
Categories
(Firefox OS Graveyard :: FxA, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.0 S1 (9may)
People
(Reporter: spenrose, Assigned: spenrose)
References
Details
Attachments
(1 file, 2 obsolete files)
4.87 KB,
patch
|
Details | Diff | Splinter Review |
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 .
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
Comment on attachment 8418825 [details] [diff] [review]
1004242-expose-resend.patch
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+
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8418825 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 5•11 years ago
|
||
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?
https://tbpl.mozilla.org/?tree=Try&rev=f0617c6bb803
Flags: needinfo?(ryanvm)
Comment 6•11 years ago
|
||
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 https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices a look when you get a chance :)
Flags: needinfo?(ryanvm)
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 7•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 11 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.
Description
•