-
Notifications
You must be signed in to change notification settings - Fork 84
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
fix: improve debugging/DX when enabling MFA #822
Conversation
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.
👍 Looks good to me!
- Reviewed the entire pull request up to eb6a599
- Looked at
181
lines of code in9
files - Took 1 minute and 21 seconds to review
More info
- Skipped
1
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. lib/ts/recipe/multifactorauth/api/implementation.ts:110
:
- Assessed confidence :
10%
- Comment:
The error message here is clear and provides useful information for debugging. Good job on improving the error handling. - Reasoning:
The changes in the PR seem to be mostly about improving error handling and providing more informative error messages. The code changes in the files 'authUtils.ts', 'implementation.ts', and 'implementation.js' are about adding more detailed error messages and throwing errors in certain situations. The changes in the files 'version.d.ts', 'version.js', and 'version.ts' are about updating the version number of the software. The change in 'package.json' is also about updating the version number. There don't seem to be any logical bugs, performance bugs, or security bugs in the changes. The changes seem to follow best practices.
Workflow ID: wflow_Rw1l7LpWj2poeqRY
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Hey @ellipsis. Please review this |
OK! Reviewing this PR... Responding to this comment by @rishabhpoddar. For more information about Ellipsis, check the documentation. |
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.
👍 Looks good to me!
- Performed an incremental review on bddfae3
- Looked at
83
lines of code in2
files - Took 2 minutes and 9 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. test/mfa/mfa.api.test.js:229
:
- Assessed confidence :
10%
- Comment:
The new test case seems to be correctly implemented and checks the expected behavior when the user is in a stuck state due to configuration issues. - Reasoning:
The changes in the test files seem to be correctly implemented. The new test case added in mfa.api.test.js checks if the MFA info endpoint throws an error when the user is in a stuck state due to configuration issues. The changes in utils.js allow the getMfaInfo function to expect different status codes, not just 200, which is necessary for the new test case.
Workflow ID: wflow_ycOE5UAwN8RXiWs7
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Summary of change
Related issues
Test Plan
Existing tests already cover this. (E2E tests needed an update in related auth-react PR)
Documentation changes
N/A
Checklist for important updates
coreDriverInterfaceSupported.json
file has been updated (if needed)lib/ts/version.ts
frontendDriverInterfaceSupported.json
file has been updated (if needed)package.json
package-lock.json
lib/ts/version.ts
npm run build-pretty
recipe/thirdparty/providers/configUtils.ts
file,createProvider
function.git tag
) in the formatvX.Y.Z
, and then find the latest branch (git branch --all
) whoseX.Y
is greater than the latest released tag.add-ts-no-check.js
file to include thatsomeFunc: function () {..}
).Summary:
This PR improves the debugging and developer experience when enabling Multi-Factor Authentication (MFA) by enhancing error messages and handling user stuck states due to configuration issues.
Key points:
lib/ts/authUtils.ts
andlib/build/authUtils.js
to indicate potential issues with account linking.lib/ts/recipe/multifactorauth/api/implementation.ts
andlib/build/recipe/multifactorauth/api/implementation.js
to handle user stuck state due to configuration issues.lib/build/version.d.ts
,lib/build/version.js
,lib/ts/version.ts
, andpackage.json
.Generated with ❤️ by ellipsis.dev
Summary:
This PR improves the debugging and developer experience when enabling Multi-Factor Authentication (MFA) by enhancing error messages, handling user stuck states due to configuration issues, and updating the version to 17.0.4.
Key points:
lib/ts/authUtils.ts
andlib/build/authUtils.js
to indicate potential issues with account linking.lib/ts/recipe/multifactorauth/api/implementation.ts
andlib/build/recipe/multifactorauth/api/implementation.js
to handle user stuck state due to configuration issues.lib/build/version.d.ts
,lib/build/version.js
,lib/ts/version.ts
, andpackage.json
.Generated with ❤️ by ellipsis.dev