Skip to content
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

Solution #35

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Solution #35

wants to merge 2 commits into from

Conversation

tylerj231
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hard work on the Movie Theater API project! 🎬 While your implementation covers the core requirements and functionality, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Delete Functionality: The delete_movie function attempts to call .delete() on a NoneType object. Ensure you check if db_movie is None before attempting to delete it to avoid runtime errors.

  2. Patch Functionality: The condition in the patch_movie function should verify if db_movie is None, not movie, to correctly determine if the movie exists in the database.

  3. Schema Adjustments: In src/schemas/movies.py, correct the typo in the MovieBase class from data to date. Also, consider using Optional for fields in MovieUpdate that can be None for clarity and consistency.

Addressing these issues will ensure the API functions correctly and aligns with best practices. Once these changes are made, your pull request will be ready for approval. 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.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Great job on implementing the Movie Theater API endpoints! 🎬 While there are a few critical issues that need attention, your effort is commendable. Here are some key points to address:

  1. Delete Functionality: Ensure you check if db_movie is None before attempting to delete it. The current implementation tries to call .delete() on a NoneType object, which will cause a runtime error. Split the retrieval and deletion into two steps to avoid this issue.

  2. Patch Functionality: You've correctly adjusted the patch_movie function to check if db_movie is None, which is great for determining if the movie exists in the database.

  3. Schema Adjustments: In src/schemas/movies.py, the MovieBase class has been corrected to use date instead of data. However, the MovieUpdate class should use Optional for fields that can be None, such as name and date, to allow for partial updates and maintain consistency with other schemas.

  4. Endpoint Path: Ensure the endpoint path matches the task requirements, such as using /movies/{movie_id}/ for correct routing.

These changes are crucial for the API's functionality and aligning with best practices. The decision to approve is to help you move forward with your learning journey. Please review the comments and make the necessary improvements to enhance your code. 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! ✨

return new_movie


@router.patch("movies/{movie_id}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endpoint path should be /movies/{movie_id}/ to match the task requirements and ensure correct routing.


@router.delete("/movies/{movie_id}", status_code=status.HTTP_204_NO_CONTENT)
def delete_movie(movie_id, db: Session = Depends(get_db)):
db_movie = db.query(MovieModel).filter(MovieModel.id == movie_id).first().delete()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delete() method is being called directly on the result of first(), which will return None if no movie is found. This will raise an error. You should first check if db_movie is None before attempting to delete.

def delete_movie(movie_id, db: Session = Depends(get_db)):
db_movie = db.query(MovieModel).filter(MovieModel.id == movie_id).first().delete()

if not db_movie:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition will never be true because db_movie is already being deleted in the previous line. You should check if db_movie is None before attempting to delete it.


class MovieUpdate(BaseModel):
name: str
date: str

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The date field in MovieUpdate should be of type Optional[date] instead of str to maintain consistency with the MovieCreate and MovieDetail schemas.

Comment on lines +39 to +46
class MovieUpdate(BaseModel):
name: str
date: str
score: float
overview: str
status: str
budget: float
revenue: float

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using Optional for fields in MovieUpdate that can be None to allow partial updates. For example, name: Optional[str], date: Optional[date], etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants