Skip to content
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

Jackie & Olivia's VideoStoreAPI #13

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

secretsharer
Copy link

@secretsharer secretsharer commented May 12, 2017

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

Question Answer
Explain how you came up with the design of your ERD, based on the seed data. We started with the baseline relationships and added rental model in after the fact, using FK's of movie and customer id.
List all the completed endpoints of your application. GET movies, GET movies/:title, POST movies, GET customers, GET customers/:id, POST rentals/:titles/checkout, PATCH(or post) rentals/:title/checkin, GET zomg
Describe a set of positive and negative test cases you implemented for a model. customer - we checked that a new customer was valid with a name and phone number, invalid if no name or phone number included
Describe a set of positive and negative test cases you implemented for a controller. creating a valid movie will increase the db count, conversely, creating a movie with missing data will not change the db.
How does your API respond when bad data is sent to it? Responds with JSON with "errors", i.e., "that customer does not exist".
Describe one of your custom model methods and why you chose to wrap that functionality into a method. example - def decrease_available_inventory self.available_inventory = self.available_inventory - 1 end, to change the inventory upon initializing a rental instance
Do you have any recommendations on how we could improve this project for the next cohort? The only thing we can think of is that the directions were a little hard to follow. It was unclear at times what was expected.
Link to Trello https://trello.com/b/ytavzmGE/video-store-api
Link to ERD https://www.lucidchart.com/documents/edit/ee044aa2-ec6a-47da-b1fa-bc83053f5be3#

secretsharer and others added 30 commits May 9, 2017 14:29
@kariabancroft
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Looks good - shared commits
Comprehension questions Yes
General
Business Logic in Models Some - it seems like a lot of it has to do with rentals which makes sense
All 3 required endpoints return expected JSON data Yes
Requests respond with appropriate HTTP Status Code Yes
Errors are reported Yes (status codes)
Testing
Passes all Smoke Tests Yes
Model Tests - all relations, validations, and custom functions test positive & negative cases Yes - I like that you verify name & # but it might also be good to test each field separately. For you movie: title present tests, it makes sense to have both assertions, but its not logical to have both the positive and the negative case in the same test. It's be good to split these out into two.
Controller Tests - URI parameters and data in the request body have positive & negative cases Customer controller tests are good positive cases but no negative cases. Slightly more negative cases in movies but still room for some additional scenarios.
Optionals
POST routes use URI parameter and request body to add a new record to the database Yes
GET /customers shows how many movies are checked out by a customer N/A
GET /movies/:title shows updated inventory N/A
Overall Nice job hitting the major pieces of creating an API. Still some room for improvement on the negative test cases and moving logic into the model, though I like the way that you have some logic mapped out in the model even if you didn't get to the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants