Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amy^2 #16

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Amy^2 #16

wants to merge 44 commits into from

Conversation

amyesh
Copy link

@amyesh amyesh commented May 16, 2019

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 knew that movies and customers were disparate entities, and that in order to satisfy a many to many relationship, there needed to be a join table, stage left: rentals.
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) linear time complexity where n represents the number of movies or customers.
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(n) linear time complexity, where n represents the number of elements in the rentals collection.
Describe a set of positive and negative test cases you implemented for a model. That due date can be set given valid rental data, and that it can't be set if rental data is invalid.
Describe a set of positive and negative test cases you implemented for a controller. We tested that a rental can't be created without a valid movie or customer, and tested that it can be created with valid data.
How does your API respond when bad data is sent to it? It displays a list of errors.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We wrapped the set_available_inventory data in a model method. This method sets the available_inventory to the given inventory unless the user specifies a different availability in the HTTP request. We chose to do this in the model because the value of available_inventory relies upon the value given as the initial inventory.
Do you have any recommendations on how we could improve this project for the next cohort? It would be helpful to have more context about how this particular API is intended to be used - user stories would have been super helpful. The smoke tests felt less like tests and more prescriptive in terms of how the data was structured, whereas the assignment felt like you had more control over how your API functioned. Also, the smoke tests for Wave 3 included optional requirements.
Link to Trello https://trello.com/b/JSDQhqTj/scarecrow-20
Link to ERD (included in trello board)
Summary Thanks for pairing us together - y'all are the best <3

aphunk and others added 30 commits May 14, 2019 12:17
finished tests and methods for customer controller
Ap/movies Index, Show & Create actions and tests
removed show and create methods from customers controller
added custom method/tests to movies model, and updated controller method and tests
@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, you're welcome
General
Business Logic in Models Somewhat, it would make sense to have a method to check out and check in a movie.
All required endpoints return expected JSON data Required stuff done
Requests respond with appropriate HTTP Status Code Check
Errors are reported Check
Testing
Passes all Smoke Tests All required tests pass
Model Tests - all relations, validations, and custom functions test positive & negative cases Check, but you should have more model methods. What happens if you check in a movie that's not checked out, or check out a movie with not enough inventory.
Controller Tests - URI parameters and data in the request body have positive & negative cases Check, you should also have some tests in the effects of checking in and checking out.
Overall I'm not sure how to pluralize Amy (Amies?). You both hit the major learning goals. You did everything required. See my inline comments and let me know if you have any questions.

class RentalsController < ApplicationController

def checkin
rental = Rental.find_by(customer_id: rental_params[:customer_id], movie_id: rental_params[:movie_id])

Choose a reason for hiding this comment

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

How is this changing the Rental status? There's no way to tell in the DB if the rental was checked in!

@@ -0,0 +1,7 @@
class Customer < ApplicationRecord
has_many :rentals

Choose a reason for hiding this comment

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

So no method to tell how many movies they have checked out.

validates :overview, presence: true
validates :release_date, presence: true

def set_available_inventory

Choose a reason for hiding this comment

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

There are ways in Rails to invoke this method when a Movie is created, saved or updated.

if rental
render json: {id: rental.id, movie_id: rental.movie.id, customer_id: rental.customer.id}, status: :ok
else
render json: [{errors: {rental: ["Rental not found for customer #{rental_params[:customer_id]}, and movie #{rental_params[:movie_id]}"]}}],

Choose a reason for hiding this comment

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

This line is a bit long, feel free to put in line breaks to make this more readable.


if rental.save
rental.set_due_date
render json: [{ id: rental.id, movie_id: rental.movie.id, customer_id: rental.customer.id, due_date: rental.due_date }], status: :ok

Choose a reason for hiding this comment

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

This line is also long. You also need to have some impact on the available inventories.

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