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: Mariko, Kate, and Katie #10

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

Conversation

kcforsman
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. We based it on the seed data. We talked through it and then pulled up the seed data to assign specific names attributes. That is all.
Describe a set of positive and negative test cases you implemented for a model. We checked that something must be valid with particular attribute (in the case of a customer that all attributes are present). For a negative case, that a model was invalid without certain fields (for example, a customer need a name).
Describe a set of positive and negative test cases you implemented for a controller. A rental can be created for an existing customer and an existing movie in the checkout action, which confirms that a record of a rental is created. A rental cannot be created if there is no available inventory, and the rental count doesn't change.
How does your API respond when bad data is sent to it? ok: false, error: "some message related to the problem" or model.errors and either the :not_found for :bad_request
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We didn't do any though we could always pulled out some the logic to build either the new movie or a rental and to handle inventory management. But given the size of the project, I think we ended up just leaving it all in the controllers (may to be refactored later?)
Do you have any recommendations on how we could improve this project for the next cohort? More information around testing in PostMan, like how to interpret the smoke tests. More practice with navigating what one might call legacy code including tests.
Link to Trello https://trello.com/b/0hyMWkKX/blockbuster-thing
Link to ERD https://www.lucidchart.com/documents/edit/11605e6a-0450-4ae8-8431-b2d65885cc3e

kcforsman and others added 30 commits May 7, 2018 12:44
… and customers controllers with appropriate actions
…r empty array if there are no existing customers
…entory, and altered seed.rb to set the value equal to json inventory values
…ntroller

creates tests for the create method in the movies controller
changes code to get smoke tests working
marikoja and others added 16 commits May 8, 2018 12:58
…e tests and added more details to the negative tests
creates test for movie relationships to rentals
adds tests for rental model validation tests and relationship tests
relationship testing customer has many rentals
… the controller and use strong params, and fixed movie controller tests to respect foreign key violation
…nteresting' changes to occur on the available_inventory of movies and the movies_checkout_count for a customer
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good commit messages and good number of commits, all team members participated
Comprehension questions Check,
General
Business Logic in Models MISSING you're doing all your logic in the controller. Not a good idea.
All required endpoints return expected JSON data Check
Requests respond with appropriate HTTP Status Code . Check
Errors are reported Check
Testing
Passes all Smoke Tests Check
Model Tests - all relations, validations, and custom functions test positive & negative cases Check
Controller Tests - URI parameters and data in the request body have positive & negative cases You are failing tests you'd started for overdue rentals. Just a note they are optional, good that you got to start on them.
Overall Small note: I assume it's good, but I don't have access to your ERD. You hit all the requirement for the project, nice work, congratulations, you are finished with Rails!

if customer && movie
if movie.available_inventory > 0
rental = Rental.new(customer: customer, movie: movie, checkout_date: Date.today, due_date: Date.today + 7.days)
customer.movies_checked_out_count += 1

Choose a reason for hiding this comment

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

This kind of business logic really doesn't go here.

rental = Rental.find_by(customer: customer, movie: movie, returned: false)
if rental
rental.returned = true
customer.movies_checked_out_count -= 1

Choose a reason for hiding this comment

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

ditto on business logic


it "can have no rentals" do
customer = customers(:shell)
customer.rentals.length.must_equal 1

Choose a reason for hiding this comment

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

Why are you testing for a rental length of 1, if there are no rentals?

body.must_include "id"

# Check that the ID matches
Movie.find(body["id"]).title.must_equal "The Princess Bride"

Choose a reason for hiding this comment

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

Great movie 👍

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