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 #737

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Solution #737

wants to merge 3 commits into from

Conversation

sofiiasavkova
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 this project! While your code meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the incorrect usage of super() in the TicketSerializer's validate method is a significant issue that could lead to errors in functionality. Additionally, the paginate_queryset method in MovieViewSet returning None effectively disables pagination, which might not be the intended behavior. Please address these issues to ensure the code functions as expected. Once these changes are made, 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.

cinema/serializers.py Outdated Show resolved Hide resolved
cinema/tests/test_movie_api.py Outdated Show resolved Hide resolved
cinema/views.py Show resolved Hide resolved
cinema/views.py Show resolved Hide resolved
cinema/views.py Show resolved Hide resolved
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 submission! 🎉 Your code is on the right track, but there are a few critical issues that need attention to ensure everything functions smoothly:

  1. TicketSerializer's validate Method: The current use of super().validate(data) is incorrect since ModelSerializer does not have a validate method that takes data as an argument. Please adjust this to properly validate the data.

  2. OrderSerializer's create Method: The logic attempts to pop 'tickets' from validated_data, but since 'tickets' is marked as read_only=True, it won't be present, leading to a KeyError. Revise this logic to handle ticket creation appropriately.

  3. Pagination in MovieViewSet: The paginate_queryset method currently returns None, disabling pagination. Implementing pagination is crucial, especially for the orders endpoint, so consider using the default or a custom pagination class.

  4. OrderListCreateView Implementation: This class is defined but lacks implementation details like queryset and serializer_class. Ensure it's fully implemented if intended for use.

  5. Code Style and Cleanliness: Make sure all files end with a blank line, group imports with parentheses if needed, and remove unnecessary comments or debugging prints.

These improvements will enhance the functionality and clarity of your code. Keep up the great work, and use this feedback to refine your skills further. Remember, addressing these issues will help you avoid similar problems in the future. Happy coding! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines +107 to +108
def validate(self, data):
data = super().validate(data)

Choose a reason for hiding this comment

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

The super().validate(data) call in the validate method of TicketSerializer is incorrect. The validate method should not call super().validate(data) because ModelSerializer does not have a validate method that takes data as an argument. Consider removing this line or adjusting it to properly validate the data.

fields = ("id", "created_at", "tickets")

def create(self, validated_data):
tickets_data = validated_data.pop("tickets")

Choose a reason for hiding this comment

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

The create method in OrderSerializer attempts to pop 'tickets' from validated_data, but 'tickets' is marked as read_only=True in the serializer, meaning it won't be present in validated_data. This will raise a KeyError. Consider revising this logic to handle the creation of tickets appropriately.

Comment on lines +93 to +94
def paginate_queryset(self, queryset):
return None

Choose a reason for hiding this comment

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

The paginate_queryset method in MovieViewSet returns None, which disables pagination. If pagination is required, consider using the default pagination or a custom pagination class instead of returning None.

Comment on lines +139 to +140
class OrderListCreateView(viewsets.ModelViewSet):
pagination_class = OrderPagination

Choose a reason for hiding this comment

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

The OrderListCreateView class is defined but lacks implementation details such as queryset and serializer_class. Ensure that this class is fully implemented if it is intended to be used.

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