Emilce and Maja Octos #23
Conversation
… into development
… into development
…stom validation method
… into development
Video StoreWhat We're Looking For
|
| validates :address, presence: true | ||
| validates :city, presence: true | ||
| validates :state, presence: true | ||
| validates :postal_code, presence: true |
There was a problem hiding this comment.
You're over-validating here. Your app will not break if the phone number or zip code is missing, and spending the time to validate, test the validations, and make sure your other tests respect them is more effort than you should be spending, especially for a school project where working with models is not the core learning goal.
| # if rental is back, due_date is nil | ||
| def available_inventory | ||
| unavailable = 0 | ||
| self.rentals.each do |rental| |
There was a problem hiding this comment.
I like that you calculate this on the fly rather than storing the value.
|
|
||
| # calculates and returns a movie's available inventory | ||
| # if rental is out, then due_date is a Date object | ||
| # if rental is back, due_date is nil |
There was a problem hiding this comment.
I'm not sure that I agree with this design decision. Having information about the due dates of old rentals might be useful, especially in a real-world version of this app. For the context of this project it works fine, but destroying this information makes me nervous.
| private | ||
| def available? | ||
| if self.movie.available_inventory <= 0 | ||
| errors.add(:movie_id, "This movie is not in stock.") |
There was a problem hiding this comment.
Nice work with this custom validation!
| # rental = Rental.where(movie_id: movie_id, customer_id: customer_id).where.not(due_date: nil).first | ||
|
|
||
| rental = Rental.where.not(due_date: nil).where(movie_id: movie_id, customer_id: customer_id).order(checkout_date: :desc).first | ||
| # rental = Rental.where(movie_id: movie_id, customer_id: customer_id).order(checkout_date: :desc).where.not(due_date: nil) |
There was a problem hiding this comment.
This is a really complex database query! Could you encapsulate it as a self method on Rental? Maybe something like Rental.outstanding(movie_id, customer_id)
|
|
||
| # increase the customer's checked-out movie number | ||
| customer.movies_checked_out_count += 1 | ||
| customer.save |
There was a problem hiding this comment.
You do a lot of work in this function: calculating dates, creating a Rental, increasing the movie count. It would be good to encapsulate all that in one model method.
| end | ||
| else | ||
| render json: {errors: { id: ["Must enter a valid movie and customer"]}}, status: :bad_request | ||
| end |
There was a problem hiding this comment.
Here you should probably specify which field is bad. id by itself doesn't correspond to either of the parameters the client sent.
Dear Reviewer: did we do error handling correctly (ie, is this fine for the next iteration of this assignment)? What could we have done better with error handling and inventory checking?
Comprehension Questions