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

bo and sai's video store API #16

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

bo and sai's video store API #16

wants to merge 74 commits into from

Conversation

ssamant
Copy link

@ssamant ssamant 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 determined what the fields of the the model would be by looking at the seed data.
List all the completed endpoints of your application. GET /movies, GET movies/:title, GET /customers, POST /rentals/:title/check-out, POST /rentals/:title/check-in, WIP (hoping to get this done by Monday!): GET /rentals/overdue
Describe a set of positive and negative test cases you implemented for a model. We had postivie and negative tests for the due_date attribute in the Rental model. We tested the presence, data type and wrote a custom method to make sure the due_date is set for a future date. We tested all of the edge cases that could occur with these requirements.
Describe a set of positive and negative test cases you implemented for a controller. For the movies index controller, we checked that the body of the JSON returned the required keys for a good request and that we got the appropriate error messages for a bad request.
How does your API respond when bad data is sent to it? We send a :bad_request response and an error message that relates to the reason for the bad response (for instance for getting a particular movie, our error message says ":Title not found"
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We wrote a custom method in the Movie model to return a movie's available inventory. This way we didn't have to add an extra field to the movie db and we could use the method to return the json key-value pair as required.
Do you have any recommendations on how we could improve this project for the next cohort? No -- it felt like a reasonable project that helped us learn more about APIs (and testing!)
Link to Trello https://trello.com/b/My2eCvMP/video-store-api
Link to ERD (we also did an ERD on the whiteboard if you need a pic of that) erd.pdf

ssamant and others added 30 commits May 9, 2017 14:10
baseline with test route
added gems needed for testing and error views
generated models for movies and customers
botrethewey and others added 25 commits May 11, 2017 09:41
wrote controller action for rental create and updated tests
updated tests and controller logic
WIP overdue action in rental controller
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Lots of commits, contributions from both team members and good commit messages. Well done!
Comprehension questions Check, glad you enjoyed the project
General
Business Logic in Models Check, interesting custom validation method to verify inventory & due date. I think the due date could be done with an existing validator. It' also interesting how you overrode the initialize method.
All 3 required endpoints return expected JSON data Check!
Requests respond with appropriate HTTP Status Code Check
Errors are reported Good use of the errors hash to report errors.
Testing
Passes all Smoke Tests Check, very nicely done!
Model Tests - all relations, validations, and custom functions test positive & negative cases Check, very good edge-case testing. There is one broken test, returns an error message if available inventory is less than 1
Controller Tests - URI parameters and data in the request body have positive & negative cases There is one broken test, returns an error if there is not enough inventory. Both of these errors come from the check_inventory method in rental.rb.
Optionals
POST routes use URI parameter and request body to add a new record to the database Well done
GET /customers shows how many movies are checked out by a customer Check
GET /movies/:title shows updated inventory Well done
Overall Nicely done, some small bugs in checking out videos when there's not enough stock, but that's a minor issue. I like how you experimented with custom validators and you have good business logic in the models. A good end to our Rails unit!

Copy link

@brianaeng brianaeng left a comment

Choose a reason for hiding this comment

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

Overall great job! I didn't find any glaring issues or anything, keep up the great work 💯

validates :title, presence: true
validates :overview, presence: true
validates :release_date, presence: true, format: { with: /\A\d{4}-(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[0-1])\z/, message: "must be in YYYY-MM-DD format" }
validates :inventory, presence: true, numericality: { only_integer: true, greater_than: 0}

Choose a reason for hiding this comment

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

Really great job with the validations!

if movie.available_inventory <= 0
rentals = Rental.where(movie_id: movie.id)
earliest = rentals.min_by { |rental| rental.due_date}
errors.add(:availability, "Sorry, this movie is not in stock. One is due back on #{earliest.due_date}")

Choose a reason for hiding this comment

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

Noting the earliest due date is a nice touch 👍

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

Choose a reason for hiding this comment

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

Yay tests! I dig how thorough these ones are 🎉


one:
title: Titanic
overview: ship disaster

Choose a reason for hiding this comment

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

Omg these movie overviews 😂

overdue_rentals = []
customer_ids.each do |id|
customer = Customer.find_by_id(id)
hash = Hash.new

Choose a reason for hiding this comment

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

For consistency you could do {} (since you do [] for a new array)...but this is me completely nitpicking bc you did such a great job 😁

end

def overdue
#what is the syntax for a conditional for due date?

Choose a reason for hiding this comment

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

Remember to clean up comments! 🔍

end

def valid_due_date
if due_date.class != Date

Choose a reason for hiding this comment

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

How do you take in the due date? Would checking if the due date is too far in the future (like if I tried to check something out for 2183721983 years) be necessary?


# Ignore Byebug command history file.
.byebug_history
/coverage/

Choose a reason for hiding this comment

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

Could also ignore that pesky .DS_Store



# Bundle edge Rails instead: gem 'rails', github: 'rails/rails'
gem 'rails', '~> 5.0.2'

Choose a reason for hiding this comment

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

Ohhh that's why some of this looks weird to me, shiny new rails! ✨ (we used 4.2)

Choose a reason for hiding this comment

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

I had the same reaction!


def check_inventory
if movie
if movie.available_inventory <= 0

Choose a reason for hiding this comment

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

Would the avail inventory ever go below 0?

Copy link

@rpavilanis rpavilanis left a comment

Choose a reason for hiding this comment

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

Fantastic job - it looks like you both went above and beyond the requirements and learned a lot about building an API. nice work. :)



# Bundle edge Rails instead: gem 'rails', github: 'rails/rails'
gem 'rails', '~> 5.0.2'

Choose a reason for hiding this comment

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

I had the same reaction!

# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw, :jruby]

group :development do

Choose a reason for hiding this comment

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

It looks like you have two 'group :development do' sections - you could combine them to clean up the Gemfile.


# update later after adding rental
def movies_checked_out_count
return rentals.where(returned: false).length

Choose a reason for hiding this comment

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

Nice job pulling out business logic into models. Good work on being thorough with validations.

value(response).must_be :success?
end

end

Choose a reason for hiding this comment

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

Wow, I can tell that you really put a lot of time into writing thorough tests. Nice job testing edge cases as well.

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.

5 participants