Kate S. and Kelly #30
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kes/rental tests
Add movies routes, index & show actions and tests
Controller for customers
Creates rental controller
Migrations
Checkout branch
Add movie and cutomer ids to rental
Update finding customer and movie
Revise checkin action and add tests
Add movie controller tests
Clean up merge
Video StoreWhat We're Looking For
|
CheezItMan
reviewed
May 24, 2019
| has_many :rentals | ||
|
|
||
| after_create do |movie| | ||
| movie.available_inventory = movie.inventory |
There was a problem hiding this comment.
This available_inventory makes better sense as a method instead of as a field. That way you don't have to engage in this chicanery.
Instead I suggest putting available_inventory method and movies_checked_out_count as methods in movie.rb and customer.rb.
| it "returns JSON and the route is working" do | ||
| get customers_path, as: :json | ||
|
|
||
| expect(response.header["Content-Type"]).must_include "json" |
There was a problem hiding this comment.
You should also test the required keys.
| body.length.must_equal Movie.count | ||
| end | ||
|
|
||
| it "returns movies with exactly the required fields" do |
| } | ||
| } | ||
|
|
||
| describe "checkout" do |
There was a problem hiding this comment.
What about checking out a movie with no available inventory left?
| end | ||
|
|
||
| describe "check-in" do | ||
| let(:checkin_params) { |
There was a problem hiding this comment.
What if you check in a movie that's not checked out?
| if !customer | ||
| render json: { "errors": { "customer": ["Customer not found"] } }, status: :bad_request | ||
| else | ||
| checkout_date = Date.today |
There was a problem hiding this comment.
Look at how much code is here... This would indicate a good case for a model method.
| rental = Rental.find_by(customer_id: rental_params[:customer_id], movie_id: rental_params[:movie_id]) | ||
|
|
||
| if rental | ||
| rental.movie.available_inventory += 1 |
There was a problem hiding this comment.
This business logic would look lovely in a model.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Video Store API
Congratulations! You're submitting your assignment!
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.
Comprehension Questions
POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.