Water - Sophia #50
Open
Water - Sophia #50
Conversation
This file contains 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
…with basic HTML & made navigation, header, footer
…ly complete) view all media page
…plete, broken links
…how (w/o buttons), revised ROUTES
…DIT form, updated WORKS controller
|
I'm sorry this is late & incomplete :( |
Media RankerFunctional Requirements: Manual Testing
Major Learning Goals/Code Review
Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code
Testing Rails Apps
Overall FeedbackEverything that you completed looks good! And you did an impressive job matching the styling! The way that the server wasn't working without me fixing something makes me wonder if you were running the server to test out your work as you were doing it. I highly, highly encourage running the server and testing the website out for yourself regularly as you're working on Rails work! There are some pieces missing around login and voting and cover the primarily learning objectives for the week. Again, everything that's here is good!
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
jmaddox19
reviewed
Nov 20, 2020
config/routes.rb
Outdated
| delete '/works/:id', to: 'works#delete', as: 'delete_work' | ||
| patch '/works/:id/complete', to: 'works#complete', as: 'complete_work' | ||
|
|
||
| root to: 'homepage#index' |
There was a problem hiding this comment.
When I test out the server, I get the error uninitialized constant HomepageController.
Suggested change
| root to: 'homepage#index' | |
| root to: 'homepages#index' |
|
|
||
| <div clss="form_group"> | ||
| <%= f.label :title %> | ||
| <%= f.text_area :title, class: "form-control" %> |
There was a problem hiding this comment.
These fields would all be more appropriate as text_field. text_area creates a large box and is appropriate to be used when you're expecting the user to implement a full sentence or paragraph that takes up multiple lines.
Not a huge deal but just looks kind of awkward on the page.
config/routes.rb
Outdated
Comment on lines
3
to
19
| get '/homepages', to: 'homepages#index' | ||
| get '/works', to: 'works#index', as: 'works' | ||
| get 'works/new', to: 'works#new', as: 'new_work' | ||
| post '/works', to: 'works#create' | ||
| get '/users', to: 'users#index', as: 'users' | ||
| get 'users/new', to: 'users#new', as: 'new_user' | ||
| post '/users', to: 'users#create' | ||
|
|
||
| # Routes for using a specific task page | ||
| get '/works/:id', to: 'works#show', as: 'work' | ||
| get '/works/:id/edit', to: 'works#edit', as: 'edit_work' | ||
| patch '/works/:id', to: 'works#update' | ||
| get '/works/:id/confirm_delete', to: 'works#confirm', as: 'confirm_work' | ||
| delete '/works/:id', to: 'works#delete', as: 'delete_work' | ||
| patch '/works/:id/complete', to: 'works#complete', as: 'complete_work' | ||
|
|
||
| root to: 'homepage#index' |
There was a problem hiding this comment.
I'm curious why you didn't user resources here at all. I hope you're able to practice that and get more comfortable with it in bEtsy
…10. Tried adding flex for columns and commented it out
Media Ranker (Round 2)Great work! I'm really glad to see you were able to get login and voting working! Overall the app operated cohesively now. I get the impression you had more of a habit of running the server and manually testing your changes this time :D There is still a notable gap in testing here but the core learning objectives around login and voting functionality are certainly met :) I would encourage you to talk with me or someone else about the struggle you seemed to have with the work validation tests. I get the impression a brief conversation could probably help clear up the purpose of validation tests and how to write them. But again, so glad to see the core features you were able to add in a pretty short amount of extra time 👏 Functional Requirements: Manual Testing
Major Learning Goals/Code Review
Previous Rails learning, Building Complex Model Logic, DRYing up Rails Code
Testing Rails Apps
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
jmaddox19
reviewed
Dec 1, 2020
Comment on lines
+15
to
+32
| <hr class="root__hr"> | ||
| <section class=top-ten__container"> | ||
| <section class="top-ten__list-container"> | ||
| <h2 class="top-ten__header"> | ||
| Top Albums | ||
| </h2> | ||
| <ul class="list-group top-ten__list"> | ||
| <% @albums.each do |work| %> | ||
| <li class="list-item"> | ||
| <h4> | ||
| <%= link_to work.title, work_path(work.id) %> | ||
| <small class="top-ten-creator">by <%= work.creator %></small> | ||
| </h4> | ||
| <p><%= work.votes.count %></p> | ||
| </li> | ||
| <% end %> | ||
| </ul> | ||
| </section> |
There was a problem hiding this comment.
These top ten container sections would be a great candidate for a partial view that took a couple arguments because they're all three almost exactly the same. Sorry I wasn't very clear about explaining that in my previous feedback.
Comment on lines
+4
to
+24
| <section class="media-table"> | ||
| <h4>ALBUMS</h4> | ||
| <table class="table"> | ||
| <tr> | ||
| <th>Votes</th> | ||
| <th>Title</th> | ||
| <th>Created By</th> | ||
| <th>Published</th> | ||
| <th>Upvote</th> | ||
| </tr> | ||
| <% @albums.each do |work| %> | ||
| <tr> | ||
| <td><%= work.votes.count %></td> | ||
| <td><%= link_to work.title, work_path(work.id) %></td> | ||
| <td><%= work.creator %></td> | ||
| <td><%= work.publication_year %></td> | ||
| <td><%= link_to "VOTE", work_path(work.id), class: "btn btn-primary"%></td> | ||
| </tr> | ||
| <% end %> | ||
| </table> | ||
| </section> |
There was a problem hiding this comment.
These media-table sections would be a great candidate for a partial view that took a couple arguments because they're all three almost exactly the same. Sorry I wasn't very clear about explaining that in my previous feedback.
Comment on lines
+31
to
+37
| # change these below back to FALSE later on, for now change true so tests are passing. | ||
| # You've wasted enough time trying to debug this | ||
|
|
||
| it "must have a creator" do | ||
| @work.creator = nil | ||
| expect(@work.valid?).must_equal true | ||
| end |
There was a problem hiding this comment.
It looks like you ended up fixing the validation for creator so that this test now fails and line 36 here can be changed back to false
Suggested change
| # change these below back to FALSE later on, for now change true so tests are passing. | |
| # You've wasted enough time trying to debug this | |
| it "must have a creator" do | |
| @work.creator = nil | |
| expect(@work.valid?).must_equal true | |
| end | |
| # change these below back to FALSE later on, for now change true so tests are passing. | |
| # You've wasted enough time trying to debug this | |
| it "must have a creator" do | |
| @work.creator = nil | |
| expect(@work.valid?).must_equal false | |
| end |
Comment on lines
+39
to
+47
| it "must have a publication year" do | ||
| @work.publication_year = nil | ||
| expect(@work.valid?).must_equal true | ||
| end | ||
|
|
||
| it "must have a description" do | ||
| @work.description = nil | ||
| expect(@work.valid?).must_equal true | ||
| end |
There was a problem hiding this comment.
These weren't passing when you asserted they were false because there are no validations for these fields in work.rb. It's not always necessary to have validations for every field. When a field has no validations, no validation tests are needed for them.
Suggested change
| it "must have a publication year" do | |
| @work.publication_year = nil | |
| expect(@work.valid?).must_equal true | |
| end | |
| it "must have a description" do | |
| @work.description = nil | |
| expect(@work.valid?).must_equal true | |
| end |
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.
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?Assignment Submission: Media Ranker
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection
sessionandflash? What is the difference between them?