Earth - Maha & Olga #30
Open
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
…ller. Added a new view file to passegner that contains a form to create a new task.
…ler and corresponding views
…r_controller and their view files.
… is more accurate
Added tests for trip controller, all except destroy
…isplay if trip has no cost, added bad_request to update action in the trip controller
…per method to instanciate trip, driver and passenger for tests
CheezItMan
reviewed
Nov 12, 2020
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 | ✔️, basic, but it works |
| The app uses/is compatible with database seeds | ✔️ |
Router code is clean: uses resources and RESTful routes |
✔️, but see my one comment |
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 | ✔️, you see errors on the creation of a passenger, but not updating a passenger. |
| 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 | ✔️, you don't see validations with the edit action. |
| 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 | ✅ |
| Concise | ✅ |
| Logical/Organized | ✅ |
Summary
This is pretty well done, you hit the main learning goals. You also had some pretty good tests. I had a few suggestions on displaying validation errors, and using enumerables in model methods.
| redirect_to driver_path @driver.id | ||
| return | ||
| else # if save is failed | ||
| render :new, status: :bad_request #shows new book form again |
There was a problem hiding this comment.
Book?
Suggested change
| render :new, status: :bad_request #shows new book form again | |
| render :new, status: :bad_request |
| redirect_to passenger_path # go to the passenger details page | ||
| return | ||
| else # save failed | ||
| render :edit # show the new passenger form view again |
There was a problem hiding this comment.
Suggested change
| render :edit # show the new passenger form view again | |
| render :edit, status: :bad_request # show the new passenger form view again |
Comment on lines
+8
to
+16
| sum = 0 | ||
| trips.each do |trip| | ||
| if trip.cost < 0 | ||
| return 0 | ||
| else | ||
| sum += (trip.cost - 165) * 0.8 | ||
| end | ||
| end | ||
| return sum.round(2) |
There was a problem hiding this comment.
Just a suggestion to use an enumerable.
Suggested change
| sum = 0 | |
| trips.each do |trip| | |
| if trip.cost < 0 | |
| return 0 | |
| else | |
| sum += (trip.cost - 165) * 0.8 | |
| end | |
| end | |
| return sum.round(2) | |
| sum = trips.sum do |trip| | |
| trip.cost < 0 ? 0 : (trip.cost - 165) * 0.8 | |
| end | |
| return sum.round(2) |
| @@ -0,0 +1,13 @@ | |||
| <h2> New driver page</h2> | |||
|
|
|||
| <% if @driver.errors.any? %> | |||
| @@ -0,0 +1,28 @@ | |||
| <!DOCTYPE html> | |||
Comment on lines
+103
to
+109
| # describe "can go online" do | ||
| # # Your code here | ||
| # end | ||
| # | ||
| # describe "can go offline" do | ||
| # # Your code here | ||
| # end |
There was a problem hiding this comment.
So no test or method to go online/offline.
| describe "relationships" do | ||
| # Your tests go here | ||
| end | ||
| it "can have one passenger:many to 1" do |
There was a problem hiding this comment.
Suggested change
| it "can have one passenger:many to 1" do | |
| it "belongs to a Passenger" do |
| # Tests for methods you create should go here | ||
| describe "custom methods" do | ||
| # Your tests here | ||
| it "must have a cost" do |
There was a problem hiding this comment.
You also need to test the numericality.
I would also test that a trip is required to have a passenger and a driver.
| resources :passengers do | ||
| resources :trips, only: [:index, :create] | ||
| end | ||
| resources :trips |
There was a problem hiding this comment.
You don't need all the RESTful routes for trips here since you are doing index and create nested under passengers.
| <%= link_to 'Edit', edit_driver_path(@driver) %> | ||
| <%= link_to 'Delete', @driver, method: :delete, data: { confirm: 'Are you sure?' } %> | ||
|
|
||
| <table id='trips-table' class="table table-hover"> |
There was a problem hiding this comment.
You can convert this content to a partial. Then you can display the same table anywhere you list trips.
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