-
Notifications
You must be signed in to change notification settings - Fork 27
Jocelyn & Vera - Time #2
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
base: master
Are you sure you want to change the base?
Conversation
…gic to check if video is available
…create a new video
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Additional FeedbackGreat work y'all! The API is fully functional and the code is so clean! The only small area for growth I see is around getting more familiar/confident with HTTP status codes. The comprehension questions mentioned returning 5XX codes when the user gives bad input, which is inaccurate. Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| expect(video.available_inventory).must_equal inventory_count | ||
| expect(customer.videos_checked_out_count).must_equal video_count |
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 think I might be missing something but I'm confused why these numbers aren't meant to change when the user checks in a rental.
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.
From looking at the source code in the controller, I'm surprised and confused why this passes. I must be reading something wrong.
| render json: { | ||
| ok: false, | ||
| errors: rental.errors.messages, | ||
| }, status: :bad_request |
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's actually no chance for the user input to be invalid in this case because we've already validated that we can find the customer and video specified by the user.
If we are going to add an else block here, we should actually return an :internal_server_error because the failure to save would be due to something wrong with the database.
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.
Please reach out to me if this comment is unclear! :)
| render json: { | ||
| ok: false, | ||
| errors: rental.errors.messages, | ||
| }, status: :bad_request |
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.
Similar to with checkout, this would be more accurate as :internal_server_error
| else | ||
| render json: { | ||
| errors: @video.errors.messages, | ||
| }, status: :bad_request |
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.
Here it is appropriate to respond with a :bad_request status because a failure to save is most likely due to failed validations.
Technically, it's also possible that the save fails due to a connection failure, even if the validations pass, but that's more depth of error-handling than we've gotten into in our curriculum.
Assignment Submission: Video Store API
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
If you didn't get to the functionality the question is asking about, reply with what you would have done if you had completed it.
Reflection
/customers&/videosendpoints? What does the time complexity depend on? Explain your reasoning.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.