Skip to content

Conversation

@amythetester
Copy link

@amythetester amythetester commented May 17, 2019

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. Based on the JSON files we were able to create models for customer and movie. When we read the requirements we determined that Rentals were comprised of one movie and one customer. This led us to create a belongs_to relationship where rentals belong to movies and customers.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. We believe the time complexity is O(n) where n is the number of records in the database for that model. This is because as records are added to the database the iterations the method has to go through increases linearly.
What is the Big-O time complexity of the POST /rentals/check-in endpoint? What does the time complexity depend on? Explain your reasoning. We believe the time complexity is O(n) where n is the number of records in the rentals database for the Rental model. This is because the rental you are looking for could be at the end of the dataset.
Describe a set of positive and negative test cases you implemented for a model. For the movie model, we had a positive test where we tested that the available inventory increased and we had a negative test where available inventory was not increased because all movies were already checked in.
Describe a set of positive and negative test cases you implemented for a controller. For the rental controller, our positive test was testing that a new rental was created and our negative test was to ensure a rental was not created when an invalid movie id was provided.
How does your API respond when bad data is sent to it? Most of the time it returns a bad request and an error message in JSON.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. Reduce available inventory was one of our custom model methods for movie. We chose to wrap this functionality into a method because this keeps the business logic out of the controller, but allows the method to be called from the controller.
Do you have any recommendations on how we could improve this project for the next cohort? This note in Wave 2 of the instructions was confusing because the smoke tests include a test that required optionals to be complete: "movies_checked_out_count: This will be 0 unless you've completed optional requirements"
Link to Trello https://trello.com/b/AWPyA1yo/videostoreapi
Link to ERD https://drive.google.com/file/d/1275Oly6fSqGLj-yq_7e-UMTvUAvGDkL6/view?usp=sharing
Summary We worked very intentionally to implement TDD red/green refactor approach and we also did a PR/Merge workflow as well.

amythetester and others added 30 commits May 14, 2019 11:39
describe block
Customer model tests (can delete when merged)
Customer controller tests and CRUD
updated for fields that only need to be returned
@CheezItMan
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages
Comprehension questions Check, I'm glad you were able to practice TDD in this exercise
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 Nope skimpy testing
Controller Tests - URI parameters and data in the request body have positive & negative cases Nope Very skimpy testing here as well, mostly just copy-paste of the instructor tests in class.
Overall You have a nice working API. However your testing was very weak. I suggest when looking at testing taking a step back and try to play gremlin with the situation. Look at the check-in process and the variables involved, movie_id and customer_id and see what could be passed in wrongly.

@@ -0,0 +1,24 @@
class Movie < ApplicationRecord
validates :title, :inventory, presence: true
validates :available_inventory, numericality: { greater_than: -1 }

Choose a reason for hiding this comment

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

This is causing the seeds file to fail!

require "test_helper"

describe Rental do
let(:rental) { rentals(:one) }

Choose a reason for hiding this comment

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

No tests here?

Also you should try to see what happens if you create a rental with no available inventory for the movie.

@@ -0,0 +1,9 @@
class Customer < ApplicationRecord

Choose a reason for hiding this comment

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

No has_many?

@@ -0,0 +1,24 @@
class Movie < ApplicationRecord

Choose a reason for hiding this comment

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

No has many rentals?

end
end

describe "relationships" do

Choose a reason for hiding this comment

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

Missing tests!!!

end

describe "custom methods" do
describe "checkin" do

Choose a reason for hiding this comment

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

What about a customer trying to return a movie that wasn't checked out? Or a movie that wasn't checked out by them?

render json: movie.as_json(only: [:id]),
status: :ok
else
render json: { errors: ["Movie could not be saved"] },

Choose a reason for hiding this comment

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

No validation errors reported?

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