Implement token authentication #671
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! There are a few critical issues that need to be addressed before we can approve it:
-
Syntax Error in Import Statement: There's a syntax error in the import statement for permissions. Please correct it to
from rest_framework.permissions import BasePermission, SAFE_METHODS. -
Misplaced
permission_classes: Thepermission_classesattribute is incorrectly placed in theOrderPaginationclass. This should be moved to the appropriate viewset class. -
CreateTokenViewAuthentication: TheCreateTokenViewshould not haveauthentication_classesset toTokenAuthentication. This view is for obtaining a token, so authentication is not required. -
read_onlyOption: Theread_onlyoption should be corrected toread_only_fieldsin theUserSerializer. -
Namespace Configuration: Ensure that both
cinema.urlsanduser.urlshaveapp_namedefined to work correctly with namespaces. -
Timezone Setting: It's recommended to set
USE_TZ = Truefor timezone-aware datetimes, especially if your application will be used across multiple time zones.
Please address these issues to ensure the functionality and maintainability of the code. Once these changes are made, we can proceed with the approval. 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.
| @@ -0,0 +1,13 @@ | |||
| from rest_framework.permissions import BasePermission, SAFE_METHODS | |||
There was a problem hiding this comment.
There is a syntax error in the import statement. The correct syntax should be from rest_framework.permissions import BasePermission, SAFE_METHODS without the slash /.
cinema/views.py
Outdated
| @@ -116,14 +148,22 @@ def get_serializer_class(self): | |||
| class OrderPagination(PageNumberPagination): | |||
| page_size = 10 | |||
| max_page_size = 100 | |||
| permission_classes = (IsAuthenticated,) | |||
There was a problem hiding this comment.
The permission_classes attribute is misplaced in the OrderPagination class. Pagination classes typically do not handle permissions. Consider removing this line or placing it in the appropriate viewset class.
cinema_service/settings.py
Outdated
| @@ -127,7 +126,6 @@ | |||
|
|
|||
| USE_TZ = False | |||
There was a problem hiding this comment.
The USE_TZ setting is set to False. It's generally recommended to set USE_TZ = True to enable timezone-aware datetimes, especially if your application will be used in multiple time zones.
| @@ -4,5 +4,6 @@ | |||
| urlpatterns = [ | |||
| path("admin/", admin.site.urls), | |||
| path("api/cinema/", include("cinema.urls", namespace="cinema")), | |||
| path("api/user/", include("user.urls", namespace="user")), | |||
There was a problem hiding this comment.
The include function is used with a namespace argument, but the cinema.urls and user.urls modules must define an app_name attribute for namespaces to work correctly. Ensure that both cinema.urls and user.urls have app_name defined.
user/serializers.py
Outdated
| "password", | ||
| "is_staff", | ||
| ) | ||
| read_only = ["id", "is_staff"] |
There was a problem hiding this comment.
The read_only option should be read_only_fields. The correct syntax is read_only_fields = ['id', 'is_staff'] to specify fields that should be read-only.
user/views.py
Outdated
|
|
||
| class CreateTokenView(ObtainAuthToken): | ||
| renderer_classes = api_settings.DEFAULT_RENDERER_CLASSES | ||
| authentication_classes = (TokenAuthentication,) |
There was a problem hiding this comment.
The CreateTokenView should not have authentication_classes set to TokenAuthentication. The purpose of this view is to obtain a token, so authentication is not required. Consider removing this line.
There was a problem hiding this comment.
Review Summary
Great job on addressing most of the issues from the previous review! 🎉 Your code is almost there, but there are a few critical points to consider:
-
Import Statement Syntax Error: Ensure the import statement is corrected to
from rest_framework.permissions import BasePermission, SAFE_METHODSto avoid syntax errors. -
CreateUserViewAuthentication: Theauthentication_classesshould not be set for theCreateUserView, as it is unnecessary for user registration. Please remove it to ensure proper functionality. -
Namespace Configuration: Make sure both
cinema.urlsanduser.urlshaveapp_namedefined to work correctly with namespaces. This is crucial for thenamespaceargument in theinclude()function. -
Missing
cinema.urlsFile: The file forcinema.urlsis missing. Please include and configure it to ensure consistency and completeness.
Your progress is commendable, and these adjustments will help you refine your skills. Keep up the good work, and remember to address these issues in future projects to avoid similar problems. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| @@ -0,0 +1,13 @@ | |||
| from rest_framework.permissions import BasePermission, SAFE_METHODS | |||
There was a problem hiding this comment.
There's a syntax error in the import statement. It should be from rest_framework.permissions import BasePermission, SAFE_METHODS.
| @@ -4,5 +4,6 @@ | |||
| urlpatterns = [ | |||
| path("admin/", admin.site.urls), | |||
| path("api/cinema/", include("cinema.urls", namespace="cinema")), | |||
| path("api/user/", include("user.urls", namespace="user")), | |||
There was a problem hiding this comment.
Ensure that both cinema.urls and user.urls have app_name defined to work correctly with namespaces. This is necessary for the namespace argument to function properly.
|
|
||
| class CreateUserView(generics.CreateAPIView): | ||
| serializer_class = UserSerializer | ||
| authentication_classes = (TokenAuthentication,) |
There was a problem hiding this comment.
The authentication_classes should not be set for the CreateUserView, as it is not necessary for user registration.
No description provided.