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

&& Steffany & Nicoleta #13

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

Conversation

nbrandolini
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. Based on our instructions we initially created two tables Customers and Movies. When we got to Wave 3 we realized we need an intermediate table for Rentals.
Describe a set of positive and negative test cases you implemented for a model. For Movie we implemented a test that checks the validity and we checked that a movie will not be created without a title and release date.
Describe a set of positive and negative test cases you implemented for a controller. For MoviesController we tested that show will return a valid movie and we'll return 404 for movies that are not found.
How does your API respond when bad data is sent to it? It returns bad request and list errors.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We implemented an update check_out method for cleaner code and to keep the business logic in the model
Do you have any recommendations on how we could improve this project for the next cohort? Better instructions on how to use and debug Postman.
Link to Trello https://trello.com/b/dW7ULgkP/videostore
Link to ERD https://www.lucidchart.com/invitations/accept/29fd31a4-1ed1-41b2-998f-1ef7f44f962c

@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good commit messages and good number of commits, 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 There are some serious problems with your model tests.
Controller Tests - URI parameters and data in the request body have positive & negative cases Check
Overall You hit the major learning goal, see my comments on your model testing. I think you got a bit too eager to get started on controller & smoke tests and didn't pay enough attention to Model tests. Overall very well done.

proc { Customer.new(address: "1234 Candy Cane Lane") }.must_change 'Customer.count', 0
end

it "must not save if name data is invalid" do

Choose a reason for hiding this comment

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

With these you really need to explicitly test that there is an error related to the validation.

Also Customer.new will never change the customer count (you haven't saved).

An example of a good validation test would be:

it "must be invalid without an address" do
  customer = customers(:bob)
  customer.address = nil
  value(customer).wont_be :valid?
  customer.errors.messages.must_include :address
end

end

it "must not create without title" do
proc { Movie.new(release_date: "2015-12-2", inventory: 2) }.must_change 'Movie.count', 0

Choose a reason for hiding this comment

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

You have issues here similar to customer_test.rb

require "test_helper"

describe Rental do
let(:rental) { Rental.new(movie_id: movies(:bride).id, customer_id: customers(:ada).id, checkout_date: Date.today, due_date: Date.today + 7)}

Choose a reason for hiding this comment

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

tests????


it "must not create without release date" do
proc { Movie.new(title: "Party", inventory: 2) }.must_change 'Movie.count', 0
end

Choose a reason for hiding this comment

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

Missing tests on your custom method

body.must_be_instance_of Array
end

it "returns all of the customers" do

Choose a reason for hiding this comment

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

What about a test when there are no customers?

body["errors"].must_include "Customer not found"
end

it "returns an error if there is no customer" do

Choose a reason for hiding this comment

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

you mean no 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.

3 participants