Develop #745
Develop #745
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. The pagination settings are very restrictive, which might not be suitable for all scenarios, and the condition in get_queryset for MovieViewSet needs to be corrected. Additionally, the tickets_available field requires a proper source or method to compute its value. Once these issues are resolved, your solution 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.
| @@ -59,6 +69,7 @@ class MovieSessionListSerializer(MovieSessionSerializer): | |||
| cinema_hall_capacity = serializers.IntegerField( | |||
| source="cinema_hall.capacity", read_only=True | |||
| ) | |||
| tickets_available = serializers.IntegerField(read_only=True) | |||
There was a problem hiding this comment.
The tickets_available field is marked as read-only but does not have a source or method to calculate its value. Ensure that there is a method or property in the MovieSession model to provide this data.
cinema/views.py
Outdated
| queryset = Movie.objects.filter(genres__id__in=genres_ids) | ||
| elif title: | ||
| queryset = Movie.objects.filter(title__contains=title) | ||
| if self.action == ("list", "retrieve"): |
There was a problem hiding this comment.
The condition if self.action == ("list", "retrieve"): is incorrect. You should check for each action separately, like if self.action in ["list", "retrieve"]:.
| page_size = 1 | ||
| page_size_query_param = "page_size" | ||
| max_page_size = 1 |
There was a problem hiding this comment.
The pagination settings are very restrictive with a page_size and max_page_size of 1. Ensure this is intentional, as it might not be suitable for all scenarios.
There was a problem hiding this comment.
Great job on addressing many of the issues in your code! 🎉 While there are still some areas that need improvement, your efforts are commendable. Here are a few key points to focus on:
-
Pagination Settings: The
OrderSetPaginationclass has very restrictive settings withpage_sizeandmax_page_sizeboth set to 1. Consider increasing these values to improve user experience by allowing more items per page. -
get_querysetinMovieViewSet: Currently, the use ofelifstatements restricts filtering to one parameter at a time. Switching toifstatements will enable filtering by multiple parameters like actors, genres, and title simultaneously. -
Transaction Handling in
OrderCreateSerializer: Ensure that the ticket creation logic within thecreatemethod is robust. While usingtransaction.atomicis a good practice, make sure to handle any exceptions that might occur during ticket creation to maintain data integrity. -
tickets_availableField: This field is marked as read-only but lacks a method to compute its value. Ensure it is properly calculated, possibly by adding a method to compute available tickets based on cinema hall capacity and tickets sold.
Your progress is impressive, and these adjustments will help you further enhance your code. Keep up the great work, and remember to address these critical issues in future iterations to avoid similar problems. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -59,6 +69,7 @@ class MovieSessionListSerializer(MovieSessionSerializer): | |||
| cinema_hall_capacity = serializers.IntegerField( | |||
| source="cinema_hall.capacity", read_only=True | |||
| ) | |||
| tickets_available = serializers.IntegerField(read_only=True) | |||
There was a problem hiding this comment.
The tickets_available field is marked as read-only, but there is no source or method provided to compute its value. Ensure that this field is properly calculated, possibly by adding a method in the serializer to compute the available tickets based on the cinema hall capacity and the number of tickets already sold.
| @transaction.atomic | ||
| def create(self, validated_data): | ||
| tickets_data = validated_data.pop("tickets") | ||
|
|
||
| order = Order.objects.create(**validated_data) | ||
| tickets = [ | ||
| Ticket(order=order, **ticket_data) | ||
| for ticket_data in tickets_data | ||
| ] | ||
| Ticket.objects.bulk_create(tickets) | ||
| return order |
There was a problem hiding this comment.
Ensure that the ticket creation logic within the create method of OrderCreateSerializer is correctly handling the transaction. The transaction.atomic decorator is used, which is good, but make sure that any potential exceptions during ticket creation are handled to maintain data integrity.
| queryset = Movie.objects.filter(actors__id__in=actors_ids) | ||
| elif genres: | ||
| genres_ids = [int(str_id) for str_id in genres.split(",")] | ||
| queryset = Movie.objects.filter(genres__id__in=genres_ids) | ||
| elif title: | ||
| queryset = Movie.objects.filter(title__contains=title) |
There was a problem hiding this comment.
The current implementation of get_queryset in MovieViewSet uses elif statements, which means only one filter can be applied at a time. Consider using if statements instead to allow filtering by multiple parameters simultaneously, such as actors, genres, and title.
| page_size = 1 | ||
| page_size_query_param = "page_size" | ||
| max_page_size = 1 |
There was a problem hiding this comment.
The pagination settings in OrderSetPagination are very restrictive, with both page_size and max_page_size set to 1. Consider increasing these values to provide a more user-friendly experience, allowing users to view more items per page.
No description provided.