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 - Nora and Sara - VideoStoreAPI #14

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

Conversation

CheerOnMars
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 project spec. Since both movies and customers have many rentals, we designed the Rentals model to have both foreign keys in addition to other fields.
Describe a set of positive and negative test cases you implemented for a model. For the movie model, a positive test case was confirming that a movie was valid if it had a title and an inventory. We also checked to make sure that it would be invalid if there was no title.
Describe a set of positive and negative test cases you implemented for a controller. For the rental controller test, we checked that we could checkout a movie if there was a copy in stock. If the inventory for a specific title were zero, we tested to confirm that it would not create a new rental.
How does your API respond when bad data is sent to it? It will respond with 'bad request', for example, when trying to rent a movie with an invalid customer or movie ID.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We wanted to get overdue rentals, and realized that it would take a couple steps, so we broke it up into a couple helper methods (get_checked_out and get_overdue).
Do you have any recommendations on how we could improve this project for the next cohort? More practice with RABL, reminder that the jbuilder gem was necessary, a little more explanation about how smoketests are written.
Link to Trello https://trello.com/b/JUxFklrv/videostoreapi
Link to ERD https://www.lucidchart.com/invitations/accept/1b192e7d-45c1-420f-9446-22250ff07bce

CheerOnMars and others added 30 commits May 7, 2018 11:42
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages, both team members contributed.
Comprehension questions Check
General
Business Logic in Models Check
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, I left some notes in your code.
Controller Tests - URI parameters and data in the request body have positive & negative cases Check
Overall Nice work, you hit all the major learning goal. I left a few comments in your code, let me know if you have questions. Congratulations, you've finished Rails!

end
end

describe "movies_checked_out_count" do

Choose a reason for hiding this comment

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

You should also test with no rentals

@@ -0,0 +1,51 @@
require "test_helper"

describe Movie do

Choose a reason for hiding this comment

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

Since you wrote an as_json method, you should test it.

babe.available_inventory.must_equal 3
end

it "returns a bad request for a non-existent rental" do

Choose a reason for hiding this comment

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

👍


def index
if params[:sort] == "title"
movies = Movie.all.order(:title).paginate(page: params[:p], per_page: params[:n])

Choose a reason for hiding this comment

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

Paginate, Nice!

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