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

Ampers: Angela, Alex #3

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

Ampers: Angela, Alex #3

wants to merge 39 commits into from

Conversation

brownav
Copy link

@brownav brownav commented May 11, 2018

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 decided to definitely have models for our seed data (movies, customers) and since we knew rentals would come into the picture and require both a customer and movie to be initialized, rentals then became it's own model as a join table between movies and customers.
Describe a set of positive and negative test cases you implemented for a model. For our movie model, we have a positive test for creating a valid movie, and a negative test for trying to create a movie without valid attributes.
Describe a set of positive and negative test cases you implemented for a controller. For our rentals controller, we have a positive test for updating a rental with valid data, and a negative test for updating a rental with bogus data.
How does your API respond when bad data is sent to it? It renders an appropriate http code such as bad_request or not_found accompanied by an error message.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We don't have custom model methods but we do validate for certain fields, in our movie model we validate the presence of a title, and the numericality and range of inventory.
Do you have any recommendations on how we could improve this project for the next cohort? Maybe showing where to find more information on errors and failures when smoke tests are run, it requires some digging, and quickly showing the logger.debug tool.
Link to Trello https://trello.com/b/NCNXIvxj/videostoreapi
Link to ERD https://www.lucidchart.com/invitations/accept/b18facf2-5f19-4e54-91f0-321f064c6ea6

AngelaPoland and others added 30 commits May 7, 2018 13:19
@tildeee
Copy link

tildeee commented May 21, 2018

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
General
Business Logic in Models Didn't have functionality for business logic besides validations, which were appropriately placed here
All required endpoints return expected JSON data x
Requests respond with appropriate HTTP Status Code x
Errors are reported x
Testing
Passes all Smoke Tests 4 tests failing in wave 3: Customer's movie check out count increased, Movie available_inventory decreased, Customer's movie check out count decreased, Movie available_inventory increased
Model Tests - all relations, validations, and custom functions test positive & negative cases x
Controller Tests - URI parameters and data in the request body have positive & negative cases x
Overall

It's a shame that a lot of the logic around counts increasing didn't make it into this project -- otherwise overall good job and good clean, readable, slim code

Just some small suggestions:
Your model tests are pretty good -- I think in the tests on Movie and on Customer, you two have tests that set values on rentals(:one) when that isn't necessary-- that rental should be already set

Your rentals.yml file looks pretty random-- what's going on with rentals(:two)? and rentals(:three) has a customer and a movie_id

Overall I particularly think the controller tests and model tests you all wrote are great, good work on that! Also the project is pretty good.

@brownav
Copy link
Author

brownav commented May 21, 2018

@AngelaPoland

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.

3 participants