-
Notifications
You must be signed in to change notification settings - Fork 25
Video Store _ Christina & Maryam #24
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
Video StoreWhat We're Looking For
|
|
|
||
| def create | ||
| @movie = Movie.new(movie_params) | ||
| @movie.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.
You should return a json response if the move wasn't able to be saved. Provide error messages.
| rental = Rental.checkout!(rental_params[:customer_id], rental_params[:movie_id]) | ||
| render json: { ok: true, message: 'Checkout successful!' }, status: :ok | ||
| else | ||
| render json: { ok: false, message: 'Unable to checkout' }, |
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.
This wouldn't be a not found but rather a bad request.
| end | ||
|
|
||
| rescue ArgumentError | ||
| render json: { ok: false, message: 'Unable to checkout: movie or customer does not exist' }, status: :not_found |
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.
Again, not_found is probably not the right response here.
|
|
||
| it "has rentals" do | ||
|
|
||
| movie.rentals.new |
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 is this line trying to accomplish?
|
|
||
| movie.rentals.new | ||
|
|
||
| expect (movie.rentals.length).must_be :>=, 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.
I would also add a test that rentals is a collection of Rental instances.
|
|
||
| end | ||
|
|
||
| it 'creates a movie given correct params' 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.
negative cases?
| } | ||
|
|
||
| post rentals_checkout_path(rental.id), {:params => rental_params} | ||
| rental.reload |
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.
You also need negative cases as well.
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.