Solution #665
Solution #665
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! While you've made great progress, there are a few critical issues that need to be addressed before we can approve it:
-
Permission Class Implementation: The
IsAdminOrIfAuthenticatedReadOnlypermission class needs to be adjusted to ensure administrators have full access while regular authenticated users have read-only access. This is crucial for meeting the core requirements of the task. -
ViewSet Permissions: The
GenreViewSet,CinemaHallViewSet, andActorViewSetshould allow both list and create actions for all users, not just admins. This misalignment with the task requirements affects functionality. -
Missing Tests: There are no tests provided for
user/testsandcinema/tests. Tests are essential to ensure the functionality works as expected and meets the task requirements.
Please address these issues to ensure the code meets the core requirements and functions correctly. Once these are resolved, we'll be happy to take another look. 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/permissions.py
Outdated
| return bool( | ||
| request.method | ||
| in SAFE_METHODS |
There was a problem hiding this comment.
The check request.method in SAFE_METHODS is redundant here because it is already checked in the if statement on line 11. You can simplify the logic by removing the repeated check within the bool() function.
cinema/views.py
Outdated
| return MovieSessionSerializer | ||
|
|
||
|
|
||
| class OrderPagination(PageNumberPagination): | ||
| page_size = 10 | ||
| max_page_size = 100 | ||
| permission_classes = (IsAuthenticated,) |
There was a problem hiding this comment.
The OrderPagination class should not have permission_classes defined. Pagination classes do not handle permissions, so this line should be removed.
cinema/views.py
Outdated
|
|
||
|
|
||
| class MovieViewSet(viewsets.ModelViewSet): | ||
| queryset = Movie.objects.prefetch_related("genres", "actors") | ||
| serializer_class = MovieSerializer | ||
| permission_classes = [IsAuthenticated] | ||
| http_method_names = ["get", "post"] |
There was a problem hiding this comment.
The http_method_names in MovieViewSet should include 'put' and 'patch' to allow updating movies, as specified in the task requirements.
| @@ -5,4 +5,5 @@ | |||
| path("admin/", admin.site.urls), | |||
| path("api/cinema/", include("cinema.urls", namespace="cinema")), | |||
There was a problem hiding this comment.
Ensure that the cinema.urls module defines a namespace 'cinema' since you are using namespace="cinema" in the include function.
| @@ -5,4 +5,5 @@ | |||
| path("admin/", admin.site.urls), | |||
| path("api/cinema/", include("cinema.urls", namespace="cinema")), | |||
| path("__debug__/", include("debug_toolbar.urls")), | |||
| path("api/user/", include("user.urls", namespace="user")), | |||
There was a problem hiding this comment.
Ensure that the user.urls module defines a namespace 'user' since you are using namespace="user" in the include function.
There was a problem hiding this comment.
Great job on implementing several key aspects of the task! 🎉 Your work on the IsAdminOrIfAuthenticatedReadOnly permission class and the UserSerializer is spot on, ensuring secure and appropriate access control and user data handling. However, there are a few critical areas that need attention:
-
ViewSet Permissions: The
GenreViewSet,CinemaHallViewSet, andActorViewSetcurrently restrict thecreateaction to administrators only. The task requires that both list and create actions be accessible to all users. Please adjust the permissions to align with this requirement. -
Missing Tests: There are no test files provided for
user/testsandcinema/tests. Tests are crucial for verifying that your code functions as expected and meets the task requirements. Make sure to add these to ensure robustness.
Remember, addressing these issues will not only help you meet the task requirements but also enhance your coding skills. Keep up the good work, and use this feedback to improve your implementation. Your progress is commendable, and with a few adjustments, your solution will be even stronger. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| if self.action == "create": | ||
| return [IsAdminUser()] | ||
| return [IsAuthenticated()] |
There was a problem hiding this comment.
The get_permissions method in GenreViewSet restricts the create action to admins only. According to the task requirements, both list and create actions should be accessible to all users, not just admins. Consider removing the permission restriction for the create action.
| if self.action == "create": | ||
| return [IsAdminUser()] | ||
| return [IsAuthenticated()] |
There was a problem hiding this comment.
The get_permissions method in ActorViewSet restricts the create action to admins only. As per the task requirements, both list and create actions should be accessible to all users. Consider adjusting the permissions to allow all users to create.
| if self.action == "create": | ||
| return [IsAdminUser()] | ||
| return [IsAuthenticated()] |
There was a problem hiding this comment.
The get_permissions method in CinemaHallViewSet restricts the create action to admins only. The task specifies that both list and create actions should be available to all users. Please modify the permissions to meet this requirement.
No description provided.