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

VideoStoreAPI - Pipes - Rebecca and Bennett #5

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

Conversation

bennettrahn
Copy link

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. There were two classes in the seed data, so we started with those models. We talked about the relationship of rentals and the components based on the requirements.
Describe a set of positive and negative test cases you implemented for a model. Save if all validations are there, otherwise test all other things. There wasn't a lot of interesting logic in the models.
Describe a set of positive and negative test cases you implemented for a controller. Testing that the body response has the right keys, vs what error message sends back if there was no matches.
How does your API respond when bad data is sent to it? If bad data, responds with bad request. If data doesn't exist responds with not found.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We set the available inventory to equal inventory in an after create method. Because it seemed cleaner than making the creator enter it in twice.
Do you have any recommendations on how we could improve this project for the next cohort? idk
Link to Trello https://trello.com/b/dhqi3tNC/videostoreapi
Link to ERD we whiteboarded it and erased it whoops.

*We didn't fully finish the checkin/checkout optionals and testing so they might not be totally covered.

rbergena and others added 30 commits November 6, 2017 12:23
create customers model and controller
add basic controller actions, tests, and model validation for movie
fix tests to have different way of collecting params
@droberts-sea
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Business Logic in Models mostly - see inline comments
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 yes
Controller Tests - URI parameters and data in the request body have positive & negative cases yes
Overall Great work overall! Good effort on the optionals - you were very close to getting inventory tracking working.

else
render(
json: { errors: {
customers: ["No customers found."]}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this behavior isn't technically wrong, the convention in a case like this is to consider it successful and return an empty array.

status: :ok
)
rental.customer.movies_checked_out_count +=1
rental.movie.available_inventory -= 1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to update the values in the DB, you'll have to call save on rental.customer and rental.movie. If you're looking for some additional reading, this sort of situation (where several models need to be updated all at once) is a good candidate for wrapping in a database transaction.

This sort of logic would be great as a model method - perhaps a model filter (similar to a controller filter, but happens before writing to the DB).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other option here would be to not track these as separate columns in the DB, but generate them dynamically whenever they're needed by counting rentals associated with this movie/customer.

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