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

Isabel & Lauren -- Carets #17

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

Conversation

isabeldepapel
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 used the seed data to determine which fields we would need in our customers and movies tables. We knew that rentals would be a sort of index table with additional fields like due_date and returned, based on the many-to-many relationship structure between movies and customers (i.e., a customer can rent many movies, and a movie has many customers that rent it.
Describe a set of positive and negative test cases you implemented for a model. In the Customer model, for the movies_checked_out_count, we tested that it would return the correct number of movies a customer currently had checked out. And then we also tested that it would correctly return 0 if the customer had no movies currently rented.
Describe a set of positive and negative test cases you implemented for a controller. For the Rentals controller, in checkin (update), one of the positive test cases was testing that it changed the value of the returned field to true. And then a negative test case was a test to verify that it would return an error message and respond with bad request when attempting to update a rental that has already been returned (since there's nothing to update once it's been returned).
How does your API respond when bad data is sent to it? It returns an error message explaining the error (as a hash) and then a status code of either bad request or not found depending on the error.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. The Rental model has a checkin method that implements the business logic called by the Rentals controller. It checks to see whether the rental has been returned, and will either return the rental (if it hasn't been returned) and also make the record read-only to prevent any further updates, or return an error if it's already been returned. We wrapped this in a custom model method because it keeps the business logic in the model and out of the controller.
Do you have any recommendations on how we could improve this project for the next cohort? No
Link to Trello https://trello.com/invite/b/d9kmk7Pn/5b1c0ff1a02d0107372ef20ae0e697fe/video-store-api
Link to ERD It's in the root folder of our project as "erd.pdf"

isabeldepapel and others added 30 commits November 6, 2017 11:05
… to movie model. All tests passing, 82% coverage in rentals controller.
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and both teammates participating. Good commit messages.
Comprehension questions Check
General
Business Logic in Models Very elegantly done!
All 3 required endpoints return expected JSON data Check
Requests respond with appropriate HTTP Status Code Check, different in some optional smoke tests, but appropriate status codes regardless
Errors are reported Check
Testing
Passes all Smoke Tests For the required test, yes, and for most of the optionals
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 Check
Optionals
POST routes use URI parameter and request body to add a new record to the database Check
GET /customers shows how many movies are checked out by a customer Check
GET /movies/:id shows updated inventory Well done!
Overall This was truly excellent. Well done.

validates :inventory, numericality: {only_integer: true, greater_than: 0 }

def available_inventory
unavailable = Rental.where(movie_id: self.id, returned: false).count

Choose a reason for hiding this comment

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

Good that you're using .where to find the rentals, the Database is faster.

end

it "returns customers with the required fields" do
keys = %w(id movies_checked_out_count name phone postal_code registered_at)

Choose a reason for hiding this comment

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

nicely written

it "returns not found if movie id is invalid" do
dune.destroy
get movie_path(dune.id)
must_respond_with :not_found

Choose a reason for hiding this comment

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

what about the fields in the JSON returned?


body = JSON.parse(response.body)
body.must_be_kind_of Hash
body.must_include "errors"

Choose a reason for hiding this comment

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

Good

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