Conversation
…d new.html.erb and edit.html.erb files
CheezItMan
left a comment
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 | ✔️, but no tests for uniqueness of VIN or phone number!!! |
| 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 mostly works |
| The app uses/is compatible with database seeds | ✔️ |
Router code is clean: uses resources and RESTful routes |
✔️, you have some unused routes defined in your routes.rb see my inline comment. Next project this will be a |
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 | ✔️, yes, but I can give a driver a 100 rating if I want... 😎 |
Overall Feedback
| Overall Feedback | Criteria | yes/no |
|---|---|---|
| Yellow (Approaches Standards) | 5+ in Code Review && 4+ in Functional Requirements, 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? |
|---|---|
| Descriptive/Readable | ✅ |
| Logical/Organized | ✅ |
Summary
It's clear you struggled with the project, most notably on testing, but you also didn't display validation errors. I'd like you to make a priority on MediaRanker to do adequate testing and make sure you have that skill down. It will really pay off on bEtsy.
On the other hand you have the basic features working and that's a lot. Take a look at my comments and let me know what questions you have here. I'm happy to walk you through some areas in Zoom.
|
|
||
| describe "create" do | ||
| # Your tests go here | ||
| it "can create a new driver with valid information accurately, and redirect" do #PASSING |
| end | ||
|
|
||
| def new | ||
| @passengers = Passenger.new |
There was a problem hiding this comment.
Just a note, this should be singular.
| def create | ||
| @passengers = Passenger.new(passengers_params) | ||
| if @passengers.save # save returns true if the database insert succeeds | ||
| redirect_to passengers_path(@passengers.id) # go to the index so we can see the task in the list |
There was a problem hiding this comment.
This is why your test is failing
| redirect_to passengers_path(@passengers.id) # go to the index so we can see the task in the list | |
| redirect_to passenger_path(@passengers.id) # go to the index so we can see the task in the list |
| redirect_to passengers_path(@passengers.id) # go to the index so we can see the task in the list | ||
| return | ||
| else # save failed :( | ||
| render :new # show the new task form view again |
There was a problem hiding this comment.
You should also include a status code
| render :new # show the new task form view again | |
| render :new, status: :bad_request # show the new task form view again |
| redirect_to passenger_path(@passengers.id) | ||
| return | ||
| else | ||
| render :edit |
There was a problem hiding this comment.
| render :edit | |
| render :edit, status: :bad_request |
| Rails.application.routes.draw do | ||
| # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html | ||
| # root to 'homepages#index' | ||
| get '/homepages', to: 'homepages#index', as: 'homepages' |
There was a problem hiding this comment.
Just so the homepage goes to the homepages controller
| get '/homepages', to: 'homepages#index', as: 'homepages' | |
| root to: 'homepages#index' |
| resources :passengers do | ||
| resources :trips, only: [:create] | ||
| end | ||
| resources :trips |
There was a problem hiding this comment.
Do you need all the RESTful routes for trips? I don't think you do.
| <ul> | ||
| <% @drivers.trips.each do |trip| %> | ||
| <li><strong><%= link_to trip.passenger.name, passenger_path(trip.passenger.id) %></strong> : Trip ID: <%=trip.id %> | Trip Rating : <%=trip.rating %> | Passenger ID: <%=trip.passenger.id%> | Cost: <%=cents_dollars(trip.cost) %> | <%= link_to "Delete Trip", trip_path(trip.id), method: :delete, data: {confirm: "Are you sure?"} %> |</li> | ||
| <% end %> | ||
| </ul> |
There was a problem hiding this comment.
Notice that you do this a lot. You can make this a partial and use it on all the show pages or anyplace you need a list of trips.
You can also check the documentation of the rails api
| @@ -0,0 +1,19 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
I suggest taking advantage of this page a lot more. You can put the common sitewide header and navigation links here and they'll show up on each page. Very handy.
| @@ -0,0 +1,13 @@ | |||
| <%= form_with model: @passengers, class: 'edit-passenger' do |f| %> | |||
There was a problem hiding this comment.
Above this form would be a good place to put validation notifications.
3 participants
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