Earth - Rose Video - Denise and Anya#30
Conversation
Route set up
had customers instead of rentals - copy paste error
…into customer-controller
Customer controller
Customer controller
rental validations added but tests commented out for now rentals fixture created
changed customer infor show not pass smoke test now doesnt show addre…
video validation tests added and passing
video models tests passing
rental validations tests passing
wave1 smoke tests all passing
All tests passing except for postman checkin
CheezItMan
left a comment
CheezItMan
left a comment
There was a problem hiding this comment.
Video Store API
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
| Practices git with at least 10 small commits and meaningful commit messages | ✔️ |
| Understands the importance of how APIs handle invalid/erroneous data in their reflection questions | ✔ |
| Practices Rails best practices and well-designed separation, and encapsulates business logic around check-out and check-in in models | |
| Uses controller tests to ensure quality code for every route, and checks for response status and content, with positive cases and negative cases | |
Uses controller tests to ensure correctness for check out and check in requirement, and that checked out counts and inventories appropriately change |
✔️, except as noted above |
Functional Requirements
| Functional Requirement | yes/no |
|---|---|
| All provided smoke tests for Wave 1 pass | ✔️ |
| All provided smoke tests for Wave 2 pass |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 3+ in Code Review && 2 in Functional Requirements | 💚 Nice work |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | ✅ |
| Descriptive/Readable | ✅ |
| Logical/Organized | ✅ |
Summary
Nice work, you hit the main learning goals here. Well done. You can check out my comments, mostly on missing tests and model methods. Otherwise it works!
| validates :total_inventory, presence: true | ||
| validates :overview, presence: true | ||
| validates :release_date, presence: true | ||
|
|
There was a problem hiding this comment.
It might make a good idea to have a checkout and check_in model methods to hold the business logic.
| # end | ||
|
|
||
|
|
||
| def check_out |
There was a problem hiding this comment.
These check_out and check_in methods are pretty big. It would be better to abstract out the logic of creating a rental record into a model method.
The controller should focus solely on handling the request and passing business logic to the models.
| required_fields.each do |field| | ||
| customer_one[field] = nil |
| require "test_helper" | ||
|
|
||
| describe Customer do | ||
| describe "validations" do |
There was a problem hiding this comment.
You are missing validations for uniqueness like for phone number.
| validates :customer_id, presence: true | ||
| validates :video_id, presence: true |
There was a problem hiding this comment.
Just FYI, by belonging to video and customer, these validations are already on by default.
| validates :available_inventory, presence: true | ||
| validates :total_inventory, presence: true |
There was a problem hiding this comment.
No validations to ensure that the number in inventory is 0 or positive?
| it "responds with JSON and success" do | ||
| get videos_path | ||
|
|
||
| expect(response.header['Content-Type']).must_include 'json' | ||
| must_respond_with :ok | ||
| end |
There was a problem hiding this comment.
This should probably be in the describe for index
| # checkout_date: | ||
| # due_date: | ||
| # status: |
| returned: false} | ||
| } | ||
|
|
||
| it "can create a rental check in" do |
3 participants
Assignment Submission: Video Store API
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
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.
Reflection
/customers&/videosendpoints? What does the time complexity depend on? Explain your reasoning.POST /rentals/check-inendpoint? What does the time complexity depend on? Explain your reasoning.