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 - Andrea Valliere & Kaitlin Ramirez - VideoStoreAPI #9

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

Conversation

avalliere
Copy link

@avalliere avalliere 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. We referenced the seed data and carefully considered the relations that we would need between the models.
List all the completed endpoints of your application. get "customers" get "movies" get "movies/:title"
Describe a set of positive and negative test cases you implemented for a model. For Movie model, we had a validation for movie title - checking for cases in which it has a title and when it doesn't have a title.
Describe a set of positive and negative test cases you implemented for a controller. It can get a movie if it exists and it does not get a movie that doesn't exist.
How does your API respond when bad data is sent to it? Each method gives 404 not found and show gives a specific custom message.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We created custom model methods to get the available inventory for a movie. We chose this route because it wasn't part of the seed data or CRUD functionality - business logic!
Do you have any recommendations on how we could improve this project for the next cohort? Nope. We really appreciated how do-able the baseline requirements were and the detailed Readme file.
Link to Trello https://goo.gl/IJSvoH
Link to ERD https://goo.gl/m31HLQ

@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 (not much needed before optionals)
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 N/A
GET /customers shows how many movies are checked out by a customer N/A
GET /movies/:title shows updated inventory N/A
Overall

Good work overall!

Copy link

@miriam-cortes miriam-cortes left a comment

Choose a reason for hiding this comment

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

nice job! Leah and I worked together (that's why you won't see comments from her screen name)

validates :name, presence: true

def movies_count
return 0

Choose a reason for hiding this comment

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

not sure what this method is doing. Why should it always return 0?

has_many :rentals
has_many :customers, :through => :rentals
validates :title, presence: true

Choose a reason for hiding this comment

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

Leah says she doesn't like these two spaces here, you might as well delete :)

<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<style>
/* Email styles need to be inline */

Choose a reason for hiding this comment

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

get out of the habit of leaving comments in your code...it will help you during internship 🙄 (bc I do the same)

Rails.application.routes.draw do
get "customers", to: "customers#index", as: "customers"
get "movies", to: "movies#index", as: "movies"
get "movies/:title", to: "movies#show", as: "movie"

Choose a reason for hiding this comment

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

awesome job of keeping them dry :)

end
end

# describe "show" do

Choose a reason for hiding this comment

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

if you don't need this just delete

@@ -0,0 +1,50 @@
require "test_helper"

describe MoviesController do

Choose a reason for hiding this comment

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

yayyyyy tests!

end

# No customer show per READ ME
# def show

Choose a reason for hiding this comment

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

delete any code you're not using

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.

4 participants