Skip to content

Conversation

@elle-johnnie
Copy link

@elle-johnnie elle-johnnie commented Nov 10, 2018

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. Based on the data tables it felt relatively straight forward to draw up an ERD where Movies and Customers were joined via the Rentals table, which contained information beyond just the specific customer and movie to include relevant dates.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. Before sorting time complexity is O(n), since both queries are dependent on the length of the records stored in the customers and movies tables.
What is the Big-O time complexity of the POST /rentals/check-in endpoint? What does the time complexity depend on? Explain your reasoning. Time complexity for check-in is O(n) this is based on the number of rentals that have to be searched to find the matching movie , matching customer id's and nil for checkin date.
Describe a set of positive and negative test cases you implemented for a model. Positive tests for Customer are validations on fields where data passed in is valid, such as a name exists, an address exists etc. Negative tests are situations where a customer does not get created if any of the validators are not met.
Describe a set of positive and negative test cases you implemented for a controller. In the controller test for movies, we ensured that when a new movie with valid data was entered the database actually stored that movie. For a negative we passed in invalid to ensure that the database did not change or add a movie record with bad data.
How does your API respond when bad data is sent to it? Generally an http response code is returned containing 400, status is bad request, as well as a response ok = false, and cause indicating "validation errors."
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We wrote a custom model method for checking out a movie to handle all of the business logic of ensuring that a movie could be checked out, for example is the movie id valid?, is the customer id valid?, and is the movie inventory actually available (>0 in inventory available).
Do you have any recommendations on how we could improve this project for the next cohort? None that we can think of.
Link to Trello https://trello.com/b/H8ZijX0W/lj-sj-videostoreapi
Link to ERD https://www.lucidchart.com/documents/edit/b8dbf3b7-2cab-44b2-bc68-0b7f06b12d15?shared=true&
Summary It was good to have ample time to get the requirements taken care of and spend our remaining time stretching our learning on the optional features. The optionals were a big stretch from lecture material, so it was good that they weren't included as project requirements. SJ: Don’t make it harder! I appreciated that the project was heavy on optionals and light on requirements. I used the flexibility to check my understanding of the Rails unit (confirm, solidify, synthesize) in a way that we don’t usually get to as we fly through course material. But because this is the end of a unit, and also post-Betsy interview and catchup week, the timing was perfect for this.

sjlee3157 and others added 26 commits November 7, 2018 15:15
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages.
Comprehension questions Check, glad you found the lower stress week useful.
General
Business Logic in Models Check
All required endpoints return expected JSON data Check, except for movie, the name of the available inventory field is a bit off.
Requests respond with appropriate HTTP Status Code Check
Errors are reported Check
Testing
Passes all Smoke Tests You are failing a few smoke tests. You have a differently named inventory_available field it seems.
Model Tests - all relations, validations, and custom functions test positive & negative cases You could use more model testing of your custom methods for Rental.
Controller Tests - URI parameters and data in the request body have positive & negative cases Some more tests on your optionals, like query params, and sorting are appropriate.
Overall Overall you did well. You hit a number of the optionals which is nice. Just a note to be more through on negative cases in your tests, and watch for the one problem with your smoke tests.

rental_out3:
checkout_date: 2018-11-05
checkin_date:
due_date: Date.current + 30

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work setting due dates in the future. 😁

overdue_rentals = Rental.includes(:movie).includes(:customer).where(checkin_date: nil).where("rentals.due_date < ?", Date.current)

# @sorters will always be a 1-d ordered array of unique values.
@sorters.each do |sorter|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of logic would make an excellent candidate for a model method.

@sjlee3157
Copy link

@CheezItMan Thanks for the feedback! 1. I was wondering if you can give an example of where we could be more thorough with negative tests. 2. We forgot to note this in our pull request comprehension questions, but we edited the variable name in the smoke test JSON (duplicated in the /test folder) to make it pass (though I see why we should have followed the test and renamed our column)

# post check_in_path, params: rental, as: :json
# end.wont_change 'Rental.count'

get "/customers/#{id}/history", as: :json

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use a named path, and add a test with an invalid id.

# return movie to provide a history
post check_in_path, params: rental, as: :json

get "/movies/#{id}/history", as: :json

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto with the previous comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants