-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat: Pass event information to verifyUserEmails
#9651
base: alpha
Are you sure you want to change the base?
Conversation
🚀 Thanks for opening this pull request! |
verifyUserEmails
The description doesn't sound like the change that has been described in the feature. The feature was only to pass more information to the method, not to change when it will be invoked. Could you please check. |
Hey @mtrezza just wanted to confirm that i am on the right track like not missing out something, i will revert this changes and only pass more information such as {action, authProvider} to the method. Something more like |
@mtrezza awaiting for your suggestion! |
This looks strange, because I also suggest to update the PR description, because it seems to describe changes that are neither part of the issue, nor will they be included in this PR. |
@mtrezza Hope this changes serves what has been asked for let me know if there is any area for improvement or suggestion |
6655eb3
to
1a01272
Compare
Hello @mtrezza any update on the latest push? |
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 overall, just a few changes
Signed-off-by: Vishal Vishwakarma <[email protected]>
@mtrezza sorry for the formatting issue hope this will be the final push! If any changes please suggest |
Hello @mtrezza what should be the next step? |
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.
The tests could be more robust and e2e. Currently they are testing very specific parts of the code path. Testing with toHaveBeenCalledWith
doesn't consider the order of method arguments. An e2e test would be calling reconfigureServer
and setting the verifyUserEmails
option to a function, then sign up / log in a user and making sure the function is called and the 2nd argument is the authContext with the expected value. I think if you structure the tests well then you only need to call reconfigureServer
once in a beforeAll
and then do the logins / signups in individual it
s.
Signed-off-by: Manuel <[email protected]>
Signed-off-by: Manuel <[email protected]>
Pull Request
Issue
Closes: #9505
What I Did
Added authContext support in getUserController to pass authentication details.
Updated setEmailVerifyToken to include authContext when determining whether email verification is required.
Approach
Modified getUserController to accept authContext and pass it to UserController.
Updated setEmailVerifyToken to check this.authContext when calling verifyUserEmails.
Ensured email verification is only triggered based on the authentication context
@mtrezza Just passed event information to verifyUserEmails function as mentioned in the feature!