Solution #734
Solution #734
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While your code is mostly well-structured and aligns with Django and Django REST Framework best practices, there is a critical issue that needs to be addressed before we can approve it.
The condition self.action == ("list", "retrieve") is incorrect and affects the functionality of your viewsets. It should be self.action in ("list", "retrieve") to properly check if the action is either 'list' or 'retrieve'. This change is crucial for ensuring that your custom querysets and serializers are applied correctly based on the action.
Once you make this adjustment, your code should be ready for approval. Keep up the great work, and don't hesitate to reach out if you have any questions! 🚀
✨ 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/views.py
Outdated
| @@ -34,6 +39,28 @@ class MovieViewSet(viewsets.ModelViewSet): | |||
| queryset = Movie.objects.all() | |||
| serializer_class = MovieSerializer | |||
|
|
|||
| def get_queryset(self): | |||
| queryset = super().get_queryset() | |||
| if self.action == ("list", "retrieve"): | |||
There was a problem hiding this comment.
The condition self.action == ("list", "retrieve") is incorrect. It should be self.action in ("list", "retrieve") to check if the action is either 'list' or 'retrieve'.
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 Your code is well-structured and adheres to Django and Django REST Framework best practices. The unique constraints and validations are correctly implemented, and the endpoints function as expected.
However, there are a couple of minor improvements you can make:
- Redundant
select_relatedCall: InMovieSessionViewSet, theselect_relatedcall for 'movie' and 'cinema_hall' is already handled in the 'retrieve' action. Consider removing this redundant call within the 'list' and 'retrieve' check to avoid unnecessary repetition. - Redundant
read_only_fields: InTicketInfoSeatsSerializer, theread_only_fieldsattribute is unnecessary since all fields are already read-only by default. Removing this line can help clean up the code.
These issues are not critical, but addressing them will improve your code quality. 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! ✨
| class Meta: | ||
| model = Ticket | ||
| fields = ("row", "seat") | ||
| read_only_fields = fields |
There was a problem hiding this comment.
The read_only_fields attribute is redundant here because all fields in TicketInfoSeatsSerializer are already marked as read-only by default. You can remove this line to clean up the code.
| if self.action in ("list", "retrieve"): | ||
| queryset = queryset.select_related("movie", "cinema_hall") |
There was a problem hiding this comment.
The select_related call for 'movie' and 'cinema_hall' is already handled in the 'retrieve' action. You can remove this redundant call within the 'list' and 'retrieve' check to avoid unnecessary repetition.
No description provided.