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

Lynn & Chantal's Video Store #6

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

Lynn & Chantal's Video Store #6

wants to merge 54 commits into from

Conversation

ghost
Copy link

@ghost ghost 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. Originally we created a join table for customers and movies because they both had a "has many" relationship. We later realized that the join table was unnecessary and removed it. For each model, we looked at the seed data and created fields.
List all the completed endpoints of your application. get/customers, get/movies, get/movies/:title, post/rentals/:title/checkout, post/rentals/:title/checkin, get/rentals/overdue. We also have the query parameters of sort = name.
Describe a set of positive and negative test cases you implemented for a model. Neg: For our customer model, we wrote a test that the phone number needed to be unique. We tried to create a customer with the same phone number as someone in our yml file and expected that result to be false. Pos: We tested that someone could be created with a unique phone number.
Describe a set of positive and negative test cases you implemented for a controller. Pos: For our rentals controller, we tested that when you check out a rental it updates the "movies checked out count" to increase by one. Neg: We tried to checkout a movie that was not available, and we checked that the count for that customer did not increase.
How does your API respond when bad data is sent to it? We show a JSON object with an error message that gives some info about what is missing. The status is either bad request, not acceptable, or not found.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We wrote a method called overdue which checks to see if the rentals due date has passed. We chose to wrap it in a method to minimize the amount in our controller. It is similar to getting rental.all, so it makes sense to put it in the model.
Do you have any recommendations on how we could improve this project for the next cohort? The first wave felt like it was semi-easily accomplished in the given time. We really enjoyed that it helped us focus more on TDD!
Link to Trello https://trello.com/b/8iwXSXps/video-store
Link to ERD https://www.lucidchart.com/documents/view/39eaccfa-087e-4bbb-8346-0821ab28b99a

Chantal Rollison and others added 30 commits May 9, 2017 13:33
…o match requirements if movie title not found
ltrickey and others added 24 commits May 11, 2017 09:45
…rned in overdue controller method to not have both created_at and checkout_date
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good commit messages, adequate number of commits and both partners contributed.
Comprehension questions I couldn't get access to your Trello Board, if you can re-share it with me and send a Slack message and I'll review it.
General
Business Logic in Models You should have some custom methods in Customer & Movie to pull business logic into the models and away from the controllers. Most of the check-in and check-out logic should be in the models.
All 3 required endpoints return expected JSON data Check
Requests respond with appropriate HTTP Status Code Check, good use of :not_found and :bad_request.
Errors are reported Check, but I would make use of the .errors hash to ensure that any validation errors are reported.
Testing
Passes all Smoke Tests The required routes pass.
Model Tests - all relations, validations, and custom functions test positive & negative cases Check, I would write tests to use the fixture data rather than hard-coded value, but what you have works for the most part. I would also make sure that specific validation error are what cause model validations to fail, by using the errors hash.
Controller Tests - URI parameters and data in the request body have positive & negative cases Positive and negative tests and tests for the keys returned in the JSON. Also tests for the error codes. Also good tests for how the database is changed on check-in and check-out.
Optionals
POST routes use URI parameter and request body to add a new record to the database Check in and check out are in place, but some things aren't working, like changes to inventory and error codes.
GET /customers shows how many movies are checked out by a customer Not quite working right, You shouldn't have customer or movie keep their own attribute to track the number of items checked out. It's easier to instead provide them an instance method to calculate the number of items checked out from rentals.
GET /movies/:title shows updated inventory Similar to GET / customers.
Overall You covered all the requirements and made significant progress on the optionals. Good work. Better focus needs to be done on putting business logic in the models and avoiding duplication in fields like inventory and number of items checked out. When you do that you create the opportunity to get them out of sync.

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.

4 participants