Leaves AND Branches - Elizabeth AND Macaria by north108 · Pull Request #29 · Ada-C12/video-store-api

north108 · 2019-11-08T22:53:49Z

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.	The seed data informed us on which columns to include in our schema and made us think about how we were going to connect movies to customers and customers to movies.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning.	O(n) n being the number of customers and movies because it has to loop through each element and return the attributes of each element.
What is the Big-O time complexity of the `POST /rentals/check-in` endpoint? What does the time complexity depend on? Explain your reasoning.	O(1) because we search using params which access hashes.
Describe a set of positive and negative test cases you implemented for a model.	In the movie model tests, we tested that the movie is valid with the presence of a title and invalid without a title.
Describe a set of positive and negative test cases you implemented for a controller.	In the movie controller test #show we tested that it returned the movie with a valid id and responds with a bad request if given an invalid id.
How does your API respond when bad data is sent to it?	Bad request error.
Describe one of your custom model methods and why you chose to wrap that functionality into a method.	In the rental model we wrote a method called due_date and put it in a model method because it is business logic.
Do you have any recommendations on how we could improve this project for the next cohort?	Smoke tests led us to have write non-optimal code.
Link to Trello	We pair programmed the whole time so we didn't have the need for one.
Link to ERD	https://imgur.com/a/IVHIglv
Summary	Elizabeth was pleasure to work with! A true dynamo! Her calm attitude and efficient work style made this project a dream for me! Also, I liked the size of the project for a pair. We were able to do most of the work together without taking too much work home throughout the week. True pair programming!
Macaria was a dream, as per usual!

…working properly at this point.

…ts passing

added decrementing and incrementing of available_inventory. Smoke tes…

En/tests

CheezItMan · 2019-11-13T04:21:08Z

Video Store

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	Good number of commits and mostly granular commits
Comprehension questions	For the check-in endpoint you need to look up the proper movie and customer, so... this will depend on how this data is organized O(n) worst-case if the database has to do a linear search, but more likely O(1) since the database will use something like a hash to index the values.
General
Business Logic in Models	Overall pretty good
All required endpoints return expected JSON data	Check
Requests respond with appropriate HTTP Status Code	Check
Errors are reported	Error status is reported, but no validation errors reported.
Testing
Passes all Smoke Tests	Check
Model Tests - all relations, validations, and custom functions test positive & negative cases	See my inline comments, overall ok
Controller Tests - URI parameters and data in the request body have positive & negative cases	You're not covering all your edge cases here.
Overall	It works, but it's quite a bit unpolished. Check out my inline comments and let me know if you have questions.

CheezItMan · 2019-11-13T00:02:50Z

app/controllers/rentals_controller.rb

+      if (!movie.available_inventory)
+        movie.available_inventory = movie.inventory
+      end
+
+      movie.available_inventory -= 1
+      movie.available_inventory_will_change!
+      movie.save


This kind of business logic looks like a prime candidate for a model method!

CheezItMan · 2019-11-13T00:03:14Z

app/controllers/rentals_controller.rb

+    if rental = Rental.find_by(customer_id: customer.id, movie_id: movie.id)
+      Customer.check_in_movie(customer)
+
+      if (!movie.available_inventory)
+        movie.available_inventory = movie.inventory
+      end
+
+      movie.available_inventory += 1
+      movie.available_inventory_will_change!
+      movie.save
+
+      rental.status = "checked_in"
+      rental.save


This is more business logic to put in the model.

CheezItMan · 2019-11-13T04:04:18Z

app/models/customer.rb

+  def self.check_out_movie(id)
+    customer = Customer.find_by(id: id)
+    movie_count = customer.movies_checked_out_count
+    customer.update(movies_checked_out_count: movie_count + 1)
+
+    customer.save
+    return customer
+  end


So the checked_out_count has nothing to do with the number of Rentals the customer has. Hmmm... Any kind of subtle bug could get these out of sync. This isn't an out-and-out bug, but in an imperfect system it's an invitation for these kinds of problems.

CheezItMan · 2019-11-13T04:05:01Z

app/models/rental.rb

+  def self.due_date(rental)
+   due_date = Date.today + 7.days
+   rental.due_date = due_date
+   rental.save
+   return rental
+  end
+
+  def self.status_checkout(rental)
+    rental.status = "checked out"
+    rental.save
+    return rental
+  end


These make much more sense as instance methods as opposed to class methods.

CheezItMan · 2019-11-13T04:07:55Z

app/models/movie.rb

+class Movie < ApplicationRecord
+  has_many :rentals, dependent: :nullify
+  validates :title,  presence: true
+  validates :inventory, presence: true, numericality: {greater_than: 0}


Running the seeds file, I'm getting validation errors, so your seeds file doesn't work with this validation.

CheezItMan · 2019-11-13T04:14:02Z

test/models/customer_test.rb

+  end
+
+  describe "check_out_movie" do 
+    it "increases the customers movie count if given valid data" do 


You should have a test with invalid data. Also this method doesn't make much sense in that someone could have a larger checked out count without more rentals.

CheezItMan · 2019-11-13T04:15:22Z

test/models/rental_test.rb

+      expect(result).must_equal true
+    end
+
+    it "is invalid with a movie_id that is not a number" do


You should also have a test so that it's invalid to check out a movie, if there are none left in inventory.

CheezItMan · 2019-11-13T04:17:47Z

test/controllers/movies_controller_test.rb

+MOVIE_FIELDS = ["available_inventory", "inventory", "overview", "release_date", "title"]
+
+describe MoviesController do
+  describe "index" do


CheezItMan · 2019-11-13T04:18:16Z

test/controllers/movies_controller_test.rb

+      must_respond_with :bad_request
+      body = JSON.parse(response.body)
+      expect(body['errors']).must_include "BAD Request"


You should also have some validation error messages.

CheezItMan · 2019-11-13T04:18:55Z

test/controllers/rentals_controller_test.rb

+    end
+  end
+
+  describe "check_in" do


What about invalid check ins and checkouts? Trying to check out a movie with no more inventory, or checking in a movie that's not checked out. More testing needed.

north108 and others added 18 commits November 5, 2019 14:31

Created rails project and migrations.

688e6a3

Schema is set.

cfcc858

FOR REAL schema DONE.

b78ac3f

Wrote the validation and relation tests. All pass.

feed1ee

Wrote actions for controllers and tests. All passes.

f31bfcb

worked on rental controller and model and tests.

559a431

Added Rental model tests

c001842

Edited rentals controller.

fb66d77

Finished checkout method and tests.

03e4bb1

Worked on testing and tried to implement availalbe_inventory. Is not …

83532ac

…working properly at this point.

Wrote customer controller tests.

f46d23b

Need to test check_in

6d4bfe2

Finished tests and model methods.

6cfd98e

added decrementing and incrementing of available_inventory. Smoke tes…

2218c2c

…ts passing

Merge pull request #1 from north108/mac_experiments

44f2a22

added decrementing and incrementing of available_inventory. Smoke tes…

fixed merge conflicts.

0f46cbe

Merge pull request #2 from north108/en/tests

f4af184

En/tests

Fixed tests and cleaned up comments.

3b4618c

CheezItMan reviewed Nov 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leaves AND Branches - Elizabeth AND Macaria#29

Leaves AND Branches - Elizabeth AND Macaria#29
north108 wants to merge 18 commits intoAda-C12:masterfrom
north108:master

north108 commented Nov 8, 2019

Uh oh!

CheezItMan commented Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

CheezItMan Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

north108 commented Nov 8, 2019

Video Store API

Comprehension Questions

Uh oh!

CheezItMan commented Nov 13, 2019

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!

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