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

Sign-up Page Functionality #202

Merged
merged 9 commits into from
Nov 23, 2023
Merged

Conversation

kevin-pierce
Copy link
Contributor

@kevin-pierce kevin-pierce commented Nov 16, 2023

Notion task link

Signup Page Functionality

Implementation description

  • Added two exceptions ; UserNotInvitedException and EmailAlreadyInUseException with corresponding error messages
  • Added 3 error states for email (invalid email, user not invited, email already in use)
  • Cleaned up dead code (unused Signup.tsx file)
  • Restyled Signup.tsx form to use Chakra built-in components

Steps to test

Invalid email

  1. Attempt registration flow with invalid email (for example, testatshouldnotwork.com)
  2. Verify that we see an "Invalid email" error appear below the email entry field
  3. Verify that when we FIRST enter an invalid email, that we DO NOT see the error appear ; we should only see this type of error appear AFTER we have clicked the signup button

User Not Invited

  1. Attempt to register a user with an email that has not been invited (doesn't exist in your database)
  2. Verify that we see an "Email not invited" error appear below the email entry field

Email Already in Use

  1. Attempt to register a user with an email already associated with an Active account
  2. Verify that we see an "Email already in use" error appear below the email entry field

General

  • Verify that the sign up button remains disabled when any of the fields are blank
  • Verify that the sign up button remains disabled when an invalid email is present
  • Verify that successful registration flow operates as normal

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
  • If I have made API changes, I have updated the REST API Docs
  • IF I have made changes to the db/models, I have updated the Data Models Page
  • I have documented this PR in the Production Release Page
  • I have updated other Docs as needed

Sorry, something went wrong.

Copy link

github-actions bot commented Nov 16, 2023

Visit the preview URL for this PR (updated for commit fb2dcf3):

https://blueprintsupportivehousing--pr202-kevin-signup-functio-hto1ingq.web.app

(expires Thu, 30 Nov 2023 01:34:28 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09

@kevin-pierce kevin-pierce marked this pull request as ready for review November 19, 2023 19:23
@@ -3,6 +3,7 @@
InvalidPasswordException,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this needs to be addressed in this PR, but I was trying to sign up with an account that is "Invited" but clicking "Create Account" didn't do anything. I looked in the networks tab and found that the register endpoint failed with the error: "Invalid password string. Password must be a string at least 6 characters long." This could be unintuitive for the users since it didn't show an error box so they don't know that this password character limit exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved ; I approached this by offloading the password verification to the frontend (by simply doing a length check to verify the password is >= 6 characters long). This check is performed both in the onClick handler for createAccount and within the handlePasswordChange function (which functions the same way as the valid email error does, in which we only check for the validity of these fields after the create account button has been clicked)

Copy link
Contributor

@braydonwang braydonwang left a comment

Choose a reason for hiding this comment

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

Besides the comment I left, everything else looks good and works well!

JSON.stringify(authUser),
);
setAuthenticatedUser(authUser);
if (isInvited !== "Not Invited") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above check would trigger as long as some string was being returned here (which this function always did, even in the error case) hence we just changed the logic around

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain what you mean by this? Do you mean that it would show not invited even if an invalid string was passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, before the condition was simply if (isInvited). However, the function call was returning (even in the error case) the string Not Invited. This meant that, even when we were supposed to be erroring out, we were interpreting the API call as successful since if (isInvited) would be true if the string was not empty

Comment on lines +4 to +9
export const getAuthErrMessage = (axiosErrRes: AxiosError["response"], flow: AuthFlow): string => {
if (axiosErrRes && axiosErrRes.data && axiosErrRes.data.error) {
return axiosErrRes.data.error;
}
return `Error ${flow === 'LOGIN' ? "logging in" : "signing up"}. Please try again later.`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Takes in the auth flow (either 'LOGIN' or 'SIGNUP') from which we're accessing this method, as to consolidate the two functiosn we had before into one

Comment on lines -29 to -34
const isLoginErrorResponse = (
res: AuthTokenResponse | ErrorResponse,
): res is ErrorResponse => {
return res !== null && "errCode" in res;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a more comprehensive version of this function isAuthErrorResponse

Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

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

muy bien

Copy link
Contributor

@danielk1345 danielk1345 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, and it works as expected. Only thing that I kind of struggled to understand was one of your comments

@kevin-pierce kevin-pierce merged commit e123bf5 into main Nov 23, 2023
@kevin-pierce kevin-pierce deleted the kevin-signup-functionality-new branch November 23, 2023 02:06
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.

None yet

4 participants