Skip to content

Conversation

@RPerry
Copy link

@RPerry RPerry commented Apr 19, 2019

Rideshare-Rails

Congratulations! You're submitting your assignment! These comprehension questions should be answered by both partners together, not by a single teammate.

Comprehension Questions

Question Answer
Describe the types of entity relationships you set up in your project and why you set up the relationships that way Trips belong to passenger and driver, drivers have many trips, passengers have many trips.
Describe the role of model validations in your application It allows us to require certain parameters for an instance to be created. Combined with strong params, this gives our app a measure of security.
How did your team break up the work to be done? Pulled tickets from our trello board and coordinated over slack.
What features did you choose to prioritize in your project, and what features, if any, did you have to set aside to meet the deadline? Prioritized getting the database correctly set up and made sure to fulfill the user stories before starting on design. Due to this, we had to set aside many of our design goals because of time constraints.
What was one thing that your team collectively gained more clarity on after completing this assignment? Nested routes
What is your Trello board URL? https://trello.com/b/lBhnjLjK/rideshare-rails
What is the Heroku URL of your deployed application? https://priyo-rideshare.herokuapp.com/passengers
What are two discussion points that you and your pair discussed when giving/receiving feedback from each other that you would be willing to share? We discussed that establishing our preferred methods of communication, directness about what the other would like us to do and what is expected of us. We thought the project went well in terms of what was accomplished

RPerry and others added 28 commits April 18, 2019 18:21
… can rate their previous trip regardless of whether there are available drivers
@kaidamasaki
Copy link

Rideshare Rails

What We're Looking For

Feature Feedback
Baseline
Appropriate git usage with no extraneous files checked in and all team members contributing 👍🏽
Answered comprehension questions 👍🏽
Uses named routes (like _path) 👍🏽
RESTful routes utilized Mostly. See inline comments.
Project Requirements
Table relationships 👍🏽
Business logic is in the models 👍🏽
Controller tests 👍🏽
Database is seeded from the CSV files 👍🏽
Trello board is created and utilized in project management 👍🏽
Heroku instance is online 👍🏽
The app is styled to create an attractive user interface 👍🏽
Overall While your app overall was very strong it looks like you were unable to resist the siren call of the last minute code change.

Remember, when you make a change always run your tests and fix failures, even if you are in a hurry. The instinct to clean up your code was good, but you do need to be extra careful when you're approaching a deadline.

It also looked like you had some trouble with nested routes. Generally when you have a one-to-many relationship it's worth at least considering whether or not you should nest routes there. You were really close here, it's just something to pay a little extra attention to in the future.

Aside from the issues above everything looked great though, good job!

// <h1>Welcome to the Trip List</h1>

// <ul>
// <% @trips.each do |driver| %>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what happened here. It looks like maybe this file was supposed to be commented out or just not checked in at all.

This seems like it was definitely supposed to look like this though:

Suggested change
// <% @trips.each do |driver| %>
// <% @trips.each do |trip| %>

Also the fact that trip doesn't exist causes errors with the rest of the view.


post '/drivers/:id/available', to: 'drivers#available', as: "driver_availability"
# post '/drivers/:id/destroy', to: 'drivers#destroy', as: "destroy_driver"
# post '/passengers/:id/destroy', to: 'passengers#destroy', as: "destroy_passenger"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these were removed at the last minute but were still referenced by tests.

Total Fare Paid: $<%= @passenger.total_fare %>
</p>

<% if @passenger.trips.last.rating == nil %>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes a test (works for a passenger that exists) to fail. (You will get an undefined method `rating' for nil:NilClass when there are no trips for the passenger.)

It also causes the same error when trying to show a newly created Passenger.

You need to make sure that @passenger.trips.last isn't nil before calling rating:

Suggested change
<% if @passenger.trips.last.rating == nil %>
<% if @passenger.trips.last && @passenger.trips.last.rating == nil %>

return
end

if driver.available == true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: you can leave off the == true here in Ruby it's implied.


if driver.available == true
driver.available = !driver.available
elsif driver.available == false || driver.available == nil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: since false and nil are the only "falsey" objects in Ruby this could simply be:

Suggested change
elsif driver.available == false || driver.available == nil
elsif !driver.available


trip.destroy

redirect_to trips_path

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should redirect to the appropriate Passenger or Driver. See notes on routes.rb.

@@ -0,0 +1,26 @@
<h1>Welcome to the Driver List</h1>

<ul>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: a <table> can make displaying this sort of information easier.

@@ -0,0 +1,5 @@
class AddDriverIdColumnToTrips < ActiveRecord::Migration[5.2]
def change
add_reference :trips, :driver, foreign_key: true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip: you are allowed to run multiple commands in a single change method.

root 'homepages#index'

resources :drivers
resources :trips

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trips should have been nested under :drivers as well.

resources :trips

resources :passengers do
resources :trips, only: [:index, :new, :create]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trips#new shouldn't live here since the passenger isn't treated specially when you make a new trip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants