fix(avatars): add profile server client to proxy remote images #1496
Conversation
| return msg; | ||
| }; | ||
|
|
||
| var ERROR_TO_CODE = { |
There was a problem hiding this comment.
Should these be moved to their own module?
There was a problem hiding this comment.
I kind of want to refactor things and put all the errors into an error.js. The variety of error modules is starting to feel cluttered and potentially confusing. Follow up PR?
|
@shane-tomlinson updated! |
| }); | ||
|
|
||
| afterEach(function () { | ||
| Assertion.generate = Assertion; |
There was a problem hiding this comment.
Is it syntactic sugar to allow for Assertion() and Assertion.generate() to do the same thing?
There was a problem hiding this comment.
Yeah, I don't think we're actually using Assertion() anywhere.
There was a problem hiding this comment.
To be clear, the Assertion module aliases Assertion() to Assertion.generate() and the line above is resetting Assertion.generate after it was overwritten in beforeEach.
There was a problem hiding this comment.
Okay cool that makes sense. Thanks @zaach. Worth adding a comment to explain that?
|
@vladikoff or @ckarlof, can you review? |
| function ($, _, p, Session, ConfigLoader, OAuthClient, Assertion, AuthErrors) { | ||
|
|
||
| function ProfileClient(options) { | ||
| if (!(this instanceof ProfileClient)) { |
There was a problem hiding this comment.
What situation does this cover?
There was a problem hiding this comment.
This is an idiom for allowing var client = ProfileClient() as well as var client = new ProfileClient(). I coppied it from the OAuth client.
There was a problem hiding this comment.
Okay cool. I figured it was something like that. My opinion is that it encourages inconsistent use without a lot of actual benefit, but that's just my opinion 😄.
There was a problem hiding this comment.
Yeah, I tend to agree.
| @@ -14,7 +14,7 @@ define([ | |||
| var NAMESPACE = '__fxa_session'; | |||
|
|
|||
| // and should not be saved to sessionStorage | |||
| var DO_NOT_PERSIST = ['client_id', 'prefillPassword', 'prefillYear', 'error', 'service']; | |||
| var DO_NOT_PERSIST = ['client_id', 'prefillPassword', 'prefillYear', 'error', 'service', 'cropImgWidth', 'cropImgHeight', 'cropImgSrc']; | |||
There was a problem hiding this comment.
Let's avoid clogging sessionStorage with image data.
|
Issue have been fixed or filed. I think we're ready to merge @ckarlof or @vladikoff! |
fix(avatars): add profile server client to proxy remote images
Fixes #1458.
Fixes #1447.
Fixes #1469. It fixes the client portion, at least. The server depends on mozilla/fxa-profile-server#31.
Not sure if we want to merge until the server side is ready.