Closed espy closed 7 years ago
Thanks for those, @mbad0la. As for the signUp
, I’ll leave it out, we might have situations later where one user can sign up other users, plus it doesn’t break anything if we allow this.
I’d suggest when the last typo is fixed, use the Squash and merge
button with the commit message "fix: prevent signIn if session present".
This is just fyi, I know it’s rather complex: the last 3 commit messages should have used test:
as they only touch test files. All commit messages with a fix:
commit will appear in the change logs for the users of this library, and for them "more descriptive test name" or "typo" is not very helpful. Does that make sense?
Great review @mbad0la 👍
This is just fyi, I know it’s rather complex: the last 3 commit messages should have used test: as they only touch test files. All commit messages with a fix: commit will appear in the change logs for the users of this library, and for them "more descriptive test name" or "typo" is not very helpful.
I was confused about this myself. Thanks for stepping in @gr2m
fix: prevent signIn if session present"
Is this accurate? The fix will prevent the propagation of the signin
event, but not the signIn
method when cache.username === options.username
The fix will prevent the propagation of the signin event, but not the signIn method when cache.username === options.username
I’m not sure what you mean with it will not prevent the sign in method? What happens is that if the curernt user already has a session and tries to sign in with a different username than the current who they are signed in with, then don’t do anything (no events, no change of state) and reject with an explanatory error message
I think this is good now, are you happy with the changes, @mbad0la ? If you approve the changes, I can squash and merge :)
Thanks again!
LGTM @espy :+1:
Closes #154 by checking on signIn whether a session exists, and whether that session belongs to anyone but the user trying to sign in. :no_good_man:
Oh, I didn’t touch
signUp
for now, although it’s mentioned in the issue.Ready for review @hoodiehq/maintainers !