Amy & Christiane -- Carets · Pull Request #20 · Ada-C8/VideoStoreAPI

ghost · 2017-11-09T00:54:10Z

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 at the seed data and identified two classes: Customer and Movie, and determined that they had a many to many relationship that we decided to implement as a many through Rental relationship
Describe a set of positive and negative test cases you implemented for a model.	We test that the release_date of a new movie is present and in the right format. As a positive test we check that a new movie is created given all the right input.
Describe a set of positive and negative test cases you implemented for a controller.	A positive test case is that customer#index return all the right fields and only the right fields for all customers. A negative test we have is that when we fail to find a movie we send back an error message.
How does your API respond when bad data is sent to it?	If any required inputs are missing, we return a JSON hash, with an error message and a status of bad_request.
Describe one of your custom model methods and why you chose to wrap that functionality into a method.	We made a Customer#overdue_rentals method that returns an array of all Rentals that belong to a customer and are overdue: We built this to use in our Customer#overdue method, which returns an array of hashes; where each hash corresponds to a customer that has an overdue rental, with their associated customer information and an array of all of their overdue Rentals.
Do you have any recommendations on how we could improve this project for the next cohort?	We spent a lot of time talking about the design of the check_in and the use of a POST method instead of PATCH and using the given route instead of a nested route including the rental_id. Some insight into why the POST verb and the unnested route were used instead of a PATCH route to update a specific Rental would have been nice.
Link to Trello	https://trello.com/b/3p9XeJ1i/video-store-api
Link to ERD	Done on paper

… and ZOMG

…e a test given valid data

…modate validations

…nges controller test accordingly

…fully created

Serializer

…accordingly

…ributes

…ntals serializer, created a pretty date method (not working as expected, need to go back to it later), implemented the check_out method in the rentals controller

…oute, laid out pseudocode for Customers#overdue

CheezItMan · 2017-11-15T00:29:00Z

Video Store

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	Good number of commit and good commit messages, both team members participated
Comprehension questions	Check, noted on the post routes
General
Business Logic in Models	Some, but you have a lot of code in the controllers which should be in the models.
All 3 required endpoints return expected JSON data	Some problems with `/movies` and `/customers`
Requests respond with appropriate HTTP Status Code	Check, once place where you returned 201 instead of 200, but that's ok.
Errors are reported	Some, other places if there's an error it just crashes.
Testing
Passes all Smoke Tests	Nope! get customer list fails!
Model Tests - all relations, validations, and custom functions test positive & negative cases	No tests for `Rental`!!!
Controller Tests - URI parameters and data in the request body have positive & negative cases	Good negative tests here
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	Missing!
GET /movies/:id shows updated inventory	Yes... if I update the seeds file to include `available_inventory` or you make sure all models start with the field set.
Overall	There's some real issues here, I wasn't expecting this. I think you should have turned in a more solid product. Did you forget to merge a branch?

CheezItMan

Some comments

CheezItMan · 2017-11-14T23:30:09Z

app/controllers/rentals_controller.rb

+    movie = Movie.find_by_id(params[:movie_id])
+    customer = Customer.find_by_id(params[:customer_id])
+
+    unless movie.available_inventory > 0


What if movie is nil??

I notice the seeds file has all the movies started off with nil for available inventory, when I ran this locally.

CheezItMan · 2017-11-14T23:55:04Z

app/serializers/movie_serializer.rb

@@ -0,0 +1,3 @@
+class MovieSerializer < ActiveModel::Serializer
+  attributes :title, :overview, :release_date, :inventory, :available_inventory


id field???

CheezItMan · 2017-11-15T00:06:45Z

app/controllers/customers_controller.rb

@@ -0,0 +1,59 @@
+class CustomersController < ApplicationController
+  def index
+    customers = Customer.all


Your customers don't have a count of the movies checked out!

CheezItMan · 2017-11-15T00:07:48Z

app/controllers/customers_controller.rb

+  def overdue
+    # overdue_rentals = Rental.where("movie_id = ? AND customer_id = ?", movie.id, customer.id)
+    # theoretical: has_overdue?
+    customers_with_overdue= Customer.all.select {|customer| customer.has_overdue?}


While this will work, it's usually better to let the database do a lot of the work, with some sort of where clause, probably in a class method of Customer which selects movies which are overdue.

The database is almost always faster about this.

CheezItMan · 2017-11-15T00:08:22Z

app/serializers/customer_serializer.rb

@@ -0,0 +1,3 @@
+class CustomerSerializer < ActiveModel::Serializer


Using Serializers, Neat!

CheezItMan · 2017-11-15T00:12:28Z

app/controllers/application_controller.rb

@@ -0,0 +1,24 @@
+class ApplicationController < ActionController::API
+
+  def build_overdue_array(rentals_array)


Why is this here? This looks like it should be in the model!

Come to think of it it looks like it is!

CheezItMan · 2017-11-15T00:16:08Z

app/controllers/customers_controller.rb

+
+    overdue_customers_array = []
+
+    customers_with_overdue.each do |customer|


This should really be in the model

CheezItMan · 2017-11-15T00:17:11Z

app/controllers/rentals_controller.rb

+      render json: { ok: false, message: "No checked out copy of #{movie.title} for #{customer.name} found" }, status: :not_found
+    end
+
+    if checked_out.count > 0


Again this kind of functionality should go in the model.

CheezItMan · 2017-11-15T00:18:47Z

app/serializers/customer_serializer.rb

@@ -0,0 +1,3 @@
+class CustomerSerializer < ActiveModel::Serializer
+  attributes :registered_at, :name, :address, :city, :state, :postal_code, :phone, :movies_checked_out, :id


However no movies_checked_out_count

ayjlee and others added 30 commits November 6, 2017 12:39

setup new rails API project and initial routes for Movies, Customers,…

fe5cf19

… and ZOMG

created Movie Model

66d25df

Added Customer model and relationships, updated the schema

f868fdb

Created Rental Model with relationships and foreign keys

e06981c

Adds a account_credit column to customer. Seeds the database

d7e9806

Creates Movies controller with index, show, create and check actions

d5b37be

Test the json response

c2795e7

Creates Customers controller with index action

43e6f54

Customer index controller passes all tests

281d19b

seeded data and ran migrations from git pull

fff6070

ran migration to add account_credit to customers

2834cf8

tested and wrote Movie#show method

ac8bc5f

Created Movie#create method, passing basic positive test of can creat…

fa464fa

…e a test given valid data

tested Movie validations, refactored Movie controller methods to acco…

e189b14

…modate validations

Removes require(:movie) from the Movies controller strong params. Cha…

89436e4

…nges controller test accordingly

Returns the movie id in the json response when a new movie is success…

43d8e5d

…fully created

Customers controller index passes all tests

5c42cb7

starts validating title uniqueness within same year scope. Not finished

dcefa3b

removes comment in Movies controller

e94e39a

comments out the uniq title validation for the movie model

6f0a696

added gem active_model_serializers

8db92f7

deleted DS_store to solve merge conflict

4adaa97

Merge pull request #1 from ayjlee/serializer

0a20af6

Serializer

added serializer for Movies controller, updated controller methods

50f3bd2

comments out the test for movie uniqueness by release date year

245c482

adds serializer for Customers controller, updated Controller methods …

81c5504

…accordingly

added placeholder serializer for rentals

7f192a2

added routes and placeholder methods for Rental controller

3e4c536

Merge branch 'serializer'

b26709a

added movies_checked_out field to Customer with default value of 0

18326bd

ayjlee and others added 15 commits November 7, 2017 13:25

added available_inventory column to Movie table

f5e6eea

added default values to available inventory for movies

b12dc08

Adds a movies_checked_out field to the Customer Serializer

5ca00c8

fixes movies and customers controller test to take new serializer att…

270bcde

…ributes

Added default values for customers movies_checked_out, implemented re…

fc81591

…ntals serializer, created a pretty date method (not working as expected, need to go back to it later), implemented the check_out method in the rentals controller

added belongs to relationship for Rental

0bd6cdb

added logic to find a Rental given a customer id and movie id

6680806

working Rental#check-in method, needs refactoring. Passes smoke test

a1ec535

Implements Rentals Check_out

05d6eb1

moved overdue method to Customers controller, updated corresponding r…

432d046

…oute, laid out pseudocode for Customers#overdue

updated serializer to include customer id

a699d20

created Customer#overdue method, passing smoke tests

58bbd62

Merge branch 'overdue'

bea4f0b

Created customers route

501fde7

Started implementing customer#query method

e3f5edf

CheezItMan reviewed Nov 15, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amy & Christiane -- Carets#20

Amy & Christiane -- Carets#20
ghost wants to merge 45 commits intoAda-C8:masterfrom
ayjlee:master

ghost commented Nov 9, 2017

Uh oh!

CheezItMan commented Nov 15, 2017

Uh oh!

CheezItMan left a comment

Uh oh!

CheezItMan Nov 14, 2017

Uh oh!

CheezItMan Nov 14, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

CheezItMan Nov 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,3 @@
		class MovieSerializer < ActiveModel::Serializer
		attributes :title, :overview, :release_date, :inventory, :available_inventory

		@@ -0,0 +1,3 @@
		class CustomerSerializer < ActiveModel::Serializer

		@@ -0,0 +1,24 @@
		class ApplicationController < ActionController::API

		def build_overdue_array(rentals_array)


		overdue_customers_array = []

		customers_with_overdue.each do \|customer\|

		@@ -0,0 +1,3 @@
		class CustomerSerializer < ActiveModel::Serializer
		attributes :registered_at, :name, :address, :city, :state, :postal_code, :phone, :movies_checked_out, :id

Conversation

ghost commented Nov 9, 2017

Video Store API

Comprehension Questions

Uh oh!

CheezItMan commented Nov 15, 2017

Video Store

What We're Looking For

Uh oh!

CheezItMan left a comment

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!

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!

3 participants