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

Reset password and verification email to public routes #1766

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

NolanTrem
Copy link
Collaborator

@NolanTrem NolanTrem commented Jan 7, 2025

Important

Make password reset and verification email functionalities public by updating API routes, backend logic, client SDK, and tests.

  • API Changes:
    • Made /users/send-verification-email and /users/reset-password public in users_router.py.
    • Updated /users/verify-email to return WrappedGenericMessageResponse.
  • Backend Logic:
    • Added send_verification_email method in auth.py and r2r_auth.py.
    • Modified register method in r2r_auth.py to use send_verification_email.
  • Client SDK:
    • Added sendVerificationEmail method in users.ts and users.py.
    • Updated package.json version to 0.4.10.
  • Testing:
    • Added test Request verification email in UsersIntegrationSuperUser.test.ts.

This description was created by Ellipsis for 5292872. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 everything up to 32158fd in 1 minute and 49 seconds

More details
  • Looked at 449 lines of code in 11 files
  • Skipped 2 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. js/sdk/__tests__/UsersIntegrationSuperUser.test.ts:76
  • Draft comment:
    Consider adding an assertion to check the expected message in the response to ensure the email was sent successfully.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test for sending a verification email is missing an assertion for the expected message in the response. This is important to ensure that the correct message is returned, indicating that the email was sent successfully.
2. js/sdk/src/v3/clients/users.ts:84
  • Draft comment:
    The sendVerificationEmail function should send the email as an object with an email key.
      data: { email: options.email },
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests restructuring the data, but there's no clear evidence this is needed. The function already receives an object with an email field. Other similar email-related functions in this file pass the options object directly. Without seeing the API spec or server code, we can't be certain the suggested change is correct.
    I could be wrong if the API specifically requires the email to be nested under an email key. The comment author may have knowledge of the API requirements that isn't visible in this code.
    While possible, we should err on the side of assuming the code is correct unless there's clear evidence otherwise. The consistent pattern across similar functions suggests the current implementation is correct.
    Delete the comment since there's no strong evidence the suggested change is necessary, and the current implementation follows the pattern used by similar functions in the codebase.
3. py/sdk/v3/users.py:91
  • Draft comment:
    The send_verification_email method should send the email as an object with an email key.
            json={"email": email},
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_Ww6Y9MI7mEGnbtV7


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 5292872 in 43 seconds

More details
  • Looked at 66 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/main/api/v3/users_router.py:337
  • Draft comment:
    The code sample for the verify_email function still uses verify_email instead of send_verification_email. This should be updated to reflect the changes made in the PR.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_biILigsgsLzCgxt5


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@NolanTrem NolanTrem merged commit e2c1804 into main Jan 7, 2025
12 of 14 checks passed
@NolanTrem NolanTrem deleted the Nolan/ResetPasswordVerificationEmail branch January 7, 2025 09:12
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.

1 participant