Conversation
…ages#index as root path
…rk on partial views
Merge branch 'master' of https://github.com/roshni-patel/ride-share-rails into master
…lable post a trip
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 | ✔️ |
| There are reasonable tests to test the validation requirements for all models | ✔️, yes, but you 're missing a test for the numericality of rating. |
| 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 more table-like styling woudl be an improvement. Also and more importantly some of the text is hard to read. Lastly the trips index page is just a list of ids. |
| The app uses/is compatible with database seeds | ✔️ |
Router code is clean: uses resources and RESTful routes |
✔️ |
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 | ✔️, yes but expenses is not displayed as money. Further the list of trips is just a list of id numbers... you should show more information. |
| 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 | ✅ |
| Descriptive/Readable | ✅ |
| Logical/Organized | ✅ |
Summary
Overall nice work, y'all hit the learning goals well. I have some criticisms and you can find them inline in the code review. The biggest things revolve around giving the user feedback when they do something not allowed.
| validates :name, presence: true | ||
| validates :phone_num, presence: true | ||
|
|
||
| def net_expenditures |
| def self.request_trip(passenger) | ||
| available_drivers = Driver.where(:available => true) | ||
|
|
||
| raise ArgumentError, 'No drivers available' if available_drivers.empty? |
| available_driver.update({available: false}) | ||
| redirect_to trip_path(@trip.id) | ||
| else | ||
| redirect_to trips_path |
There was a problem hiding this comment.
You should do something to handle if there is a error creating the trip.
|
|
||
| # Assert | ||
| # Check that the controller redirects | ||
| must_respond_with :redirect |
There was a problem hiding this comment.
Again I would suggest re-rendering the new passenger form and displaying validation errors.
| patch passenger_path(id), params: new_passenger_hash | ||
| end.wont_change 'Passenger.count' | ||
|
|
||
| must_respond_with :not_found |
| patch passenger_path(id), params: new_passenger_hash | ||
| end.wont_change 'Passenger.count' | ||
|
|
||
| must_redirect_to passenger_path |
There was a problem hiding this comment.
Like new I suggest re-rendering :edit and displaying validation errors.
| @@ -61,21 +61,33 @@ | |||
| # Tests for methods you create should go here | |||
| describe "custom methods" do | |||
There was a problem hiding this comment.
No validation or relationship tests?
| end | ||
| end | ||
|
|
||
| describe "validations" do |
Co-authored-by: Chris M <chris@adadevelopersacademy.org>
Co-authored-by: Chris M <chris@adadevelopersacademy.org>
Co-authored-by: Chris M <chris@adadevelopersacademy.org>
Co-authored-by: Chris M <chris@adadevelopersacademy.org>
Co-authored-by: Chris M <chris@adadevelopersacademy.org>
Co-authored-by: Chris M <chris@adadevelopersacademy.org>
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