fix(avatars): integrate the profile images backend #1584

zaach · 2014-08-21T00:03:54Z

We shouldn't merge this before the dev backend is ready: mozilla/fxa-dev#24.

Fixes #1556.

zaach · 2014-08-21T23:20:31Z

Functional tests are now working locally, but we need to wait for the fxa-dev server to be updated before we merge.

zaach · 2014-08-22T19:38:37Z

This is ready for review. @ckarlof or @nchapman

pdehaan · 2014-08-22T20:27:04Z

app/scripts/views/settings/avatar_gravatar.js


  function t (s) { return s; }
-
-  var GRAVATAR_URL = 'https://www.gravatar.com/avatar/';
+  var EXPORT_LENGTH = 600;


What's dis? 600px? 600chars?

ckarlof · 2014-08-26T01:28:19Z

Looks good, @zaach, but I would consider refactoring some of tests (particularly the view tests) to use mocked deps.

ckarlof · 2014-08-26T01:58:15Z

app/scripts/lib/profile.js

+      client_id: config.oauthClientId,
+      scope: 'profile:write'
+    };
+    var oauthClient = new OAuthClient({


For testing this, it would be cool to mock out the OAuthClient, ProfileClient, and Assertion modules and check that the expected methods on those objects are called with the expected parameters (e.g., the expected audience to Assertion.generate). In general, this is how we'd like to use mocks to test our objects, and I think this would be a good trial of using that approach (i.e., mocking out the direct object dependencies and not necessarily the actual servers).

zaach · 2014-09-10T23:50:12Z

This is ready for another round of reviews.

pdehaan · 2014-09-10T23:59:02Z

app/scripts/templates/settings/avatar_change.mustache

@@ -25,7 +25,6 @@
    <nav id="avatar-options">
      <input type="file" id="imageLoader" name="imageLoader" accept="image/png, image/jpeg"/>


No GIFs? NNNOOOOOOOO!!!

ckarlof · 2014-09-12T22:47:22Z

app/tests/spec/lib/profile.js

+              //assert.equal(result, 'data:image/jpeg;base64,ohhai');
+            //});
+        //});
+      //});


See comment for https://github.com/mozilla/fxa-content-server/pull/1584/files#r17506479

ckarlof · 2014-09-12T22:50:47Z

LGTM except for those commented out tests.

ckarlof · 2014-09-13T00:15:16Z

@shane-tomlinson I'll let you merge after you feel confident about whatever issue you were having running it.

shane-tomlinson · 2014-09-15T10:23:02Z

@zaach - verifier is now patched, thanks for that. Now I'm seeing other oddness:

I open http://127.0.0.1:3030/settings/avatar/camera, click on "Take picture", and in the profile server console, I see the following error:

[p18275] fxa.CRITICAL: Error: write EPIPE
    at errnoException (net.js:901:11)
    at Object.afterWrite (net.js:718:19)
[p18247] fxa.compute.CRITICAL: Fatal IPC error with image-cc.js compute process (18275) dies with code: 1
[nodemon] app crashed - waiting for file changes before starting...
[p18246] fxa.routes.avatar.upload.ERROR: Processing error Error: socket hang up
    at createHangUpError (http.js:1472:15)
    at Socket.socketOnEnd [as onend] (http.js:1568:23)
    at Socket.g (events.js:180:16)
    at Socket.EventEmitter.emit (events.js:117:20)
    at _stream_readable.js:920:16
    at process._tickDomainCallback (node.js:459:13)
[p18246] fxa.summary.ERROR: {"code":500,"errno":103,"path":"/v1/avatar/upload","agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0","t":188,"stack":"Error: Image processing error\n    at Function.processingError (/Users/stomlinson/development/fxa-profile-server/lib/error.js:114:10)\n    at Request._callback (/Users/stomlinson/development/fxa-profile-server/lib/routes/avatar/upload.js:39:25)\n    at self.callback (/Users/stomlinson/development/fxa-profile-server/node_modules/request/request.js:123:22)\n    at Request.EventEmitter.emit (events.js:117:20)\n    at ClientRequest.self.clientErrorHandler (/Users/stomlinson/development/fxa-profile-server/node_modules/request/request.js:244:10)\n    at ClientRequest.EventEmitter.emit (events.js:95:17)\n    at Socket.socketOnEnd (http.js:1568:9)\n    at Socket.g (events.js:180:16)\n    at Socket.EventEmitter.emit (events.js:117:20)\n    at _stream_readable.js:920:16"}
[p18246] fxa.server.web.INFO: POST /v1/avatar/upload - 500 (193ms) <1410776360429-18246-51617>
[p18246] fxa.server.web.VERBOSE: Response: {
  "code": 500,
  "errno": 103,
  "error": "Internal Server Error",
  "message": "Image processing error",
  "_res": {
    "code": "ECONNRESET"
  }
} <1410776360429-18246-51617>

This results in an "incorrect password" message displayed to the user:

The request log to the profile server:

Request URL:    http://127.0.0.1:1111/v1/avatar/upload
Request Method:     POST
Status Code:    HTTP/1.1 500 Internal Server Error
Request Headers 11:22:14.000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Referer:    http://127.0.0.1:3030/settings/avatar/camera
Pragma: no-cache
Origin: http://127.0.0.1:3030
Host:   127.0.0.1:1111
DNT:    1
Content-Type:   image/jpeg
Content-Length: 43863
Connection: keep-alive
Cache-Control:  no-cache
Authorization:  Bearer 20b25485929bd2f957e2ab9135ba29093661f65a6025e23b220b938e800fd431
Accept-Language:    en
Accept-Encoding:    gzip, deflate
Accept: application/json

Fetch the request body (42863 bytes)
Response Headers Δ8ms
Vary:   accept-encoding
Transfer-Encoding:  chunked
Date:   Mon, 15 Sep 2014 10:22:14 GMT
Content-Type:   application/json; charset=utf-8
Content-Encoding:   gzip
Connection: keep-alive
Cache-Control:  no-cache
access-control-max-age: 86400
access-control-expose-headers:  WWW-Authenticate, Server-Authorization
Access-Control-Allow-Origin:    *
access-control-allow-methods:   GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS
access-control-allow-headers:   Authorization, Content-Type, If-None-Match
Response Body Δ0ms
{"code":500,"errno":103,"error":"Internal Server Error","message":"Image processing error","_res":{"code":"ECONNREFUSED","errno":"ECONNREFUSED","syscall":"connect"}}

shane-tomlinson · 2014-09-15T10:26:05Z

The above error is due to not having GraphicsMagick installed. Once that was installed, no problem. The error message should be changed to something other than incorrect password for this case.

shane-tomlinson · 2014-09-15T10:30:10Z

I do not know what caused this console error, but I see this:

NS_ERROR_NOT_AVAILABLE:  cropper.js:219

shane-tomlinson · 2014-09-15T10:36:14Z

@zaach - Every time I attempt to use a gravatar image, two calls are made to session/status, and then one to /avatar on the profile server. I keep receiving the error "No Gravatar found", even though this email address has an associated gravatar.

GET http://127.0.0.1:9000/v1/session/status [HTTP/1.1 200 OK 6ms]
GET http://127.0.0.1:9000/v1/session/status [HTTP/1.1 200 OK 16ms]
GET http://127.0.0.1:1111/v1/avatar [HTTP/1.1 200 OK 4ms]

shane-tomlinson · 2014-09-15T10:39:08Z

@zaach - should the image cropper be available when taking a snapshot from the camera?

shane-tomlinson · 2014-09-15T10:40:52Z

If I change my profile photo and visit /settings, the profile photo is displayed. If I sign out, sign back in, visit /settings, the profile photo is reset to the default. If I visit /settings/avatar, then go back to /settings, the photo appears.

zaach · 2014-09-15T15:56:02Z

@shane-tomlinson No image cropper after a camera photo, as per discussion with Ryan.

For /settings, I didn't want to add another network request to fetch the avatar until we enable avatar images in the UI. For now it's only fetched when you visit /settings/avatar. Should we go ahead and do a request in /settings anyway?

Fixes #1660. This issue occurred because the context of an error was being discarded as the error was rethrown in the form submission handler. Rather than depend on error context being passed around, we store it on individual error objects.

shane-tomlinson · 2014-09-16T08:45:25Z

app/scripts/views/base.js

@@ -11,14 +11,16 @@ define([
  'lib/promise',
  'lib/session',
  'lib/auth-errors',
+  'lib/oauth-errors',


These are not actually used anywhere in base.

* remove unused deps from base.js. * remove `errors` parameter in `displayError` calls in views/mixins/service-mixin

coveralls · 2014-09-16T09:25:49Z

Coverage increased (+0.15%) when pulling 2160650 on profile-images-backend into a23ca63 on master.

fix(avatars): integrate the profile images backend I give this my r+.

shane-tomlinson · 2014-09-16T09:55:16Z

@zaach - I merged this as "good to go" but I still cannot use gravatar images. I am unsure if this is a problem with my setup, or whether this is a legitimate problem. If this is a problem, let's file a bug and open a follow on PR.

Nice work on this, and thanks for working through it with me!

zaach · 2014-09-16T18:38:18Z

@shane-tomlinson Hmm, what email address are you using? I can try it locally and see how it goes on my setup.

shane-tomlinson · 2014-09-16T19:40:29Z

@zaach - sent you an email with it.

zaach force-pushed the profile-images-backend branch from 018f0e1 to 0316885 Compare August 21, 2014 00:08

zaach added the WIP label Aug 21, 2014

zaach force-pushed the profile-images-backend branch from 0316885 to c16090e Compare August 21, 2014 22:55

zaach removed the WIP label Aug 22, 2014

pdehaan reviewed Aug 22, 2014
View reviewed changes

ckarlof reviewed Aug 26, 2014
View reviewed changes

zaach force-pushed the profile-images-backend branch 3 times, most recently from 3f97c56 to 69add1c Compare September 10, 2014 22:45

ckarlof added the in review label Sep 10, 2014

pdehaan reviewed Sep 10, 2014
View reviewed changes

ckarlof reviewed Sep 12, 2014
View reviewed changes

zaach force-pushed the profile-images-backend branch from bc36725 to 55ea03f Compare September 12, 2014 22:59

zaach added 2 commits September 15, 2014 12:38

fix(avatars): disable remote URL option

17e76ac

fix(avatars): integrate profile server backend for profile images

66ebc83

shane-tomlinson self-assigned this Sep 15, 2014

zaach mentioned this pull request Sep 16, 2014

Incorrect message shown for profile server errors #1660

Closed

zaach force-pushed the profile-images-backend branch from 55ea03f to 9eba71a Compare September 16, 2014 00:47

zaach force-pushed the profile-images-backend branch from 9eba71a to a3c4f2a Compare September 16, 2014 01:12

shane-tomlinson reviewed Sep 16, 2014
View reviewed changes

Pre-merge cleanup

2160650

* remove unused deps from base.js. * remove `errors` parameter in `displayError` calls in views/mixins/service-mixin

shane-tomlinson pushed a commit that referenced this pull request Sep 16, 2014

Merge pull request #1584 from mozilla/profile-images-backend

559516e

fix(avatars): integrate the profile images backend I give this my r+.

shane-tomlinson merged commit 559516e into master Sep 16, 2014

shane-tomlinson removed the waffle:needs review label Sep 16, 2014

shane-tomlinson deleted the profile-images-backend branch September 16, 2014 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(avatars): integrate the profile images backend #1584

fix(avatars): integrate the profile images backend #1584

zaach commented Aug 21, 2014

zaach commented Aug 21, 2014

zaach commented Aug 22, 2014

pdehaan Aug 22, 2014

ckarlof commented Aug 26, 2014

ckarlof Aug 26, 2014

zaach commented Sep 10, 2014

pdehaan Sep 10, 2014

ckarlof Sep 12, 2014

ckarlof commented Sep 12, 2014

ckarlof commented Sep 13, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

zaach commented Sep 15, 2014

shane-tomlinson Sep 16, 2014

coveralls commented Sep 16, 2014

shane-tomlinson commented Sep 16, 2014

zaach commented Sep 16, 2014

shane-tomlinson commented Sep 16, 2014

		@@ -25,7 +25,6 @@
		<nav id="avatar-options">
		<input type="file" id="imageLoader" name="imageLoader" accept="image/png, image/jpeg"/>

fix(avatars): integrate the profile images backend #1584

fix(avatars): integrate the profile images backend #1584

Conversation

zaach commented Aug 21, 2014

zaach commented Aug 21, 2014

zaach commented Aug 22, 2014

pdehaan Aug 22, 2014

Choose a reason for hiding this comment

ckarlof commented Aug 26, 2014

ckarlof Aug 26, 2014

Choose a reason for hiding this comment

zaach commented Sep 10, 2014

pdehaan Sep 10, 2014

Choose a reason for hiding this comment

ckarlof Sep 12, 2014

Choose a reason for hiding this comment

ckarlof commented Sep 12, 2014

ckarlof commented Sep 13, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

shane-tomlinson commented Sep 15, 2014

zaach commented Sep 15, 2014

shane-tomlinson Sep 16, 2014

Choose a reason for hiding this comment

coveralls commented Sep 16, 2014

shane-tomlinson commented Sep 16, 2014

zaach commented Sep 16, 2014

shane-tomlinson commented Sep 16, 2014