Kasey && Shubha #28
Kasey && Shubha #28
Conversation
…into model-testing
…into model-testing
…ovie by the same user
Video StoreWhat We're Looking For
|
| def index | ||
| customers = Customer.all.map { |customer| customer.as_json(only: [:id, :name, :registered_at, :address, :city, :state, :postal_code, :phone]).merge({ "movies_checked_out_count": customer.checked_out_count }) } | ||
| render status: :ok, json: customers.as_json | ||
| end |
There was a problem hiding this comment.
I would definitely encourage you to split line 3 up. Most style guides encourage no more than 80 characters on a line.
|
|
||
| def customer_params | ||
| params.permit(:name, :registered_at, :address, :city, :state, :postal_code, :phone) | ||
| end |
There was a problem hiding this comment.
I don't think this method is ever used.
| rental = Rental.new(movie_id: params[:movie_id], | ||
| customer_id: params[:customer_id], | ||
| checkout: DateTime.now, | ||
| due: DateTime.now + 7.days) |
There was a problem hiding this comment.
This code is pretty concise, but it might work even better as a model method, something like:
Rental.check_out(movie_id, customer_id)
That means you don't need the date logic here in the controller, and that you could include some targeted model testing for it.
|
|
||
| def checked_out_count | ||
| return self.rentals.select { |rental| !rental.returned }.count | ||
| end |
There was a problem hiding this comment.
I love that you made Customer#checked_out_count and Movie#number_available methods instead of storing them in the database. That way there's no way to forget to update these values.
| describe "checkout" do | ||
| it "customer can checkout and create a new rental" do | ||
| expect { | ||
| post checkout_path, params: @params |
There was a problem hiding this comment.
Are there some failure cases you could cover here? What if that movie isn't available?
| describe "checkin" do | ||
| it "customer can checkin a rental" do | ||
| post checkout_path, params: @params | ||
|
|
There was a problem hiding this comment.
What if no outstanding rental exists for that customer / movie?
|
|
||
| it "can return the number currently available" do | ||
| expect(movie.number_available).must_equal 0 | ||
| end |
There was a problem hiding this comment.
I would also like to see that this works when there are some movies available
| it "can return the number of movies currently checked out" do | ||
| expect(customer.checked_out_count).must_equal 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
I would stick this in a separate describe. There might be an interesting edge case if there are no movies checked out.
3 participants
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.POST /rentals/check-inendpoint time complexity is O(n) because the find_by method iterates through each value in the table until it finds one where all three of the column values that were given as arguments tofind_bymatch.movies#showmethod check to see if the correct movie is returned when a valid id is given and whether an error message is returned when an invalid id is given.