Queues - Erica Case, Hyunji Kim - VideoStoreAPI #23
Conversation
…of the same movie
Video StoreWhat We're Looking For
|
There was a problem hiding this comment.
Hi, I'm Erin from C6 just helping out my pals who were assigned to review this but are swamped right now. You're probably swamped too with internship interviews (good luck!); but I wanted you to have feedback to look over whenever you're ready. You're also probably going to look at this and say '24 comments - wtf?!' Don't worry. They're just little things, things I've been given feedback on in the past, things to consider more broadly as you head into industry, and a few caught typos. Overall, ya'll should be super proud of the huge amount of work you put it. You crushed it! If you have any questions, please let me know - happy to chat more about comments, etc.
| 1. Then Select `Add` | ||
|  | ||
| 1. Lastly add a key `url` and value `http://localhost:3000` | ||
|  | ||
| 1. Click the blue `Run` button. This will launch the collection runner. | ||
| 1. In the collection runner, scroll down in the center pane and click the blue `Start Test` button | ||
|
|
There was a problem hiding this comment.
⭐️ for including a README! Documentation - especially good documentation - is so often overlooked and is super useful!
| if rentals == [] | ||
| render json: {errors: "Cannot Find the Rental Records"}, status: :not_found | ||
| else | ||
| not_returned = rentals.where(returned_date: nil) |
There was a problem hiding this comment.
nit: I've gotten feedback in industry that it is often more readable to use a positive name for clarity (e.g. checked_out rather than not_returned or signed_out rather than not_signed_in). Could just a be personal preference, though!
| if not_returned == [] | ||
| render json: {errors: "All movies are already returned"}, status: :not_found | ||
| else | ||
| #if the customer has checked out mutiple copies of the same movie, check in the movie with the earliest checkout date first. |
There was a problem hiding this comment.
interesting edge case here - good catch!
| @@ -0,0 +1,39 @@ | |||
| class RentalsController < ApplicationController | |||
| def zomg | |||
There was a problem hiding this comment.
makes sense here, but in the future when you're awesome devs working in big codebases, I would expect checks like this to be removed in production code. Same idea for the debuggers in your movie model.
| end | ||
|
|
||
| def overdue | ||
| if Rental.all.empty? |
There was a problem hiding this comment.
Not sure if you need .all here? Also you might consider syncing on consistency of .empty? or == [] or .nil? Each is used throughout your code.
| must_respond_with :ok | ||
| end | ||
|
|
||
| it "returns 404 if overdue rentals don't exit" do |
There was a problem hiding this comment.
typo - exist
Now that I think about it, 404 might not be the response you want here. 404 means that the information you were looking for was not found - there's some kind of problem. What you want to be saying here is that the information/category was found, but that there are zero things in the category. Might make more sense to give the user some kind of message: "There are no overdue rentals at this time".
|
|
||
| tusk: | ||
| title: Tusk | ||
| overview: Walrus Horror Film |
| it "can have many movies" do | ||
| customer = customers(:one) | ||
| customer.movies.count.must_equal 3 | ||
| end |
There was a problem hiding this comment.
This seems strange to me. What does it mean to "have a movie" if not to rent it?
| movie.valid?.must_equal false | ||
| movie.errors.messages.must_include :title | ||
| end | ||
|
|
| movie.available_inventory.must_equal 0 | ||
| end | ||
|
|
||
| it "does not subtract movies that have been checked in" do |
There was a problem hiding this comment.
test for checking in a movie? should increase inventory?
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