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

Pipes - Iuliia and Irene - VideoStoreAPI #4

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

Conversation

idevera
Copy link

@idevera idevera commented Nov 7, 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. If we had continued with the optionals, our group thought of an ERD implementing the Rental model which is found in the link below. Currently we do not have relations set up between Customer and Movie
Describe a set of positive and negative test cases you implemented for a model. One positive case is if all the required parameters are present, it will create an instance of the model. One negative case is when there is at least one missing required parameter, the model will not be created and will not be valid.
Describe a set of positive and negative test cases you implemented for a controller. For the show method, if the movie does not exist, it will return not found and will return with a success if it is found
How does your API respond when bad data is sent to it? For the MovieController create method, if the end user submits bad data like an empty parameter, errors will be rendered back to the user and a bad_request will be raised
Describe one of your custom model methods and why you chose to wrap that functionality into a method. The only custom we have is the set_available_inventory method in the movie model. We chose to wrap this in a method because we need to set the available inventory to the initial inventory when the movie is initially created. This was a set as a private method so that it can only be accessed by the movie class and will not be available to other classes
Do you have any recommendations on how we could improve this project for the next cohort? Serializer did not work as expected. Maybe find an alternative gem?
Link to Trello https://trello.com/b/eLFHKzXr/video-store-api
Link to ERD https://www.lucidchart.com/documents/edit/4013935b-96d8-4681-a32b-119fb1866d60#

@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
All 3 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
Controller Tests - URI parameters and data in the request body have positive & negative cases yes
Overall Great work overall!

else
render(
json: {nothing: true}, status: :not_found
)

Choose a reason for hiding this comment

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

This will literally send back { nothing: true } as a JSON response. Not sure that's what you intended.

@@ -0,0 +1,14 @@
class Movie < ApplicationRecord
before_create :set_available_inventory

Choose a reason for hiding this comment

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

Good use of a model filter here.

Another strategy is, instead of storing available inventory as a column in the DB, compute it on the fly based on what rentals currently exist for this movie. This would be a little more work, but avoids storing redundant information.

movie = Movie.new(invalid_data)

movie.wont_be :valid?
Movie.count.must_equal before_count

Choose a reason for hiding this comment

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

Here (and above in the same test for Customer) it would be good to check which fields are failing to validate, to ensure that all the problems are being caught. As is, I could remove any one of the validations and this test would continue to pass.

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