-
Notifications
You must be signed in to change notification settings - Fork 714
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 #734
base: master
Are you sure you want to change the base?
solution #734
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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 is mostly well-structured and aligns with best practices, there are a couple of critical issues that need to be addressed before we can approve the pull request:
-
Ticket
Model Validation: In themodels.py
file, theTicket
model'sclean
method is using incorrect attribute names (count_rows
andcount_seats_in_row
) for validation. These should be corrected torows
andseats_in_row
to avoidAttributeError
during validation. -
MovieListSerializer
Issue: In theserializers.py
file, theMovieListSerializer
is usingSlugRelatedField
withslug_field='full_name'
. Sincefull_name
is a property and not a model field, this will cause an error. Consider using aSerializerMethodField
to properly serialize the full name.
Once these issues are resolved, your code will be in great shape! Keep up the good work, and feel free to reach out if you have any questions or need further assistance. 🚀
✨ 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.
actors = serializers.SlugRelatedField( | ||
many=True, read_only=True, slug_field="full_name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actors
field in MovieListSerializer
uses SlugRelatedField
with slug_field='full_name'
. However, full_name
is a property, not a model field, which will cause an error. Consider using a SerializerMethodField
to serialize the full name instead.
cinema/urls.py
Outdated
# write urls here | ||
from django.urls import path, include | ||
from rest_framework import routers | ||
from rest_framework.urls import app_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import app_name
from rest_framework.urls
is not used in this file. Consider removing it to clean up the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on addressing the issues in your code! 🎉 Your solution now meets the task requirements, and all necessary components are implemented correctly. However, there are a few points to consider for future improvements:
-
Ticket
Model Validation: You've successfully corrected the attribute names in theclean
method of theTicket
model. This change will preventAttributeError
during validation. Keep up the good work! -
MovieListSerializer
: The use ofSlugRelatedField
withslug_field='full_name'
for theactors
field is incorrect sincefull_name
is a property, not a model field. Consider using aSerializerMethodField
to properly serialize the actor's full name. This will ensure that your serialization logic aligns with the model structure. -
URL Configuration: Your
urls.py
files are well-configured, and no changes are needed. It's great to see that you have set up the URL routing correctly using Django REST Framework's router. -
Viewsets and Serializers: Your
views.py
file is well-structured, with appropriate use of viewsets and serializer classes. The dynamic switching between serializers based on actions is implemented correctly.
Remember, the decision to approve is to help you move forward with your learning. Please review the comments to improve your code further. Addressing critical issues like the SerializerMethodField
will enhance your understanding and prevent similar problems in the future. 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! ✨
actors = serializers.SlugRelatedField( | ||
many=True, read_only=True, slug_field="full_name" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SlugRelatedField
with slug_field='full_name'
is incorrect because full_name
is a property, not a model field. Use a SerializerMethodField
to serialize the actor's full name.
No description provided.