Elle and Cyndi by dev-elle-up · Pull Request #23 · Ada-C11/VideoStoreAPI

dev-elle-up · 2019-05-17T21:35:23Z

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.	Upon looking at the seed data, we noticed we'd need two models - movie and customer - and used the column headings to determine what our columns would be called. Then we read through the project spec and noticed we'd need one more column in each of these tables and we also needed a table for Rentals.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning.	The time complexity to look up /customers and /movies is O(n) because the amount of time it takes to find all of the customers or movies depends on how many entries are in each table.
What is the Big-O time complexity of the `POST /rentals/check-in` endpoint? What does the time complexity depend on? Explain your reasoning.	The time complexity of this endpoint is O(log n). The time complexity depends on what the code uses to look up the customer and movie. In our case, it uses the id of the customer and movie to look up the rental, which means a binary search can be used. This results in O(log n) time complexity.
Describe a set of positive and negative test cases you implemented for a model.	For the Movie model, we tested that a movie can be created given valid data and that a movie won't be created if the title is missing, since we require title.
Describe a set of positive and negative test cases you implemented for a controller.	We wrote tests for a rental that both can and cannot be checked in. Specifically, we tested that a rental can be checked in if a matching valid rental exists, and made sure that we got the expected status response if there was no matching rental (no customer_id & movie_id that both match a rental).
How does your API respond when bad data is sent to it?	We return a status of either :no_content or :bad_request and a corresponding message, as json.
Describe one of your custom model methods and why you chose to wrap that functionality into a method.	One of our custom model methods takes a customer id and a movie id and returns a rental. We put this in a method because it's not part of a controller action and felt more like business logic. We wanted to move more of this action to the model but it would break tests and we were approaching the deadline. We also put logic for checking in & out a movie in the rentals model. This made sense here because it updated the available_inventory and movies_checked_out_count for the associated movie and customer, which was business logic.
Do you have any recommendations on how we could improve this project for the next cohort?	The description of the project was a little vague / unclear and hard to interpret. Putting more info in the intro regarding the final expected outcome would be helpful.
Link to Trello	We did not create a Trello board. We did pair program the entire time and kept track of everything on paper.
Link to ERD	https://drive.google.com/file/d/1fvZMhhrWEoHXR_MejhaDgQ3P6IVwlG9Y/view?usp=sharing
Summary	Pairing with a person in another class was fun!

…ler tests to be finished.

…ests

…te and checkout date in the checkout action.

…s available inventory to model method in rental

CheezItMan · 2019-05-23T21:24:34Z

Video Store

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	Reasonable number of commits and good commit messages
Comprehension questions	Check
General
Business Logic in Models	Yes, but see my comments in `rental.rb`.
All required endpoints return expected JSON data	Check
Requests respond with appropriate HTTP Status Code	See my notes on some of your response codes.
Errors are reported	Check
Testing
Passes all Smoke Tests	Check
Model Tests - all relations, validations, and custom functions test positive & negative cases	Very skimpy model testing.
Controller Tests - URI parameters and data in the request body have positive & negative cases	Overall pretty good, see my inline notes
Overall	Nice work, everything functions as required. For your design I think the `available_inventory` and `checkout_count` should be methods instead of data fields just because it could otherwise be possible to get them out of sync. You also have some status codes that could be better chosen. You also had some of the optionals done, nice work. You hit the major learning goals.

CheezItMan · 2019-05-23T20:59:36Z

app/models/rental.rb

+  end
+
+  def self.return(customer_id, movie_id)
+    rental = Rental.find_by(customer_id: customer_id, movie_id: movie_id)


How do you know the customer only had one checked out?

CheezItMan · 2019-05-23T21:00:23Z

app/models/rental.rb

+  belongs_to :movie, dependent: :delete
+  belongs_to :customer, dependent: :delete
+
+  def checkout_update_customer_movie(customer, movie)


The number checked out and available inventory sound like methods instead of data fields to me. I could see they getting out of sync especially if other apps impacted the same database.

CheezItMan · 2019-05-23T21:01:19Z

app/controllers/customers_controller.rb

+    if !customers.empty?
+      render json: customers.as_json(except: [:created_at, :updated_at]), status: :ok
+    else
+      render json: {message: "There are currently no customers."}


You should include a status code.

CheezItMan · 2019-05-23T21:01:28Z

app/controllers/movies_controller.rb

+    if !movies.empty?
+      render json: movies.as_json(except: [:created_at, :updated_at]), status: :ok
+    else
+      render json: {message: "There are currently no movies."}


Status code?

CheezItMan · 2019-05-23T21:02:35Z

app/controllers/rentals_controller.rb

+  def checkout
+    movie = Movie.find_by(id: params[:movie_id])
+    if movie == nil
+      render json: { error: "Movie was not found in database or no movie id was provided." }, status: :no_content


Status: no_content is usually for when the app has no content to return to the user, not when the operation fails. Instead I suggest, :bad_request.

CheezItMan · 2019-05-23T21:06:29Z

test/models/customer_test.rb

+
+describe Customer do
+  let(:customer) { Customer.new }
+


No validations???

CheezItMan · 2019-05-23T21:06:44Z

test/models/movie_test.rb

+
+    it "returns false for invalid movie data" do
+      movie = Movie.new
+      movie.valid?.must_equal false


No test for why it's not valid?

CheezItMan · 2019-05-23T21:09:42Z

test/models/rental_test.rb

+      rental.customer.must_equal customers(:one)
+    end
+  end
+


For these methods, what about testing checkouts when the inventory isn't available?

What about checking in a movie that's never been checked out, or was already checked in?

CheezItMan · 2019-05-23T21:19:51Z

test/controllers/customers_controller_test.rb

+      get customers_path
+      body = JSON.parse(response.body)
+      body.each do |customer|
+        customer.keys.must_equal keys


If you don't care about the order, I'd suggest that you sort the keys before comparing them.

CheezItMan · 2019-05-23T21:21:20Z

test/controllers/rentals_controller_test.rb

+      must_respond_with :success
+    end
+
+    it "will return bad request if given invalid data" do


You should also try this with invalid customer_id

cyndilopez and others added 20 commits May 14, 2019 13:44

created models for Customer and Movies and controllers

9895380

index and show actions for movies

1061e16

movies create action

ce28a4e

customer index action

d352c42

moved index from model to customer controller

953d83f

movie to Move in #create

2b23869

Wrote tests for movies and customers controllers. More movies control…

4ac6609

…ler tests to be finished.

fixed a missing end

492628c

movie validation test for title, added movie create and show action t…

4c01890

…ests

rentals table

31b2e16

tests for checkout

1c79508

tests for checkin

67600ad

dont know what I changed

7a7bb3b

Wrote remaining tests for rentals_controller.

71b6f87

Wrote model tests. Updated controller to accomodate saving the due da…

f44fc72

…te and checkout date in the checkout action.

Finished model tests for Rental.

491c428

Fixed tests that were broken by adding more columns.

c1ae8e7

Cleaned up tests.

01e0bb4

moved logic on updating customers movies checked out count and movie'…

1c038ea

…s available inventory to model method in rental

deleted unncessary comments

6b2aed3

CheezItMan reviewed May 23, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elle and Cyndi#23

Elle and Cyndi#23
dev-elle-up wants to merge 20 commits intoAda-C11:masterfrom
cyndilopez:master

dev-elle-up commented May 17, 2019 •

edited

Loading

Uh oh!

CheezItMan commented May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

CheezItMan May 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dev-elle-up commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Video Store API

Comprehension Questions

Uh oh!

CheezItMan commented May 23, 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

dev-elle-up commented May 17, 2019 •

edited

Loading