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.

Sign up for GitHub

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

Jump to bottom

Update createUser function #41

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Update createUser function #41

merged 1 commit into from
Oct 29, 2024

Conversation

sthuray
Copy link
Contributor

@sthuray sthuray commented Oct 22, 2024 •
edited
Loading

Notion ticket link

Ticket Name

Implementation description

  • Fixed Types and CreateUserDtoValidator
  • Removed status from CreateUserDTO (redundant)

Steps to test

  1. Use user endpoint to post a new user
Screenshot 2024-10-21 at 8 08 48 PM 2. Ensure user is created in firebase

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@sthuray sthuray requested a review from jerry-cheng5 October 22, 2024 00:17
jerry-cheng5
jerry-cheng5 reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@jerry-cheng5 jerry-cheng5 left a comment

Choose a reason for hiding this comment

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

This looks good!

Did you confirm that the user is created in firebase? Also, just to be safe, try and trigger a reset password flow from the firebase auth dashboard to see if it still works on a user without a password set.

backend/typescript/services/implementations/userService.ts Show resolved Hide resolved
backend/typescript/services/implementations/authService.ts Show resolved Hide resolved
@jerry-cheng5 jerry-cheng5 requested review from trinity-y and removed request for trinity-y October 22, 2024 23:12
jerry-cheng5
jerry-cheng5 approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@jerry-cheng5 jerry-cheng5 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!

@sthuray sthuray added this pull request to the merge queue Oct 29, 2024
Merged via the queue into main with commit fe50390 Oct 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerry-cheng5 jerry-cheng5 jerry-cheng5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sthuray @jerry-cheng5