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

test: admin.signOut() throws error when signed out #113

Merged
merged 3 commits into from Aug 8, 2016

Conversation

capellini
Copy link
Member

admin.signOut() throws TypeError: Cannot read property 'session' of null when a user is not signed in. This commit contains documentation and failing unit/integration test updates in order to catch the condition where a user has attempted to sign out without being signed in and, rather than throwing the current error, throw an UnauthenticatedError: Not signed in error, instead.

closes hoodiehq/camp#12

admin.signOut() throws `TypeError: Cannot read property 'session' of null` when a user is not signed in.  This commit contains documentation and failing unit/integration test updates in order to catch the condition where a user has attempted to sign out without being signed in and, rather than throwing the current error, throw an `UnauthenticatedError: Not signed in` error, instead.
Fix implemented.  Unit and integration tests pass.

// simulate user who has not yet signed in
store.clear()
store.setObject('account', {})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When simulating a user who is not signed in, Is it sufficient to set the account object to an empty object literal, or should there be more items in the object? From the code, it seems that having a session object inside of the account object implies that the user is logged in, so I wouldn't want to add a session object, but I didn't know if any other information is necessary in the account object to simulate a user who is not signed in.

Copy link
Member

@gr2m gr2m Aug 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the store.clear() should be sufficient here. The account.signOut() call should not require account to be stored as an empty {} in the store. Compare for example to the .signIn() integration test


function signOut (state) {
if (!internals.isSignedIn(state)) {
return Promise.reject(new Error('UnauthenticatedError: Not signed in'))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me 👍

1. Only clearing store and not creating an empty account object, as one is not required for `account.signOut()`.
2. Removing `id` from `account` instance, as it's not required for this test and one will be randomly assigned.
@capellini
Copy link
Member Author

@gr2m Thanks for your guidance. Corrections made.

@varjmes
Copy link
Contributor

varjmes commented Aug 8, 2016

LGTM ✨

@gr2m
Copy link
Member

gr2m commented Aug 8, 2016

Great work, thanks @capellini 👏 LGTM

@gr2m gr2m merged commit 2dd965e into hoodiehq:master Aug 8, 2016
@gr2m
Copy link
Member

gr2m commented Aug 8, 2016

released as v4.1.1

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.

admin.signOut() throws "TypeError: Cannot read property 'session' of null"
3 participants