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

Haby and Kelly Video API #11

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

Conversation

kellysouza
Copy link

@kellysouza kellysouza 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 looked at relationships which were needed to achieve the endpoints. We also build schemas based on the data provided.
List all the completed endpoints of your application. '/movies'
'/movies/:title'
'/movies'
'/customers'
'/rentals/:title/check-out'
'/rentals/:title/check-in'
'/rentals/overdue'
Describe a set of positive and negative test cases you implemented for a model. Update mcoc if rental exists, don't update mcoc if rental doesn't exist
Describe a set of positive and negative test cases you implemented for a controller. Create movie if valid data, not create movie with invalid data.
How does your API respond when bad data is sent to it? render :bad_request and send custom error messages in the body
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We created custom model methods to update the Customer's mcoc as well as the Movie's available inventory. We felt good doing this because we did not call a specific controller action for these methods. We also felt the rental controller doesn't need to be involved in the inventory adjustments, it just needs to tell the movies and customers to go update their own numbers (channeling Sandi Metz).
Do you have any recommendations on how we could improve this project for the next cohort? Baseline was overly simple and it felt like "optional 1" was not really optional.
Link to Trello https://trello.com/b/3PDVLagT/video-store-api
Link to ERD Coming soon (we did it on paper!)

kellysouza and others added 30 commits May 9, 2017 13:47
@kariabancroft
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Yes - commits look nicely shared (even though your git images look similar and are hard to distinguish :-) )
Comprehension questions Yes
General
Business Logic in Models Yes - primarily around inventory. I think you could simplify even further and have a method that adjusts inventory and then each of the methods you have now just called those with either +1 or -1. Note that one thing you can do (that we haven't learned yet) is to do an after filter on a model save/create, similar to our before_action on a controller. This would allow you do put the available inventory logic into the movie model rather than the controller.
All 3 required endpoints return expected JSON data Yes
Requests respond with appropriate HTTP Status Code Yes
Errors are reported Yes
Testing
Passes all Smoke Tests yes
Model Tests - all relations, validations, and custom functions test positive & negative cases No :-(
Controller Tests - URI parameters and data in the request body have positive & negative cases Looks great
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 Yes
GET /movies/:title shows updated inventory Yes
Overall While I'm glad you got some optional rental requirements done, it seems like you skipped model testing of the primary requirements. Nice job overall working through the major learning goals.

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