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

Kiera # Ana lisa - VideoStoreAPI - Octos #19

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

Conversation

Krashaune
Copy link

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 seeded the database and reviewed it in the browser. Then based our models off the information in each hash.
Describe a set of positive and negative test cases you implemented for a model. Made sure that all validations had test cases where they were present and not present.
Describe a set of positive and negative test cases you implemented for a controller. through each method we implemented an edge case where the movie would not be available for some reason
How does your API respond when bad data is sent to it? renders errors in json based on the particular error. If available inventory is 0 it will render an available inventory error
Describe one of your custom model methods and why you chose to wrap that functionality into a method. After completing Wave 3 we have no custom model methods.
Do you have any recommendations on how we could improve this project for the next cohort? Walk through it together as a class to identify requirements. Provide detailed examples of smoke test workflow.
Link to Trello https://trello.com/b/88rCy6nY
Link to ERD n/a hand drawn provided upon request

Krashaune and others added 30 commits May 7, 2018 15:27
@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 no
All required endpoints return expected JSON data yes
Requests respond with appropriate HTTP Status Code yes
Errors are reported mostly - you're not checking for errors in a couple places, but this mostly looks good.
Testing
Passes all Smoke Tests almost!
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 - great work, especially on the RentalsController. Here is another case you could test: when checking in a movie, what happens if the customer has the same movie checked out multiple times?
Overall Good work overall! I would definitely like to see some of your business logic moved to the model, but your testing looks great and the API functions almost to spec. Keep up the hard work!


new_rental = Rental.new(rental_params)
new_rental[:checkout] = date
new_rental[:due_date] = date + 7

Choose a reason for hiding this comment

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

You should take all of the logic in this controller action and encapsulate it in a model method. Perhaps something like Movie#checkout(customer_id). You could have it raise an exception if there's not enough inventory available, or implement a custom validation on the rental.

if rental.checked_in
render json: {
errors: {
checked_in: ["Movie has not been checked out."]

Choose a reason for hiding this comment

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

I like that you're checking for this


validates :name, presence: true
validates :phone, presence: true
validates :phone, length: { is: 14 }

Choose a reason for hiding this comment

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

You might be over-validating here. Your app probably won't break if the customer's phone number isn't right, so validating it (and testing the validation, and making your tests provide the phone number in the right format) is more trouble than it's worth.

Choose a reason for hiding this comment

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

Same thing goes for many of your model validations.


it "returns an error for an invalid movie" do
bad_data = @movie_data.clone()
bad_data.delete(:title)

Choose a reason for hiding this comment

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

Good job catching this error case!

require "test_helper"

describe CustomersController do
describe 'index' do

Choose a reason for hiding this comment

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

What about show and create?

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