-
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 #700
base: master
Are you sure you want to change the base?
solution #700
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.
GJ!
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 you've done a great job overall, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Syntax Error in
Ticket
Class: There's a syntax error in the__str__
method due to an incorrect line continuation character. This needs to be fixed to ensure the code runs correctly. -
validate_ticket
Method: The call tovalidate_ticket
is missing theerror_to_raise
parameter. This is required according to the method definition, so please ensure it's included. -
Security Concern in
settings.py
: TheSECRET_KEY
is hardcoded, which is a security risk. Consider using environment variables to manage sensitive information securely. -
Test Cases for API Methods: The
test_put_movie
andtest_delete_movie
methods expect a405 Method Not Allowed
response. Please verify that the API is configured correctly to disallow these requests if that's the intended behavior, or adjust the tests accordingly.
Once these issues are resolved, your code will be in great shape! Keep up the fantastic 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.
cinema/models.py
Outdated
return f"{str(self.movie_session)} "\ | ||
f"(row: {self.row}, seat: {self.seat})" |
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.
There is a syntax error in the __str__
method of the Ticket
class. The line continuation character \
should be placed at the end of line 123, not at the beginning of line 124. This will fix the syntax error.
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.
😭 whyyyy
Ticket.validate_ticket(attrs["row"], attrs["seat"], | ||
attrs["movie_session"]) |
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 validate_ticket
method call is missing the error_to_raise
parameter. According to the validate_ticket
method definition in the Ticket
model, this parameter is required. You should pass ValidationError
as the fourth argument to match the method signature.
self.assertEqual( | ||
response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED) |
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 test_put_movie
method expects a 405 Method Not Allowed
response. Ensure that the API is configured to disallow PUT requests on the movie detail endpoint if this is the expected behavior. Otherwise, adjust the test to match the actual API behavior.
self.assertEqual( | ||
response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED) |
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 test_delete_movie
method expects a 405 Method Not Allowed
response. Verify that the API is set up to disallow DELETE requests on the movie detail endpoint. If DELETE requests are allowed, update the test to reflect the correct expected status code.
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.
agree, usually delete methor returns 302
status
"django-insecure-6vubhk2$++agnctay_4" | ||
"pxy_8cq)mosmn(*-#2b^v4cgsh-^!i3" |
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 SECRET_KEY
is hardcoded and visible in the settings file. For security reasons, it's important to keep this key secret in production. Consider using environment variables or a separate configuration file to manage sensitive information.
user/serializers.py
Outdated
read_only = ( | ||
"id", | ||
"is_staff", | ||
) |
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 read_only
attribute in the Meta
class should be read_only_fields
. This is the correct attribute name for specifying fields that should be read-only in Django REST Framework serializers.
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.
Great job on your submission! 🎉 You've made significant progress in addressing the issues identified in your code. Here are some key points to focus on:
-
Syntax Error in
Ticket
Class: Ensure that the syntax error in the__str__
method is corrected by placing the line continuation character\
at the end of the appropriate line. This will ensure your code runs smoothly. -
validate_ticket
Method: Make sure to include theerror_to_raise
parameter in thevalidate_ticket
method call. This is crucial for handling validation errors correctly in yourTicketSerializer
. -
Security Concern in
settings.py
: The hardcodedSECRET_KEY
is a security risk. It's important to use environment variables to manage sensitive information securely, especially in production environments. -
Test Cases for API Methods: Verify that the
test_put_movie
andtest_delete_movie
methods align with the intended API behavior. If the API is configured to disallow PUT and DELETE requests, ensure the tests reflect this. Otherwise, adjust the tests accordingly.
Your effort in addressing these issues is commendable. Keep up the good work, and remember to review the comments to further improve your code. Addressing these points will enhance the security and functionality of your application. Keep learning and growing! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
cinema/models.py
Outdated
return f"{str(self.movie_session)} "\ | ||
f"(row: {self.row}, seat: {self.seat})" |
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.
There is a syntax error in the __str__
method of the Ticket
class. The line continuation character \
should be placed at the end of line 123, not at the beginning of line 124. This will fix the syntax error.
Ticket.validate_ticket(attrs["row"], attrs["seat"], | ||
attrs["movie_session"]) |
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 validate_ticket
method call is missing the error_to_raise
parameter. According to the method definition in the Ticket
model, this parameter is required. Please ensure to pass the appropriate exception class, such as serializers.ValidationError
, to handle validation errors correctly.
self.assertEqual( | ||
response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED) |
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 test_put_movie
method expects a 405 Method Not Allowed
response. Please verify that the API is configured to disallow PUT requests on the movie endpoint if this is the intended behavior. Otherwise, adjust the test to match the actual API behavior.
self.assertEqual( | ||
response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED) |
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 test_delete_movie
method expects a 405 Method Not Allowed
response. Please verify that the API is configured to disallow DELETE requests on the movie endpoint if this is the intended behavior. Otherwise, adjust the test to match the actual API behavior.
"django-insecure-6vubhk2$++agnctay_4" | ||
"pxy_8cq)mosmn(*-#2b^v4cgsh-^!i3" |
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 SECRET_KEY
is hardcoded, which poses a security risk. Consider using environment variables to manage sensitive information securely, especially for production environments. This aligns with best practices for Django projects.
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.
I just 😭 💀
cinema/models.py
Outdated
user = models.ForeignKey(settings.AUTH_USER_MODEL, | ||
on_delete=models.CASCADE) |
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.
why?
cinema/models.py
Outdated
return f"{str(self.movie_session)} "\ | ||
f"(row: {self.row}, seat: {self.seat})" |
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.
😭 whyyyy
cinema/serializers.py
Outdated
genres = serializers.SlugRelatedField(many=True, | ||
read_only=True, | ||
slug_field="name") |
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.
💀
self.assertEqual( | ||
response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED | ||
) | ||
self.assertEqual(response.status_code, status.HTTP_405_METHOD_NOT_ALLOWED) |
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.
agree, usually delete methor returns 302
status
No description provided.