Carets- Jan and Julia#24
Open
julmeier wants to merge 65 commits into
Open
Conversation
…enable and then enter a command e.g. Driver.all)
…Data button only saves the trip rating- needs to be fixed
…no rating. we modified our driver model to ignore trips without rating data when calculating the average rating
…footer placement and centering
Rideshare-RailsWhat We're Looking For
|
CheezItMan
reviewed
Oct 8, 2017
| return (total * 0.85) | ||
| end | ||
|
|
||
| def ave_rating |
There was a problem hiding this comment.
This method always does integer division to calculate the average. So you can't have a rating of 2.5.
Also what if they have no trips! This will generate an error!
| belongs_to :driver | ||
| belongs_to :passenger | ||
| validates :date, presence: true | ||
| validates :cost, presence: true |
There was a problem hiding this comment.
You should have a validation for rating to limit them between 0 and 5 or 1 and 5.
Author
|
Hi Chris,
Thanks for your review! I have a couple of follow-up questions. Feel free
to answer via email or I can follow-up with you to discuss in person next
week with Jan. Hope you had a great vacation!
- Regarding the "RESTful routes utilized" - how could we change our code to
to apply restful routes (like rating a trip)? This principle is still
elusive to me.
-Do we still need to put validations for the rating, even if we are only
providing the user a drop-down menu of choices of integers 1 through 5?
Thank you,
Julia
…On Sat, Oct 7, 2017 at 5:18 PM, Chris M ***@***.***> wrote:
Rideshare-Rails What We're Looking For
Feature Feedback
*Baseline*
Appropriate Git Usage with no extraneous files checked in and both
partners contributing Good number of commits and both partner contributed.
Answered comprehension questions Check, we'll do more discussion about
where methods for business logic go next week I think.
Uses named routes (like _path) check,
RESTful routes utilized Check, except for routes like rating a trip.
*Rideshare Rails Specific Content*
Table relationships Check
Validation rules for Models You have validations for the presence of
fields, but you should have validations to limit ratings to specific values
and
Business logic is in the models Great work here putting business logic in
the models.
Database is seeded from the CSV files Check
Trello board is created and utilized in project management It doesn't
look like you used the Trello board much. Most things are in the to-do
category.
Postgres database is used Check
Heroku instance is online Check
The app is styled to create an attractive user interface
*Overall* I like how if I delete things like a trip it goes back to the
passenger page. The stylings look nice and attractive. I like how you had
columns used for lists of passengers and drivers. You hit all the
requirement in terms of functionality. Note my comment in your code that
you have a bug for when a driver has no trips, the show page will generate
an error because they have no trips (divide by 0). Other than that good
work!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AUSbfQ3DHhherO3CdoVCChMeHD4KXMF9ks5sqBTkgaJpZM4PxMTO>
.
|
|
- Regarding the "RESTful routes utilized" - how could we change our code to
to apply restful routes (like rating a trip)? This principle is still
elusive to me.
No since this isn't a generic CRUD operation, you're not going to have a
RESTful route. That's ok as long as for the generic
Create-Update-Read-Delete operations you're following the pattern.
…-Do we still need to put validations for the rating, even if we are only
providing the user a drop-down menu of choices of integers 1 through 5?
Yes, because you might change the UI at some point, like providing a mobile
app interface, and want the guarantee of valid data. Also your app could
later get input directly from other apps, we'll talk about them when you
make web APIs, and the validations are important here.
It's really a 2-layer approach, protect data at the UI end, and at the
database layer.
I hope these answers are helpful. Thanks for the wishes regarding the
holiday.
On Sat, Oct 7, 2017 at 11:02 PM, julmeier ***@***.***> wrote:
Hi Chris,
Thanks for your review! I have a couple of follow-up questions. Feel free
to answer via email or I can follow-up with you to discuss in person next
week with Jan. Hope you had a great vacation!
- Regarding the "RESTful routes utilized" - how could we change our code to
to apply restful routes (like rating a trip)? This principle is still
elusive to me.
-Do we still need to put validations for the rating, even if we are only
providing the user a drop-down menu of choices of integers 1 through 5?
Thank you,
Julia
On Sat, Oct 7, 2017 at 5:18 PM, Chris M ***@***.***> wrote:
> Rideshare-Rails What We're Looking For
> Feature Feedback
> *Baseline*
> Appropriate Git Usage with no extraneous files checked in and both
> partners contributing Good number of commits and both partner
contributed.
> Answered comprehension questions Check, we'll do more discussion about
> where methods for business logic go next week I think.
> Uses named routes (like _path) check,
> RESTful routes utilized Check, except for routes like rating a trip.
> *Rideshare Rails Specific Content*
> Table relationships Check
> Validation rules for Models You have validations for the presence of
> fields, but you should have validations to limit ratings to specific
values
> and
> Business logic is in the models Great work here putting business logic in
> the models.
> Database is seeded from the CSV files Check
> Trello board is created and utilized in project management It doesn't
> look like you used the Trello board much. Most things are in the to-do
> category.
> Postgres database is used Check
> Heroku instance is online Check
> The app is styled to create an attractive user interface
> *Overall* I like how if I delete things like a trip it goes back to the
> passenger page. The stylings look nice and attractive. I like how you had
> columns used for lists of passengers and drivers. You hit all the
> requirement in terms of functionality. Note my comment in your code that
> you have a bug for when a driver has no trips, the show page will
generate
> an error because they have no trips (divide by 0). Other than that good
> work!
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#24#
issuecomment-334974263>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/
AUSbfQ3DHhherO3CdoVCChMeHD4KXMF9ks5sqBTkgaJpZM4PxMTO>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#24 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABH3SKm4GyKjCFlWL3k1wtD5M86PKMYJks5sqGVtgaJpZM4PxMTO>
.
--
Chris McAnally
Ada Developer Academy Instructor
Pronouns: He/Him <http://pronoun.is/>
Change the world, one line at a time.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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