fix(avatars): load profile image on settings and sign in pages if available #1733

zaach · 2014-10-02T06:51:45Z

This refactor changes a number of things:
* fix avatar background (fixes #1722)
* loads profile images on settings and sign in account chooser (fixes #1727)
* introduces User and Account models
* imports old Session based sessions into User
* handles direct link to settings page from RPs for a specified account (fixes #1686)
* redirects unverified accounts that visit /settings (fixes #1716)
* introduces a way to pass ephemeral data between views
* removes unused avatar-change-via-URL resources/tests (fixes #1544, fixes #1618)

zaach · 2014-10-02T07:32:54Z

Now that signin checks the profile server we'll have to rethink running those functional tests on travis...

pdehaan · 2014-10-06T16:28:47Z

@zaach is this ready for review, or still WIP while we sort out Travis issues?

zaach · 2014-10-06T16:33:28Z

@pdehaan You can begin review, I should only need to fix the tests.

nchapman · 2014-10-06T21:18:37Z

app/scripts/views/mixins/avatar-mixin.js

+        promise = this.profileClient.getAvatar()
+          .then(function (result) {
+            if (result.avatar && result.id) {
+              Session.set('avatar', result.avatar);


Since we're trying to use Session less, do you think it makes sense to use sessionStorage or localStorage directly here?

Hmm, well we're using it in other views that access the avatar also. I'm inclined to refactor it all at once, e.g. if we merge it into a user model.

Yep that makes sense. Carry on!

I'm introducing a user model in #1646, be prepared. I'm somewhat inclined to think an Avatar could be its own model that is a child of the User model...

nchapman · 2014-10-06T21:23:12Z

app/scripts/views/sign_in.js

@@ -55,6 +56,13 @@ function (_, p, BaseView, FormView, SignInTemplate, Session, PasswordMixin, Auth
      'click .use-different': 'useDifferentAccount'
    },

+    beforeRender: function () {


Could this slow down sign in if it takes a while to fetch the image? Would it be better to do this after render?

Sure if we're okay with it flashing the default image.

Yeah I'm not sure what's preferable. Since images traditionally load async, I don't think it'd feel too unnatural for it to load post render. I think we'd want to leave it blank and just drop in the image, rather than re-rendering the view to minimize any flicker. This isn't a show stopper. Just something to consider.

This might be a tricky issue. I'd prefer not to see the flashing if the image load is quick, but if it's slow for some reason (say 5-10s), showing the default image (or spinner) after some reasonable amount time may signal that "nothing's obviously broken".

Agreed. I do think though in many cases that loading the image after render (especially in the cached case) would be imperceptible to the user as long as nothing shifts position. It does feel a bit risky to make the sign in flow dependent on the avatar when it's not absolutely critical.

Good point... Maybe we should switch to afterRender and if flashing is noticeable during dogfooding, we can do something more involved with spinners.

nchapman · 2014-10-06T21:31:18Z

So just to be clear: there's still no way to set an avatar, correct? Everyone will be seeing the default for now?

zaach · 2014-10-06T21:33:00Z

@nchapman Right, only people who know the super secret URL will be able to change their avatar.

nchapman · 2014-10-06T21:35:13Z

@nchapman Right, only people who know the super secret URL will be able to change their avatar.

Okay great.

nchapman · 2014-10-06T21:54:35Z

app/tests/spec/views/settings.js

+
+        return view.render()
+          .then(function () {
+            assert.equal(view.$('.avatar-wrapper img').length, 0);


So the default image doesn't use an img tag?

Right, but that might change when #1722 is fixed.

Ah ok cool.

zaach · 2014-10-07T00:24:47Z

app/scripts/views/settings.js

@@ -43,8 +45,28 @@ function (_, FormView, BaseView, Template, Session, Constants) {
                  success: t('Signed out')
                });
              });
+    },
+
+    afterVisible: function () {


Updated per @nchapman's suggestion.

You know, I completely forgot about afterVisible, this would have been useful in the "smooth out the verification flow" PR.

ckarlof · 2014-10-07T02:10:50Z

app/scripts/views/mixins/avatar-mixin.js

+], function (p, Session) {
+
+  return {
+    // Attempt to load a profile image from the profile server and cache it on Session


How does this cache get invalidated if the image changes?

It won't be. Alternatively, we can always check the server.

Let's remove the URL caching for now and push this out to the next train.

It's now loaded from the server each time 👍

ckarlof · 2014-10-07T02:18:53Z

app/scripts/views/mixins/avatar-mixin.js

+            }
+          });
+      } else {
+        // We already have a profile image in Session


This means we have a profile image URL, correct?

shane-tomlinson · 2014-10-07T07:59:21Z

app/scripts/views/settings.js

+    },
+
+    afterVisible: function () {
+      return this.loadProfileImage();


Does the base class's afterVisible need to be called?

shane-tomlinson · 2014-10-07T07:59:47Z

app/scripts/views/sign_in.js

+        }, function () {
+          // Ignore errors and just show the default image
+        });
+    },


I think the base class' afterVisible should be called to force an autofocus.

shane-tomlinson · 2014-10-07T08:01:04Z

app/scripts/views/sign_in.js

  'views/decorators/progress_indicator'
 ],
-function (_, p, BaseView, FormView, SignInTemplate, Session, PasswordMixin, AuthErrors, Validate, ServiceMixin, showProgressIndicator) {
+function (_, p, BaseView, FormView, SignInTemplate, Session, PasswordMixin, AuthErrors, Validate, ServiceMixin, AvatarMixin, showProgressIndicator) {


Can you break this line, it's running off the end of the screen.

shane-tomlinson · 2014-10-07T08:09:39Z

app/scripts/views/settings.js

  'lib/session',
  'lib/constants'
 ],
-function (_, FormView, BaseView, Template, Session, Constants) {
+function (_, FormView, BaseView, AvatarMixin, Template, AuthErrors, Session, Constants) {


Is AuthErrors used?

shane-tomlinson · 2014-10-07T08:12:13Z

app/tests/spec/views/sign_in.js


      view = new View({
        router: routerMock,
        metrics: metrics,
        window: windowMock,
        fxaClient: fxaClient,
+        profileClient: profileClientMock,


@ckarlof brought up the idea a few weeks back of adding all these dependencies to a single object, that then gets passed around to the various views. I'm really beginning to see the value of that. We have both had to update tests just to pass in dependencies recently.

shane-tomlinson · 2014-10-08T09:39:47Z

app/scripts/views/mixins/avatar-mixin.js

+        })
+        .then(function (found) {
+          if (found) {
+            self.logEvent(self.className + '.profile_image_shown');


Should we log a profile_image_now_shown if found === false, or is that taken care of in the error handler?

shane-tomlinson · 2014-10-08T09:40:36Z

app/scripts/views/mixins/avatar-mixin.js

+            return true;
+          }
+        })
+        .then(function (found) {


Can this then be collapsed into the one above?

shane-tomlinson · 2014-10-08T09:44:42Z

app/scripts/views/mixins/avatar-mixin.js

+      return this.profileClient.getAvatar()
+        .then(function (result) {
+          if (result.avatar && result.id) {
+            Session.set('avatar', result.avatar);


It looks like we are fetching the avatar in multiple views, but then saving the avatar into the Session. Could we just skip the storage into the Session and return the avatar from profileClient.getAvatar()? Does profileClient.getAvatar cache the images?

No, there's no caching in the profileClient. The other avatar pages still use Session.avatar, but we could rip that out and always load it from the server there, too.

shane-tomlinson · 2014-10-08T09:46:01Z

app/scripts/views/sign_in.js

+      return this._fetchProfileImage()
+        .then(function () {
+          if (Session.avatar) {
+            self.$('.avatar-wrapper').append(new Image());


This code is also in settings.js, can it ripped out and added to the avatar-mixin?

zaach · 2014-11-13T17:38:08Z

@shane-tomlinson Aha– those tests should be removed since settings redirects unverified users.

shane-tomlinson · 2014-11-13T20:52:07Z

~~@zaach - can you try this Sync case:~~

~~1) Sign in to the sync flow~~
~~2) Open the browser's preferences, click "Forget this email" or "disconnect"~~
~~3) Click "Sign in"~~
~~4) Open another tab to stomlinson.dev.lcip.org/signin~~
~~5) No password field!~~
~~6) Click "Sign in".~~
~~7) A "Session expired" error.~~

~~Expected~~
~~In step 5, a password field should be shown.~~

This also reproduces in prod - filed this bug as #1877

shane-tomlinson · 2014-11-13T23:03:55Z

@zaach - another strange Sync case:

Open the browser, if signed into Sync, sign out.
Restart browser, immediately open dev tools
Click the Sync icon from the home screen.
Sign in
Click the hamburger icon, click your sync username
Under Firefox Account, click "Disconnect"
Go back to the original tab.
Explosion and redirect to the /500 page.

Error in the console:

"no response from browser: session_status" 5ed8b225.main.js:14:13299

"Critical error:" Error: Unexpected error
Stack trace:
.toError@https://stomlinson.dev.lcip.org/scripts/5ed8b225.main.js:7:12632
f/a.timeout<@https://stomlinson.dev.lcip.org/scripts/5ed8b225.main.js:14:13361

…ilable Fixes #1727.

…rofile image

This refactor changes a number of things: - introduces User and Account models - imports old Session based sessions into User - handles direct link to settings page from RPs for a specified account (fixes #1686) - redirects unverified accounts that visit /settings (fixes #1716) - introduces a way to pass ephemeral data between views - removes unused avatar-change-via-URL resources/tests (fixes #1544, fixes #1618)

Adds tests for issue #1716 and #1686.

…ount models

…sent

fix(avatars): load profile image on settings and sign in pages if available Fixes #1727, #1722, #1686, #1716, #1544, #1618 r=ckarlof,shane-tomlinson

ckarlof · 2014-11-15T00:27:57Z

Congratz @zaach!

zaach added the waffle:needs review label Oct 2, 2014

zaach force-pushed the issue-1727-load-avatar-on-profile branch 2 times, most recently from b3c8f01 to cfbed6d Oct 6, 2014

nchapman reviewed Oct 6, 2014
View changes

nchapman self-assigned this Oct 6, 2014

zaach force-pushed the issue-1727-load-avatar-on-profile branch 2 times, most recently from f604d04 to ec51fd9 Oct 6, 2014

zaach reviewed Oct 7, 2014
View changes

ckarlof reviewed Oct 7, 2014
View changes

shane-tomlinson reviewed Oct 7, 2014
View changes

app/scripts/views/settings.js

},

afterVisible: function () {

return this.loadProfileImage();

This comment has been minimized.

Sign in to view

shane-tomlinson Oct 7, 2014
Member

Does the base class's afterVisible need to be called?

shane-tomlinson reviewed Oct 7, 2014
View changes

zaach force-pushed the issue-1727-load-avatar-on-profile branch from ec51fd9 to 7c49d22 Oct 7, 2014

zaach assigned shane-tomlinson and unassigned nchapman Oct 8, 2014

shane-tomlinson reviewed Oct 8, 2014
View changes

app/scripts/views/mixins/avatar-mixin.js

return true;

}

})

.then(function (found) {

This comment has been minimized.

Sign in to view

shane-tomlinson Oct 8, 2014
Member

Can this then be collapsed into the one above?

shane-tomlinson reviewed Oct 8, 2014
View changes

shane-tomlinson mentioned this pull request Nov 13, 2014

add functional tests for unverified users that visit the /settings/avatar* pages #1876

Closed

shane-tomlinson mentioned this pull request Nov 13, 2014

Missing password field on /signin page in convoluted Sync flow #1877

Closed

This was referenced Nov 14, 2014

account creation after 1017 (DESKTOP_CHANNEL_TIMEOUT) errors? #1830

Closed

Remove calls to session_status in Desktop Sync broker #1883

Closed

zaach force-pushed the issue-1727-load-avatar-on-profile branch from 6e0fe08 to 5ef6277 Nov 14, 2014

zaach added 12 commits Oct 2, 2014

fix(avatars): load profile image on settings and sign in pages if ava…

382c9db

…ilable Fixes #1727.

fix(avatars): ensure the default image background is covered by the p…

387e324

…rofile image

refactor(avatars): remove Session cache and load avatar from server

5d598db

refactor(account): User always returns an account; no more null

fccb7a4

refactor(views): use private var notation and othe clean-up

003a5b1

tests(settings): more functional tests coverage of settings page

02c1da6

Adds tests for issue #1716 and #1686.

refactor(account): update fx-desktop broker to work with user and acc…

dc7ab1c

…ount models

tests(change_password): remove unnecessary unverified account checks

3d83464

refactor(accounts): only try a profile request if access token is pre…

d4f0d58

…sent

fix(accounts): only fetch access token if verified

d316885

chore(convict): update convict

1e902be

zaach force-pushed the issue-1727-load-avatar-on-profile branch from 8034039 to 3a6dd96 Nov 14, 2014

fix(broker): fix desktop broker to forward account data before confirm

0492140

zaach force-pushed the issue-1727-load-avatar-on-profile branch from 3a6dd96 to 0492140 Nov 15, 2014

ckarlof merged commit 8724e62 into master Nov 15, 2014
1 check passed

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

ckarlof removed the waffle:needs review label Nov 15, 2014

ckarlof deleted the issue-1727-load-avatar-on-profile branch Nov 15, 2014

zaach mentioned this pull request Nov 17, 2014

Uploading an avatar by URL gives un-actionable error message #1613

Closed

shane-tomlinson mentioned this pull request Nov 20, 2014

Sign-in form should remain when navigating back from TOS/PP #1713

Closed

mozilla / fxa-content-server Archived

fix(avatars): load profile image on settings and sign in pages if available #1733

fix(avatars): load profile image on settings and sign in pages if available #1733

Conversation

zaach commented Oct 2, 2014

zaach commented Oct 2, 2014

pdehaan commented Oct 6, 2014

zaach commented Oct 6, 2014

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

zaach Oct 6, 2014 Author Contributor

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

zaach Oct 6, 2014 Author Contributor

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

ckarlof Oct 6, 2014 Contributor

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

zaach Oct 6, 2014 Author Contributor

nchapman commented Oct 6, 2014

zaach commented Oct 6, 2014

nchapman commented Oct 6, 2014

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

zaach Oct 6, 2014 Author Contributor

This comment has been minimized.

nchapman Oct 6, 2014 Contributor

This comment has been minimized.

zaach Oct 7, 2014 Author Contributor

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

ckarlof Oct 7, 2014 Contributor

This comment has been minimized.

zaach Oct 7, 2014 Author Contributor

This comment has been minimized.

ckarlof Oct 7, 2014 Contributor

This comment has been minimized.

zaach Oct 7, 2014 Author Contributor

This comment has been minimized.

ckarlof Oct 7, 2014 Contributor

This comment has been minimized.

zaach Oct 7, 2014 Author Contributor

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

zaach Oct 7, 2014 Author Contributor

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

shane-tomlinson Oct 7, 2014 Member

This comment has been minimized.

zaach Oct 7, 2014 Author Contributor

This comment has been minimized.

shane-tomlinson Oct 8, 2014 Member

This comment has been minimized.

shane-tomlinson Oct 8, 2014 Member

This comment has been minimized.

shane-tomlinson Oct 8, 2014 Member

This comment has been minimized.

zaach Oct 8, 2014 Author Contributor

This comment has been minimized.

shane-tomlinson Oct 8, 2014 Member

zaach commented Nov 13, 2014

shane-tomlinson commented Nov 13, 2014

shane-tomlinson commented Nov 13, 2014

nchapman Oct 6, 2014
Contributor

zaach Oct 6, 2014
Author Contributor

nchapman Oct 6, 2014
Contributor

shane-tomlinson Oct 7, 2014
Member

nchapman Oct 6, 2014
Contributor

zaach Oct 6, 2014
Author Contributor

nchapman Oct 6, 2014
Contributor

ckarlof Oct 6, 2014
Contributor

nchapman Oct 6, 2014
Contributor

zaach Oct 6, 2014
Author Contributor

nchapman Oct 6, 2014
Contributor

zaach Oct 6, 2014
Author Contributor

nchapman Oct 6, 2014
Contributor

zaach Oct 7, 2014
Author Contributor

shane-tomlinson Oct 7, 2014
Member

ckarlof Oct 7, 2014
Contributor

zaach Oct 7, 2014
Author Contributor

ckarlof Oct 7, 2014
Contributor

zaach Oct 7, 2014
Author Contributor

ckarlof Oct 7, 2014
Contributor

zaach Oct 7, 2014
Author Contributor

shane-tomlinson Oct 7, 2014
Member

shane-tomlinson Oct 7, 2014
Member

zaach Oct 7, 2014
Author Contributor

shane-tomlinson Oct 7, 2014
Member

shane-tomlinson Oct 7, 2014
Member

shane-tomlinson Oct 7, 2014
Member

zaach Oct 7, 2014
Author Contributor

shane-tomlinson Oct 8, 2014
Member

shane-tomlinson Oct 8, 2014
Member

shane-tomlinson Oct 8, 2014
Member

zaach Oct 8, 2014
Author Contributor

shane-tomlinson Oct 8, 2014
Member