AC's Video Rental API #21

acgillette · 2017-05-14T05:42: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.	Initially there were no relationships established so they were just two tables, one for customers and one for movies. Later I added the rental model, which belonged to a movie and a customer.
List all the completed endpoints of your application.	get /customers get /movies get /movies/:title post rentals/:title/check-out
Describe a set of positive and negative test cases you implemented for a model.	In the movie controller, I tested that the inventory had to be a positive number by using a valid fixture and in invalid fixture
Describe a set of positive and negative test cases you implemented for a controller.	For the rentals controller, I tested the create by making sure it returned success if the data was good, and bad_request if the data was not good
How does your API respond when bad data is sent to it?	With a json containing the error messages
Describe one of your custom model methods and why you chose to wrap that functionality into a method.	I chose to make available_inventory a class method instead of a column because I was having a lot of trouble figuring out how to automatically set it to inventory otherwise.
Do you have any recommendations on how we could improve this project for the next cohort?
Link to Trello	I forgot to make a trello ):
Link to ERD	https://www.lucidchart.com/invitations/accept/2c551072-b6c8-4df8-9a69-0d199aab8812

…e to class method

kariabancroft · 2017-05-15T20:22:19Z

Video Store

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	Good
Comprehension questions	Yes
General
Business Logic in Models	Not really - though you are managing the available inventory here
All 3 required endpoints return expected JSON data	Yes - hmmm it seems like you might have some code that has never run... looks like a typo on line #13 in rentals controller
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	Some - though I think you skimped out on these a bit
Controller Tests - URI parameters and data in the request body have positive & negative cases	Yes - looks good. Covers most scenarios in movie and the code you have for rentals
Optionals
POST routes use URI parameter and request body to add a new record to the database	Yes
GET /customers shows how many movies are checked out by a customer	Yes
GET /movies/:title shows updated inventory	Yes
Overall	Nice job hitting the major learning goals on this assignment

Erin007

Wahoo! Nice work - especially flying solo. Just a few things to think about. Please let me know if you have any follow up questions or would like me to take a closer look at something. -Erin

Erin007 · 2017-05-25T22:08:08Z

app/controllers/rentals_controller.rb

+       render status: :bad_request, json: { error: "Not enough inventory" }
+     elsif rental.save
+       render status: :ok, json: { id: rental.id }
+       rental.customer.movies_checked_out_count += 1


If the rental is processed, the customer.movies_checked_out_count increments up one, which makes sense. Does the movie.available_inventory also need to decrease by one?

Erin007 · 2017-05-25T22:15:04Z

app/models/customer.rb

+  validates :address, presence: true
+  validates :city, presence: true
+  validates :state, presence: true
+  validates :postal_code, presence: true


You could consider validating whether the data you're getting for these fields is consistent with what's expected. For example, is the postal code 6 digits? Big picture, you want to always validate that what is required exists, and if it exists, that the format (data type, length, etc) is what you want it to be.

Erin007 · 2017-05-25T22:17:03Z

app/models/movie.rb

+  validates :title, presence: true, uniqueness: true
+  validates :overview, presence: true
+  validates :release_date, presence: true
+  validates :inventory, presence: true, numericality: { greater_than: -1, only_integer: true }


yeah! like, here where you check that the inventory is an integer or that movie names are unique. That's what I was thinking in my previous comment 🙂

Erin007 · 2017-05-25T22:26:13Z

config/routes.rb

+Rails.application.routes.draw do
+
+  get '/movies', to: 'movies#index', as: 'movies'
+  get '/movies/:title', to: 'movies#show', as: 'movie'


It's totally great as is, just know that you will more often than not (in my -admittedly limited - experience) see :id in the routes for show. There's a little Rails magic you can tap into if you use the convention of /model/:id in routes. If you're curious to Google or you come across something like resources :movies in a routes file, it's generating all of the routes for you and will format the show method with the id in the route.

Erin007 · 2017-05-25T22:28:07Z

config/secrets.yml

@@ -0,0 +1,22 @@
+# Be sure to restart your server when you modify this file.


Shhhhh. These are secrets. They shouldn't float around out in the open. Pop this file in your gitignore.

Erin007 · 2017-05-25T22:29:42Z

db/migrate/20170509235403_add_relationships.rb

@@ -0,0 +1,4 @@
+class AddRelationships < ActiveRecord::Migration[5.0]
+  def change


What's going on with this empty change method in this migration? Seems like you could delete this migration if it's not doing anything...

Erin007 · 2017-05-25T22:33:16Z

db/schema.rb

+
+  create_table "movies", force: :cascade do |t|
+    t.string   "title"
+    t.string   "overview"


How long is the overview? I ask because I wonder if you want t.text rather than t.string.

Nevermind, I re-read the instructions, but something to consider in the future.

Erin007 · 2017-05-25T22:42:03Z

test/controllers/customers_controller_test.rb

+    r = JSON.parse(response.body)
+    r.length.must_equal Customer.count
+  end
+end


You could also spot check that the json data you're getting back is consistent with what you would expect for customers. Do the objects returned, for example, have telephone numbers etc?

Erin007 · 2017-05-25T22:47:17Z

test/models/customer_test.rb

+    customers(:no_code).valid?.must_equal false
+    customers(:no_phone).valid?.must_equal false
+    customers(:no_credit).valid?.must_equal false
+  end


can a customer have negative credit?

You'll want to test any of your relationships in these model tests, too. Like has_many etc

Erin007 · 2017-05-25T22:48:07Z

test/models/rental_test.rb

+
+  it "must be valid" do
+    value(rental).must_be :valid?
+  end


what happens if you test an invalid rental?

acgillette added 29 commits May 9, 2017 18:40

Initial API app made

57a635c

Created initial models and seeded data

9209823

Added controllers

f585e05

Added index for customers and movies

0d7503b

Added model validations

c6a1e01

First movie test green

fc7767e

Validation presence tests green

294b884

Testing negative inventory passed

9fd41cb

unique title test passing

5a75609

added validation for legitimate year format

e51426a

Finished movies validation tests

78b60ff

Starting on customer model validations

0a743be

Basic validations for customers green

8a34a8c

Added default movies checked out count

aa0edbb

Starting on movies show method

7dfda2a

Added error handling for movie not found

a0b5862

Added rental model with relationships

b47278c

Added rentals controller

b0f9f71

Added available inventory column to movie model

38fa756

Started on check out functionality

f4ffbff

Figuring out available_inventory logic, changed it from model variabl…

3352ac0

…e to class method

Got checkout functionality working

e7c7bee

Ok finally figured out controller testing

d1e3f67

Index tests for movie green:

db4f71d

Finished show controller tests

d6ca7d1

Starting on customer controller tests

21a1e5e

Customer index tests complete

7007f94

Starting on rentals controller tests

b37dd33

Working on rental check out tests

58f0afc

Hamled assigned Erin007 May 19, 2017

Erin007 reviewed May 25, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AC's Video Rental API #21

AC's Video Rental API #21

acgillette commented May 14, 2017

kariabancroft commented May 15, 2017

Erin007 left a comment

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

Erin007 May 25, 2017

		@@ -0,0 +1,22 @@
		# Be sure to restart your server when you modify this file.

		@@ -0,0 +1,4 @@
		class AddRelationships < ActiveRecord::Migration[5.0]
		def change

AC's Video Rental API #21

Are you sure you want to change the base?

AC's Video Rental API #21

Conversation

acgillette commented May 14, 2017

Video Store API

Comprehension Questions

kariabancroft commented May 15, 2017

Video Store

What We're Looking For

Erin007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment