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

Caroline Nardi and Kat Eastman - RideshareRails - Octos #19

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

Conversation

kseastman
Copy link

@kseastman kseastman 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 We have a driver, a passenger and a trip. A driver has many trips, a passenger has many trips and a trip belongs to only one passenger and one driver. We did not need a join table because it was a one to many relationship from a trip, so a new trip contains a passenger_id and a driver_id.
Describe the role of model validations in your application We required a trip to have a passenger and a driver, a driver to have a name and a vin, and a passenger to have a name and a phone number. A trip rating could be set to nil, or between 1 and 5. We then displayed error messages to the end user if an instance could not be saved to the database and raised a validation error.
How did your team break up the work to be done? We did the setup together, and the project planning. We pulled the user stories and identified associated tasks, then set general due dates for task completion and used Trello to assign them. We initially split up tasks, but ended up working together on specific problems.
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 the required functionality, we decided to not have a driver status that could be manipulated from the website (since it was not a requirement), and did not include profile pictures for drivers or passengers. We didn't exhaustively identify all possible edge and error cases, and neither of us had any happiness or joy in what it looked like so it is very basic.
What was one thing that your team collectively gained more clarity on after completing this assignment? Objects passed to the view, and rendering of partials.
What is your Trello URL? kanban and backlog
What is the Heroku URL of your deployed application? cmnkse-rideshare
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? Neither of us feels fully comfortable pairing yet, but we made an effort to fully pair for some of the challenging partial rendering passing objects and collections to the view. I think the project went really well, even if we didn't do anything beyond the scope of the requirements.

kseastman and others added 30 commits April 2, 2018 15:35
Because we are syncing our changes

trips_with_cost = self.trips.select do |trip|
trip.cost != nil
end

Choose a reason for hiding this comment

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

Could you reuse the completed_trips method here?


def average_rating
return "---" if completed_trips.empty?

Choose a reason for hiding this comment

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

Good work covering this edge case. Generally, you should return nil from a model method if there's no reasonable value, and your views should handle turning this into something like ---.

def find_driver
drivers = Driver.all.select { |driver| driver.available? }
available_driver = drivers.first

Choose a reason for hiding this comment

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

I like that you've made this a model method, but I'm not sure an instance method on Trip makes sense. I would probably do a self method, possibly on Driver.

@@ -0,0 +1,6 @@
<section class="r-cell header">ID</section>
<section class="r-cell header">Date</section>
<section class="r-cell c2-fr header">Passenger</section>

Choose a reason for hiding this comment

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

I like that you've broken the logic to display a table of trips out into a partial - good job keeping things DRY. I have two questions:

  1. Why are the headers and table rows in separate files? It would be easier to read / understand if the whole table was combined into one partial.
  2. Why not use a <table> element? Using that would both be more semantic, since you're displaying a table, and possibly save you some styling work.


<ul>
<%= paginate @passengers %>
</ul>

Choose a reason for hiding this comment

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

I like that you use a gem to handle pagination - no reason to reinvent the wheel when you can borrow someone else's well-thought out, well-tested version.

@droberts-sea
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 yes
Answered comprehension questions yes
Uses named routes (like _path) yes
RESTful routes utilized yes
Rideshare Rails Specific Content
Table relationships yes
Validation rules for Models yes
Business logic is in the models yes - great work
Database is seeded from the CSV files yes
Trello board is created and utilized in project management yes
Postgres database is used yes
Heroku instance is online yes
The app is styled to create an attractive user interface
Overall Great work overall! I've left a few nitpicks below, but in general I am quite happy with this submission. Keep up the hard work.

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