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

Ari/Brenda- Octos- VideoApiMuncher #20

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

Conversation

arrriiii
Copy link

@arrriiii arrriiii 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 wrote out the fields in each model and figure out the relationships between the Customer, Movie, and Rental models. We decided on using a join table for the Rentals due to the relationships.
Describe a set of positive and negative test cases you implemented for a model. We tested that a movie could be created with valid input and then tested that a movie is not created without a title.
Describe a set of positive and negative test cases you implemented for a controller. For the show method in the movies controller we tested that it can get a movie and also that it yields a not found status and return an error if the movie does not exist.
How does your API respond when bad data is sent to it? It returns JSON with error messages in a hash.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. In model rental we have custom methods such as build_rental which updates the available inventory, the customer movies checked out count and the status checked out. So we tested that it updates for a newly rented movie and then tested that will not update with a bogus rental.
Do you have any recommendations on how we could improve this project for the next cohort? Maybe explain more what the smoke tests are testing.
Link to Trello https://trello.com/b/mdITg1lC/videostoreapi
Link to ERD https://www.lucidchart.com/documents/edit/206f7695-d492-41fa-b485-37ee539c1a1a/0

arrriiii and others added 30 commits May 7, 2018 12:05
@kariabancroft
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Looks good
Comprehension questions Yes
General
Business Logic in Models Yes - primarily some methods in the Rentals to handle checking in and checking out
All 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, good. Only a few negative cases missing.
Controller Tests - URI parameters and data in the request body have positive & negative cases Yes, good
Overall You did a nice job overall. You covered the major logic in the controller and models as well as testing them.


# TODO: To make this happen automatically
def checkout
@rental = Rental.new(rental_params)

Choose a reason for hiding this comment

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

Since you're not rendering views, you shouldn't need instance variables in your controllers.

self.due_date= (self.checkout_date + 7)
end

def build_rental

Choose a reason for hiding this comment

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

Nice job switching these from class methods to instance methods per our conversation

new_inventory = self.movie.available_inventory -= 1
customer_movie_count = self.customer.movies_checked_out_count += 1

self.update_attribute(:checked_out, true)

Choose a reason for hiding this comment

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

When you're updating a single property, it is more overhead than necessary to use update_attribute. You should just set the property directly self.checked_out = true.

require "test_helper"

describe Movie do
let(:movie) { Movie.new }

Choose a reason for hiding this comment

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

Looks like you aren't ever using this. While it is in a let block that will be lazy loaded, you still shouldn't have it if it never used.


describe Rental do
describe 'relations' do
let(:rental1) { rentals(:rental1) }

Choose a reason for hiding this comment

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

Also looks like you're not using this variable

end
end
describe 'validations' do
it "must be a valid rental" do

Choose a reason for hiding this comment

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

Will it be valid without a due date?

@@ -0,0 +1,120 @@
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.

Watch your indentation here

end

it "gets index with no movies" do
Movie.all.each { |movie| movie.destroy }

Choose a reason for hiding this comment

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

You can also use destroy_all

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