Space - Charlotte & Antonia #15
Conversation
…for movies and customers
…ests. still working on create
…deo controller test
… typos causing errors
…sts to pass. still some failures
…t some smoke/unit tests to pass
…ypo in migrations during setup. No other changes were made to tests. All smoke tests pass now
… validations (thanks dee)
|
NOTE: Because of an initial error in our migrations which we were told would be better left alone, we changed "video_id" in the Wave 2 smoke tests to "videos_id" which helped us to get them to pass. Dee told us this was okay, and they were aware of the initial typo because they helped us troubleshoot some earlier issues, as well as some issues in the end. That is the only modification we made to those tests. |
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
SummaryNote for the comprehension questions that the time complexity of check-in could be O(1) if the check-in action has the id of the rental to check in, as it can look up things directly. You also need to work much more on testing, both model and controller testing. See my inline comments, but the biggest problem is that you aren't covering the edge cases around the check-in and check-out logic. Further you don't have much model testing. That's a bit problematic and I highly encourage you to focus on through testing in the bEtsy project. That said you do satisfy the requirements so this is a green result. |
| return | ||
| end | ||
|
|
||
| if rental.save |
There was a problem hiding this comment.
What about if rental.save returns false? You should have some kind of response with an error response code here.
| }, status: :not_found | ||
| return | ||
| end | ||
| render json: video.as_json(only: [:title, :overview, :release_date, :total_inventory, :available_inventory]) |
| def decrease_inventory | ||
| self.available_inventory -= 1 | ||
| self.save | ||
| end | ||
|
|
||
| def increase_inventory | ||
| self.available_inventory += 1 | ||
| self.save | ||
| end |
There was a problem hiding this comment.
This works, but I would suggest instead writing a method to check the available inventory by the number checked out and the number total, rather than make a separate field for available inventory.
| describe CustomersController do | ||
| CUSTOMER_FIELDS = ["id", "name", "registered_at", "postal_code", "phone", "videos_checked_out_count"].sort | ||
|
|
||
| it "must get index" do |
|
|
||
| describe RentalsController do | ||
|
|
||
| describe "index" do |
There was a problem hiding this comment.
You should also have a test when there are no rentals.
| @@ -0,0 +1,18 @@ | |||
| require "test_helper" | |||
|
|
|||
| describe Video do | |||
There was a problem hiding this comment.
Noting that you only have validations tests and only one really at that...
| @@ -0,0 +1,10 @@ | |||
| Rails.application.routes.draw do | |||
|
|
||
| # Act | ||
| get customers_path | ||
|
|
| end | ||
| end # describe index end | ||
|
|
||
| describe "check_out" do |
There was a problem hiding this comment.
You should also test what happens if you check out more copies than are in the inventory...
| end # describe check-out end | ||
|
|
||
|
|
||
| describe "check_in" do |
There was a problem hiding this comment.
What happens when you check-in a video which isn't currently checked out?
3 participants
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.