VideoStoreAPI - Angela and Maria - Octos by knockknockhusthere · Pull Request #4 · Ada-C9/VideoStoreAPI

knockknockhusthere · 2018-05-11T19:09:32Z

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 started with two classes, Customer and Movie. We then considered a join table to connect the movie id with customer id being rented. We later on during the project decided on a Rental model instead.
Describe a set of positive and negative test cases you implemented for a model.	A movie can be created with all valid fields. A movie cannot be created without a title, or with an inventory of zero or less.
Describe a set of positive and negative test cases you implemented for a controller.	We check that a movie that exists will return all required fields, and we check that it yields a not_found status if the movie does not exist.
How does your API respond when bad data is sent to it?	with a 404 not_found and the error
Describe one of your custom model methods and why you chose to wrap that functionality into a method.	We wrote the logic for incrementing and decrementing the movies_checked_out_count inside the customer model.
Do you have any recommendations on how we could improve this project for the next cohort?	Write better requirements so we know that the movies_checked_out_count was not an optional. Demo the smoketests and how to find your bugs/problems.
Link to Trello	https://docs.google.com/document/d/11SUxikq4CTRo-3FCuA3QQfJmbMQdzBHVnFhHLlhydR8/edit
Link to ERD	https://www.lucidchart.com/documents/edit/959e2eb3-0b26-4482-87d4-12db975ee52c/0?shared=true&

…toreAPI into wave_1

…toreAPI

… default

…ing inventory of movies

…o movie model

…ed on movie

…d to handle setting available_inventory

…tionality, added inventory validation

…ixed a customer controller test to include movies_checked_out_count in fields to return

… create method

…an smoke tests

kariabancroft · 2018-05-20T20:12:54Z

Video Store

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	Used the commits - not too happy with the lopsided commits towards the end of the project though
Comprehension questions	Yes
General
Business Logic in Models	Yes - custom validations, increment and decrement methods. Per inline comments, you could still move some rental logic into the model.
All required endpoints return expected JSON data	Yes
Requests respond with appropriate HTTP Status Code	Most, missing some `status: ok` on success cases
Errors are reported	Yes
Testing
Passes all Smoke Tests	Yes
Model Tests - all relations, validations, and custom functions test positive & negative cases	Yes, though there is room for improvement on the relationship tests.
Controller Tests - URI parameters and data in the request body have positive & negative cases	Customer and Movies look good. Would like to see more tests for the Rentals controller on check in and check out.
Overall	When creating your tests for model relationships, consider that you want to test from both sides of the relationship. Whichever test file you're in, you want to be testing the relationship from that model object. The customer and movie controller methods were easier to test, but since the rentals controller does more logic, it would be a bigger "bang for your buck" if you tested that because there is a larger opportunity for these methods to go wrong. Overall, you demonstrated your learning of this API functionality.

kariabancroft · 2018-05-20T19:24:41Z

app/models/customer.rb

+  validates :movies_checked_out_count, numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+
+  after_initialize do |customer|
+      self.movies_checked_out_count ||= 0


You could also use a database default value to set this

kariabancroft · 2018-05-20T19:33:23Z

app/controllers/movies_controller.rb

+        }
+      }, status: :not_found
+    else
+      render json: @movie.as_json(only: [:id, :title, :overview, :release_date, :inventory, :available_inventory])


Don't forget status codes even when its the positive case

kariabancroft · 2018-05-20T19:37:48Z

app/controllers/rentals_controller.rb

+
+class RentalsController < ApplicationController
+
+  def check_out


It would make sense to move some of this logic from the controller into the model. You could have a single Rental model method that would complete all of these actions: update the dates and take care of updating the movie and customer inventory.

kariabancroft · 2018-05-20T19:38:19Z

app/controllers/rentals_controller.rb

+    #set available_inventory to inventory in movie controller?
+
+    if rental.save
+      movie = Movie.find_by(id: rental.movie_id)


The rental should already have a method to retrieve the movie without going through the overall Rental object. You should simply be able to use render.movie

kariabancroft · 2018-05-20T19:38:32Z

app/controllers/rentals_controller.rb

+      movie.dec_avail_inventory
+      movie.save
+
+      customer = Customer.find_by(id: rental.customer_id)


Same comment re: the method on the rental relationships to retrieve this object

kariabancroft · 2018-05-20T19:48:11Z

test/models/customer_test.rb

+  let(:ava) {customers(:ava)}
+
+  describe 'relations' do
+    it 'checks that a customer already exists' do


You don't really need a test for this - this is somewhat just testing that fixtures work which isn't part of your code

kariabancroft · 2018-05-20T19:49:31Z

test/models/customer_test.rb

+      movie = Movie.first
+      customer = Customer.first
+      rental = Rental.new(movie_id: movie.id, customer_id: customer.id)
+      rental.customer_id.must_equal customer.id


By going from the rental to the customer, you're really testing this code on the rental side. In this customer model test, you should be testing the customer.rental object instead.

kariabancroft · 2018-05-20T19:55:27Z

test/models/movie_test.rb

+    it "connects customer and rental customer_id" do
+      movie = Movie.first
+      rental = Rental.new(movie_id: movie.id, customer_id: @customer.id)
+      rental.movie_id.must_equal movie.id


Again, note how checking the movie properties through the rental object, but you should be able to check the rental object through the movie instead.

knockknockhusthere and others added 30 commits May 7, 2018 14:32

new rails api run

402f194

zomg it works

c21e04a

create movie model

bcb0b77

create migration - movie

5007ca2

create movies yml and movie_test

d5cd62c

update schema

01aafa4

created customer model

7b0fee9

added registered date and seeded

2a5406c

create join table customersmovies; update schema; update relations

631b813

added movies model relation test and renamed join table

ded7ff1

finished movie model relations testing

b6bddae

movie model validation tests

92ace8c

update yml

953621c

Merge branch 'master' of https://github.com/knockknockhusthere/VideoS…

eb53abd

…toreAPI into wave_1

created routes

e1dce6a

update ymls again

8ad2a07

Merge branch 'master' of https://github.com/knockknockhusthere/VideoS…

69682e1

…toreAPI

create index tests for customers controller

f808d1c

add movies controller tests for index method

e2546ff

wrote and tested movie show action

c073879

wrote and tested show method for movies controller

6969629

add create and movie_params method to movies controller and their tests

481ee1b

generate rentals controller and model; add check_in check_out to routes

0b2d4ec

update rental model and controller

2bf87a4

update rental model and controller - again

65aa797

even more rental updates

46e84b6

moved some things into the controller logic

d544069

checkin and checkout routes

2b62a26

fixed movie params to match smoketests

60916cb

updated rentals checkout method, now working

89dd223

knockknockhusthere and others added 28 commits May 9, 2018 23:38

wrote rentals controller check in method tests

fea1948

add two tests: won't create a rental when movie_id and customer_id DNE

15d66ab

add YML for 'purple' with available_inventory attribute set to nil by…

d4ece01

… default

add movie model tests for available_inventory, decreasing and increas…

36fff26

…ing inventory of movies

add available_inventory methods - setting, decreasing, increasing - t…

fe7f1d5

…o movie model

update rentals_controller to include available_inventory methods call…

85e3087

…ed on movie

add miscellaneous comments and TODOs

16677cc

wrote a check in test

dd1da3b

resolved merge conflicts

ebd476a

resolved merge conflicts

40a1619

resolved merge conflicts and syntax errors

2c983e8

add simplecov to gemfile; add coverage to .gitignore

035d824

add simplecov to test_helper

08d4112

add TODO for setting up and running test for rental model

4b87831

modify movies_controller test to include available_inventory attribute

4b6e5b9

update movie model and tests; add custom validation and private metho…

48484c7

…d to handle setting available_inventory

resolved merge conflicts

222719b

wrote positive test for rental model, tested available_inventory func…

956fdf6

…tionality, added inventory validation

skipped tests for old join tables

1f0870e

wrote some validation test stubs for customers

96d988d

fixed a validation test to instantiate movie with an inventory, and f…

a898d07

…ixed a customer controller test to include movies_checked_out_count in fields to return

fixed some more tests, deleted unnecessary line from movie controller…

910170e

… create method

rewrote relations model tests

ce4cddb

added two validation tests to customer

f84ab77

wrote tests for customer check_out_count methods

db30769

wrote validation tests for rentals

92e13e0

fixed rental controller test, edited rentals controller to pass postm…

056bb8b

…an smoke tests

add extra test - invalid if available_inventory is zero or less

d7d4607

kariabancroft reviewed May 20, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VideoStoreAPI - Angela and Maria - Octos#4

VideoStoreAPI - Angela and Maria - Octos#4
knockknockhusthere wants to merge 60 commits intoAda-C9:masterfrom
knockknockhusthere:master

knockknockhusthere commented May 11, 2018

Uh oh!

kariabancroft commented May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

kariabancroft May 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		class RentalsController < ApplicationController

		def check_out

Conversation

knockknockhusthere commented May 11, 2018

Video Store API

Comprehension Questions

Uh oh!

kariabancroft commented May 20, 2018

Video Store

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants