Closed
Bug 1058424
Opened 10 years ago
Closed 10 years ago
Create a module for fetching FxA profile information
Categories
(Firefox :: Firefox Accounts, defect)
Firefox
Firefox Accounts
Tracking
()
People
(Reporter: MattN, Assigned: vladikoff)
References
()
Details
Attachments
(1 file, 3 obsolete files)
Since FxA profile information could be useful outside of Loop, I think it makes sense to make a new module for it. Information about errors for the API: https://github.com/mozilla/fxa-profile-server/blob/master/docs/API.md#errors ckarlof, any chance someone from your team could help since this isn't Loop-specific? :)
Attachment #8478849 -
Flags: feedback?(ckarlof)
Flags: firefox-backlog+
Reporter | ||
Updated•10 years ago
|
Flags: qe-verify-
Comment 1•10 years ago
|
||
Yes, we can help out, but we'll need review.
Updated•10 years ago
|
Assignee: MattN+bmo → vlad
Comment 2•10 years ago
|
||
Comment on attachment 8478849 [details] [diff] [review] WIP module and empty test file Review of attachment 8478849 [details] [diff] [review]: ----------------------------------------------------------------- We can take it from here.
Attachment #8478849 -
Flags: feedback?(ckarlof) → feedback+
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Chris Karlof [:ckarlof] from comment #2) > We can take it from here. Thanks! :)
Updated•10 years ago
|
Iteration: 34.3 → ---
Points: 5 → ---
Comment 4•10 years ago
|
||
:vlad, we should probably use this to make the requests to the profile server: http://mxr.mozilla.org/mozilla-central/source/services/common/rest.js
Assignee | ||
Comment 5•10 years ago
|
||
Based on https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/lib/profile-client.js and FxAccountsOAuthClient.jsm Can be tested manually using: ``` Cu.import("resource:///modules/loop/MozLoopService.jsm"); MozLoopService.logInToFxA().then(tokenData => console.log(tokenData)) Cu.import("resource://gre/modules/FxAccountsProfileClient.jsm"); let client = new FxAccountsProfileClient({token:"TOKEN_FROM_TOKEN_DATA", profileUrl: "https://latest.dev.lcip.org/profile/v1"}); client.getProfile().then(profile => console.log(profile)) ```
Attachment #8480429 -
Flags: review?(ckarlof)
Attachment #8480429 -
Flags: review?(MattN+bmo)
Comment 6•10 years ago
|
||
Comment on attachment 8480429 [details] [diff] [review] 0001-Bug-1058424-Adds-a-module-for-fetching-FxA-profile-i.patch Review of attachment 8480429 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccountsProfileClient.jsm @@ +26,5 @@ > +const ERROR_UNEXPECTED = "Unexpected response"; > +const ERRNO_UNEXPECTED = 997; > + > +const ERROR_METHOD_NOT_ALLOWED = "Method Not Allowed"; > +const CODE_METHOD_NOT_ALLOWED = 405; We should move these to FxAccountsCommon.js, which already contains some of these constants. That file also exports the constants, which will allow you to avoid using magic numbers in your tests for the error numbers and messages. @@ +69,5 @@ > + > + /** > + * Interface for making remote requests. > + */ > + _requestInterface: RESTRequest, Strictly speaking, this isn't the interface, but the actual "request" implementation. I'm not sure what Desktop conventions are, but this would be clearer to me if this property was _Request, which makes it more clear that it's a constructor. @@ +80,5 @@ > + * @param {String} [type] > + * Type of request, i.e "GET". > + * @private > + */ > + _request: function(path, type = "GET") { _createRequest might be a better name here. Please add some documentation/information on the promise that this method returns. ::: services/fxaccounts/tests/xpcshell/test_profile_client.js @@ +84,5 @@ > + null, > + function (e) { > + do_check_eq(e.name, "ProfileClientError"); > + do_check_eq(e.code, 200); > + do_check_eq(e.errno, 997); By moving these constants to FxAccountsCommon.js, we can avoid using magic numbers here.
Attachment #8480429 -
Flags: feedback+
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8480429 [details] [diff] [review] 0001-Bug-1058424-Adds-a-module-for-fetching-FxA-profile-i.patch Review of attachment 8480429 [details] [diff] [review]: ----------------------------------------------------------------- I agree with ckarlof's feedback too, especially the magic number ones. Awesome, looks good to me with the proposed fixes! ::: services/fxaccounts/FxAccountsProfileClient.jsm @@ +58,5 @@ > + /** > + * {nsIURI} > + * The server to fetch profile information from. > + */ > + profileUrl: null, something like 'serverURL' is more clear because this makes it sounds like it's the URL to fetch a specific profile from but it's actually just the base of the URL. Having profile in the name is also somewhat redundant since this is on a FxAccountsProfileClient object. Also, URL is normally all-caps as a suffix at least for Fx. @@ +77,5 @@ > + * > + * @param {String} path > + * Profile server path, i.e "/profile". > + * @param {String} [type] > + * Type of request, i.e "GET". Nit: I believe "method" is more accurate than "type" @@ +131,5 @@ > + })); > + } > + }); > + }, > + /** Nit: add a new line @@ +138,5 @@ > + * @return Promise > + * Resolves: {Object} Successful response from the Profile server. > + * Rejects: {Object} ProfileClientError. > + */ > + getProfile: function () { The "get" prefix like this seems foreign to me. How about fetchProfile? Unless "get" in this case is referring to the HTTP method. ::: services/fxaccounts/tests/xpcshell/test_profile_client.js @@ +36,5 @@ > + */ > +let mockResponseError = function (error) { > + return function () { > + return { > + setHeader: function () {}, setHeader on the response? I'm not sure if this is needed. @@ +47,5 @@ > + > +/** > + * Successful response > + */ > +add_test(function () { If you name the functions then they get output in the test logs and could replace the comment if you'd like. The function names make it easier to see which test failed.
Attachment #8480429 -
Flags: review?(MattN+bmo) → review+
Comment 8•10 years ago
|
||
> The "get" prefix like this seems foreign to me. How about fetchProfile? Unless "get" in this case is referring to the HTTP method.
I think "get" is fine. Some clients use "get", some use "fetch" for this operation. You could probably drop the "Profile" from "getProfile" though. E.g., "profileClient.getProfile()" vs "profileClient.get()" or s/get/fetch/ if you prefer.
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8478849 -
Attachment is obsolete: true
Attachment #8480429 -
Attachment is obsolete: true
Attachment #8480429 -
Flags: review?(ckarlof)
Attachment #8480918 -
Flags: review?(ckarlof)
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #7) > Comment on attachment 8480429 [details] [diff] [review] > 0001-Bug-1058424-Adds-a-module-for-fetching-FxA-profile-i.patch > The "get" prefix like this seems foreign to me. How about fetchProfile? > Unless "get" in this case is referring to the HTTP method. > > ::: services/fxaccounts/tests/xpcshell/test_profile_client.js > @@ +36,5 @@ > > + */ > > +let mockResponseError = function (error) { > > + return function () { > > + return { > > + setHeader: function () {}, > > setHeader on the response? I'm not sure if this is needed. Updated attachment, "setHeader" is there for the request can assign the header and proceed. Otherwise it is not defined. Matt, I marked the commit as an r+ from you, but feel free to give feedback if you have time. ckarlof is r? as of right now.
Assignee | ||
Comment 11•10 years ago
|
||
Added documentation: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/FxAccountsProfileClient.jsm
Updated•10 years ago
|
Attachment #8480918 -
Flags: review?(ckarlof) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 12•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=15ddf2f9cc5f
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f66dd3d63f2
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5f66dd3d63f2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Iteration: --- → 34.3
Assignee | ||
Comment 15•10 years ago
|
||
Updated to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1060465
Attachment #8480918 -
Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/8d48127a01e3
Whiteboard: [fixed-in-fx-team]
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d48127a01e3
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Updated•7 years ago
|
Product: Core → Firefox
Updated•7 years ago
|
Target Milestone: mozilla34 → Firefox 34
You need to log in
before you can comment on or make changes to this bug.
Description
•