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

Ports - Jessica & Kasey #9

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

Ports - Jessica & Kasey #9

wants to merge 44 commits into from

Conversation

kaseea
Copy link

@kaseea kaseea 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 Trip belongs to Passenger and Trip belongs to Driver. Each Driver has many trips and each Passenger has many Trips. We did it this way to we can access Trips by Passenger or Driver and Passenger or Driver by Trip.
Describe the role of model validations in your application We required Drivers to have names and vin and Passengers to have names and phone numbers so their information can be complete and professional.
How did your team break up the work to be done? We did a lot of the project together, and split up the tasks that would be the repetitious parts, being sure each person had a chance to experience coding an aspect at least once, like forms, we each made at least one form, but someone made way more forms than the other.
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 prioritized all the baseline functions and required features and left all stylizing/CSS til the end.
What was one thing that your team collectively gained more clarity on after completing this assignment? Restful Routes.
What is your Trello board URL? https://trello.com/b/UJBuBMqZ/ride-share-rails
What is the Heroku URL of your deployed application? young-springs-28660.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? That we both appreciated the others willingness to work together, so we both have a better understanding of the whole project and code base and how everything works together.

kaseea and others added 30 commits April 15, 2019 15:54
…avigation to get to all of them from every page
…stead of two identical ones but I don't know how to references it from two different folders)
@CheezItMan
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 Good number of commits and both partners contributing. Good commit messages & no extra files committed.
Answered comprehension questions Check
Uses named routes (like _path) Check
RESTful routes utilized Check, some routes missing and some routes are unnecessarily nested. See my inline comments.
Project Requirements
Table relationships Check
Business logic is in the models Check, but see my inline notes. You should make use of enumerable methods.
Controller tests Check
Database is seeded from the CSV files Check
Trello board is created and utilized in project management Check
Heroku instance is online Check
The app is styled to create an attractive user interface Check
Overall You hit the majority of learning goals. One thing I noticed is that a passenger can still request a ride, even if they have an incomplete trip. Also drivers cannot go online or offline. You did however have the core requirements down. Nice work!

must_redirect_to drivers_path
end

it "deletes trips associated with driver (but not passenger)" do

Choose a reason for hiding this comment

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

👍

<p><%= "Form for editing a trip" %></p>

<%= f.label :date %>
<%= f.datetime_field :date %>

Choose a reason for hiding this comment

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

Just a note, if I use the field to pick a date, I get a front-end validation error.

resources :drivers

resources :passengers do
resources :trips

Choose a reason for hiding this comment

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

You should limit this to only the routes you need, like:

resources :passengers do
  resources :trips, only [:create]
end

resources :trips, only [:update, :edit]

resources :passengers do
resources :trips
end

Choose a reason for hiding this comment

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

Missing routes to put a driver online/offline and to mark a trip complete.

@@ -0,0 +1,29 @@
<h2>trips:</h2>

Choose a reason for hiding this comment

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

No output of validation errors are here

def total_charged
return 0 if trips.length == 0
sum = 0
trips.each do |trip|

Choose a reason for hiding this comment

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

You can also use the enumerable method .sum

class Trip < ApplicationRecord
belongs_to :passenger
belongs_to :driver

Choose a reason for hiding this comment

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

You don't need to write validations for the foreign key relationships. Rails does that by default.

You could also add validations to limit the values that the rating could be, and to limit the cost to be greater than 0.

has_many :trips
validates :name, presence: true
validates :phone_num, presence: true

Choose a reason for hiding this comment

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

I suggest you consider making a request_trip business logic method in the Passenger model here, then you could call it from the controller, and dry up that method

must_respond_with :redirect
end

it "should respond with a 422 if it doesn't crete" do

Choose a reason for hiding this comment

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

Spelling

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