-
Notifications
You must be signed in to change notification settings - Fork 693
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Solution #718
base: master
Are you sure you want to change the base?
Solution #718
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this project! 😊 While your code is mostly well-implemented and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Undefined Method: The
MovieSessionViewSet
uses a static method_params_to_ints
which is not defined within the class. This could lead to errors and needs to be resolved to ensure the functionality works as expected. -
Inconsistent Permissions: The
MovieSessionViewSet
andOrderViewSet
have different permission classes (IsAdminOrIfAuthenticatedReadOnly
andIsAuthenticated
, respectively). Please verify that these permissions align with the intended access control policies for each viewset to ensure consistent and secure access control. -
Security Concerns in
settings.py
: TheDEBUG
mode is set toTrue
, and theSECRET_KEY
is hardcoded. These are critical issues for production environments and should be addressed by using environment variables or a configuration management tool. -
File Management: There are duplicate files for
user/views.py
anduser/urls.py
, and unrelated files included in the submission. Please clean up these files to avoid confusion and ensure only relevant files are included.
Once these issues are resolved, 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.
@@ -87,6 +115,8 @@ class MovieSessionViewSet(viewsets.ModelViewSet): | |||
) | |||
) | |||
serializer_class = MovieSessionSerializer | |||
authentication_classes = (TokenAuthentication,) | |||
permission_classes = (IsAdminOrIfAuthenticatedReadOnly,) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MovieSessionViewSet
uses IsAdminOrIfAuthenticatedReadOnly
for permission classes, while the OrderViewSet
uses IsAuthenticated
. Verify that these permissions align with the intended access control policies for each viewset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: 7]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
No description provided.