-
Notifications
You must be signed in to change notification settings - Fork 702
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
Develop #730
base: master
Are you sure you want to change the base?
Develop #730
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.
Great job on implementing the cinema application! 🎬 While there are a few minor issues like the use of ValueError
instead of ValidationError
in the Ticket
model, a leftover print statement in the tests, and some code style improvements needed, these do not significantly impact the functionality of your code. Keep up the good work, and consider addressing these minor points to enhance your code quality even further! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
cinema/models.py
Outdated
row=self.row, | ||
seat=self.seat, | ||
movie_session=self.movie_session, | ||
error_to_raise=ValueError |
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.
In the clean
method of the Ticket
model, ValueError
is used as the error_to_raise
parameter. However, for validation errors in Django, you should use ValidationError
instead of ValueError
. This ensures that the error is properly handled by Django's validation system.
@@ -83,7 +83,8 @@ def test_get_movie_sessions_filtered_by_movie(self): | |||
|
|||
def test_get_movie_sessions_filtered_by_movie_and_data(self): | |||
movie_sessions = self.client.get( | |||
f"/api/cinema/movie_sessions/?movie={self.movie.id}&date=2022-09-2" | |||
f"/api/cinema/movie_sessions/?movie=" | |||
f"{self.movie.id}&date=2022-09-2" |
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 date format in the query string should be zero-padded for the day part. Change 2022-09-2
to 2022-09-02
to maintain consistency and avoid potential issues with date parsing.
cinema/tests/test_order_api.py
Outdated
@@ -82,6 +82,7 @@ def test_movie_session_detail_tickets(self): | |||
|
|||
def test_movie_session_list_tickets_available(self): | |||
response = self.client.get(f"/api/cinema/movie_sessions/") | |||
print(response.data) |
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.
Remove the print(response.data)
statement. Debug print statements should be removed from the final version of test cases to keep the output clean and focused on the test results.
cinema_service/settings.py
Outdated
|
||
# SECURITY WARNING: don't run with debug turned on in production! | ||
DEBUG = True | ||
DEBUG = os.getenv("DEBUG", "True") == "True" |
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 DEBUG
setting defaults to 'True'. It's important to ensure that this is set to 'False' in production environments to avoid exposing sensitive information. Consider using a more secure default or ensuring the environment variable is correctly set in production.
No description provided.