Solution #747
Solution #747
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can approve it:
-
Redundancy in REST Framework Configuration: The
DEFAULT_FILTER_BACKENDSis set twice in the settings file. This redundancy could lead to confusion and inconsistency. Please remove one of these entries to streamline the configuration. -
Typo in Model Reference: There's a typo in the reference to
movie_session.cinema_hалл.seats_in_row. It should bemovie_session.cinema_hall.seats_in_row. This typo could cause runtime errors, so it's important to correct it. -
Validation in
TicketSerializer: Thevalidatemethod should safely accessmovie_sessionusingdata.get('movie_session')to prevent potentialKeyErrorexceptions. This will make your code more robust and user-friendly. -
Error Handling in
filter_by_actor_ids: While catching aValueErroris a good practice, consider logging the error or providing feedback to the user. This will enhance debugging and improve the user experience.
Once these issues are addressed, 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.
cinema/filters.py
Outdated
| def filter_by_actor_ids(self, queryset, name, value): | ||
| try: | ||
| # Преобразуем значения в список ID | ||
| actor_ids = [ | ||
| int(id.strip()) | ||
| for id in value.split(",") | ||
| if id.strip().isdigit() | ||
| ] | ||
| return queryset.filter(actors__id__in=actor_ids) | ||
| except ValueError: | ||
| # Если передан некорректный ID | ||
| return queryset.none() |
There was a problem hiding this comment.
The filter_by_actor_ids method currently catches a ValueError if the IDs are not valid integers. This is a good practice, but consider logging the error or providing feedback to the user to improve debugging and user experience.
cinema/serializers.py
Outdated
| movie_session = data["movie_session"] | ||
| row = data["row"] | ||
| seat = data["seat"] | ||
| if Ticket.objects.filter( | ||
| movie_session=movie_session, row=row, seat=seat | ||
| ).exists(): | ||
| raise serializers.ValidationError("Это место уже занято.") | ||
| if not (1 <= row <= movie_session.cinema_hall.rows): | ||
| raise serializers.ValidationError("Недопустимый номер ряда.") | ||
| if not (1 <= seat <= movie_session.cinema_hалл.seats_in_row): | ||
| raise serializers.ValidationError("Недопустимый номер места.") |
There was a problem hiding this comment.
In the validate method of TicketSerializer, ensure that movie_session is included in the data dictionary. If movie_session is not provided, this will raise a KeyError. Consider using data.get('movie_session') to safely access the value.
cinema/serializers.py
Outdated
| if not (1 <= seat <= movie_session.cinema_hалл.seats_in_row): | ||
| raise serializers.ValidationError("Недопустимый номер места.") |
There was a problem hiding this comment.
There is a typo in movie_session.cinema_hалл.seats_in_row. It should be movie_session.cinema_hall.seats_in_row to correctly reference the CinemaHall model.
cinema_service/settings.py
Outdated
| REST_FRAMEWORK = { | ||
| "DEFAULT_FILTER_BACKENDS": ( | ||
| "django_filters.rest_framework.DjangoFilterBackend", | ||
| ), | ||
| } | ||
|
|
||
| REST_FRAMEWORK["DEFAULT_FILTER_BACKENDS"] = [ | ||
| "django_filters.rest_framework.DjangoFilterBackend" | ||
| ] |
There was a problem hiding this comment.
There is a redundancy in the REST framework configuration. The DEFAULT_FILTER_BACKENDS is set twice, once as a tuple and once as a list. You should remove one of these to avoid confusion and ensure consistency in the settings.
There was a problem hiding this comment.
Your Pull Request has been reviewed and is approved! 🎉
Review Summary:
Great job on addressing the previous issues and improving your code! Here are some key points from the review:
-
Redundancy in REST Framework Configuration: The
DEFAULT_FILTER_BACKENDSwas set twice in the settings file. Make sure to remove one of these entries to streamline the configuration. -
Unused Methods in
MovieSerializer: Theget_genresandget_actorsmethods are not used in the serializer fields. Consider removing these methods to clean up the code. -
Validation in
TicketSerializer: You've improved thevalidatemethod by usingdata.get('movie_session')to safely accessmovie_session, preventing potentialKeyErrorexceptions. -
Error Handling in
filter_by_actor_ids: Consider providing feedback to the user or handling errors more gracefully when an invalid actor ID is encountered. This will enhance the user experience and robustness of your application.
Your code is well-structured, and the critical issues have been addressed. Keep up the good work and continue to refine your skills. Remember to review the comments to further improve your code. This decision is made to help you move forward with your learning journey. 🚀
If there are any critical issues, make sure to address them in the future to avoid similar problems. Keep learning and improving! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| except ValueError as e: | ||
| # Логируем ошибку для отладки | ||
| logger.warning(f"Invalid actor ID input: '{value}'. Error: {e}") | ||
| # Возвращаем пустой queryset, если данные некорректны | ||
| return queryset.none() |
There was a problem hiding this comment.
Consider providing feedback to the user or handling the error more gracefully when an invalid actor ID is encountered. This will improve the user experience and make the application more robust.
| fields = ("id", "row", "seat", "movie_session") | ||
|
|
||
| def validate(self, data): | ||
| movie_session = data.get("movie_session") |
There was a problem hiding this comment.
Consider using data.get('movie_session') to safely access movie_session and prevent potential KeyError exceptions.
| def get_genres(self, obj): | ||
| return [genre.name for genre in obj.genres.all()] | ||
|
|
||
| def get_actors(self, obj): | ||
| return [ | ||
| f"{actor.first_name} {actor.last_name}" | ||
| for actor in obj.actors.all() | ||
| ] |
There was a problem hiding this comment.
The get_genres and get_actors methods are not used in the serializer fields. Consider removing these methods to clean up the code.
| REST_FRAMEWORK = { | ||
| "DEFAULT_FILTER_BACKENDS": ( | ||
| "django_filters.rest_framework.DjangoFilterBackend", | ||
| ), | ||
| } |
There was a problem hiding this comment.
The DEFAULT_FILTER_BACKENDS is set twice in the settings file. Please remove one of these entries to streamline the configuration.
No description provided.