-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/adhithya christen/firebase auth, sign up routes #40
Conversation
…b.com/TritonSE/ALUM-Mobile-Application into feature/AdhithyaChristen/FirebaseAuth Adding mentor.ts to models
…b.com/TritonSE/ALUM-Mobile-Application into feature/AdhithyaChristen/FirebaseAuth "Adding route files"
…r bad strings/duplicated email
…r bad strings/duplicated email
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 route works for me properly. The error checking messages and mentor/mentee schema will need to be updated, but the route is good to be used for developing the sign-up flow on front-end.
* Have to do this workaround since lint doesn't let | ||
* us export vars | ||
*/ | ||
const port = portV; |
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.
I had to add this thing to work around the linting error. Not an important issue right now but if there is a better workaround this, that would be nice because it is a bit messy.
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.
Gotcha! Note to resolve this in the clean backend setup task we will make
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.
build(attr: MentorInterface): MentorDoc; | ||
} | ||
|
||
const mentorSchema = new mongoose.Schema({ |
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.
After looking at Figma I realize that we need to update mentor and mentee schema to store additional values (images, major, interests, etc.). Not important now but it will be updated in future PR's.
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.
It is kinda important so that the the frontend can be properly integrated with the Signup flow. Since this shouldn't take too long, can we add the fields?
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.
to be resolved - #46
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.
All looks good to me, there still seems to be lint errors in user.ts but if those console messages are supposed to be present then everything is good!
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.
Great work setting up all the auth stuff! Left some comments on finishing up the sign-up route but other than that looks good. Please ping on slack when you address the changes so that I can approve
@@ -68,6 +68,7 @@ | |||
); | |||
path = ALUM; | |||
sourceTree = "<group>"; | |||
usesTabs = 0; |
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.
what is this?
let serviceAccountKeyV = ""; | ||
|
||
/** | ||
* Todo: these should throw errors instead of logging messages |
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.
Noting to make an issue for this task
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.
* Have to do this workaround since lint doesn't let | ||
* us export vars | ||
*/ | ||
const port = portV; |
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.
Gotcha! Note to resolve this in the clean backend setup task we will make
* This class is used to contain any error messages pertaining | ||
* to validating inputs. | ||
*/ | ||
const INVALID_EMAIL_ID = "Invalid email was found, email must not have @iusd.org."; |
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.
An issue I faced with this approach in LAKTAA was that as we developed more, we had a lot of error messages so maintaining all of them like this became a pain. Maybe something to note for future
backend/src/routes/routes.ts
Outdated
@@ -1,19 +1,19 @@ | |||
import express, { Request, Response } from "express"; | |||
import { User } from "../models/users"; | |||
// import express, { Request, Response } from "express"; |
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.
will we need this commented out code in future?
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.
so right, we don't need this anymore
* | ||
* Mentee: {type: string, name: string, email: string,password: string} | ||
*/ | ||
router.post( |
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.
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.
add grade, topics of interest, career interests, and what do you hope question
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.
build(attr: MentorInterface): MentorDoc; | ||
} | ||
|
||
const mentorSchema = new mongoose.Schema({ |
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.
It is kinda important so that the the frontend can be properly integrated with the Signup flow. Since this shouldn't take too long, can we add the fields?
@@ -0,0 +1,31 @@ | |||
/** |
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.
Awesome!! Creating a service for auth and firebase is a great idea. These organizational things might seem not important right now but as our codebase increases in size, its usefulness will become clearer.
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.
Great work! Let's merge this PR. There are some tasks pending on the mentor/mentee sign-up that we need to resolve as backlog. Can you make 1 task to resolve all this?
- Add email regex check for mentor as well
- See if logic can be reused between mentor and mentee routes
- Add fields from figma so that Sign-Up Flow is done on backend
Tracking Info
Resolves #27
Make sure your branch name conforms to:
<feature|staging|bugfix|...>/<username>/<3-4 word description separated by dashes>
. Otherwise, please rename your branch and create a new PR.Changes
We implemented POST routes for both mentee and mentor with error handling. We also connected our routes to Firebase, so for every user created, Firebase will also store their data along with a personal access token.
Testing
We ran test requests to Postman and checked to ensure the data was actually being posted on MongoDB, which it was.
Confirmation of Change
Validation functions for routes can be found in the middleware folder validation.ts
Error handling changes can be found in the errors folder, errors.ts, validationError.ts.
Firebase connections can be found in the services folder, auth.ts, firebase.ts
Routes can be found in the routes folder, routes.ts
Updated mentee/mentor document models can be found in models, mentee.ts, mentor.ts