Skip to content

Sara (branches) & Julia K (leaves)#33

Open
Kalakalot wants to merge 67 commits intoAda-C12:masterfrom
Kalakalot:master
Open

Sara (branches) & Julia K (leaves)#33
Kalakalot wants to merge 67 commits intoAda-C12:masterfrom
Kalakalot:master

Conversation

@Kalakalot
Copy link

@Kalakalot Kalakalot commented Nov 8, 2019

Question Answer
Explain how you came up with the design of your ERD, based on the seed data. We didn't actually use the seed data for the design of the ERD -- we designed it based on the endpoints described in the project waves. We organized all the endpoints by subject, analyzed how the topics would need to talk to each other, and used that information as the basis of our ERD.
What would be the Big-O time complexity of your /customers & /movies endpoints? What does the time complexity depend on? Explain your reasoning. We need to loop through all of the instances of customer/movie and print them out, so the time complexity is O of (n), with n as the count of each instance of customer or movie. To determine time complexity, look at how adding a new instance/element/integer/etc. to the input would impact the number of operations the program needs to perform. If adding something new to the input means performing one additional operation, the time complexity is O of (n).
What is the Big-O time complexity of the POST /rentals/check-in endpoint? What does the time complexity depend on? Explain your reasoning. O of (n) again because when the check in request is sent to the controller, the program may have to search through all the instances of movies and customers to find the one requested (or to determine that one or the other doesn't exist).
Describe a set of positive and negative test cases you implemented for a model. In the customer model, the positive test case is that when all the required fields are provided, a new customer is created. A negative test case is if the name isn't included, the program throws an error stating that name is required.
Describe a set of positive and negative test cases you implemented for a controller. In the rentals controller for the checkout action, the positive test is that if the customer id is valid and the movie id is valid and there is a copy of the movie available, a new rental is created. A negative test is that if there are no copies of the movie available, a rental is not created.
How does your API respond when bad data is sent to it? It responds with an appropriate status code ("not found" if the movies#show action is passed a bogus id, "bad request" if the rental checkout params aren't legit) and a relevant error message.
Describe one of your custom model methods and why you chose to wrap that functionality into a method. We don't have any custom model methods -- our functionality all lives in the controllers (which, now that we think about it, was probably the wrong design choice.)
Do you have any recommendations on how we could improve this project for the next cohort? Nope -- we found this project well explained and documented. Chris' video explaining how to run smoke tests was invaluable.
Link to Trello https://trello.com/b/ZWiwEFjL/video-store-api
Link to ERD https://tinyurl.com/y3ajp7g5 Sorry, didn't realize we'd be sharing this! The top item (hard to see because it's crossed out) is rentals.
Summary

sarashahbaig and others added 30 commits November 5, 2019 15:26
Added column movies_checkout_out_count to customers table
…mers_controller_test so that keys are correctly alphabetized. All tests passing.
@Kalakalot Kalakalot changed the title Sara & Julia K Sara (branches) & Julia K (leaves) Nov 9, 2019
sarashahbaig and others added 9 commits November 8, 2019 16:39
…idation for rentals, which throws off tests. Commenting out until Sara and I talk about this.
…te is calculated in rentals_controller, considered adding due_date method to rental model but decided against it. Rentals controller due date test now passing.
…tal model tests for validations, all passing.
@jmaddox19
Copy link

Video Store

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene It's great how frequent the commits are. One comment: I notice a few generic messages like "code added", "refectored", "test added", etc. that do not make it clear what actually changed. Most of your messages are clear and explicit. Making a point to always be explicit like that with every commit is important for when you end up needing to retrace your git history.
Comprehension questions Yes, glad Chris's video was so helpful :)
General
Business Logic in Models Nope, as you mention in the comprehension question. This is most noticeable in the RentalsController. Breaking this code out into model methods is an area for improvement.
All required endpoints return expected JSON data yes!
Requests respond with appropriate HTTP Status Code yes! Including the POST requests, which return 201 as they should even though the smoke tests ask for a 200. (Good catch!)
Errors are reported Yes! Great job with error messages.
Testing
Passes all Smoke Tests No, checkout doesn't seem to actually be working and thus check-in is failing.
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 The tests are there but there should be more assertions.
Overall The code looks good overall! Much of what is in rentals controller would be cleaner in smaller methods on the rental model. The check-in/check-out functionality not working together is a gap that would be worth noting as a message in the PR itself so we as an instructional staff know that you know it's missing.


describe RentalsController do

describe "check out" do

Choose a reason for hiding this comment

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

None of your tests here check for a decrease in available_inventory, which would've caught the problem that the smoke tests surface.

Choose a reason for hiding this comment

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

...at least that's my assumption about where the bug is based on the error message I'm seeing on the checkin API

@jmaddox19
Copy link

Oh another note, the movies index page produced an error when there were no movies. This is an edge case a unit test for that edge case would've caught.

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

Comments