Develop #734
Develop #734
Conversation
There was a problem hiding this comment.
It looks like there are a few critical issues that need to be addressed before we can approve the pull request. The most important ones are:
-
ImageField Implementation: The
ImageFieldin theMoviemodel is missing, which is a core requirement of the task. This needs to be implemented to handle image uploads correctly. -
AuthTokenSerializer Message: The error message in the
validatemethod should refer to 'email' instead of 'username', as authentication is based on email. -
MovieImageSerializer Source: The
sourceparameter inMovieImageSerializershould be corrected to refer to the fieldmovieinstead ofmovie.image. -
Upload Path Function: The
upload_toparameter in theImageFieldshould reference thecreate_custom_pathfunction correctly without quotes, as it is a callable. -
Function Definition: Ensure that the
create_custom_pathfunction is defined and accessible in your project to avoid runtime errors during image uploads.
Please address these issues to ensure the functionality meets the task requirements. Once these are resolved, your work will be in great shape! 😊 Keep up the good work and don't hesitate to reach out if you need any help.
✨ 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.
| name='image', | ||
| field=models.ImageField(null=True, upload_to='create_custom_path'), |
There was a problem hiding this comment.
Ensure that the create_custom_path function is defined in your project. This function should return the desired path for storing the uploaded images. If it's not defined, this will cause an error when trying to upload an image.
cinema/models.py
Outdated
| class Movie(models.Model): | ||
| title = models.CharField(max_length=255) | ||
| description = models.TextField() | ||
| duration = models.IntegerField() | ||
| genres = models.ManyToManyField(Genre) | ||
| actors = models.ManyToManyField(Actor) | ||
| image = models.ImageField(upload_to="create_custom_path", null=True) |
There was a problem hiding this comment.
The upload_to parameter in the ImageField should reference the create_custom_path function without quotes, as it is a callable. Change upload_to="create_custom_path" to upload_to=create_custom_path.
cinema/serializers.py
Outdated
| @@ -69,6 +92,7 @@ class MovieSessionListSerializer(MovieSessionSerializer): | |||
| source="cinema_hall.capacity", read_only=True | |||
| ) | |||
| tickets_available = serializers.IntegerField(read_only=True) | |||
| movie_image = MovieImageSerializer(source="movie.image", read_only=True) | |||
There was a problem hiding this comment.
The source parameter in MovieImageSerializer should refer to the field movie, not movie.image. Change source="movie.image" to source="movie" to correctly serialize the image field.
| elif self.action == "upload_image": | ||
| return MovieImageSerializer |
There was a problem hiding this comment.
Ensure that the MovieImageSerializer is correctly handling the image upload. The upload_to parameter in the Movie model's ImageField should reference the create_custom_path function correctly to avoid any issues during image upload.
| MEDIA_ROOT = BASE_DIR / "media" | ||
|
|
||
| MEDIA_URL = "/media/" |
There was a problem hiding this comment.
The MEDIA_ROOT and MEDIA_URL settings are correctly configured for handling media files. Ensure that the directory specified in MEDIA_ROOT exists and is writable by the web server to avoid issues with file uploads.
| migrations.AlterModelManagers( | ||
| name="user", | ||
| managers=[ | ||
| ("objects", user.models.UserManager()), |
There was a problem hiding this comment.
Ensure that the UserManager class is defined in the user.models module and is correctly implemented to handle user creation and management without the username field.
| msg = _('Must include "username" and "password".') | ||
| raise serializers.ValidationError(msg, code="authorization") |
There was a problem hiding this comment.
The error message in the validate method of AuthTokenSerializer should refer to 'email' instead of 'username', as the authentication is based on email. Change the message to _('Must include "email" and "password".')
There was a problem hiding this comment.
Great job on implementing the features! 🎉 While the Pull Request is approved to help you move forward with your learning, there are a few critical issues that need attention:
-
ImageField in Movie Model: The
upload_toparameter should reference thecreate_custom_pathfunction without quotes, as it is a callable. Please change'create_custom_path'tocreate_custom_path. -
AuthTokenSerializer Validate Method: The error message in the
validatemethod should refer to "email" instead of "username". Update the message to _('Must include "email" and "password".') to align with the authentication process. -
Upload Path Function: Ensure the
create_custom_pathfunction is defined to avoid runtime errors during image uploads.
These changes are crucial for the functionality of your code. Keep up the good work and continue improving your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| name='image', | ||
| field=models.ImageField(null=True, upload_to='create_custom_path'), |
There was a problem hiding this comment.
The upload_to parameter should reference the create_custom_path function without quotes, as it is a callable. Change 'create_custom_path' to create_custom_path.
| msg = _('Must include "username" and "password".') | ||
| raise serializers.ValidationError(msg, code="authorization") |
There was a problem hiding this comment.
The error message should refer to 'email' instead of 'username'. Change the message to _('Must include "email" and "password".') to align with the authentication process.
No description provided.