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

Ampers Leti and Steffany #13

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

Conversation

steffnay
Copy link

@steffnay steffnay commented Apr 6, 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
Our ERD: erd
Describe the types of entity relationships you set up in your project and why you set up the relationships that way: We used the Trips class as a connector. Driver has many trips, while Trip has one Driver. Passenger has many trips, while Trip has one Passenger.
Describe the role of model validations in your application Our model validations make sure that each Driver, Passenger, and Trip has the minimum required attributes when it is created or updated.
How did your team break up the work to be done? We used Trello and tried to make sure the split was even. After Leti was ill, we communicated about which parts we each could finish up.
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 set aside stylesheets. Having the ability to request a new trip and rate a trip came toward the end.
What was one thing that your team collectively gained more clarity on after completing this assignment? We both learned more about migrations, seeding, git branching.
What is your Trello URL? Link
What is the Heroku URL of your deployed application? Link
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? Not getting sick would be awesome. We had strong communication, especially when dividing up work at the beginning and planning the app. We were able to work together really quickly at the beginning, which set Steffany up to be able to keep working even though Leti was sick. That was a major plus to the project.

@CheezItMan
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 Check, good commit messages and both teammates contributed.
Answered comprehension questions Check, it was too bad about the illness.
Uses named routes (like _path) Check
RESTful routes utilized Check
Rideshare Rails Specific Content
Table relationships Check, it's interesting that you didn't enforce the foreign keys so a Driver for a trip can be deleted. A clever way to work around the issue.
Validation rules for Models Your PR didn't have validation, but your home repository on the master branch does.
Business logic is in the models I can't put comments in your code as it's not in the PR, but you're doing WAAAAAAY too much business logic in the controllers. You need to have methods like next_available_driver and total_earnings in the Model classes.
Database is seeded from the CSV files Check
Trello board is created and utilized in project management NOTE I can't access the Trello board.
Postgres database is used Check
Heroku instance is online Check
The app is styled to create an attractive user interface Check, nicely done here. It's not responsive, but that wasn't required.
Overall You did hit most of the requirements, which is very impressive given your illness related issues. The biggest thing to work on is putting business logic in the models. Keep your controller code minimal.

NOTE:: You did a PR from Leti's branch LetiTran:working-on-passenger, not from master. Do you need to redo the PR?

@@ -0,0 +1,2 @@
<h1>Trips#show</h1>

Choose a reason for hiding this comment

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

In the future deleting unused files like these, is preferred.

@@ -0,0 +1,3 @@
class Driver < ApplicationRecord
has_many :trips

Choose a reason for hiding this comment

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

No validations?

@@ -0,0 +1,3 @@
class Passenger < ApplicationRecord
has_many :trips

Choose a reason for hiding this comment

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

No validations?

<% @drivers.each do |driver| %>

<ul >
<li> <img src="https://via.placeholder.com/50x50" /> <%= link_to driver.name, show_driver_path(driver[:id]) %> <%= link_to "Edit", edit_driver_path(driver[:id]) %> <%= link_to "Delete", delete_driver_path(driver[:id]), method: :delete %></li>

Choose a reason for hiding this comment

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

It's human nature to want to click on the driver's image as well as the name, so making that a link would be nice.

@rating == 0 ? "Driver has no trips" : @rating = @rating/number_of_ratings

# Calculate total earnings:
@total_earnings = 0

Choose a reason for hiding this comment

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

All this logic should be done in the model.

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