-
Notifications
You must be signed in to change notification settings - Fork 25
Angelica Ceja - Wini Irarrazaval - Octos #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
… modify tests accordingly
Video StoreWhat We're Looking For
|
|
|
||
| if rental.save | ||
| customer = Customer.find_by(id: customer_id) | ||
| customer.movies_checked_out_count += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this work around assigning dates, decreasing movies_checked_out_count, etc. should be encapsulated in one testable model method.
| if rental.save | ||
| customer = Customer.find_by(id: rental.customer_id) | ||
| customer.movies_checked_out_count -= 1 | ||
| customer.save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, all this work ought to be wrapped up in one model method.
| def checkin | ||
| rental = Rental.find_by(movie_id: params[:movie_id], customer_id: params[:customer_id]) | ||
|
|
||
| unless rental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this rental has already been returned?
| def get_available_inventory | ||
| checked_out_count = 0 | ||
| self.rentals.each do |rental| | ||
| if rental.return_date.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you calculate this on the fly rather than saving it in the DB.
| def enough_inventory_for_rent | ||
| if movie | ||
| count = 0 | ||
| self.movie.rentals.each do |rent| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this validation interact with returning movies?
|
|
||
| it "must respond with bad_request for a movie with no available inventory" do | ||
| movie = Movie.first | ||
| customer = Customer.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
|
||
| it "must respond with not_found for a rental that DNE" do | ||
|
|
||
| movie_id = Movie.last.id + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the rental has already been checked in
|
|
||
| end | ||
|
|
||
| describe "relationships" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some custom methods you're not testing here.
|
|
||
| it "a rental can not be completed if all copies are rented for a date range" do | ||
| movie = Movie.first | ||
| customer = Customer.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work getting a test in on this.
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