Mud - Christabel (Water) & Lina (Earth) #16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… controller actions
…pt for validations
…ssengers, render :new for failed .save, added validation requirements, (+pasted Driver test code..unedited)
…or invalid form submissions
…o public/404.html
….vin, added driver#average_rating test
…make_available and driver#make_unavailable
…enger nested route
…iver show views -- implemented nested routing instead
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 | ✔️ Styling! |
| 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
Excellent job overall. This code is well-written and well-tested. It is clear that the learning goals around model relationships, validations, and seeding were met. The site has great styles and intuitive user experience. I've left an inline comment about how you could move logic that's in the view for changing the status of a driver into a custom controller method. Please let me know if you have any questions. Keep up the hard work!
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Green (Meets/Exceeds Standards) | 8+ in Code Review && 5+ in Functional Requirements | ✔️ |
| Yellow (Approaches Standards) | 5+ in Code Review && 4+ in Functional Requirements, or the instructor judges that this project needs special attention | |
| Red (Not at Standard) | 0-4 in Code Review or 0-3 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging, or the instructor judges that this project needs special attention |
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 | ✅ |
Comment on lines
+18
to
+22
| <% if @driver.availability_status == true %> | ||
| <%= button_to "Change Availability", driver_path(@driver.id), method: :patch, params: {driver:{name: @driver.name, vin: @driver.vin, availability_status: false}}, class: "btn btn-secondary btn-sm" %> | ||
| <% else %> | ||
| <%= button_to "Change Availability", driver_path(@driver.id), method: :patch, params: {driver:{name: @driver.name, vin: @driver.vin, availability_status: true}}, class: "btn btn-secondary btn-sm" %> | ||
| <% end %> |
There was a problem hiding this comment.
Consider refactoring using a variable and ternary to DRY up your code:
Suggested change
| <% if @driver.availability_status == true %> | |
| <%= button_to "Change Availability", driver_path(@driver.id), method: :patch, params: {driver:{name: @driver.name, vin: @driver.vin, availability_status: false}}, class: "btn btn-secondary btn-sm" %> | |
| <% else %> | |
| <%= button_to "Change Availability", driver_path(@driver.id), method: :patch, params: {driver:{name: @driver.name, vin: @driver.vin, availability_status: true}}, class: "btn btn-secondary btn-sm" %> | |
| <% end %> | |
| <% status = @driver.availability_status == true ? false : true %> | |
| <%= button_to "Change Availability", driver_path(@driver.id), method: :patch, params: {driver:{name: @driver.name, vin: @driver.vin, availability_status: status}}, class: "btn btn-secondary btn-sm" %> |
There was a problem hiding this comment.
On second thought, this is a lot of work for a view. Consider making a custom controller action that is mark_available or mark_unavailable and uses your custom model methods.
Comment on lines
+55
to
+59
| <% if trip.passenger %> | ||
| <%= link_to trip.passenger.name, passenger_path(trip.passenger.id) %> | ||
| <% else %> | ||
| <p><em>Not Available</em></p> | ||
| <% end %> |
There was a problem hiding this comment.
Consider refactoring with a ternary.
| expect(total_earnings).must_be_close_to expected_earnings, 0.01 | ||
| end | ||
|
|
||
| it "returns 0 for a driver with no trips" do |
|
|
||
| def create | ||
| if params[:passenger_id].nil? | ||
| @trip = Trip.new(trip_params) |
There was a problem hiding this comment.
It is not clear to me how you would ever receive trip params as there is not form the create a trip.
| @trip = Trip.new(trip_params) | ||
| else | ||
| passenger = Passenger.find_by_id(params[:passenger_id]) | ||
| @trip = passenger.request_trip |
There was a problem hiding this comment.
Nice use of a custom model method
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
has_manyTrips, Driverhas_manyTrips, Tripsbelongs_toDriver and Passenger. We set them up this way because a single trip cannot have multiple drivers and per our database, all trips are associated with a Passenger's unique ID, even if the passengers are in the same car, it would be recorded as a unique trip (different costs per passenger, different rating, etc).