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

Octos- Analisa & Ari- Rideshare- Rails #23

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

Conversation

arrriiii
Copy link

@arrriiii arrriiii 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 Given our ERB diagram, we knew a single trip belongs to one driver and one passenger.--Driver & passenger would have a one to many trip relationship.
Describe the role of model validations in your application The validation, :dependent => :destroy, to remove dependencies from the trip was instrumental in allowing the driver or passenger to delete a trip if needed. Validations are crucial to a user friendly experience.
How did your team break up the work to be done? We split driver and passenger models, controller & view while working together on the trip model, controller, & view.
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 decided styling should be completely optional and we were more focused on functionality.
What was one thing that your team collectively gained more clarity on after completing this assignment? Creating models with validations and methods to perform business logic.-- Also working with dependencies in the model and how that correlates to the view.
What is your Trello URL? https://trello.com/b/vtlVhbbl/rideshare-ari-and-ana-lisa
What is the Heroku URL of your deployed application? https://aaride-app.herokuapp.com/trips
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?

The-Beez-Kneez and others added 30 commits April 2, 2018 16:39
Updating working_with_drivers with master
Adding drivers to master

def trip_cost_dollars
if self.cost == nil
return (self.cost = 0)

Choose a reason for hiding this comment

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

Why do this vs just returning 0?

<%= paginate @drivers, outer_window: 3 %>
</div>

<div class="driver_title">

Choose a reason for hiding this comment

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

For CSS class names, we do not follow the Ruby naming conventions. For CSS this should be driver-title not driver_title.

@@ -0,0 +1,41 @@
<h2>Driver Details</h2>

Choose a reason for hiding this comment

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

This collection of data should be inside of one semantic HTML element to encapsulate it


<section>
Passenger ID:
<%= @passenger[:id] %><br>

Choose a reason for hiding this comment

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

You should not be using the hash-like syntax for retrieving attribute values, you should be using the built-in "dot methods". So this should be @passenger.id instead.

<%= form_for @trip do |t| %>
<%= t.label :date %>
<%= t.date_field :date %>
<% unless @trip.passenger_id %>

Choose a reason for hiding this comment

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

Watch the indentation here - since you're using a conditional, the contents inside should be indented

<%= t.label :id %>
<%= t.text_field :id %>
<%= t.label :passenger %>
<%= t.text_field :passenger_id %>

Choose a reason for hiding this comment

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

This should be using the drop down that you utilize in the 'new' form

flash[:alert] = "Driver deleted"
end

def driver_params

Choose a reason for hiding this comment

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

This should be under a private section

def new
if params[:passenger_id]
passenger = Passenger.find_by(id: passenger_id)
@trips = passenger.trips.new

Choose a reason for hiding this comment

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

Hmmmm... I'm curious why the new method in the passengers controller would need to instantiate a trip object.

def destroy
@passenger = Passenger.find(params[:id])
@passenger.destroy
redirect_to '/passengers'

Choose a reason for hiding this comment

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

You should use an _path here instead of the string referencing the route


def new
@trip = Trip.new(passenger_id: params[:passenger_id])
@trip.driver_id = Driver.get_random_driver

Choose a reason for hiding this comment

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

Yes - nice job having this method's logic in the model not here in the controller

@kariabancroft
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 Mostly - I still feel like the git contributions do not show equal team contribution.
Answered comprehension questions Yes
Uses named routes (like _path) Yes - views look good using lots of named routes.
RESTful routes utilized Yes - looks great!
Rideshare Rails Specific Content
Table relationships Yes - looks good in the schema and the model files
Validation rules for Models Yes - using the presence validations in the models
Business logic is in the models Yes - using the models for some of the cost, price, total logic
Database is seeded from the CSV files Yes - data set on Heroku looks good
Trello board is created and utilized in project management Its private so I can't see it - but I think so??
Postgres database is used Yes
Heroku instance is online Yes
The app is styled to create an attractive user interface Yes
Overall Nice job overall hitting the major learning goals of this assignment. The most concerning thing I noticed is when you're using the hash syntax like [:field] rather than the built-in dot method like .field throughout controllers and views. You should always be using the dot methods.

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