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 & Nkiru - Pipes - VideoStoreAPI #8

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

Conversation

nkiruka
Copy link

@nkiruka nkiruka commented Nov 8, 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. Since there was a many to many relationship between movies and customers, we decided that an intermediary table will be needed. We decided to create a rental model (intermediary) that will connect movies and customers. Therefore, we ended up using has_many :through association such as for movies (i.e movies has many customers through rentals; customers has many movies through rentals).
Describe a set of positive and negative test cases you implemented for a model. We included validations for movie and rental models, but testing is not required for this project. For refactoring, we will move methods from the controllers to the model, specifically calculation for movie rental due date.
Describe a set of positive and negative test cases you implemented for a controller. Positive test: we tested that the route is accessible, and also tested that a new movie or customer can be added with valid fields. Negative test: we tested if bad data was given that a valid customer, movie or rental will not be returned and it responds with a bad request.
How does your API respond when bad data is sent to it? Bad request and occasionally, "not found".
Describe one of your custom model methods and why you chose to wrap that functionality into a method. Custom model method: due_date (Calculation is based on the day the customer checks the movie out plus the set rental days allowed.)
Do you have any recommendations on how we could improve this project for the next cohort? If it is a three day week, it will be great to have a new list of requirements or assign project as an individual project since there were more optionals available for this project.
Link to Trello
Link to ERD https://www.lucidchart.com/documents/edit/9ef69130-df51-4c9d-8a5d-08547500cb05#

@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
All 3 required endpoints return expected JSON data yes
Requests respond with appropriate HTTP Status Code yes
Errors are reported yes
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
Overall Great work overall!

1 similar comment
@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
All 3 required endpoints return expected JSON data yes
Requests respond with appropriate HTTP Status Code yes
Errors are reported yes
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
Overall Great work overall!

)
else
render json: {nothing: true}, status: :not_found
end

Choose a reason for hiding this comment

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

This will literally send back { nothing: true } as a JSON response. Not sure that's what you intended.


def customer_params
params.require(:customer).permit(:id, :name, :registered_at, :address, :city, :state, :postal_code, :phone, :account_credit, :movies_checked_out_count)
end

Choose a reason for hiding this comment

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

You shouldn't need to define strong params if you don't have a create or update action. As far as I can tell this method is never called.

#
=======

>>>>>>> d895f891acf50da64888798d075b4c23e843b99f

Choose a reason for hiding this comment

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

Why are there git merge markers in this file?

Choose a reason for hiding this comment

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

If missing these markers is a regular problem for you, you might want to set up a pre-commit hook:
https://jondowdle.com/2015/02/block-merge-conflicts-in-commits/

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