Ann and Regan's Fine Fine Video Store API #3
Conversation
Yay! looks awesome
Rental post checkout
Video StoreWhat We're Looking For
|
|
Chris - Trello is public: https://trello.com/b/GmLbDpiz/videostore-api.
For the validation in question -- I was going for a test is to make sure
whatever is created with "good data" (data that satisfied the validations)
has parameters that are accessible. Probably could have combined with the
firs.t
Ann
…On Fri, May 19, 2017 at 9:16 AM, Regan Huff ***@***.***> wrote:
I think you have to make the Trello public for Cheezitman to see it...
Begin forwarded message:
*From:* Chris M ***@***.***>
*Date:* May 18, 2017 at 9:09:50 PM PDT
*To:* Ada-C7/VideoStoreAPI ***@***.***>
*Cc:* Regan Huff ***@***.***>, Author ***@***.***
>
*Subject:* *Re: [Ada-C7/VideoStoreAPI] Ann and Regan's Fine Fine Video
Store API (#3)*
*Reply-To:* Ada-C7/VideoStoreAPI <reply+014440879fe485b4704561a
00e554f7395af8d93384ee4b692cf000000011536320e92a169ce0d9d679
***@***.***>
Video Store What We're Looking For
Feature Feedback
*Core Requirements*
Git hygiene
Comprehension questions I can't get access to your Trello, *can you
reshare it with me and I'll revisit the project?*
*General*
Business Logic in Models Good work creating custom methods to calculate
values and provide data to the JSON.
All 3 required endpoints return expected JSON data Check
Requests respond with appropriate HTTP Status Code Check
Errors are reported Check, well done using errors hash. Also good work
using JSON to report the errors along with status codes.
*Testing*
Passes all Smoke Tests Check, well done
Model Tests - all relations, validations, and custom functions test
positive & negative cases You have a test, it "must be able to create
valid customer and return corresponding data" do, that I'm not sure what
it's actually testing. You do have lots of positive and negative tests.
Good use of Chronic in the rental_test.
Controller Tests - URI parameters and data in the request body have
positive & negative cases Check, positive and negative tests & also tests
for the correct order things are delivered.
*Optionals*
POST routes use URI parameter and request body to add a new record to the
database Nice work!
GET /customers shows how many movies are checked out by a customer Check,
nicely done
GET /movies/:title shows updated inventory Check
*Overall* Nicely done, you've done a great job with testing & had all the
options. Excellent work!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AURAh_ribE9KUOTGaSMQ5LZtbv-O9E1mks5r7RYOgaJpZM4NZ1St>
.
|
There was a problem hiding this comment.
Nice job on this! I think you did a really great job of designing your solution for the assignment and structuring it in a logical, intuitive way. Your code was really easy to follow and made sense to me right away. I've left a few comments on specific sections where I had questions or where you might be able to refactor for greater clarity or to make sure edge cases are covered. If you have any questions, feel free to let me know. Again, I think you did a really good job! :)
| render status: :bad_request, json: { error: "no customer found"} | ||
| else | ||
| # find the rental in question based on params[:title] and params[:customer_id] | ||
| rental = Rental.find_by(customer_id: customer.id, movie_id: movie.id) |
There was a problem hiding this comment.
I think this might not be a watertight way of identifying rentals. For instance, supposed a customer has three simultaneously checked-out copies of Jaws. I believe that find_by will only give you the one of these that happens to be first in the list. Since you don't currently limit retrieval by whether the rental is checked in, I think the same (already checked in) Jaws rental instance would be retrieved each time, making it impossible to check the other Jaws rental instances in. At least, this seems to be what I observe when I test via Postman. Not a big deal, but maybe something to look into if you plan to use this API in a future project.
| rental.save | ||
| # movie.inventory += 1 | ||
| # movie.save | ||
| render status: :ok, json: { nothing: true } |
There was a problem hiding this comment.
This might just be my own lack of familiarity with API development, but I found the {"nothing": true} JSON response a little confusing, especially since it could appear in the context of both error statuses and 200 OK statuses. It might be helpful for users of the API if you could customize the "nothing": true responses to provide a little more specific information, i.e., {"result": "Rental of Movie X by Customer Y was successfully checked in"}. Though this may be a convention/requirement of the assignment that I'm just not aware of :)
| if @rental.save | ||
| # movie.inventory -= 1 | ||
| # movie.save | ||
| render status: :ok, json: @rental.as_json |
There was a problem hiding this comment.
You might want to filter what's returned here to eliminate the created_at and updated_at timestamps from the database record. They seem to be in a different timezone than the local user's timezone (i.e., for me they said it was already tomorrow, "2017-06-05T03:08:04.102Z", while the checkout date was today as expected, "2017-06-04T00:00:00.000Z"). It's not a big deal but could be somewhat confusing from the API user's standpoint.
| #should subtract from the inventory the number of copies out in active rentals (checkin_date = nil) | ||
| rentals = Rental.where(movie_id: self.id, checkin_date: nil) | ||
| return self.inventory - rentals.size | ||
| end |
There was a problem hiding this comment.
Nice! This is an elegant way of making sure the available inventory always tracks with the rental database in its most updated form.
| # if you're sharing your code publicly. | ||
|
|
||
| development: | ||
| secret_key_base: df155bf0d4a6b976d3353d144d683fe2cc4a813adb6be6677373df15a64e12f4c269ef434ae3c1c3ecaf5232beb500d448ba39b72a9bab543ff1408ef864eec3 |
There was a problem hiding this comment.
Should this file be git ignored to avoid sharing these keys publicly? Or is that only an issue for the prod key (which I see is already hidden)?
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