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

Kal(Earth)xBeatrice(Fire) #21

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

Conversation

kashenafi
Copy link

@kashenafi kashenafi commented Nov 7, 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 Trip belongs to passenger and driver so a trip can only have one passenger/driver. Driver and passenger have many trips each because that is what the data has and how it works in the real world.
Describe the role of model validations in your application We wanted to make sure that each model had the appropriate attributes filled in. ‘Appropriate’ meaning as close to the real world application as possible.
How did your team break up the work to be done? Made branches and worked on certain parts individually, then made and merged pull requests.
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 basic CRUD for driver, passenger, and trip first. We set aside CSS until these were taken care of . We did not give the user the ability to complete a trip. We also did not use nested routes.
What was one thing that your team collectively gained more clarity on after completing this assignment? Git! Also, model testing, and how multiple controllers/models can interact
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 both agreed that we could have split up more of the work both amongst ourselves and among the week - a lot of time on Zoom the past two days. We were both very pleased that we got a lot of practice in the roles we were weaker on (one of us was more comfortable driving and one being passenger and we switched that up a lot this project).
Optional: What is the URL of your deployed Heroku app? https://space-rs.herokuapp.com/trips

@kashenafi kashenafi changed the title Earth+ Fire - Kal & Beatrice Earth x Fire - Kal x Beatrice Nov 7, 2020
@kashenafi kashenafi changed the title Earth x Fire - Kal x Beatrice Kal x Beatrice Nov 7, 2020
@kashenafi kashenafi changed the title Kal x Beatrice Kal(Earth) x Beatrice(Fire) Nov 7, 2020
@kashenafi kashenafi changed the title Kal(Earth) x Beatrice(Fire) Kal(Earth)xBeatrice(Fire) Nov 10, 2020
Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

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 ✔️
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 ✔️
There are reasonable tests to test the validation requirements for all models ✔️
There are reasonable tests to test the relationship requirements for all models ✔️
There are reasonable tests to test the controller actions for all controllers ✔️
The app has an attractive and usable user interface ✔️, looks really nice
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 ✔️
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 ✔️
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 ✔️
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 ✔️

Overall Feedback

Overall Feedback Criteria yes/no
Green (Meets/Exceeds Standards) 8+ in Code Review && 5+ in Functional Requirements

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

Summary

Really well done. This hit all the learning goals here. I did have some suggestions around returning status codes and testing. However this is a very solid submission. Nice work!

redirect_to driver_path(@driver.id)
return
else
render :new

Choose a reason for hiding this comment

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

It's best to include an HTTP status code.

Suggested change
render :new
render :new, status: :bad_request

redirect_to driver_path(@driver.id)
return
else
render :edit

Choose a reason for hiding this comment

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

Suggested change
render :edit
render :edit, status: :bad_request

redirect_to passenger_path(@passenger.id)
return
else
render :new

Choose a reason for hiding this comment

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

Suggested change
render :new
render :new, status: :bad_request

redirect_to passenger_path(@passenger.id)
return
else
render :edit

Choose a reason for hiding this comment

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

Suggested change
render :edit
render :edit, status: :bad_request

redirect_to trip_path(@trip.id)
return
else
render :new

Choose a reason for hiding this comment

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

Suggested change
render :new
render :new, status: :bad_request

describe "can go online" do
# Your code here
end
describe "total earnings" do

Choose a reason for hiding this comment

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

Also include a test for when the driver has no trips.

end

describe "complete trip" do
# Your code here
describe "total spent" do

Choose a reason for hiding this comment

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

You also need a test for when the passenger has no trips.

end

describe "validations" do
# Your tests go here
it "had a numeric cost" do

Choose a reason for hiding this comment

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

You should also test that the cost is not missing.

Comment on lines +14 to +15
<div class="parallax">
<h1 class="title"><%= link_to "🛸Ride Share Control Panel🛸", root_path %></h1>

Choose a reason for hiding this comment

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

Interesting styling


root to: 'homepages#index'

resources :trips

Choose a reason for hiding this comment

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

Noting that the trips new/create can be nested under passenger.

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