FIX firing onLogin twice

[brucejo75]

Fixes #10853 & #11446.

This was introduced by #10543 which fixed #10157.

I was cleaning up my github tasks and noticed that this PR was referenced in @namenotrequired comment. Someone should have mentioned me in the thread I would have responded earlier!

Problem: I used this.connection._userId as a test for whether the user was logged in. I should have used Meteor.user().

Sorry I goofed it up! This should fix it.

[jasongrishkoff]

If I'm not mistaken, Meteor.user() on the server fetches the full user document from the users collection? I know the recommended practice is not to store everything on the user document, but some of us have done it anyway, and these documents can become quite large. Perhaps something like Meteor.userId() would be a bit better?

[brucejo75]

@jasongrishkoff, I do not like that call either. But...

• I essentially was using this.connection._userId which is equivalent to Meteor.userId() and that is what broke it. A login is not apparently complete until the users collection is ready.
• on the server _startupCallback is a noop.
• this is only used in client code so all minimongo calls

[brucejo75]

@jasongrishkoff,

I just looked at using Meteor.user({fields: {_id: 1}}) but the projection calculation in minimongo is more costly than just returning the resulting doc. So simpler and better to use Meteor.user().

[brucejo75]

@jasongrishkoff, thanks for giving me a little push.

I was able to repurpose a local variable called, that is set & used in the loginCallback function. With that I have definitive information on when the callbacks have been called.

I created a new member variable _loginCallbacks_called that replaces called and then it can be used in the _startupCallback method to determine if callbacks have already been called.

[brucejo75]

@filipenevola, @StorytellerCZ, Can you bump this so it will rerun the checks?

I did a commit update while it was running checks. I think that borked it.
Thanks!

[brucejo75]

@filipenevola, @StorytellerCZ,

Are the build tools somehow messed up? I am getting a stalled build. No test errors.

[filipenevola]

Hi @brucejo75 , no, they are ok.

I'm re-running. Let's see.

[brucejo75]

@filipenevola,
Note: Travis is failing after passing 685 tests.

I ran the desktop version of the tests ./meteor package-tests.
At first it says it is running 685 tests. All pass then the number of tests jumps to 1503 and it hangs. No more tests appear to be running.

As a test I checked out release-2.5.1 and ran the tests:
These tests failed:

tinytest - email - fully customizable
tinytest - email - undefined headers sends properly
tinytest - email - multiple e-mails same stream
tinytest - email - using mail composer
tinytest - email - date auto generated
tinytest - email - long lines
tinytest - email - unicode
tinytest - email - text and html

And like running the tests on the devel branch it hangs after running 685 tests.

[brucejo75]

@filipenevola,
The tests just started working on my desktop and here. Maybe a required service was hung up?

Ready to check in. I would value a review of the code. In addition to the regression tests I have been using the updated code and I no longer see multiple onLogin firings. Plus, if an onLogin is called after login callbacks have been called, the callback is immediately executed.

It is remotely possible that my last checkin fixed something that was borking the tests. Dunno!

Thanks!

[stephko90]

Is there a way to bump this PR to try and get it into the 2.5.2 release? I'm running into an issue locally caused by this bug using the 2.5.1 build that this PR would resolve.

[StorytellerCZ]

LGTM

[brucejo75]

@filipenevola, @StorytellerCZ , any chance 2.6 could sweep this in?

[StorytellerCZ]

@renanccastro can you take a look. Might be a good candidate to be added to 2.5.4 as well.