From d4e4a6300a7fd94ca2c1f1bf9c2f5c43951875b6 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 7 Oct 2012 00:10:36 -0700 Subject: [PATCH] Change interface for determining if the user doc is loaded to a new reactive function Meteor.userLoaded(), which is true if you are logged in and the user doc is loaded, and a currentUserLoaded Handlebars helper to match. If logged in and the user doc is not yet loaded, Meteor.user() now returns an object which only contains _id. The current user subscription is now named meteor.currentUser rather than being an unnamed sub. (loginServiceConfiguration is renamed meteor.loginServiceConfiguration to match.) This subscription is sub'd from when you log in and unsub'd from when you log out (or if you log in with different credentials). I was very careful to make sure that in the case of "sub #1, unsub #1, sub #2, sub #1 is ready" we do not declare the user to be ready. I could have instead modified livedata_connection to not call ready callbacks for unsub'd subscriptions (add a "delete self.sub_ready_callbacks[obj._id]" to the self.subs removed function) but this seemed less invasive. The password and email tests use this to take a more rigorous approach to waiting for the data to load, and they change the localStorage keys so that multiple tabs running tests don't interact via localStorage. --- packages/accounts-base/accounts_client.js | 74 +++++++++++--- packages/accounts-base/accounts_server.js | 17 +++- packages/accounts-base/localstorage_token.js | 29 ++---- packages/accounts-password/email_tests.js | 84 ++++++++++------ packages/accounts-password/passwords_tests.js | 98 ++++++++++--------- .../accounts-ui-unstyled/login_buttons.html | 6 +- .../login_buttons_dropdown.html | 6 +- 7 files changed, 192 insertions(+), 122 deletions(-) diff --git a/packages/accounts-base/accounts_client.js b/packages/accounts-base/accounts_client.js index c7e89ddc59..9cada0c20b 100644 --- a/packages/accounts-base/accounts_client.js +++ b/packages/accounts-base/accounts_client.js @@ -4,23 +4,64 @@ return Meteor.default_connection.userId(); }; + var userLoadedListeners = new Meteor.deps._ContextSet; + var currentUserSubscriptionData; + + Meteor.userLoaded = function () { + userLoadedListeners.addCurrentContext(); + return currentUserSubscriptionData && currentUserSubscriptionData.loaded; + }; + Meteor.user = function () { var userId = Meteor.userId(); - if (userId) { - var result = Meteor.users.findOne(userId); - if (result) { - return result; - } else { - // If the login method completes but new subcriptions haven't - // yet been sent down to the client, this is the best we can - // do - return {_id: userId, loading: true}; - } - } else { + if (!userId) return null; + if (currentUserSubscriptionData && currentUserSubscriptionData.loaded) + return Meteor.users.findOne(userId); + // Not yet loaded: return a minimal object. + return {_id: userId}; + }; + + Accounts._makeClientLoggedOut = function() { + Accounts._unstoreLoginToken(); + Meteor.default_connection.setUserId(null); + Meteor.default_connection.onReconnect = null; + userLoadedListeners.invalidateAll(); + if (currentUserSubscriptionData) { + currentUserSubscriptionData.handle.stop(); + currentUserSubscriptionData = null; } }; + Accounts._makeClientLoggedIn = function(userId, token) { + Accounts._storeLoginToken(userId, token); + Meteor.default_connection.setUserId(userId); + Meteor.default_connection.onReconnect = function() { + Meteor.apply('login', [{resume: token}], {wait: true}, function(error, result) { + if (error) { + Accounts._makeClientLoggedOut(); + throw error; + } else { + // nothing to do + } + }); + }; + userLoadedListeners.invalidateAll(); + if (currentUserSubscriptionData) { + currentUserSubscriptionData.handle.stop(); + } + var data = currentUserSubscriptionData = {loaded: false}; + data.handle = Meteor.subscribe( + "meteor.currentUser", function () { + // Important! We use "data" here, not "currentUserSubscriptionData", so + // that if we log out and in again before this subscription is ready, we + // don't make currentUserSubscriptionData look ready just because this + // older iteration of subscribing is ready. + data.loaded = true; + userLoadedListeners.invalidateAll(); + }); + }; + Meteor.logout = function (callback) { Meteor.apply('logout', [], {wait: true}, function(error, result) { if (error) { @@ -32,19 +73,22 @@ }); }; - // If we're using Handlebars, register the {{currentUser}} global - // helper - if (window.Handlebars) { + // If we're using Handlebars, register the {{currentUser}} and + // {{currentUserLoaded}} global helpers. + if (typeof Handlebars !== 'undefined') { Handlebars.registerHelper('currentUser', function () { return Meteor.user(); }); + Handlebars.registerHelper('currentUserLoaded', function () { + return Meteor.userLoaded(); + }); } // XXX this can be simplified if we merge in // https://github.com/meteor/meteor/pull/273 var loginServicesConfigured = false; var loginServicesConfiguredListeners = new Meteor.deps._ContextSet; - Meteor.subscribe("loginServiceConfiguration", function () { + Meteor.subscribe("meteor.loginServiceConfiguration", function () { loginServicesConfigured = true; loginServicesConfiguredListeners.invalidateAll(); }); diff --git a/packages/accounts-base/accounts_server.js b/packages/accounts-base/accounts_server.js index 210cf37256..83c8aceb91 100644 --- a/packages/accounts-base/accounts_server.js +++ b/packages/accounts-base/accounts_server.js @@ -233,13 +233,22 @@ /// PUBLISHING DATA /// - // Always publish the current user's record to the client. - Meteor.publish(null, function() { + // Publish the current user's record to the client. + // XXX This should just be a universal subscription, but we want to know when + // we've gotten the data after a 'login' method, which currently requires + // us to unsub, sub, and wait for onComplete. This is wasteful because + // we're actually guaranteed to have the data by the time that 'login' + // returns. But we don't expose a callback to Meteor.apply which lets us + // know when the data has been processed (ie, quiescence, or at least + // partial quiescence). + Meteor.publish("meteor.currentUser", function() { if (this.userId) return Meteor.users.find({_id: this.userId}, {fields: {profile: 1, username: 1, emails: 1}}); - else + else { + this.complete(); return null; + } }, {is_auto: true}); // If autopublish is on, also publish everyone else's user record. @@ -252,7 +261,7 @@ }); // Publish all login service configuration fields other than secret. - Meteor.publish("loginServiceConfiguration", function () { + Meteor.publish("meteor.loginServiceConfiguration", function () { return Accounts.configuration.find({}, {fields: {secret: 0}}); }, {is_auto: true}); // not techincally autopublish, but stops the warning. diff --git a/packages/accounts-base/localstorage_token.js b/packages/accounts-base/localstorage_token.js index 51ad0ac597..b5f5c11e59 100644 --- a/packages/accounts-base/localstorage_token.js +++ b/packages/accounts-base/localstorage_token.js @@ -3,6 +3,14 @@ var loginTokenKey = "Meteor.loginToken"; var userIdKey = "Meteor.userId"; + // Call this from the top level of the test file for any test that does + // logging in and out, to protect multiple tabs running the same tests + // simultaneously from interfering with each others' localStorage. + Accounts._isolateLoginTokenForTest = function () { + loginTokenKey = loginTokenKey + Meteor.uuid(); + userIdKey = userIdKey + Meteor.uuid(); + }; + Accounts._storeLoginToken = function(userId, token) { localStorage.setItem(userIdKey, userId); localStorage.setItem(loginTokenKey, token); @@ -28,27 +36,6 @@ Accounts._storedUserId = function() { return localStorage.getItem(userIdKey); }; - - Accounts._makeClientLoggedOut = function() { - Accounts._unstoreLoginToken(); - Meteor.default_connection.setUserId(null); - Meteor.default_connection.onReconnect = null; - }; - - Accounts._makeClientLoggedIn = function(userId, token) { - Accounts._storeLoginToken(userId, token); - Meteor.default_connection.setUserId(userId); - Meteor.default_connection.onReconnect = function() { - Meteor.apply('login', [{resume: token}], {wait: true}, function(error, result) { - if (error) { - Accounts._makeClientLoggedOut(); - throw error; - } else { - // nothing to do - } - }); - }; - }; })(); // Login with a Meteor access token diff --git a/packages/accounts-password/email_tests.js b/packages/accounts-password/email_tests.js index 7e4aa8df63..bc052c2cc2 100644 --- a/packages/accounts-password/email_tests.js +++ b/packages/accounts-password/email_tests.js @@ -11,6 +11,8 @@ var validateEmailToken; var enrollAccountToken; + Accounts._isolateLoginTokenForTest(); + testAsyncMulti("accounts emails - reset password flow", [ function (test, expect) { email1 = Meteor.uuid() + "-intercept@example.com"; @@ -65,6 +67,7 @@ var getValidateEmailToken = function (email, test, expect) { Meteor.call("getInterceptedEmails", email, expect(function (error, result) { + test.isFalse(error); test.notEqual(result, undefined); test.equal(result.length, 1); var content = result[0]; @@ -77,15 +80,28 @@ })); }; + var waitUntilLoggedIn = function (test, expect) { + var unblockNextFunction = expect(); + var quiesceCallback = function () { + Meteor._autorun(function (handle) { + if (!Meteor.userLoaded()) return; + handle.stop(); + unblockNextFunction(); + }); + }; + return expect(function (error) { + test.equal(error, undefined); + Meteor.default_connection.onQuiesce(quiesceCallback); + }); + }; + testAsyncMulti("accounts emails - validate email flow", [ function (test, expect) { email2 = Meteor.uuid() + "-intercept@example.com"; email3 = Meteor.uuid() + "-intercept@example.com"; Accounts.createUser( {email: email2, password: 'foobar'}, - expect(function (error) { - test.equal(error, undefined); - })); + waitUntilLoggedIn(test, expect)); }, function (test, expect) { test.equal(Meteor.user().emails.length, 1); @@ -96,16 +112,22 @@ getValidateEmailToken(email2, test, expect); }, function (test, expect) { - Accounts.validateEmail(validateEmailToken, expect(function(error) { - test.isFalse(error); - })); - // ARGH! ON QUIESCE!! - Meteor.default_connection.onQuiesce(expect(function () { - test.equal(Meteor.user().emails.length, 1); - test.equal(Meteor.user().emails[0].address, email2); - test.isTrue(Meteor.user().emails[0].validated); + // Log out, to test that validateEmail logs us back in. (And if we don't + // do that, waitUntilLoggedIn won't be able to prevent race conditions.) + Meteor.logout(expect(function (error) { + test.equal(error, undefined); + test.equal(Meteor.user(), null); })); }, + function (test, expect) { + Accounts.validateEmail(validateEmailToken, + waitUntilLoggedIn(test, expect)); + }, + function (test, expect) { + test.equal(Meteor.user().emails.length, 1); + test.equal(Meteor.user().emails[0].address, email2); + test.isTrue(Meteor.user().emails[0].validated); + }, function (test, expect) { Meteor.call( "addEmailForTestAndValidate", email3, @@ -124,15 +146,20 @@ getValidateEmailToken(email3, test, expect); }, function (test, expect) { - Accounts.validateEmail(validateEmailToken, expect(function(error) { - test.isFalse(error); + // Log out, to test that validateEmail logs us back in. (And if we don't + // do that, waitUntilLoggedIn won't be able to prevent race conditions.) + Meteor.logout(expect(function (error) { + test.equal(error, undefined); + test.equal(Meteor.user(), null); })); }, function (test, expect) { - Meteor.default_connection.onQuiesce(expect(function () { - test.equal(Meteor.user().emails[1].address, email3); - test.isTrue(Meteor.user().emails[1].validated); - })); + Accounts.validateEmail(validateEmailToken, + waitUntilLoggedIn(test, expect)); + }, + function (test, expect) { + test.equal(Meteor.user().emails[1].address, email3); + test.isTrue(Meteor.user().emails[1].validated); }, function (test, expect) { Meteor.logout(expect(function (error) { @@ -172,16 +199,13 @@ getEnrollAccountToken(email4, test, expect); }, function (test, expect) { - Accounts.resetPassword(enrollAccountToken, 'password', expect(function(error) { - test.isFalse(error); - })); + Accounts.resetPassword(enrollAccountToken, 'password', + waitUntilLoggedIn(test, expect)); }, function (test, expect) { - Meteor.default_connection.onQuiesce(expect(function () { - test.equal(Meteor.user().emails.length, 1); - test.equal(Meteor.user().emails[0].address, email4); - test.isTrue(Meteor.user().emails[0].validated); - })); + test.equal(Meteor.user().emails.length, 1); + test.equal(Meteor.user().emails[0].address, email4); + test.isTrue(Meteor.user().emails[0].validated); }, function (test, expect) { Meteor.logout(expect(function (error) { @@ -190,9 +214,13 @@ })); }, function (test, expect) { - Meteor.loginWithPassword({email: email4}, 'password', expect(function(error) { - test.isFalse(error); - })); + Meteor.loginWithPassword({email: email4}, 'password', + waitUntilLoggedIn(test ,expect)); + }, + function (test, expect) { + test.equal(Meteor.user().emails.length, 1); + test.equal(Meteor.user().emails[0].address, email4); + test.isTrue(Meteor.user().emails[0].validated); }, function (test, expect) { Meteor.logout(expect(function (error) { diff --git a/packages/accounts-password/passwords_tests.js b/packages/accounts-password/passwords_tests.js index 3add07dc33..33c5182b1b 100644 --- a/packages/accounts-password/passwords_tests.js +++ b/packages/accounts-password/passwords_tests.js @@ -3,6 +3,7 @@ if (Meteor.isClient) (function () { // XXX note, only one test can do login/logout things at once! for // now, that is this test. + Accounts._isolateLoginTokenForTest(); var logoutStep = function (test, expect) { Meteor.logout(expect(function (error) { @@ -11,6 +12,25 @@ if (Meteor.isClient) (function () { })); }; + var verifyUsername = function (someUsername, test, expect) { + var callWhenLoaded = expect(function() { + test.equal(Meteor.user().username, someUsername); + }); + return function () { + Meteor._autorun(function(handle) { + if (!Meteor.userLoaded()) return; + handle.stop(); + callWhenLoaded(); + }); + }; + }; + var loggedInAs = function (someUsername, test, expect) { + var quiesceCallback = verifyUsername(someUsername, test, expect); + return expect(function (error) { + test.equal(error, undefined); + Meteor.default_connection.onQuiesce(quiesceCallback); + }); + }; // declare variable outside the testAsyncMulti, so we can refer to // them from multiple tests, but initialize them to new values inside @@ -33,46 +53,29 @@ if (Meteor.isClient) (function () { }, function (test, expect) { - // XXX argh quiescence + tests === wtf. and i have no idea why - // this was necessary here and not in other places. probably - // because it's dependant on how long method call chains are in - // other tests - var quiesceCallback = expect(function () { - test.equal(Meteor.user().username, username); - }); - Accounts.createUser({username: username, email: email, password: password}, - expect(function (error) { - test.equal(error, undefined); - Meteor.default_connection.onQuiesce(quiesceCallback); - })); + Accounts.createUser( + {username: username, email: email, password: password}, + loggedInAs(username, test, expect)); }, logoutStep, function (test, expect) { - Meteor.loginWithPassword(username, password, expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username); - })); + Meteor.loginWithPassword(username, password, + loggedInAs(username, test, expect)); }, logoutStep, function (test, expect) { - Meteor.loginWithPassword({username: username}, password, expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username); - })); + Meteor.loginWithPassword({username: username}, password, + loggedInAs(username, test, expect)); }, logoutStep, function (test, expect) { - Meteor.loginWithPassword(email, password, expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username); - })); + Meteor.loginWithPassword(email, password, + loggedInAs(username, test, expect)); }, logoutStep, function (test, expect) { - Meteor.loginWithPassword({email: email}, password, expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username); - })); + Meteor.loginWithPassword({email: email}, password, + loggedInAs(username, test, expect)); }, logoutStep, // plain text password. no API for this, have to send a raw message. @@ -87,6 +90,7 @@ if (Meteor.isClient) (function () { })); }, function (test, expect) { + var quiesceCallback = verifyUsername(username, test, expect); Meteor.call( // right password 'login', {user: {email: email}, password: password}, @@ -96,22 +100,21 @@ if (Meteor.isClient) (function () { test.isTrue(result.token); // emulate the real login behavior, so as not to confuse test. Accounts._makeClientLoggedIn(result.id, result.token); - test.equal(Meteor.user().username, username); + Meteor.default_connection.onQuiesce(quiesceCallback); })); }, - // change password with bad old password. + // change password with bad old password. we stay logged in. function (test, expect) { + var quiesceCallback = verifyUsername(username, test, expect); Accounts.changePassword(password2, password2, expect(function (error) { test.isTrue(error); - test.equal(Meteor.user().username, username); + Meteor.default_connection.onQuiesce(quiesceCallback); })); }, // change password with good old password. function (test, expect) { - Accounts.changePassword(password, password2, expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username); - })); + Accounts.changePassword(password, password2, + loggedInAs(username, test, expect)); }, logoutStep, // old password, failed login @@ -123,14 +126,13 @@ if (Meteor.isClient) (function () { }, // new password, success function (test, expect) { - Meteor.loginWithPassword(email, password2, expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username); - })); + Meteor.loginWithPassword(email, password2, + loggedInAs(username, test, expect)); }, logoutStep, // create user with raw password function (test, expect) { + var quiesceCallback = verifyUsername(username2, test, expect); Meteor.call('createUser', {username: username2, password: password2}, expect(function (error, result) { test.equal(error, undefined); @@ -138,16 +140,13 @@ if (Meteor.isClient) (function () { test.isTrue(result.token); // emulate the real login behavior, so as not to confuse test. Accounts._makeClientLoggedIn(result.id, result.token); - test.equal(Meteor.user().username, username2); + Meteor.default_connection.onQuiesce(quiesceCallback); })); }, logoutStep, function(test, expect) { Meteor.loginWithPassword({username: username2}, password2, - expect(function (error) { - test.equal(error, undefined); - test.equal(Meteor.user().username, username2); - })); + loggedInAs(username2, test, expect)); }, logoutStep, // test Accounts.validateNewUser @@ -173,10 +172,13 @@ if (Meteor.isClient) (function () { }, // test Accounts.onCreateUser function(test, expect) { - Accounts.createUser({username: username3, password: password3}, - {testOnCreateUserHook: true}, expect(function () { - test.equal(Meteor.user().profile.touchedByOnCreateUser, true); - })); + Accounts.createUser( + {username: username3, password: password3}, + {testOnCreateUserHook: true}, + loggedInAs(username3, test, expect)); + }, + function(test, expect) { + test.equal(Meteor.user().profile.touchedByOnCreateUser, true); }, // test Meteor.user(). This test properly belongs in diff --git a/packages/accounts-ui-unstyled/login_buttons.html b/packages/accounts-ui-unstyled/login_buttons.html index b788025692..3c08ee98c2 100644 --- a/packages/accounts-ui-unstyled/login_buttons.html +++ b/packages/accounts-ui-unstyled/login_buttons.html @@ -13,10 +13,10 @@ {{> loginButtonsLoggedInDropdown}} {{else}}
- {{#if currentUser.loading}} {{! XXX this will change }} -
- {{else}} + {{#if currentUserLoaded}} {{displayName}} + {{else}} +
{{/if}}
Logout
diff --git a/packages/accounts-ui-unstyled/login_buttons_dropdown.html b/packages/accounts-ui-unstyled/login_buttons_dropdown.html index 8e02ff8523..29830be9b4 100644 --- a/packages/accounts-ui-unstyled/login_buttons_dropdown.html +++ b/packages/accounts-ui-unstyled/login_buttons_dropdown.html @@ -3,9 +3,7 @@