Conversation
…g relationships, seeded db
…tive tests for passenger controller
…d some tests in driveres_controller
… refactored accordingly
CheezItMan
left a comment
There was a problem hiding this comment.
Rideshare Rails
Major Learning Goals/Code Review
| Criteria | yes/no, and optionally any details/lines of code to reference |
|---|---|
| Practices collaborating with git, and all team members contribute git commits and best git practices | ✔️ |
| Demonstrates understanding of relationships by giving accurate answers to the reflection questions and appropriate code in the models | ✔️ |
| Logic to calculate a driver's average rating and total earnings is located in the model, and it has unit tests | ✔️, however you need to test some edge cases see my inline methods. |
| There are reasonable tests to test the validation requirements for all models | ✔️ |
| There are reasonable tests to test the relationship requirements for all models | ✔️ |
| There are reasonable tests to test the controller actions for all controllers | ✔️ |
| The app has an attractive and usable user interface | ✔️, although I think things would look better in a table-like format. |
| The app uses/is compatible with database seeds | ✔️ |
Router code is clean: uses resources and RESTful routes |
✔️, however see some of my notes |
Functional Requirements
| Functional Requirement | yes/no |
|---|---|
| On the passenger's details page, I want to be able to see total charged, list of trips, a link to edit, and delete | ✔️ |
| When adding a new passenger, I want to see errors and validations that show that a passenger must be provided a name and a phone number, so that I cannot make a passenger without name or phone number | ✔️ |
| On the passenger's details page, I can create a new trip for this passenger, with an assigned driver and no rating | ✔️ |
| On the driver's details page, I want to be able to see total earnings, average rating, list of trips, a link to edit, and delete | ✔️ |
| When adding a new driver, I want to see errors and validations that show that a driver must be provided a name and VIN, so that I cannot make a driver without name or VIN | ✔️ |
| On the trip's detail page, I want to be able to view details, assign a rating, navigate to the trip's passenger, driver, a link to edit, and delete | ✔️ |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 8+ in Code Review && 5+ in Functional Requirements |
Code Style Bonus Awards
Was the code particularly impressive in code style for any of these reasons (or more...?)
| Quality | Yes? |
|---|---|
| Perfect Indentation | ✅ |
| Elegant/Clever | ✅ |
| Descriptive/Readable | ✅ |
| Concise | ✅ |
| Logical/Organized | ✅ |
Summary
Really well done. You hit the learning goals here. I like some of your helper methods and you have some good tests too. However you needed to test some edge cases on the custom methods, and I found a few smaller things to make note of, status codes, etc. Take a look and let me know what questions you have.
| end | ||
|
|
||
| def assign_rating | ||
| raise |
| it "will redirect to the trips page if given an invalid id" do | ||
| # Your code here | ||
| id = -1 | ||
| delete trip_path(id) | ||
| must_redirect_to trips_path | ||
| end |
There was a problem hiding this comment.
I think you intended to try to give a rating to an invalid trip.
| it "will redirect to the trips page if given an invalid id" do | |
| # Your code here | |
| id = -1 | |
| delete trip_path(id) | |
| must_redirect_to trips_path | |
| end | |
| it "will redirect to the trips page if given an invalid id" do | |
| # Your code here | |
| id = -1 | |
| expect{ | |
| patch assign_rating_trip_path(id), params: {rating: 4} | |
| }.wont_change "Trip.count" | |
| must_redirect_to trips_path | |
| end |
| end | ||
| end | ||
|
|
||
| describe "update" do |
There was a problem hiding this comment.
What about if the user submits an update, but violates a validation?
|
|
||
| describe "create" do | ||
| # Your tests go here | ||
| it "can create a new trip" do |
There was a problem hiding this comment.
What about creating an invalid trip?
| end | ||
| end | ||
|
|
||
| describe "update" do |
There was a problem hiding this comment.
What about updating a passenger with invalid data?
| resources :passengers do | ||
| resources :trips, only: [:index, :create] | ||
| end | ||
| delete '/drivers/:id', to: 'drivers#delete', as: 'delete_driver' |
There was a problem hiding this comment.
Do you need this? You have resources.
| describe "complete trip" do | ||
| # Your code here | ||
| describe "total_charged" do | ||
| it "correctly displays total charged"do |
There was a problem hiding this comment.
What about a passenger with no trips?
| end | ||
| end | ||
|
|
||
| describe "total earnings" do |
|
|
||
| # Tests for methods you create should go here | ||
| describe "custom methods" do | ||
| describe "average rating" do |
| available: params[:driver][:available] | ||
| }) | ||
|
|
||
| if result |
There was a problem hiding this comment.
| if result | |
| if result.valid? |
Assignment Submission: Rideshare Rails
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions. These questions should be answered by all team members together, not by a single teammate.
Reflection