From 9535b054907cb80cc78d20badbf4acb462e3ba2a Mon Sep 17 00:00:00 2001 From: joshua stein Date: Mon, 3 Mar 2014 17:14:33 -0600 Subject: [PATCH] remove story downvoting, add story hiding stories should either be reported for spam (coming later), upvoted, or left alone rather than being downvoted for being uninteresting. since users don't like leaving uninteresting things alone, they can now hide stories from their view without affecting the story's score. hiding is implemented as a Vote with its vote set to 0 and the reason set to "H" add a /hidden url which shows all of a user's hidden stories while i'm here, simplify Vote guts and add some tests to make sure all the flip-flopping stuff works right --- app/assets/javascripts/application.js.erb | 57 +++++++------ app/assets/stylesheets/application.css | 6 ++ app/controllers/home_controller.rb | 31 +++++-- app/controllers/stories_controller.rb | 23 +++--- app/models/story.rb | 35 +------- app/models/vote.rb | 47 +++-------- app/views/home/index.html.erb | 13 ++- app/views/stories/_listdetail.html.erb | 27 +++---- config/routes.rb | 5 +- spec/models/vote_spec.rb | 98 +++++++++++++++++++++++ 10 files changed, 207 insertions(+), 135 deletions(-) create mode 100644 spec/models/vote_spec.rb diff --git a/app/assets/javascripts/application.js.erb b/app/assets/javascripts/application.js.erb index 451499e..c085612 100644 --- a/app/assets/javascripts/application.js.erb +++ b/app/assets/javascripts/application.js.erb @@ -7,35 +7,40 @@ var _Lobsters = Class.extend({ curUser: null, - storyDownvoteReasons: { <%= Vote::STORY_REASONS.map{|k,v| - "#{k.inspect}: #{v.inspect}" }.join(", ") %> }, commentDownvoteReasons: { <%= Vote::COMMENT_REASONS.map{|k,v| "#{k.inspect}: #{v.inspect}" }.join(", ") %> }, - upvote: function(voterEl) { + upvoteStory: function(voterEl) { Lobsters.vote("story", voterEl, 1); }, - downvote: function(voterEl) { - Lobsters._showDownvoteWhyAt("story", voterEl, function(k) { - Lobsters.vote("story", voterEl, -1, k); }); + hideStory: function(hiderEl) { + if (!Lobsters.curUser) + return Lobsters.bounceToLogin(); + + var li = $(hiderEl).closest(".story, .comment"); + var act; + + if (li.hasClass("hidden")) { + act = "unhide"; + li.removeClass("hidden"); + hiderEl.innerHTML = "hide"; + } else { + act = "hide"; + li.addClass("hidden"); + hiderEl.innerHTML = "unhide"; + } + + $.post("/stories/" + li.attr("data-shortid") + "/" + act); }, upvoteComment: function(voterEl) { Lobsters.vote("comment", voterEl, 1); }, downvoteComment: function(voterEl) { - Lobsters._showDownvoteWhyAt("comment", voterEl, function(k) { - Lobsters.vote("comment", voterEl, -1, k); }); - }, - - _showDownvoteWhyAt: function(thingType, voterEl, onChooseWhy) { - if (!Lobsters.curUser) - return Lobsters.bounceToLogin(); - var li = $(voterEl).closest(".story, .comment"); if (li.hasClass("downvoted")) { /* already upvoted, neutralize */ - Lobsters.vote(thingType, voterEl, -1, null); + Lobsters.vote("comment", voterEl, -1, null); return; } @@ -53,13 +58,7 @@ var _Lobsters = Class.extend({ var d = $("
"); - var reasons; - if (thingType == "comment") - reasons = Lobsters.commentDownvoteReasons; - else - reasons = Lobsters.storyDownvoteReasons; - - $.each(reasons, function(k, v) { + $.each(Lobsters.commentDownvoteReasons, function(k, v) { var a = $("" + v + ""); @@ -68,7 +67,7 @@ var _Lobsters = Class.extend({ $("#downvote_why_shadow").remove(); if (k != "") - onChooseWhy(k); + Lobsters.vote("comment", voterEl, -1, k); return false; }); @@ -205,8 +204,7 @@ var _Lobsters = Class.extend({ }) .success(function(data) { if (data && data.title) - title_field.val(data.title.substr(0, - title_field.maxLength)); + title_field.val(data.title.substr(0, title_field.maxLength)); button.val(old_value); button.prop("disabled", false); @@ -220,6 +218,7 @@ var _Lobsters = Class.extend({ bounceToLogin: function() { document.location = "/login?return=" + encodeURIComponent(document.location); + return false; }, }); @@ -235,12 +234,12 @@ $(document).ready(function() { return false; }); - $("li.story a.downvoter").click(function() { - Lobsters.downvote(this); + $("li.story a.upvoter").click(function() { + Lobsters.upvoteStory(this); return false; }); - $("li.story a.upvoter").click(function() { - Lobsters.upvote(this); + $("li.story a.hider").click(function() { + Lobsters.hideStory(this); return false; }); diff --git a/app/assets/stylesheets/application.css b/app/assets/stylesheets/application.css index afbe546..9e0949d 100644 --- a/app/assets/stylesheets/application.css +++ b/app/assets/stylesheets/application.css @@ -509,6 +509,12 @@ li .byline a.inactive_user { text-decoration: line-through; } +li.story.hidden { + opacity: 0.25; +} +ol.show_hidden li.story.hidden { + opacity: 1.0 !important; +} li.story.expired { opacity: 0.6; diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb index 87730c3..d0ba364 100644 --- a/app/controllers/home_controller.rb +++ b/app/controllers/home_controller.rb @@ -10,6 +10,15 @@ class HomeController < ApplicationController # for rss feeds, load the user's tag filters if a token is passed before_filter :find_user_from_rss_token, :only => [ :index, :newest ] + def hidden + @stories = find_stories({ :hidden => true }) + + @heading = @title = "Hidden Stories" + @cur_url = "/hidden" + + render :action => "index" + end + def index @stories = find_stories @@ -38,7 +47,6 @@ class HomeController < ApplicationController @heading = @title = "Newest Stories" @cur_url = "/newest" - @newest = true @rss_link = " "index" end @@ -145,14 +152,28 @@ private def _find_stories(how) stories = Story.where(:is_expired => false) - if @user && !(how[:newest] || how[:by_user]) - # exclude downvoted items + if @user && !how[:by_user] && !how[:hidden] + # exclude downvoted and hidden items stories = stories.where( Story.arel_table[:id].not_in( Vote.arel_table.where( Vote.arel_table[:user_id].eq(@user.id) ).where( - Vote.arel_table[:vote].lt(0) + Vote.arel_table[:vote].lteq(0) + ).where( + Vote.arel_table[:comment_id].eq(nil) + ).project( + Vote.arel_table[:story_id] + ) + ) + ) + elsif @user && how[:hidden] + stories = stories.where( + Story.arel_table[:id].in( + Vote.arel_table.where( + Vote.arel_table[:user_id].eq(@user.id) + ).where( + Vote.arel_table[:vote].eq(0) ).where( Vote.arel_table[:comment_id].eq(nil) ).project( diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index d0e6f52..3bfb258 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -1,6 +1,6 @@ class StoriesController < ApplicationController before_filter :require_logged_in_user_or_400, - :only => [ :upvote, :downvote, :unvote, :preview ] + :only => [ :upvote, :unvote, :hide, :unhide, :preview ] before_filter :require_logged_in_user, :only => [ :destroy, :create, :edit, :fetch_url_title, :new ] @@ -201,21 +201,24 @@ class StoriesController < ApplicationController render :text => "ok" end - def downvote + def hide if !(story = find_story) return render :text => "can't find story", :status => 400 end - if !Vote::STORY_REASONS[params[:reason]] - return render :text => "invalid reason", :status => 400 + Vote.vote_thusly_on_story_or_comment_for_user_because(0, story.id, + nil, @user.id, "H") + + render :text => "ok" + end + + def unhide + if !(story = find_story) + return render :text => "can't find story", :status => 400 end - if !@user.can_downvote?(story) - return render :text => "not permitted to downvote", :status => 400 - end - - Vote.vote_thusly_on_story_or_comment_for_user_because(-1, story.id, - nil, @user.id, params[:reason]) + Vote.vote_thusly_on_story_or_comment_for_user_because(0, story.id, + nil, @user.id, nil) render :text => "ok" end diff --git a/app/models/story.rb b/app/models/story.rb index 418ce28..152b12a 100644 --- a/app/models/story.rb +++ b/app/models/story.rb @@ -10,8 +10,6 @@ class Story < ActiveRecord::Base validates_length_of :description, :maximum => (64 * 1024) validates_presence_of :user_id - DOWNVOTABLE_DAYS = 14 - # after this many minutes old, a story cannot be edited MAX_EDIT_MINS = 30 @@ -119,8 +117,7 @@ class Story < ActiveRecord::Base end def calculated_hotness - # don't immediately kill stories at 0 by bumping up score by one - order = Math.log([ (score + 1).abs, 1 ].max, 10) + order = Math.log([ score.abs, 1 ].max, 10) if score > 0 sign = 1 elsif score < 0 @@ -226,14 +223,6 @@ class Story < ActiveRecord::Base "hotness = '#{self.calculated_hotness}' WHERE id = #{self.id.to_i}") end - def is_downvotable? - if self.created_at - Time.now - self.created_at <= DOWNVOTABLE_DAYS.days - else - false - end - end - def is_editable_by_user?(user) if user && user.is_moderator? return true @@ -406,26 +395,4 @@ class Story < ActiveRecord::Base def url_or_comments_url self.url.blank? ? self.comments_url : self.url end - - def vote_summary_for(user) - r_counts = {} - r_whos = {} - Vote.where(:story_id => self.id, :comment_id => nil).each do |v| - r_counts[v.reason.to_s] ||= 0 - r_counts[v.reason.to_s] += v.vote - if user && user.is_moderator? - r_whos[v.reason.to_s] ||= [] - r_whos[v.reason.to_s].push v.user.username - end - end - - r_counts.keys.sort.map{|k| - if k == "" - "+#{r_counts[k]}" - else - "#{r_counts[k]} #{Vote::STORY_REASONS[k]}" + - (user && user.is_moderator?? " (#{r_whos[k].join(", ")})" : "") - end - }.join(", ") - end end diff --git a/app/models/vote.rb b/app/models/vote.rb index ea7f536..4f4860e 100644 --- a/app/models/vote.rb +++ b/app/models/vote.rb @@ -2,16 +2,6 @@ class Vote < ActiveRecord::Base belongs_to :user belongs_to :story - STORY_REASONS = { - "O" => "Off-topic", - "Q" => "Low Quality", - "A" => "Already Posted", - "T" => "Poorly Tagged", - "L" => "Poorly Titled", - "S" => "Spam", - "" => "Cancel", - } - COMMENT_REASONS = { "O" => "Off-topic", "I" => "Incorrect", @@ -82,7 +72,7 @@ class Vote < ActiveRecord::Base v = Vote.where(:user_id => user_id, :story_id => story_id, :comment_id => comment_id).first_or_initialize - if !v.new_record? && v.vote == vote + if !v.new_record? && v.vote == vote && vote != 0 return end @@ -91,35 +81,22 @@ class Vote < ActiveRecord::Base Vote.transaction do # unvote - if vote == 0 - if !v.new_record? - if v.vote == -1 - downvote = -1 - else - upvote = -1 - end - end - - v.destroy + if vote == 0 && reason == nil + # neutralize previous vote + upvote = (v.vote == 1 ? -1 : 0) + downvote = (v.vote == -1 ? -1 : 0) + v.destroy! # new vote or change vote else - if v.new_record? - if vote == -1 - downvote = 1 - else - upvote = 1 - end - elsif v.vote == -1 - # changing downvote to upvote - downvote = -1 - upvote = 1 - elsif v.vote == 1 - # changing upvote to downvote - upvote = -1 - downvote = 1 + if !v.new_record? + upvote = (v.vote == 1 ? -1 : 0) + downvote = (v.vote == -1 ? -1 : 0) end + upvote += (vote == 1 ? 1 : 0) + downvote += (vote == -1 ? 1 : 0) + v.vote = vote v.reason = reason v.save! diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index 04db669..91b2a63 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -1,21 +1,20 @@ -
    +
      "> <%= render :partial => "stories/listdetail", :collection => @stories, :as => :story %>
    <% if @page && @page > 1 %> - <%= @page == 2 ? "/" : - "/page/#{@page - 1}" %>"><< Page <%= @page - 1 %> + <%= + @page == 2 ? "" : "page/#{@page - 1}" %>"><< Page + <%= @page - 1 %> <% end %> <% if @show_more %> <% if @page && @page > 1 %> | <% end %> - /page/<%= @page + 1 %>">Page - <%= @page + 1 %> >> + page/<%= + @page + 1 %>">Page <%= @page + 1 %> >> <% end %>
    diff --git a/app/views/stories/_listdetail.html.erb b/app/views/stories/_listdetail.html.erb index 640be6b..4db3218 100644 --- a/app/views/stories/_listdetail.html.erb +++ b/app/views/stories/_listdetail.html.erb @@ -1,6 +1,7 @@
  1. <%= story.is_expired? ? "expired" : "" %>"> +class="story <%= story.vote == 1 ? "upvoted" : "" %> <%= story.vote == -1 ? +"downvoted" : "" %> <%= story.vote == 0 ? "hidden" : "" %> <%= story.is_expired? ? +"expired" : "" %>">
    <% if @user %> @@ -8,12 +9,7 @@ class="story <%= story.vote == 1 ? "upvoted" : (story.vote == -1 ? <% else %> <%= link_to "", login_url, :class => "upvoter" %> <% end %> -
    <%= story.upvotes - story.downvotes %>
    - <% if @user && @user.can_downvote?(story) %> - - <% else %> - - <% end %> +
    <%= story.score %>
    @@ -67,6 +63,15 @@ class="story <%= story.vote == 1 ? "upvoted" : (story.vote == -1 ? <% end %> <% end %> <% end %> + <% if !story.is_gone? && @user %> + <% if story.vote == 0 %> + | <%= link_to "unhide", story_unhide_url(story.short_id), + :class => "hider" %> + <% else %> + | <%= link_to "hide", story_hide_url(story.short_id), + :class => "hider" %> + <% end %> + <% end %> <% if !story.is_gone? && (@user || story.comments_count > 0) %> | @@ -75,12 +80,6 @@ class="story <%= story.vote == 1 ? "upvoted" : (story.vote == -1 ? (story.comments_count == 1 ? "" : "s") %> <% end %> - - <% if defined?(single_story) && single_story %> - <% if story.downvotes > 0 %> - | <%= story.vote_summary_for(@user).downcase %> - <% end %> - <% end %> <% end %>
    diff --git a/config/routes.rb b/config/routes.rb index e7f7d25..4111175 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -14,6 +14,8 @@ Lobsters::Application.routes.draw do get "/newest/:user/page/:page" => "home#newest_by_user" get "/recent" => "home#recent" get "/recent/page/:page" => "home#recent" + get "/hidden" => "home#hidden" + get "/hidden/page/:page" => "home#hidden" get "/threads" => "comments#threads" get "/threads/:user" => "comments#threads" @@ -40,9 +42,10 @@ Lobsters::Application.routes.draw do resources :stories do post "upvote" - post "downvote" post "unvote" post "undelete" + post "hide" + post "unhide" end post "/stories/fetch_url_title", :format => "json" post "/stories/preview" => "stories#preview" diff --git a/spec/models/vote_spec.rb b/spec/models/vote_spec.rb new file mode 100644 index 0000000..bec5282 --- /dev/null +++ b/spec/models/vote_spec.rb @@ -0,0 +1,98 @@ +require "spec_helper" + +describe Vote do + it "applies a story upvote and karma properly" do + s = Story.make! + + s.upvotes.should == 1 + s.downvotes.should == 0 + s.user.karma.should == 0 + + u = User.make! + + Vote.vote_thusly_on_story_or_comment_for_user_because(1, s.id, + nil, u.id, nil) + + s.reload + + s.upvotes.should == 2 + s.user.karma.should == 1 + end + + it "does nothing when upvoting an existing upvote" do + s = Story.make! + + u = User.make! + + 2.times do + Vote.vote_thusly_on_story_or_comment_for_user_because(1, s.id, + nil, u.id, nil) + + s.reload + + s.upvotes.should == 2 + s.user.karma.should == 1 + end + end + + it "has no effect on a story score when casting a hide vote" do + s = Story.make! + s.upvotes.should == 1 + + u = User.make! + + Vote.vote_thusly_on_story_or_comment_for_user_because(0, s.id, + nil, u.id, "H") + s.reload + s.user.karma.should == 0 + s.upvotes.should == 1 + s.downvotes.should == 0 + end + + it "removes karma and upvote when downvoting an upvote" do + s = Story.make! + c = Comment.make!(:story_id => s.id) + c.user.karma.should == 0 + + u = User.make! + + Vote.vote_thusly_on_story_or_comment_for_user_because(1, s.id, + c.id, u.id, nil) + c.reload + c.user.karma.should == 1 + # initial poster upvote plus new user's vote + c.upvotes.should == 2 + c.downvotes.should == 0 + + # flip vote + Vote.vote_thusly_on_story_or_comment_for_user_because(-1, s.id, + c.id, u.id, Vote::COMMENT_REASONS.keys.first) + c.reload + + c.user.karma.should == -1 + c.upvotes.should == 1 + c.downvotes.should == 1 + end + + it "neutralizes karma and upvote when unvoting an upvote" do + s = Story.make! + c = Comment.make!(:story_id => s.id) + + u = User.make! + + Vote.vote_thusly_on_story_or_comment_for_user_because(1, s.id, + c.id, u.id, nil) + c.reload + c.user.karma.should == 1 + c.upvotes.should == 2 + c.downvotes.should == 0 + + Vote.vote_thusly_on_story_or_comment_for_user_because(0, s.id, + c.id, u.id, nil) + c.reload + + c.user.karma.should == 0 + c.upvotes.should == 1 + c.downvotes.should == 0 + end +end