[wip] Avatars #37
[wip] Avatars #37
Conversation
|
Cool! Excited to see this. |
| doc: 'Patterns to match a URL to ensure we only accept certain URLs.', | ||
| default: { | ||
| 'gravatar': '^http://www\\.gravatar\\.com/avatar/[0-9a-f]{32}$', | ||
| 'fxa': '^http://localhost:1112/a/[0-9a-f]{32}$' |
There was a problem hiding this comment.
TODO: figure out final pattern for prod
|
Can reliers request profile images of different sizes, e.g., |
|
@ckarlof I had not thought about variable sizes. Would we store a pre-sized version of certain sizes, or have the proxy from S3 resize on the fly? |
| providers: { | ||
| doc: 'Patterns to match a URL to ensure we only accept certain URLs.', | ||
| default: { | ||
| 'gravatar': '^http://www\\.gravatar\\.com/avatar/[0-9a-f]{32}$', |
There was a problem hiding this comment.
Just requires adding a s? in there. I didn't realize gravatar served over https.
There was a problem hiding this comment.
There was a problem hiding this comment.
Actually, I just found this today:
https://en.gravatar.com/site/implement/images/#secure-images
Secure Requests
If you're displaying Gravatars on a page that is being served over SSL (e.g. the page URL starts with HTTPS), then you'll want to serve your Gravatars via SSL as well, otherwise you'll get annoying security warnings in most browsers. To do this, simply change the URL for your Gravatars so that is starts with:
https://secure.gravatar.com/...
Everything else is the same as above (all the same options work), just make sure that the URL starts out like this.
There was a problem hiding this comment.
grump
fine, i'll change it to http(://www|s://secure)
|
I've updated the checklist of things left to do. @zaach It should work locally with a |
|
@rfk to review when ready. |
| if (config.env !== 'dev') { | ||
| logger.warn('static bin should only be for local dev'); | ||
| } | ||
|
|
There was a problem hiding this comment.
maybe worth setting process.title in this script, simply for consistency with the other two scripts in this folder?
| var id = img.id(); | ||
| db.addAvatar(id, uid, payload.url, provider, payload.selected) | ||
| .done(function() { | ||
| reply(EMPTY).code(201); |
There was a problem hiding this comment.
I like the use of 201 here; it should probably be called out explicitly in the docs.
|
👍 A few nits and thoughts there, but overall this is looking really good. The choice to use a local worker rather than all that SQS infrastructure seems to have paid off nicely in terms of simplicity for the initial version. Aside from the missing |
My concern being that this will be ignored in dev, but break stuff in prod where it actually checks the heartbeat. Perhaps we should add an explicit test for the heartbeat endpoint. |
| Key: key | ||
| }, function(err, data) { | ||
| if (err) { | ||
| reject(err); |
There was a problem hiding this comment.
does this need to be return reject(err); so it doesn;t fall through?
|
Also still needs |
|
Yea, I forgot to include a test of |
Still a work in progress.
Status:
GET /v1/avatarGET /v1/avatarsPOST /v1/avatarPOST /v1/avatar/uploadfixes #29