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

Leti & Maddie - VideoStoreAPI - Ampers #21

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

Conversation

LetiTran
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. After establishing the relationships between the models we checked the attributes from the seeds files to match our models.
Describe a set of positive and negative test cases you implemented for a model. Testing for valid attributes.
Describe a set of positive and negative test cases you implemented for a controller. Testing that the params for #create should be present and correct. It should return a bad request for incomplete params.
How does your API respond when bad data is sent to it? Gives a response of errors containing bad_request.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. In the Movie model we created a method to first find available_inventory for a specific Movie. We called upon that method with the RABL view to show the available inventory for the movie.
Do you have any recommendations on how we could improve this project for the next cohort? If there is time give more lecture on utilizing RABL. Also explain how smoke tests work prior to exposure to the project.
Link to Trello TRELLO
Link to ERD ERD

LetiTran and others added 30 commits May 7, 2018 11:36
has_many :rentals

def available_inventory
if self.inventory
Copy link

Choose a reason for hiding this comment

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

Why would self.inventory ever not be true?

@tildeee
Copy link

tildeee commented May 21, 2018

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x
General
Business Logic in Models x
All required endpoints return expected JSON data x
Requests respond with appropriate HTTP Status Code x
Errors are reported x
Testing
Passes all Smoke Tests x
Model Tests - all relations, validations, and custom functions test positive & negative cases x
Controller Tests - URI parameters and data in the request body have positive & negative cases x
Overall

f--F-F-FAAANCY pagination work!!! Also rabl working, nice!

I think that the project looks good -- I think the model and controller tests are pretty thorough

In your fixtures files, like rentals.yml, you have a rental (rental_three) that is invalid data: although you all did a good job of testing it, because everything in your fixtures files gets put into a test DB (and bypasses validations), I'd prefer if you didn't put invalid stuff in the fixtures, and made invalid stuff like that in the test itself

Otherwise, looks good -- I think there are some really nice stylistic things that you all did that I'll comment on!

end

else
render json: {ok: false, errors: "This will not work"}, status: :bad_request
Copy link

Choose a reason for hiding this comment

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

The phrase "This will not work" is a little awkward sounding?

class MoviesController < ApplicationController
def index

params[:sort] ? (sort = params[:sort].to_sym) : sort = :id
Copy link

Choose a reason for hiding this comment

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

this line is great! I'm a fan!

Ternaries also work so you can assign things like this (this does the same thing you did):

sort = params[:sort] ? params[:sort].to_sym : :id


@movie = Movie.find_by(id: params[:id])

render json: {ok: false, error: :not_found}, status: :not_found if @movie.nil?
Copy link

Choose a reason for hiding this comment

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

these post-fix conditionals look really fancy and nice!

# scope :by_name, -> (name) { where(name: name) }

def movies_checked_out_count
return Rental.where(customer_id: self.id).where(returned: false).count
Copy link

Choose a reason for hiding this comment

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

This might be an instance where instead of Rental.where(customer_id: self.id) you could use self.rentals, if you find a way to filter on these with returned == false (maybe .filter?)

Copy link

Choose a reason for hiding this comment

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

Then again, I see the benefit in the style you wrote it in already here

font-size: 30px;
}

.about-me {
Copy link

Choose a reason for hiding this comment

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

What are these styles from? :P

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