Develop#1161
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has two critical functional bugs that must be fixed: 1. MovieSerializer doesn't handle ManyToMany relationships — The actors and genres fields are marked read_only=True, and the create() method never assigns them. When creating a movie, actors and genres won't be associated. Remove read_only=True from these fields and implement proper M2M handling in both create() and update() methods using .set(). 2. GenreDetail endpoints don't persist changes — Both put() and patch() call serializer.is_valid() but never call serializer.save(). The update validates but is never written to the database. Add serializer.save() after validation in both methods.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| class MovieSerializer(serializers.Serializer): | ||
| id = serializers.IntegerField(read_only=True) | ||
| actors = serializers.PrimaryKeyRelatedField(many=True, read_only=True) |
There was a problem hiding this comment.
ManyToMany fields are read_only=True, which means they can only be displayed in responses but not set during creation. The create() method doesn't handle setting actors and genres - it would create a Movie without any associated actors/genres. Remove read_only=True from these fields and implement proper ManyToMany handling in create() and update() methods.
| def update(self, instance, validated_data): | ||
| instance.title = validated_data.get("title", instance.title) | ||
| instance.description = validated_data.get( | ||
| "description", instance.description | ||
| ) | ||
| instance.description = validated_data.get("description", instance.description) | ||
| instance.duration = validated_data.get("duration", instance.duration) | ||
|
|
||
| instance.save() | ||
|
|
||
| return instance |
There was a problem hiding this comment.
The update() method doesn't handle ManyToMany relationships. After updating basic fields, you need to check if actors/genres are in validated_data and update them using .set().
|
|
||
| if request.method == "PUT": | ||
| serializer = MovieSerializer(movie, data=request.data) | ||
| if serializer.is_valid(): | ||
| serializer.save() | ||
| return Response(serializer.data, status=status.HTTP_200_OK) | ||
| def put(self, request, pk): | ||
| genre = self.get_object(pk) | ||
| serializer = GenreSerializer(genre, data=request.data) | ||
| serializer.is_valid(raise_exception=True) | ||
| return Response(serializer.data, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
The put() method validates the data but never calls serializer.save(), so the update is never persisted to the database. The response returns serializer.data (which would just be the request data), not the actual updated instance. Add serializer.save() before returning the response.
|
|
||
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | ||
| def patch(self, request, pk): | ||
| genre = self.get_object(pk) | ||
| serializer = GenreSerializer(genre, data=request.data, partial=True) | ||
| serializer.is_valid(raise_exception=True) | ||
| return Response(serializer.data, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
The patch() method has the same issue - validates but doesn't save. The partial update is never persisted. Add serializer.save() before returning the response.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The MovieSerializer's create() and update() methods have critical bugs that will break functionality. In create(), M2M fields (actors, genres) cannot be passed directly to Movie.objects.create() — the object must be saved first before M2M relationships can be assigned. You must pop these fields from validated_data, create the instance, then use instance.actors.set(actors) and instance.genres.set(genres). In update(), direct assignment like instance.actors = validated_data.get('actors') doesn't work for M2M fields — you must use instance.actors.set(validated_data.get('actors')) instead. This affects both cinema/serializers.py and potentially cinema/urls.py if it contains custom serializer logic. The minor flake8 line-length violations in cinema_service/settings.py are non-blocking style issues and can be addressed separately. Fixing the M2M handling is required before this implementation can work correctly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| name="Actor", | ||
| fields=[ | ||
| ( | ||
| "id", | ||
| models.BigAutoField( | ||
| auto_created=True, | ||
| primary_key=True, | ||
| serialize=False, | ||
| verbose_name="ID", | ||
| ), | ||
| ), | ||
| ("first_name", models.CharField(max_length=255)), | ||
| ("last_name", models.CharField(max_length=255)), | ||
| ], | ||
| ), | ||
| migrations.CreateModel( |
There was a problem hiding this comment.
ManyToMany fields cannot be assigned directly like this. After saving the instance, use instance.actors.set(actors) and instance.genres.set(genres) to properly update the M2M relationships. The current code will fail to persist actors and genres to the database.
| cinema_hall_list = CinemaHallViewSet.as_view( | ||
| actions={ | ||
| "get": "list", |
There was a problem hiding this comment.
ManyToMany fields cannot be passed directly to objects.create(). The object must be created first, then M2M relationships assigned using .set(). Pop actors and genres from validated_data before creating the instance, then call instance.actors.set(actors) and instance.genres.set(genres) after creation.
| } | ||
| ) | ||
|
|
||
| cinema_hall_detail = CinemaHallViewSet.as_view( |
There was a problem hiding this comment.
Direct assignment to M2M fields (instance.actors = ...) doesn't work. You must use instance.actors.set(...) for ManyToMany relationships. Pop these fields from validated_data and use .set() instead of direct assignment.
| def update(self, instance, validated_data): | ||
| instance.title = validated_data.get("title", instance.title) | ||
| instance.actors = validated_data.get("actors", instance.actors) | ||
| instance.genres = validated_data.get("genres", instance.genres) |
There was a problem hiding this comment.
Direct assignment instance.actors = validated_data.get(...) does not work for ManyToMany relationships. Must use .set() method: instance.actors.set(validated_data.get("actors"))
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this implementation as an educational override. You've done an excellent job implementing all the core requirements: proper models with M2M relationships and related_name, serializers that correctly handle M2M using PrimaryKeyRelatedField and .set() method, and views using the correct class-based view types (APIView, GenericAPIView, GenericViewSet, and ModelViewSet). There's a minor typo in cinema/views.py where *argh is used instead of *args in the ActorDetail view's put, patch, and delete methods (around lines 79, 83, 87) — while this doesn't break functionality since the methods delegate to mixins, correcting it to *args follows standard Python conventions. Overall, your code is well-structured and meets the task requirements. Great work on implementing the routers for the Movie model as well!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.