Closed Bug 1058424 Opened 10 years ago Closed 10 years ago

Create a module for fetching FxA profile information

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: MattN, Assigned: vladikoff)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Attached patch WIP module and empty test file (obsolete) — Splinter Review
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+
Flags: qe-verify-
Yes, we can help out, but we'll need review.
Assignee: MattN+bmo → vlad
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+
(In reply to Chris Karlof [:ckarlof] from comment #2)
> We can take it from here.

Thanks! :)
Iteration: 34.3 → ---
Points: 5 → ---
: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
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 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+
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+
> 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.
Attachment #8478849 - Attachment is obsolete: true
Attachment #8480429 - Attachment is obsolete: true
Attachment #8480429 - Flags: review?(ckarlof)
Attachment #8480918 - Flags: review?(ckarlof)
(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.
Attachment #8480918 - Flags: review?(ckarlof) → review+
https://hg.mozilla.org/mozilla-central/rev/5f66dd3d63f2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Iteration: --- → 34.3
Depends on: 1060465
https://hg.mozilla.org/mozilla-central/rev/8d48127a01e3
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Product: Core → Firefox
Target Milestone: mozilla34 → Firefox 34
You need to log in before you can comment on or make changes to this bug.