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

Anne & Dikla - VideoStoreApi - Octos #2

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

Conversation

diklaaharoni
Copy link

@diklaaharoni diklaaharoni commented May 9, 2018

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 looked in the seeds and the assignment page for the fields
Describe a set of positive and negative test cases you implemented for a model. Customer can be created withl all required fields, customer can not be created without a name
Describe a set of positive and negative test cases you implemented for a controller. Can get a movie, yields a not found status and also return some error text if the movie D.N.E
How does your API respond when bad data is sent to it? Bad request
Describe one of your custom model methods and why you chose to wrap that functionality into a method. Increase_available_inventory, we chose to put it in movie.rb because the logic was specific to an instance of movie, so it didn't make sense to us to keep it in the rentals controller
Do you have any recommendations on how we could improve this project for the next cohort?
Link to Trello https://trello.com/b/3XeI8K6P/videostoreapi
Link to ERD https://www.lucidchart.com/documents/edit/cebe4059-53be-4ecb-9c1a-1f51fdd442c1/0?shared=true&
WE CHANGED THE SMOKE TESTS FROM CHECK-IN AND CHECK-OUT TO CHECKIN AND CHECKOUT

diklaaharoni and others added 30 commits May 7, 2018 12:01
generated customer model
diklaaharoni and others added 28 commits May 8, 2018 15:39
@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 You have a lot of model methods, but the high-level logic of what methods get called in what order still lives in the controller. See the RentalsController for details.
All 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 missing some - see inline
Controller Tests - URI parameters and data in the request body have positive & negative cases missing some negative cases
Overall Good work overall. There are a few holes in your test coverage, but in general this looks good.

def check_in

rental = Rental.find_by(rental_params)
if rental.save

Choose a reason for hiding this comment

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

You haven't changed the rental - why are you saving it here?

rental = Rental.find_by(rental_params)
if rental.save
rental.movie.increase_available_inventory
rental.customer.decrease_movies_checked_out_count

Choose a reason for hiding this comment

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

All of this work could be wrapped up in one method, maybe something like Rental.check_in

if rental.save
rental.movie.reduce_available_inventory
puts rental.customer.movies_checked_out_count
rental.customer.increase_movies_checked_out_count

Choose a reason for hiding this comment

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

Doing all this work should be in one method in the model. Maybe even something like Movie#check_out(customer_id)


it "creates a new customer" do
before_customer_count = Customer.count
post customers_url, params: { customer: customer_data }

Choose a reason for hiding this comment

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

What about if the customer data is bad?

describe "decrease_movies_checked_out_count" do
it "decrease_movies_checked_out_count" do
@customer = Customer.create(
name: "Movie Watcher",

Choose a reason for hiding this comment

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

What if the movies_checked_out_count is currently 0?

describe "reduce_available_inventory" do
it "reduce_available_inventory" do
@movie = Movie.create(
title: "test movie",

Choose a reason for hiding this comment

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

What if available_inventory is already at the total inventory?

describe "increase_available_inventory" do
it "increase_available_inventory" do
@movie = Movie.create(
title: "test movie",

Choose a reason for hiding this comment

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

What if available_inventory is already 0?

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