Conversation
…troller testing of video
…t, also reflected changes in rental controller and test
…tal#check_in and rental#check_out. Wrote corresponding tests, passing
Video Store APIMajor Learning Goals/Code Review
Functional Requirements
Overall Feedback
Additional FeedbackGood work y'all! I've left a few inline comments about some things I noticed but they are just a few smaller details. If any of the comments aren't clear, please reach out to me! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| def set_videos_checked_out_count | ||
| self.customer.checked_out_video | ||
| self.videos_checked_out_count = self.customer.videos_checked_out_count | ||
| end |
There was a problem hiding this comment.
considering the videos_checked_out_count in rental mimics the value from the customer model, it would be safer and less work to not bother having such a value on the rental model.
This means you don't need so much code in the rental model and reduces the risk that they become mismatched somehow, introducing a bug.
| def set_available_inventory | ||
| self.video.checked_out | ||
| self.available_inventory = self.video.available_inventory | ||
| end |
There was a problem hiding this comment.
The same could be said here about available_inventory as I said in my comment above about videos_checked_out_count,
| describe Customer do | ||
| describe "validations" do | ||
| before do | ||
| @customer = customers(:Kenji) |
There was a problem hiding this comment.
This isn't working for a couple reasons. I've added a code suggestion to demonstrate what it should look like to work properly.
| @customer = customers(:Kenji) | |
| @customer = customer(:customer_2) |
There was a problem hiding this comment.
However, after updating this, I still get the following error for the validation test:
NoMethodError: undefined method 'checked_out_count' for #<Array:0x00007f93a52c8130>
There was a problem hiding this comment.
I'm guessing y'all were aware these were failing and just didn't have time to fix them.
In case you were unaware they were failing, I want to remind you that it's important to run tests after writing them to make sure they work and it's important to run all tests before submitted a pull request to make sure all tests pass.
| describe "custom methods" do | ||
| describe "checked_out_movie" do | ||
| it "will increase checked_out_movie by one" do | ||
| expect{ customer.checked_out_movie }.must_differ "customer.checked_out_count", 1 | ||
| end | ||
| end | ||
|
|
||
| describe "checked_in_movie" do | ||
| it "will decrease movies_checked_out_count by one" do | ||
| expect{ customer.checked_in_movie }.must_differ "customer.checked_out_count", -1 | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
These are accidentally nested within the "Validations" describe block.
| expect { | ||
| post check_out_path, params: good_rental_data | ||
| }.must_differ 'Rental.count', 1 | ||
| must_respond_with :created |
There was a problem hiding this comment.
This test fails because the controller returns :success rather than :created.
It looks like y'all divided up who was doing the test and source code for this and maybe didn't run the tests after putting them together.
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.