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

India & Jing #11

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

India & Jing #11

wants to merge 51 commits into from

Conversation

Jing-321
Copy link

@Jing-321 Jing-321 commented Nov 6, 2020

Assignment Submission: Rideshare Rails

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions. These questions should be answered by all team members together, not by a single teammate.

Reflection

Prompt Response
Describe the types of entity relationships you set up in your project and why you set up the relationships that way drivers to trips is one to many, same as passengers to trips. We set them up this way because each driver and passenger can have one or more trips that they have taken but one trip can only have one driver and one passenger.
Describe the role of model validations in your application validates the input for creating a new driver, passenger, or trip.
How did your team break up the work to be done? India takes Driver controller&model, and erb & tests for them; Jing takes Passenger & Trip, and css.
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? Request trip for a passenger, the structure and styling of the website.
What was one thing that your team collectively gained more clarity on after completing this assignment? the biggest thing is learning how to have multiple controllers interact with each other
What are two discussion points that you and your team discussed when giving/receiving feedback from each other that you would be willing to share? we talked about how we like being more direct when giving and receiving feedback
Optional: What is the URL of your deployed Heroku app? https://rideshare-2020.herokuapp.com

Comment on lines +21 to +26
if params[:passenger_id]
@trip = Trip.request_trip(params[:passenger_id])
@trip.save
@passenger = Passenger.find(@trip.passenger_id)
@driver = Driver.find(@trip.driver_id)
return

Choose a reason for hiding this comment

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

I want to name a couple things here:

  1. Because this code is creating a new trip, it would be more appropriate for this code to live in create than new. The link in the passenger show page would need to be updated accordingly (and the nested route would need to be updated.)
  2. This code is not tested because there's no unit test for the new action the passes the passenger_id param.

I'm happy to talk in more detail about either or both of these things if they're unclear!

validates :cost, numericality: { only_integer: true, greater_than: 0 }


def self.request_trip(passenger_id)

Choose a reason for hiding this comment

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

I love that you made a controller method for this!

@jmaddox19
Copy link

Rideshare Rails

Major Learning Goals/Code Review

Criteria yes/no, and optionally any details/lines of code to reference
Practices collaborating with git, and all team members contribute git commits and best git practices ✔️ Lots of commits with meaningful messages! And looks like y'all even tried out using pull requests!
Demonstrates understanding of relationships by giving accurate answers to the reflection questions and appropriate code in the models ✔️
Logic to calculate a driver's average rating and total earnings is located in the model, and it has unit tests Logic is in the model but no unit tests
There are reasonable tests to test the validation requirements for all models ✔️
There are reasonable tests to test the relationship requirements for all models Glad to see you did write some relationship tests! But none for Trip model
There are reasonable tests to test the controller actions for all controllers ✔️ Great testing!
The app has an attractive and usable user interface ✔️ So pretty! Oh my gosh!
The app uses/is compatible with database seeds ✔️
Router code is clean: uses resources and RESTful routes ✔️

Functional Requirements

Functional Requirement yes/no
On the passenger's details page, I want to be able to see total charged, list of trips, a link to edit, and delete ✔️ All the links are there but when I click delete I get the following: `PG::ForeignKeyViolation: ERROR: update or delete on table "passengers" violates foreign key constraint "fk_rails_a3e4ffd914" on table "trips"
DETAIL: Key (id)=(1) is still referenced from table "trips".` This is consistent with the test output I see. ("PassengersController:destroy:will delete a passenger" is the one test that is failing.)
When adding a new passenger, I want to see errors and validations that show that a passenger must be provided a name and a phone number, so that I cannot make a passenger without name or phone number ✔️
On the passenger's details page, I can create a new trip for this passenger, with an assigned driver and no rating ✔️
On the driver's details page, I want to be able to see total earnings, average rating, list of trips, a link to edit, and delete ✔️ Mostly, but average rating is missing and when I try to delete a driver, I see the same error message I saw when trying to delete a passenger. In both cases, this will only happen if the driver/passenger has at least one trip. `PG::ForeignKeyViolation: ERROR: update or delete on table "passengers" violates foreign key constraint "fk_rails_a3e4ffd914" on table "trips"
DETAIL: Key (id)=(1) is still referenced from table "trips".`
When adding a new driver, I want to see errors and validations that show that a driver must be provided a name and VIN, so that I cannot make a driver without name or VIN No validation errors are displayed to the user. @indiakato I encourage you to make sure to practice displaying flash messages and validation errors to the user while completing Media Ranker since it looks like you might not have been able to for this project.
On the trip's detail page, I want to be able to view details, assign a rating, navigate to the trip's passenger, driver, a link to edit, and delete ✔️ Yup! And delete works ok for trip :)

Overall Feedback

Great work! Y'all made a whole interactive Rails application with so many ways for the user to interact! And it's fairly well tested!

See my inline comment above for a little more depth on one specific area for improvement.

But I hope y'all are proud! This is so amazing and it looks so good!

Overall Feedback Criteria yes/no
Green (Meets/Exceeds Standards) 7+ in Code Review && 5+ in Functional Requirements ✔️
Yellow (Approaches Standards) 5+ in Code Review && 4+ in Functional Requirements, or the instructor judges that this project needs special attention
Red (Not at Standard) 0-4 in Code Review or 0-3 in Functional Reqs, or assignment is breaking/doesn’t run with less than 5 minutes of debugging, or the instructor judges that this project needs special attention

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality Yes?
Perfect Indentation
Elegant/Clever
Descriptive/Readable
Concise
Logical/Organized


it "will delete a passenger" do
expect{
delete passenger_path(Passenger.find_by(id: 1))

Choose a reason for hiding this comment

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

This is failing because the id isn't necessarily 1. In tests the id is randomly assigned. The code below solves the problem.

Suggested change
delete passenger_path(Passenger.find_by(id: 1))
delete passenger_path(Passenger.first)

it "will delete a passenger" do
expect{
delete passenger_path(Passenger.find_by(id: 1))
}.must_change 'Passenger.count', 1

Choose a reason for hiding this comment

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

This should be -1 because the count is expected to go down by one, rather than up.

Suggested change
}.must_change 'Passenger.count', 1
}.must_change 'Passenger.count', -1

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