Skip to content
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

Dikla & Wenjie - Rideshare Rails - Octos #27

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

wf1101
Copy link

@wf1101 wf1101 commented Apr 7, 2018

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 A driver can have many trips. A passenger can have many trips. A trip has only one passenger and one driver.
Describe the role of model validations in your application Passenger model will validate name and phone number's presence. Driver model will validate name and vin's presence, it also validates vin's uniqueness.
How did your team break up the work to be done? We worked on one computer at a time for the whole project.
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? We added a review section for a trip, which can be accessed by a passenger. Editing a trip is not available for driver. A passenger cannot request a trip until they have rated all their trips. To meet the deadline, we decided not to style listing 10 items in a page, and assign any driver to a new requested trip.
What was one thing that your team collectively gained more clarity on after completing this assignment? Rails in general. Especially the routes.
What is your Trello URL? https://trello.com/b/4J58zX2u/rideshare-rails
What is the Heroku URL of your deployed application? https://adubera.herokuapp.com/
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 are very grateful to work with each other. For the whole project, we worked together on one computer at all time and came to agreements on everything in our project.

wf1101 and others added 30 commits April 2, 2018 15:44
Drew ERD diagram
Created trello board
Added RESTFUL routes
Added new column in trip to have foreign keys
Fixed passenger and driver id in trips data
Added trips info to passenger's show page
Added links to trip, driver and passenger
Added a link to add a review in passenger's show page
Added business logic to calculate ave rating and total cost for driver
and passenger
…heir trips

Added condition to passenger's show page to access the link request a trip

<%= render partial: "layouts/errors_messages", locals: {model: @driver } %>

<div class="new_page">

Choose a reason for hiding this comment

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

To follow CSS naming conventions, we separate classes with the - not the _ so instead, this wold be new-page

<ul class="new_info">
<li>
<%= f.label :car_make %>
<%= f.select "car_make",raw("<option>Toyota</option><option>Honda</option><option>Tesla</option><option>Lamborghini</option><option>Ford</option><option>Hyundai</option><option>Subaru</option><option>Volkswagen</option><option>BMW</option><option>Mercedes</option><option>Aston Martin</option><option>Ferrari</option><option>Jaguar</option>") %>

Choose a reason for hiding this comment

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

It might make sense to move these options into the model or somewhere other than the view (assuming they'd be accessed/utilized elsewhere)

end

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

Choose a reason for hiding this comment

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

You could probably clean this up by using except rather than only to list fewer actions

@@ -0,0 +1,13 @@
Rails.application.routes.draw do
# For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html
root 'trips#new'

Choose a reason for hiding this comment

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

This seems like an odd choice for the default home page based on what this route normally is responsible for.

</strong>

<% @passengers.each do |passenger| %>
<ul class="index_list">

Choose a reason for hiding this comment

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

Watch your indentation in the views - when you utilize the code tags and HTML within, you still want to indent to make sure that it is as readable as possible.

@kariabancroft
Copy link

Rideshare-Rails

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with no extraneous files checked in and both partners contributing There is definitely one-sided git contributions, but I see the note about working together throughout so this appropriately reflects that.
Answered comprehension questions Yes
Uses named routes (like _path) Yes
RESTful routes utilized Yes
Rideshare Rails Specific Content
Table relationships Yes - relationships look good in the schema and the model files
Validation rules for Models Yes - using presence and uniqueness validations
Business logic is in the models Yes - using for totals & rating
Database is seeded from the CSV files Yes - data on Heroku looks good
Trello board is created and utilized in project management Yes - I can definitely see how you progressed through tasks
Postgres database is used Yes
Heroku instance is online Yes
The app is styled to create an attractive user interface Yes
Overall You did a nice job exploring the major learning goals of this assignment. Check out a few more specific comments on the pull request.

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