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

Queues - AnnaElisabeth and Janice - VideoStoreAPI #15

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

Conversation

J-C-L
Copy link

@J-C-L J-C-L commented May 12, 2017

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 attributes shown in the seed data told us what tables we'd need and what columns they should have.
List all the completed endpoints of your application.
GET /customers
GET /movies
GET /movies/:title
POST /rentals/:title/check-out
POST /rentals/:title/check-in
GET /rentals/overdue
Describe a set of positive and negative test cases you implemented for a model. For our overdue method in the rental model, we tested the positive case, where it returns an array of overdue rentals, and the negative case, where it returns an empty array of no rentals are overdue.
Describe a set of positive and negative test cases you implemented for a controller. it returns a list of movies when the movies_path is called, and when no movies exist, it returns a :not_found.
How does your API respond when bad data is sent to it? It responds with either :not found or :bad_request, and provides a custom error message.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. Overdue is a class method for our rental model. It selects the rentals that are checkout out and have a due date before today. We wrapped that functionality into a method to keep the logic in the model which is the layer that interacts with the database.
Do you have any recommendations on how we could improve this project for the next cohort?
Link to Trello https://trello.com/b/zhKykge4/video
Link to ERD https://www.lucidchart.com/invitations/accept/05291f03-1018-4bd3-b5fe-596d6714bfb0

@droberts-sea
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes
General
Business Logic in Models yes - Much of this comes down to having well-designed model relations. Good work!
All 3 required endpoints return expected JSON data yes
Requests respond with appropriate HTTP Status Code yes
Errors are reported yes - good work
Testing
Passes all Smoke Tests yes
Model Tests - all relations, validations, and custom functions test positive & negative cases yes
Controller Tests - URI parameters and data in the request body have positive & negative cases yes
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

Great work overall, especially on testing and the optionals!

@esther-ng
Copy link

Nicely done!

A few notes/questions:

  • indentation was inconsistent at times (ie. movies_controller), could improve readability
  • there were some nested if statements and long elsif statements (ie. rentals_controller, customers_controller) Could you improve readability by utilizing guard clauses, extracting helper methods, or using case statements?
  • for the customers and movies controller tests, did you decide that responding with a regular response was the best response for something that was not allowed? Could there be another status code that would be more expected (perhaps forbidden?)
  • not sure if you covered using fixture test data (ie. customer(:curran).name) - was there a decision made to hard code expected result instead of using this format?
  • would you want to consider case insensitivity for query options (ie. movie title)?
  • rather than taking the first from a .where query, could you use a .find_by?

Overall, really nice job getting a lot of the optionals, using strong params and inheritance. I was especially impressed by your usage of joins, model relationships and database indices!

@dschrimm
Copy link

dschrimm commented Jun 6, 2017

Nice, thorough test cases.

Be careful about publishing secret keys in git, keep them secret whenever possible. It's often easy to create new secrets if you accidentally make yours public.

Could you add more restrictions on your db schema? For example, maybe customers are required to be initialized with a name or phone number.

Overall, great work.

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.

5 participants