Solution #727
Solution #727
Conversation
|
|
||
| from cinema.models import Genre, Actor, CinemaHall, Movie, MovieSession | ||
| from cinema.models import Genre, Actor, CinemaHall, Movie, MovieSession, Order |
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. The OrderSerializer has a problem with handling tickets due to the read-only field, which will cause a KeyError. Additionally, the get_tickets_available property in the MovieSession class does not correctly calculate available tickets for a specific session. Lastly, filtering by actors__id__icontains is not suitable for an ID field. Please address these issues to ensure the functionality meets the task requirements. Keep up the great work, and I look forward to your improvements! 🚀
✨ 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.
cinema/models.py
Outdated
| @property | ||
| def get_tickets_available(self): | ||
| return (self.cinema_hall.rows | ||
| * self.cinema_hall.seats_in_row | ||
| - len(Ticket.objects.all())) |
There was a problem hiding this comment.
The get_tickets_available property incorrectly calculates the number of available tickets. It should filter the tickets by the specific MovieSession instance rather than using all tickets. Consider using self.tickets.count() to get the number of tickets for this session.
| def create(self, validated_data): | ||
| with transaction.atomic(): | ||
| tickets_data = validated_data.pop("tickets") |
There was a problem hiding this comment.
The create method in OrderSerializer attempts to pop tickets from validated_data, but tickets is marked as read-only in the serializer, meaning it won't be part of validated_data. This will raise a KeyError. Consider revising the approach to handle ticket creation separately or adjust the serializer to accept ticket data.
cinema/views.py
Outdated
| if title: | ||
| queryset = queryset.filter(title__icontains=title) | ||
| if actors: | ||
| queryset = queryset.filter(actors__id__icontains=actors) |
There was a problem hiding this comment.
Filtering by actors__id__icontains is not appropriate for an ID field. Consider using actors__id=actors if actors is expected to be a single ID, or adjust the logic to handle multiple IDs if necessary.
There was a problem hiding this comment.
Review Summary
Great job on addressing most of the issues in your cinema application! 🎬 Your models, serializers, views, and URL configurations are well-structured and adhere to Django and Django REST Framework conventions. Here are some key points and areas for improvement:
-
OrderSerializerKeyError: TheOrderSerializeris still using a read-onlyTicketSerializerfor theticketsfield. This will cause aKeyErrorwhen trying to create an order because thecreatemethod expects writable fields to createTicketinstances. Please ensure that the serializer can handle ticket creation properly. -
get_tickets_availableCalculation: Theget_tickets_availableproperty in theMovieSessionclass is now correctly implemented. It calculates the available tickets by subtracting the count of tickets from the cinema hall's total capacity. Verify that theticketsrelated name is correctly set up and used to ensure accurate calculations. -
Filtering by Actor ID: The filtering logic in the
MovieViewSetis now correctly implemented usingactors__id=actors, which is appropriate for ID fields. This improvement enhances the functionality of your application. -
Configuration Settings: Remember to set
DEBUGtoFalsein production for security reasons and configureALLOWED_HOSTSappropriately for deployment.
Your progress is commendable, and the decision to approve this pull request is to help you move forward with your learning. Please review the comments and address the mentioned issues to improve your code further. Keep up the great work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @property | ||
| def get_tickets_available(self): | ||
| return (self.cinema_hall.rows | ||
| * self.cinema_hall.seats_in_row | ||
| - self.tickets.count()) |
There was a problem hiding this comment.
The get_tickets_available property should correctly calculate the number of available tickets for a movie session. Ensure that it accounts for the total capacity of the cinema hall minus the number of tickets already sold. The current implementation seems to do this, but please verify that the tickets related name is correctly set up and used.
No description provided.