From d7298f5a1189658786e05cb43356d9e4639c48e4 Mon Sep 17 00:00:00 2001 From: Shane Tomlinson Date: Thu, 25 May 2017 13:42:12 +0100 Subject: [PATCH] fix(sign_in): Fix how the listener is bound for account->chang:accessToken (#5103) r=@philbooth We were binding using `account.on` in `beforeRender`. This didn't cause any visible problems, but caused two problems behind the scenes: 1. The view to be retained in memory after being torn down because the account kept a reference to the View. 2. If the accessToken was invalidated after the view was destroyed, the *view would re-render anyways*, just not visibly. Using view.listenTo, the listener is removed from the account whenever the view is torn down. In addition, this is only done in afterVisible so that only one listener is ever bound. If done in `beforeRender`, a new listener was attached on every render. Not attached to an issue. --- app/scripts/views/sign_in.js | 34 ++++++++++++++++----------------- app/tests/spec/views/sign_in.js | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/app/scripts/views/sign_in.js b/app/scripts/views/sign_in.js index 91fd491f2..04ad9d7cc 100644 --- a/app/scripts/views/sign_in.js +++ b/app/scripts/views/sign_in.js @@ -9,7 +9,6 @@ define(function (require, exports, module) { const allowOnlyOneSubmit = require('views/decorators/allow_only_one_submit'); const AuthErrors = require('lib/auth-errors'); const AvatarMixin = require('views/mixins/avatar-mixin'); - const BaseView = require('views/base'); const Cocktail = require('cocktail'); const ExperimentMixin = require('views/mixins/experiment-mixin'); const FlowBeginMixin = require('views/mixins/flow-begin-mixin'); @@ -18,6 +17,7 @@ define(function (require, exports, module) { const MigrationMixin = require('views/mixins/migration-mixin'); const PasswordMixin = require('views/mixins/password-mixin'); const PasswordResetMixin = require('views/mixins/password-reset-mixin'); + const { preventDefaultThen, t } = require('views/base'); const ResumeTokenMixin = require('views/mixins/resume-token-mixin'); const ServiceMixin = require('views/mixins/service-mixin'); const Session = require('lib/session'); @@ -26,15 +26,13 @@ define(function (require, exports, module) { const SignInMixin = require('views/mixins/signin-mixin'); const SignInTemplate = require('stache!templates/sign_in'); - var t = BaseView.t; + const proto = FormView.prototype; - var View = FormView.extend({ + const View = FormView.extend({ template: SignInTemplate, className: 'sign-in', - initialize (options) { - options = options || {}; - + initialize (options = {}) { this._formPrefill = options.formPrefill; // The number of stored accounts is logged to see if we can simplify @@ -49,18 +47,25 @@ define(function (require, exports, module) { beforeRender () { this._account = this._suggestedAccount(); + }, - this._account.on('change:accessToken', () => { + afterVisible () { + proto.afterVisible.call(this); + // this.displayAccountProfileImage could cause the existing + // accessToken to be invalidated, in which case the view + // should be re-rendered with the default avatar. + const account = this.getAccount(); + this.listenTo(account, 'change:accessToken', () => { // if no access token and password is not visible we need to show the password field. - if (! this._account.has('accessToken') && this.$('.password').is(':hidden')) { + if (! account.has('accessToken') && this.$('.password').is(':hidden')) { // accessToken could be changed async by an external request after render // If the ProfileClient fails to get an OAuth token with the current token then reset the view this.chooserAskForPassword = true; - return this.render().then(function () { - this.setDefaultPlaceholderAvatar(); - }); + return this.render().then(() => this.setDefaultPlaceholderAvatar()); } }); + + return this.displayAccountProfileImage(account, { spinner: true }); }, getAccount () { @@ -111,11 +116,6 @@ define(function (require, exports, module) { 'click .use-logged-in': 'useLoggedInAccount' }, - afterVisible () { - FormView.prototype.afterVisible.call(this); - return this.displayAccountProfileImage(this.getAccount(), { spinner: true }); - }, - beforeDestroy () { this._formPrefill.set('email', this.getElementValue('.email')); this._formPrefill.set('password', this.getElementValue('.password')); @@ -195,7 +195,7 @@ define(function (require, exports, module) { /** * Render to a basic sign in view, used with "Use a different account" button */ - useDifferentAccount: BaseView.preventDefaultThen(function () { + useDifferentAccount: preventDefaultThen(function () { // TODO when the UI allows removal of individual accounts, // only clear the current account. this.user.removeAllAccounts(); diff --git a/app/tests/spec/views/sign_in.js b/app/tests/spec/views/sign_in.js index 7434b6219..881b16449 100644 --- a/app/tests/spec/views/sign_in.js +++ b/app/tests/spec/views/sign_in.js @@ -163,9 +163,11 @@ define(function (require, exports, module) { }); sinon.stub(view, '_suggestedAccount', () => account); + sinon.stub(view, 'displayAccountProfileImage', () => p()); sinon.spy(view, 'render'); return view.render() + .then(() => view.afterVisible()) .then(() => { account.set({ accessToken: null, @@ -184,6 +186,38 @@ define(function (require, exports, module) { }); }); + describe('destroy', () => { + beforeEach(() => { + initView(); + + const account = user.initAccount({ + accessToken: 'access token', + email: 'a@a.com', + sessionToken: 'session token', + sessionTokenContext: Constants.SYNC_SERVICE + }); + + sinon.stub(view, '_suggestedAccount', () => account); + sinon.stub(view, 'displayAccountProfileImage', () => p()); + sinon.spy(view, 'render'); + + return view.render() + .then(() => view.afterVisible()) + .then(() => view.destroy()) + .then(() => { + account.set({ + accessToken: null, + sessionToken: null, + sessionTokenContext: null + }); + }); + }); + + it('does not re-render once destroyed if the accessToken is invalidated', () => { + assert.equal(view.render.callCount, 1); + }); + }); + describe('migration', () => { it('does not display migration message if no migration', () => { initView();