-
Notifications
You must be signed in to change notification settings - Fork 781
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
feat: added youtube api for video-tube app #128
base: main
Are you sure you want to change the base?
Conversation
@dkcoder02 Thank you for the PR. To help us to review this PR quickly and effectively, kindly share the postman api collection for these endpoints with us. |
ok |
@hiteshchoudhary @wajeshubham This is my video tube app API collection link Happy Coding 😀 |
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 are few points I would like to highlight:
- There are no validators for the module. Each app module must have validators separately in src/validators folder where we should handle request data validation and sanitization
- Kindly revisit the logic you built and make it more optimised
- Do not update the existing user model. If you want more fields for user profile please create a separate profile module for it in your app
Kindly go through the requested changes then I'll review rest of the code again.
Thank you
src/models/apps/auth/user.models.js
Outdated
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.
Don't update this file. before updating please communicate with us on discord. User model is being used on multiple places and could break certain logic. If you want to add make the field optional and pass a default value. Please don't add any required fields without communicating with us.
@@ -33,6 +33,6 @@ const storage = multer.diskStorage({ | |||
export const upload = multer({ | |||
storage, | |||
limits: { | |||
fileSize: 1 * 1000 * 1000, | |||
fileSize: 100 * 1024 * 1024, // 100 MB in bytes |
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.
Keep it 5MB max. This code is being deployed on a live server and we want to restrict user from uploading large files to avoid cost.
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.
Don't update the existing user model or it's logic, create a separate profile model for video app user and use that as a profile for your platform. User model is just for authentication and could not be considered as a profile for user. You can refer to ecommerce model structure where we have profile for ecommerce app. This helps us to isolate separate profile and user detail logic for multiple apps without affecting the auth module.
import { getMongoosePaginationOptions } from "../../../utils/helpers.js"; | ||
|
||
/** | ||
* Retrieves the comments for a specific video. |
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.
No need to add JS Docs for controllers we are not following this convention anywhere in the application
const { videoId } = req.params; | ||
|
||
if (!isValidObjectId(videoId)) { | ||
throw new ApiError(400, "Invalid Id"); |
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.
Make errors more descriptive.
try { | ||
const { videoId } = req.params; | ||
|
||
if (!isValidObjectId(videoId)) { |
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.
request data sanitization and validation must be done in the validators and not in the controllers. This check should be in validators and not in the controllers.
const { content } = req.body; | ||
const { videoId } = req.params; | ||
|
||
if (!isValidObjectId(videoId)) { |
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.
Validation must go into the validators
throw new ApiError(400, "Video not found"); | ||
} | ||
|
||
if (!content) { |
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.
Logic must go into the validators
}, | ||
}, | ||
{ | ||
$lookup: { |
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.
This stage could go into the comments lookup pipeline.
|
||
const { page = 1, limit = 10 } = req.query; | ||
|
||
const videosCommentAggregate = Video.aggregate([ |
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.
Why are we aggregating on Video
model while fetching comments? We should query the Comment
model
@wajeshubham I understand all the points you have sent me and will very soon work on the changes Thank you |
@wajeshubham all changes are done. please check the refactored code thank you |
@dkcoder02 After reviewing I can still see some review comments are not et resolved. Kindly go through them and let us know in case of any doubts. |
@wajeshubham My side you gave me All issue is Resolved And the video tube api is working fine. If you have told Me some issue not resolved this Api not worried I am again checking the video tube api code |
Added feat/YouTube API #84
implemented YouTube API
features list of video tube application
special thank you @hiteshchoudhary @wajeshubham
Happy Coding 😀