-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Leah and Stacy's Ride-Share-Rails! #23
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | ✔️ Yes, and tests weren't required for this project. |
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 | There are 15 tests with errors, and 3 failures. I've made inline comments on a few of these. I'd be happy to go through more of them with you. |
The app has an attractive and usable user interface | ✔️ Love the flipping and colors! |
The app uses/is compatible with database seeds | ✔️ Some trips fail to save. |
Router code is clean: uses resources and RESTful routes |
There is an error with the custom availability route. |
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 | The show action fails for a passenger that has no trip: undefined method to_f' for []:Array -> | <%= number_to_currency(@passenger.total_charged.to_f / 100) %> |
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 | The links to the trips on the passenger show page do not work because they are missing the trip_id from the path | |
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 | ✔️ If I uncheck the available box when editing, there's a validation error that available can't be blank | |
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
Good work overall on this project. You implemented complex model relationships, excellent styles, and the request_trip feature works beautifully. There are a few bugs that lead to broken views and unexpected behavior that I tried to highlight in the rubric and through code comments. You also have multiple tests with errors. I encourage you to take some time to read through these errors and make corrections. Please let me know if you would like to look at it together.
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 | ✅ |
Descriptive/Readable | ✅ |
it "can access the availability path" do | ||
new_driver.save | ||
# Act | ||
patch availability_path(new_driver.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line produces an error.
The route you define is:
patch 'drivers/availability', to: 'drivers#availability', as: 'availability'
I think you meant to include the driver.id as a param:
patch 'drivers/:driver_id/availability', to: 'drivers#availability', as: 'availability'
test_0001_can access the availability path ERROR (0.00s)
Minitest::UnexpectedError: NoMethodError: undefined method `availability_path' for #<Minitest::Rails::SpecTests::Test__Driver_custom_methods_can_go_on_off_line__test_models_driver_test_rb__71:0x00007f92c0c88688>
test/models/driver_test.rb:76:in `block (4 levels) in <main>'
end | ||
|
||
describe "create" do | ||
# Your tests go here | ||
it "can create a new trip" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails. Consider the trip create
method. To create a trip you need the :passenger_id
from the params and a driver
. You make use of the nested route that can be viewed by typing rails routes
👍
passenger_trips POST /passengers/:passenger_id/trips(.:format) trips#create
Consider how to write the test given this information and let me know if you have questions.
TripsController::create
test_0001_can create a new trip FAIL (0.01s)
1.
"Trip.count" didn't change
test/controllers/trips_controller_test.rb:40:in `block (3 levels) in <main>'
@passenger = Passenger.find_by(id: passenger_id) | ||
|
||
if @passenger.nil? | ||
redirect_to passenger_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line produces an error. Remember passenger_path
needs an id
|
||
describe "can go offline" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are tests of controller actions.
resources :passengers do | ||
resources :trips, only: [:create] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of limiting nested routes.
config/routes.rb
Outdated
resources :rideshare, only: [:index] | ||
|
||
resources :drivers | ||
patch 'drivers/availability', to: 'drivers#availability', as: 'availability' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing :driver_id
from this route (I think). See comment on test.
app/views/passengers/show.html.erb
Outdated
<tr> | ||
<td>Passenger's Trips:</td> | ||
<td><% @passenger.trips.each do |trip| %> | ||
<li>Trip: <%= link_to trip.date, trip_path %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trip_path needs a trip
or trip_id
<li>Trip: <%= link_to trip.date, trip_path %> | |
<li>Trip: <%= link_to trip.date, trip_path(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