-
Notifications
You must be signed in to change notification settings - Fork 937
Propagate customData in FirebaseError when the user is disabled. #6289
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
Conversation
🦋 Changeset detectedLatest commit: 21286ad The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
let error: FirebaseError | null = null; | ||
try { | ||
await signInWithIdp(auth, request); | ||
} catch (e) { | ||
error = e; | ||
} | ||
expect(error!.customData!.email).to.eq("[email protected]"); | ||
expect(error!.code).to.eq( | ||
'auth/user-disabled' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use chai-as-promised to put all of this error testing logic into one assertion phrase. From ap/index.test.ts for example:
firebase-js-sdk/packages/auth/src/api/index.test.ts
Lines 178 to 183 in b6196d2
await expect(promise) | |
.to.be.rejectedWith(FirebaseError, 'auth/credential-already-in-use') | |
.eventually.with.deep.property('customData', { | |
appName: 'test-app', | |
_tokenResponse: response | |
}); |
i.e.
await expect(signInWithIdp(auth, request)).to.be.rejectedWith('auth/user-disabled')
.eventually.with.deep.property('customData', {email: '[email protected]'});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you attach a changeset and add an internal bug number or link to a Github issue to the PR description? Overall LGTM pending nits
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text string LG, thanks!
f7f0ea9
to
b304eeb
Compare
Thanks! I am having trouble reproing the fix in the demo app now. I will submit the changeset once I resolve that. |
SG, thanks! Giving a preemptive LGTM in the meantime. Thanks for the fix!
|
b304eeb
to
e980360
Compare
Internal bug - b/229774279