Earth - Beauttie & Water - Mackenzie#20
Conversation
… controllers for destroy to address .nil
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 | ✔️ |
| 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 | ✔️ |
| 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 | ✔️ |
| 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
Nice work, y'all hit the learning goals. Well done. I left comments on several minor issues throughout the app. Please look at them and let me know what questions you have.
|
|
||
| /public/assets | ||
| .byebug_history | ||
|
|
There was a problem hiding this comment.
I suggest you add .idea to the .gitignore file.
| .idea |
| redirect_to drivers_path | ||
| return | ||
| else | ||
| redirect_to driver_path(@driver.id) |
There was a problem hiding this comment.
So if the driver has trips you can't delete them?
| elsif @passenger.trips.empty? | ||
| @passenger.destroy | ||
| redirect_to passengers_path | ||
| return | ||
| else | ||
| redirect_to passenger_path(@passenger.id) | ||
| return | ||
| end |
There was a problem hiding this comment.
So if the passenger has trips, they can't be destroyed.
| @@ -0,0 +1,19 @@ | |||
| class Driver < ApplicationRecord | |||
| <% if @driver.errors.any? %> | ||
| <ul class="errors"> | ||
| <% @driver.errors.each do |column, message| %> | ||
| <li><strong><%= column %></strong> <%= message %></li> | ||
| <% end %> | ||
| </ul> | ||
| <% end %> |
| <ol> | ||
| <% @driver.trips.each do |trip| %> | ||
| <li> ID: <%= link_to trip.id, trip_path(trip) %> -- | ||
| Driver: <%= trip.driver.name %> -- | ||
| Passenger: <%= trip.passenger.name %> -- | ||
| Cost: <%= number_to_currency(trip.cost/100) %> -- | ||
| Rating: <%= trip.rating %> | ||
| </li> | ||
| <% end %> | ||
| </ol> |
There was a problem hiding this comment.
Just noting this is a great candidate for a partial, anytime you need a list of trips, you can just call the partial.
| @@ -0,0 +1,13 @@ | |||
| <h2>New Passenger</h2> | |||
| root to: 'homepages#index' | ||
|
|
||
| resources :passengers do | ||
| resources :trips, only: [:index, :create] |
There was a problem hiding this comment.
You're not really using index as a nested route.
| belongs_to :passenger | ||
| belongs_to :driver | ||
|
|
||
| validates :rating, allow_nil: true, numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: 5 } |
|
|
||
| validates :name, presence: true | ||
| validates :phone_num, presence: true, uniqueness: true | ||
|
|
There was a problem hiding this comment.
I would also add a method to request_trip
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