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

Phoebe & Jackie - Ampers #12

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

Phoebe & Jackie - Ampers #12

wants to merge 62 commits into from

Conversation

Jackiesan
Copy link

@Jackiesan Jackiesan commented May 11, 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. We initially created two models, one for Customers and one for Movies. This was based on the charts provided in the project description and the seed data. When we got to Wave 3, we based the intermediate table on the tables that represent the request body.
Describe a set of positive and negative test cases you implemented for a model. For initialization of a rental we tested for a case in which all required values were provided (movie ID and customer ID). To test for a negative case we simulated a post action in which the customer ID was not provided.
Describe a set of positive and negative test cases you implemented for a controller. For the get movie path we tested for an existing movie and for a non existing movie. An existing movie should return a has with the movies information while a non existing movie is a 404 not found.
How does your API respond when bad data is sent to it? Our API responds with an errors hash.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. One custom model method created is a method to calculate the available inventory. We decided to wrap the functionality into a method because it is logic. And while it is relatively short, putting it in a method in the model cleans up the code in the controller.
Do you have any recommendations on how we could improve this project for the next cohort? One recommendation I have is to provide a short intro to smoke tests.
Link to Trello https://trello.com/b/XtkHI9aY/videoapi
Link to ERD https://www.lucidchart.com/invitations/accept/c2cbdd2b-4b76-4505-aac5-7dadf203193e

pmanni02 and others added 30 commits May 7, 2018 12:42
Jackiesan and others added 28 commits May 8, 2018 14:01
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages, both partners contributed
Comprehension questions Check
General
Business Logic in Models Check
All required endpoints return expected JSON data Check
Requests respond with appropriate HTTP Status Code Check
Errors are reported Check
Testing
Passes all Smoke Tests Check
Model Tests - all relations, validations, and custom functions test positive & negative cases Check with one missing test, see my inline notes
Controller Tests - URI parameters and data in the request body have positive & negative cases Check
Overall Very well done, you hit all the requirements. Congratulations, you're done with Rails!

def get_count
items_checked_out = self.rentals.select { |rental| rental.check_in_status == false }

if !items_checked_out.empty?

Choose a reason for hiding this comment

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

Why not just return items_checked_out.length or .count why have the if statement?

end

describe "custom methods" do
it "returns an integer equal to inventory when there are no rentals" do

Choose a reason for hiding this comment

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

Seems like available_inventory needs another test.

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