feat: added youtube api for video-tube app #128
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.
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.
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.
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.
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.
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.
Make errors more descriptive.
| try { | ||
| const { videoId } = req.params; | ||
|
|
||
| if (!isValidObjectId(videoId)) { |
There was a problem hiding this comment.
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.
Validation must go into the validators
| throw new ApiError(400, "Video not found"); | ||
| } | ||
|
|
||
| if (!content) { |
There was a problem hiding this comment.
Logic must go into the validators
| }, | ||
| }, | ||
| { | ||
| $lookup: { |
There was a problem hiding this comment.
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.
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 😀