Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully fixed the $logout() bug #438

Merged
merged 1 commit into from
Oct 16, 2014
Merged

Fully fixed the $logout() bug #438

merged 1 commit into from
Oct 16, 2014

Conversation

jwngr
Copy link

@jwngr jwngr commented Oct 16, 2014

@katowulf - This is my second attempt at fixing #431. My fix last time made the login event not fire after a user logs out. This is because of some hacky code we had in place to suppress multiple logout events firing. This code is not very easy to understand and is actually not necessary anymore. I added a check to logout to not fire the logout event if no user is actually logged in. This seems logical to me and I'm surprised Simple Login itself doesn't do this. This is not a breaking change since the behavior from calling $logout() twice was already busted.

Note that this._currentUserData will get set to null within _onLoginEvent() which will get fired by the call to Simple Login's logout(). As a result, we no longer need to set it to null within $logout() itself.

cc/ @davideast

@jwngr
Copy link
Author

jwngr commented Oct 16, 2014

Note that I've tested this pretty heavily locally and it seems to be behaving as I would expect. Event upon refresh, the correct events are being fired if you are logged in or if you are logged out.

@jwngr jwngr mentioned this pull request Oct 16, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling c434ffc on jw-logout-fix-2 into 006135f on master.

katowulf added a commit that referenced this pull request Oct 16, 2014
@katowulf katowulf merged commit 191c0b1 into master Oct 16, 2014
@katowulf katowulf deleted the jw-logout-fix-2 branch October 16, 2014 20:30
@katowulf
Copy link
Contributor

☃☃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants