Sockets-Chantal #46

ChantalDemissie · 2019-05-05T06:11:38Z

Media Ranker

Congratulations! You're submitting your assignment!

Comprehension Questions

Question	Answer
Describe a custom model method you wrote.	I wrote a custom method for users to upvote and downvote using the values 1 and -1. Similar to youtube.
Describe how you approached testing that model method. What edge cases did you come up with?	I made it so my method validates a value(true) and has a inclusion of values only 1 and -1. This prevented a user from upvoting or downvoting something 100 times but i did not write a test out!!.
What are `session` and `flash`? What is the difference between them?	Session runs the whole time a user is on the browser by using cookies. Flash does not, it usually runs once and does not utilize cookies.
What was one thing that you gained more clarity on through this assignment?	How hard writing a fully functional website is! And to always give myself more time than I think i need...technically i would say bootstrap.
What is the Heroku URL of your deployed application?	https://damp-coast-69980.herokuapp.com/works note my homepage isnt working when i deployed but is locally, will fix on heroku...this also means everything that redirects back to my homepage isnt working
Do you have any recommendations on how we could improve this project for the next cohort?	Don't have this project the same week as the assessment? i underestimated how long this project would take me. It was a lot of work and every time I thought i was close to finishing I wasn't!

droberts-sea · 2019-05-07T16:49:35Z

app/views/works/homepage.html.erb

@@ -0,0 +1,22 @@
+<section>
+<% work = Work.all.sort { |work| work.votes.sum { |vote| vote.value } }.last %>
+<h2>


This is a lot of work to be doing in a view. This would probably fit better in a model method, so that it is isolated, reusable and testable.

On a related note, this calculation doesn't produce the correct result - your works come back out of order.

droberts-sea · 2019-05-07T16:50:38Z

app/views/works/homepage.html.erb

+    <ul>
+    <% Work.all.sort { |work| work.votes.sum { |vote| vote.value } }.reverse.first(10).each do |work| %>
+      <li>
+      <h4><%= link_to work.title, work_path(work.id) %><small> by <%= work.creator %></small></h4>


The code on line 14 does not organize works by category, so the three lists of work all look exactly the same.

droberts-sea · 2019-05-07T16:51:11Z

app/views/works/index.html.erb

+    <th>Downvote</th>
+  </tr>
+  <% works.sort { |work| work.votes.sum { |vote| vote.value } }.each do |work| %>
+  <tr>


Again, the code on line 13 should be in a model method.

droberts-sea · 2019-05-07T16:53:48Z

app/views/works/new.html.erb

@@ -0,0 +1,40 @@
+<h2>Add a new work</h2>
+
+<% if @work.errors.any? %>


This looks almost exactly the same as the code in edit.html.erb. You should be using a view partial to DRY this up.

https://github.com/Ada-Developers-Academy/textbook-curriculum/blob/master/08-rails/partial-views.md

As an example of why this is valuable, this file contains a dropdown menu for category, as well as a spot to show validation errors, that did not make it into the edit form.

droberts-sea · 2019-05-07T16:54:34Z

app/views/works/edit.html.erb

+  <div>
+    <%= form.label :category, "Category:" %>
+    <%= form.text_field :category, placeholder: "category" %>
+  </div>


Making this a text_field allows the user to type in anything they want, including invalid categories. A dropdown menu or set of radio buttons might be a better choice here.

droberts-sea · 2019-05-07T18:37:33Z

test/controllers/users_controller_test.rb

+    # #HELP
+    # describe "update" do
+    #   it "can update an existing user" do
+    #     user_hash = {


Your app doesn't have a way to update users, so it might be you don't need this test.

droberts-sea · 2019-05-07T18:43:25Z

app/models/vote.rb

+
+  def user
+    User.find(self.user_id)
+  end


You shouldn't need this method if you have belongs_to :user above. Think of it like an attr_accessor.

droberts-sea · 2019-05-07T18:44:02Z

app/models/work.rb

+
+  def top_voted
+    Work.all.sort { |work| work.votes.sum { |vote| vote.value } }.first
+  end


You define this method here, but I don't see it used anywhere.

droberts-sea · 2019-05-07T18:46:49Z

app/views/layouts/application.html.erb

+    <h1><%= link_to 'Media Ranker', '/' %><small>Ranking the Best of Everything</small></h1>
+    <%= link_to 'Log In', login_path %>
+    </header>
+    <%= flash[:message] %>


I would like to see a lot more functionality in this layout:

A navbar

Better support for letting the user know about their login status

More structure around displaying flash messages

droberts-sea · 2019-05-07T18:51:30Z

config/routes.rb

+
+  get "/login", to: "users#login_form", as: "login"
+  post "/login", to: "users#login"
+  post "/logout", to: "users#logout", as: "logout"


You have a route and controller action set up to log out the user, but your site doesn't have a link or button pointing at that route so there is no way for the user to do it.

droberts-sea · 2019-05-14T20:19:54Z

Media Ranker

What We're Looking For

Feature	Feedback
Core Requirements
Git hygiene	yes
Comprehension questions	yes
General
Rails fundamentals (RESTful routing, use of named paths)	some extra routes generated
Views are well-organized (DRY, use of semantic HTML, use of partials)	some - not using a view partial for the work form
Errors are reported to the user	some
Business logic lives in the models	there is some room for improvement here - see inline
Models are thoroughly tested, including relations, validations and any custom logic	nope
Controllers are thoroughly tested, including the login/logout process and multi-step workflows like voting for a work	yes - good work
Wave 1 - Media
Splash page shows the three media categories	yes
Basic CRUD operations on media are present and functional	yes - it might be wise to include some validations on your works. For example, if a work doesn't have a title, then the link to its show page doesn't appear, breaking the user workflow.
Wave 2 - Users and Votes
Users can log in and log out	log in yes, but there's no link to log out so no way for the user to do that
The ID of the current user is stored in the session	yes
A user cannot vote for the same media more than once	yes, but it would be good to display a `flash` message and let the user know what's going on
All media lists are ordered by vote count	I'm not sure what's going on with this. It's clear that the vote count affects ordering somehow, but sometimes they're in reverse order. This is a place where isolating this sorting logic in a model method and then thoroughly testing it would be a big help.
Splash page contains a media spotlight	yes
Wave 3 - Users and Votes
Media pages contain lists of voting users	yes
Individual user pages and the user list are present	yes
Optional - Styling
Bootstrap is used appropriately	yes
Look and feel is similar to the original	yes
Overall

Good job overall. Your implementation matches the demo site pretty closely, and I would say the learning goals for this assignment were definitely met. I especially appreciate your emphasis on presentation and layout - I know that's not your favorite, but you've done a really solid job here.

There are a few places where things could be cleaned up, which I've tried to outline below. In particular, I see room for growth around the way you use models - there's a lot of work that could be moved to (and tested in) the model. That would be a great thing to practice on VideoStore API, there are a few bits of complex business logic that would work well for this.

All that being said, in general I am fairly happy with this submission. Keep up the hard work!

droberts-sea · 2019-05-14T17:50:46Z

app/controllers/users_controller.rb

+    flash[:status] = :success
+    flash[:message] = "Sucessfully logged out"
+
+    redirect_to logout_path


If you have a redirect to the logout_path in the logout action, the user will end up in an infinite loop!

droberts-sea · 2019-05-14T17:52:47Z

app/controllers/works_controller.rb

+
+    previous_vote = Vote.where(work_id: work_id, user_id: user.id)
+
+    if previous_vote.empty?


Would it be possible to do this work in the model, possible as a validation?

droberts-sea · 2019-05-14T17:55:51Z

app/views/works/homepage.html.erb

+<section class="top-ten__container">
+  <% @works_by_category.each do |category, works| %>
+  <section class="top-ten__list-container">
+    <h2 class="top-ten__header">Top <%= category.capitalize %>s</h2>


Good work using a loop to keep this DRY

droberts-sea · 2019-05-14T17:57:12Z

app/views/works/homepage.html.erb

+<% work = Work.all.sort { |work| work.votes.sum { |vote| vote.value } }.last %>
+  <h2 class="spotlight__header">
+    <span class="spotlight__header--prefix">Media Spotlight</span>
+    <%= link_to work.title, work_path(work.id), class: "spotlight__link-to" %> by <%= work.creator %>


You're doing a bunch of complex work on line 2 that would probably fit better as a model method, something like Work.spotlight.

droberts-sea · 2019-05-14T18:05:17Z

config/routes.rb

+  post "/login", to: "users#login"
+  post "/logout", to: "users#logout", as: "logout"
+  get "/users/current", to: "users#current", as: "current_user"
+  resources :users


I don't think you need all 7 restful routes for users - only index and show are needed for this project.

droberts-sea · 2019-05-14T19:27:17Z

test/controllers/users_controller_test.rb

+  describe "login" do
+    it "logs in a new user" do
+      login_hash = {
+        user: {


Good work getting both of the success cases for this action. There's an interesting failure case your missing: what if the user tries to log in with bad data (e.g. username is "")?

droberts-sea · 2019-05-14T19:57:41Z

test/controllers/works_controller_test.rb

+      # Ensure a user can vote only once.
+      expect {
+        post vote_path(work.id), params: { value: value.to_s }
+      }.wont_change "Vote.count"


I like that you're checking for this! I would probably choose to break this out as a separate test, but this gets the job done.

droberts-sea · 2019-05-14T20:06:16Z

test/controllers/works_controller_test.rb

+  describe "homepage" do
+    it "displays a homepage" do
+      get home_path(work)
+      must_respond_with :success


The media spotlight means there's an interesting case here if there are no works. Also, I suspect this is the case that your deployed app is hitting (did you remember to seed your database?)

ChantalDemissie added 11 commits April 22, 2019 22:14

Initial rails setup

340cb7a

added models and controllers

76522ac

seeded database

c871ca8

skeleton added to controllers. model relationships created

513d787

added controller methods

7f14881

added routes

54817be

all views working no bugs

fcbccdd

log in

8a70d9a

added index and show views for user

7255388

voting working no bugs

2fbd099

upvote and downvotes methods complete and in views

f41b985

droberts-sea reviewed May 7, 2019

View reviewed changes

ChantalDemissie added 7 commits May 11, 2019 22:27

all crud tests passing on works controller

e2df161

users and works controller crud + custom methods tests

cc95e02

styled homepage

d679c0d

styled view all media page

84279be

styled add a new work page

d495b05

finished all styling

6e408ea

added gitignore

aa8079f

droberts-sea reviewed May 14, 2019

View reviewed changes

		@@ -0,0 +1,40 @@
		<h2>Add a new work</h2>

		<% if @work.errors.any? %>


		previous_vote = Vote.where(work_id: work_id, user_id: user.id)

		if previous_vote.empty?

Sockets-Chantal #46

Are you sure you want to change the base?

Sockets-Chantal #46

Uh oh!

Conversation

ChantalDemissie commented May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Media Ranker

Comprehension Questions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts-sea commented May 14, 2019

Media Ranker

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChantalDemissie commented May 5, 2019 •

edited

Loading