Closed Bug 981210 Opened 6 years ago Closed 6 years ago

FxAccountsManager.jsm and consumers: change "accountId" attribute to "email"

Categories

(Firefox :: Firefox Accounts, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: _6a68, Assigned: spenrose)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now, when I login using the fxa test client, the FxAccountsIACHelper.openFlow() callback gets:

{"email": "foo@bar.com", "success": true, "verified": true}

but if I then do FxAccountsIACHelper.getAccounts(), the response is:

{"accountId": "foo@bar.com", "verified": true}

We should use accountId everywhere for consistency.
Assignee: nobody → spenrose
Jed and I chatted about this and noticed that the UI refers to "email" and the value of the field must be a orutable email, so we're inclined to go with "email." Thoughts?
Flags: needinfo?(6a68)
Sounds great!
Flags: needinfo?(6a68)
This comes down to a single attribute name in services/fxaccounts/FxAccountsManager.jsm which gets copied all over the place. So it's a straightforward find-and-replace for both gecko and gaia, but I'm a little concerned about coordinating the paired patches. Jared, how would you feel about tweaking the consumers of FxAccountsIACHelper to look for email and fall back to accountId? Then I could update gecko safely. At the same time I'd like to move all this to nice-to-have. So a new gaia bug to that effect would block 974121 and this bug, and this one would block 974096. Once this bug lands, we could then remove the fallback.
Flags: needinfo?(6a68)
Per IRC with Jared, moving this to nice-to-have.
Blocks: 974096
No longer blocks: 955951
Flags: needinfo?(6a68)
Hey Sam - sorry for the lack of response here. I'll fix this inside the settings patch (we can skip opening another bug). Would you mind opening a bug to update the system app?
doh, forgot ni
Flags: needinfo?(spenrose)
Status: NEW → ASSIGNED
Flags: needinfo?(spenrose)
Summary: JSON response after login flow should use 'accountId', not 'email' → FxAccountsManager.jsm and consumers: change "accountId" attribute to "email"
Depends on: 984486
Attached patch 981210-accountId-to-email.patch (obsolete) — Splinter Review
This is a pure search-and-replace, except for the bit in FxAccountsMgmtService.jsm to catch accountId. Once this patch lands, we can take accountId out of Gaia, and then remove that kludge. Thanks Jed!
Attachment #8414771 - Flags: review?(jparsons)
Comment on attachment 8414771 [details] [diff] [review]
981210-accountId-to-email.patch

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

That looks good to me.  I searched services/ and b2g/ and don't find any other instances that need changing.

Thanks, Sam!
Attachment #8414771 - Flags: review?(jparsons) → review+
Attachment #8414771 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d05f273a2d1d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Product: Core → Firefox
Target Milestone: mozilla32 → Firefox 32
You need to log in before you can comment on or make changes to this bug.