Feature/implement user data routes #63
Conversation
| try { | ||
| res.status(200).send("Get professional information route works!"); | ||
| const user = await User.findOne({ firebaseId: req.user?.firebaseId }).select("professional"); |
There was a problem hiding this comment.
I see this line of code across multiple controller functions where you do look up for a user by firebaseId. Maybe worth abstracting this away to a common helper function?
| } catch (error) { | ||
| next(error); | ||
| console.error("Error updating personal information:", error); | ||
| //Either handle directly or next(error) |
There was a problem hiding this comment.
seems like you consider it here already but I would recommend going with next(error) since that allows it to be handled globally instead of preemptively returning a 500 error.
| } | ||
| }; | ||
|
|
||
| export const editPersonalInformation: RequestHandler = async (req, res, next) => { | ||
| export const editPersonalInformation = async (req: AuthenticatedRequest, res: Response, next: NextFunction) => { |
There was a problem hiding this comment.
Maybe we could use typescript types here for the request body? See example below:
interface PersonalInfoUpdate {
firstName: string;
lastName: string;
email: string;
phone?: string;
}
export const editPersonalInformation = async (
req: AuthenticatedRequest & { body: PersonalInfoUpdate },
res: Response,
next: NextFunction
) => { ... }
| } = req.body; | ||
|
|
||
| // Validate required fields | ||
| if (!firebaseId || !account || !personal || !personal.firstName || !personal.lastName || !personal.email) { |
There was a problem hiding this comment.
since we have the validator to validate the request don't think this is needed.
| const { name, email } = req.body as { name: string; email: string }; | ||
| if (!name || !email) { | ||
| res.status(400).json({ error: "Name and email are required" }); | ||
| // const { name, email } = req.body as { name: string; email: string }; |
|
|
||
| export const createUser = (req: Request, res: Response) => { | ||
| export const createUser = async (req: Request, res: Response, next: NextFunction) => { |
There was a problem hiding this comment.
Create a type for the req specific this request or
https://github.com/TritonSE/PAP-Inventory-Processing/blob/main/backend/src/controllers/user.ts
in PAP there is on specific to SPLAGEN
I recal in someone's PR a SPlagenREquest type is created
| display, | ||
| }); | ||
| await newUser.save(); | ||
|
|
||
| res.status(201).json({ message: "User created successfully", user: newUser }); |
There was a problem hiding this comment.
don't think we'll need the message since we'll know with the user data the user is cretaed
| res.status(201).json({ message: "User created successfully", user: newUser }); | ||
| } catch (error) { | ||
| console.error("Error creating user:", error); | ||
| //Either handle directly or next(error) | ||
| res.status(500).json({ error: "Internal server error" }); |
There was a problem hiding this comment.
call the next with the error for the error handling
something like
catch (error) { next(error); }
| try { | ||
| res.status(200).send("Get personal information route works!"); | ||
| const user = await User.findOne({ firebaseId: req.user?.firebaseId }).select("personal"); |
There was a problem hiding this comment.
why is this one a different appraoch from above?
uid should be passed in with the request like body of request or params
with the req.user?.firebaseId are you handle case where user doesn't exist?
| import { Request } from "express"; | ||
|
|
||
| export interface AuthenticatedRequest extends Request { | ||
| user?: { |
There was a problem hiding this comment.
why is user optional? should we not have a user if is an authenticated request?
firebaseId should suffice for finding the user to get the correspondign information
do we need the extra user wrapper?
|
|
||
| const updatedUser = await User.findOneAndUpdate( | ||
| { firebaseId: req.user?.firebaseId }, | ||
| { "personal.firstName": firstName, "personal.lastName": lastName, "personal.email": email, "personal.phone": phone }, |
There was a problem hiding this comment.
directory info should not use the personal.phone
This is the information to be retrieved by this route
| try { | ||
| res.status(200).send("Get directory personal information route works!"); | ||
| const user = await User.findOne({ firebaseId: req.user?.firebaseId }).select("personal"); |
There was a problem hiding this comment.
there should be a different section in the schema for the directory info (if not make one or just pull and return the necessary information). Shouldn't be the personal one.
Tracking Info
Resolves #60
Changes
Implements the Controller and Validator functions for
Get Personal Information
Edit Personal Information
Get Professional Information
Edit Professional Information
Get Directory Personal Information
Edit Directory Personal Information
Get Directory Display Info
Edit Directory Display Info
using the precise fields in our User model and applying the firebaseId in place of standard id as needed. Middleware auth is assumed as part of a separate issue for the purposes of this code.
Testing
The right middleware needed to test this is part of a separate Firebase feature which I am also working on. In terms of testing, it would be helpful to have some mock data perhaps to speed things up; the code looks correct but more testing is needed. I currently do not have the MongoDB credentials, after which I could try testing this code more carefully.