Cloudinary Setup/IN #51
Conversation
There was a problem hiding this comment.
Great work! Left you some comments, happy to see you did some data validation.
One piece of advice regarding the PR template, if you aren't using a section then remove it. And be a bit more descriptive in it!
Also, this was more back-end (BE) work rather than integration (IN)
server/controllers/profile.js
Outdated
| // @route POST /profile/create | ||
| // @desc Create new profile | ||
| // @access Public |
There was a problem hiding this comment.
Good job on documentation!
server/controllers/profile.js
Outdated
| try { | ||
| const { | ||
| firstName, | ||
| lastName, | ||
| gender, | ||
| birthday, | ||
| email, | ||
| phoneNumber, | ||
| location, | ||
| profilePic, | ||
| description, | ||
| availability, | ||
| } = req.body; | ||
| const profile = await Profile.create({ | ||
| firstName, | ||
| lastName, | ||
| gender, | ||
| birthday, | ||
| email, | ||
| phoneNumber, | ||
| location, | ||
| profilePic, | ||
| description, | ||
| availability, | ||
| }); | ||
|
|
||
| res.status(201).json({ | ||
| profile, | ||
| }); | ||
| } catch (err) { | ||
| res.status(500); | ||
| throw new Error("Something went wrong, please try again"); | ||
| } |
There was a problem hiding this comment.
So, the try/catch here is actually not needed. Now, in other applications maybe but in this one we utilize express-async-handler which actually will catch thrown errors and respond with JSON instead of crashing the application. This actually doesn't give us any context into an error that occurs.
There was a problem hiding this comment.
Please remove in all applicable cases - now, let me mention. If we're try/catch for specific errors that are expected then of course we can keep that in!
There was a problem hiding this comment.
@moffatethan My apologies, this was actually a mix up from an old version. You have made the correction in profile controllers and I have made the changes already.
Well noted!
server/controllers/profile.js
Outdated
| description, | ||
| availability, | ||
| }, | ||
| { new: true } |
server/controllers/profile.js
Outdated
| res.status(400); | ||
| throw new Error("Failed to upload photo, ensure that you have selected a valid file format") | ||
| } | ||
| const { secure_url } = await cloudinary.uploader.upload(file.path, { |
There was a problem hiding this comment.
Nice destructuring here and utilizing folders for organization!
There was a problem hiding this comment.
I just want to add here for future cases that we can often leverage the : syntax and rename destructured fields to better fit our conventions (example).
server/controllers/profile.js
Outdated
| profile.addPhoto(secure_url, "profilePic"); | ||
|
|
||
| res.status(200).json({ | ||
| msg: "Image uploaded successfully", |
There was a problem hiding this comment.
I would do message instead of msg
What this PR does (required):
Screenshots / Videos (front-end only):
Any information needed to test this feature (required):
Any issues with the current functionality (optional):