Develop #29
Develop #29
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the get_movies and list_movies functions need to handle cases where no movies are found by returning a 404 Not Found error. Additionally, the create_movie function should check for duplicate entries and return a 409 Conflict error if necessary. There are also schema-related issues that need attention, such as ensuring MovieCreateSchema and MovieUpdateSchema do not inherit unnecessary fields. Once these issues are resolved, your code will be in great shape! Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| ): | ||
| total_items = db.query(MovieModel).count() | ||
| offset = (page - 1) * per_page | ||
| movies = db.query(MovieModel).order_by(MovieModel.id.desc()).offset(offset).limit(per_page).all() |
There was a problem hiding this comment.
The get_movies function should handle cases where no movies are found for the specified page by returning a 404 Not Found error with the message: "No movies found." This is a requirement from the task description.
src/crud/movies.py
Outdated
|
|
||
|
|
||
| def create_movie(db: Session, movie: MovieCreateSchema): | ||
| db_movie = MovieModel(**movie.dict()) |
There was a problem hiding this comment.
The create_movie function should include a check for duplicate movies based on name and date. If a duplicate is found, it should return a 409 Conflict error with the message: "A movie with the name '{name}' and release date '{date}' already exists." .
src/routes/movies.py
Outdated
| if existing_movie is None: | ||
| raise HTTPException(status_code=400) |
There was a problem hiding this comment.
The check for existing_movie is None is redundant because it contradicts the previous check for existing_movie. Remove this block to avoid unnecessary code execution.
| @router.get("/movies/", response_model=list[MovieReadSchema], status_code=200) | ||
| def list_movies(db: Session = Depends(get_db)): |
There was a problem hiding this comment.
The list_movies function should accept page and per_page query parameters to handle pagination. If no movies are found for the specified page, it should return a 404 Not Found error with the message: "No movies found." .
src/routes/movies.py
Outdated
| @router.put("/movies/{movie_id}", response_model=MovieReadSchema, status_code=200) | ||
| def edit_movie(movie_id: int, movie: MovieUpdateSchema, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
The edit_movie function should use the PATCH method instead of PUT to allow partial updates. Ensure that only the fields provided in the request body are updated, leaving all other fields unchanged.
src/schemas/movies.py
Outdated
|
|
||
|
|
||
| class MovieBaseSchema(BaseModel): | ||
| id: int |
There was a problem hiding this comment.
The id field should not be part of MovieBaseSchema as it is not typically included in schemas meant for creation or updates. Consider removing it from MovieBaseSchema and adding it only to schemas where it is necessary, like MovieReadSchema.
| country: Optional[str] | ||
|
|
||
|
|
||
| class MovieCreateSchema(MovieBaseSchema): |
There was a problem hiding this comment.
The MovieCreateSchema should not inherit from MovieBaseSchema if it requires different fields. Consider defining MovieCreateSchema independently to ensure it only includes the necessary fields for movie creation.
| return value | ||
|
|
||
|
|
||
| class MovieUpdateSchema(MovieBaseSchema): |
There was a problem hiding this comment.
The MovieUpdateSchema should not inherit from MovieBaseSchema if it requires different fields. Consider defining MovieUpdateSchema independently to ensure it only includes the necessary fields for movie updates.
src/schemas/movies.py
Outdated
| @validator("id") | ||
| def validate_id(cls, value): | ||
| if movie_id := value: | ||
| return movie_id |
There was a problem hiding this comment.
The validate_id method in MovieReadSchema is redundant and does not perform any meaningful validation. Consider removing this method.
There was a problem hiding this comment.
Thank you for your submission! 🎉 While the code has several critical issues that need attention, the decision to approve is made to help you move forward with your learning journey. Here are the key points to address:
-
Error Handling: Ensure that the
create_moviefunction correctly handles duplicates by raising a409 Conflicterror, and that thedelete_moviefunction raises a404 Not Founderror when a movie is not found. -
Schema Consistency: Consider inheriting
MovieCreateSchemaandMovieUpdateSchemafromMovieBaseSchemato avoid redundancy. Usedateinstead ofdatetime.datefor consistency in therelease_datefield. -
Pagination: Update the
list_moviesfunction to acceptpageandper_pageparameters for proper pagination. -
Response Models: Remove the
response_modelfrom thedeleteroute when using a204 No Contentstatus code. -
Redundant Checks: Remove unnecessary checks for
db_moviebeingNoneto prevent incorrect error handling.
Please review these comments carefully and address them in future iterations to improve your code's robustness and adherence to RESTful standards. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/crud/movies.py
Outdated
| if db_movie is None: | ||
| raise HTTPException(status_code=400) |
There was a problem hiding this comment.
The check if db_movie is None: is unnecessary here because the previous check already handles the case where a duplicate movie exists. You should remove this block to avoid raising a 400 Bad Request error incorrectly.
| if db_movie is None: | ||
| raise HTTPException(status_code=400, detail="Invalid input data.") |
There was a problem hiding this comment.
The check if db_movie is None: is redundant because the previous check if not db_movie: already handles the case where the movie is not found. You should remove this block to avoid raising a 400 Bad Request error incorrectly.
src/crud/movies.py
Outdated
| if not db_movie: | ||
| return None |
There was a problem hiding this comment.
Instead of returning None, you should raise a 404 Not Found error when the movie is not found. This will provide a more consistent API response and better error handling.
| def list_movies(db: Session = Depends(get_db)): | ||
| db_movie = get_movies(db) |
There was a problem hiding this comment.
The list_movies function should accept page and per_page parameters to pass them to the get_movies function. This will allow for proper pagination of the movie list.
src/routes/movies.py
Outdated
| return db_movie | ||
|
|
||
|
|
||
| @router.delete("/movies/{movie_id}", response_model=MovieReadSchema, status_code=204) |
There was a problem hiding this comment.
The delete route should not specify a response_model when the status code is 204 No Content. Consider removing the response_model parameter to align with the semantics of a 204 response.
|
|
||
| class MovieBaseSchema(BaseModel): | ||
| name: str | ||
| date: datetime.date |
There was a problem hiding this comment.
Use date instead of datetime.date for the date field to be consistent with the release_date field in MovieCreateSchema.
src/schemas/movies.py
Outdated
| country: Optional[str] | ||
|
|
||
|
|
||
| class MovieCreateSchema(BaseModel): |
There was a problem hiding this comment.
Consider inheriting MovieCreateSchema from MovieBaseSchema to ensure consistency and avoid redundancy in field definitions.
src/schemas/movies.py
Outdated
| return value | ||
|
|
||
|
|
||
| class MovieUpdateSchema(BaseModel): |
There was a problem hiding this comment.
Consider inheriting MovieUpdateSchema from MovieBaseSchema to ensure consistency and avoid redundancy in field definitions.
No description provided.