From 73b19bee05be213986857ce9f5f5a56d0b9f728c Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 23 Mar 2017 02:54:31 -0500 Subject: [PATCH 1/7] Fix exception in `h(post.source)` when source is nil. Fixes this exception: TypeError exception raised no implicit conversion of nil into String app/presenters/presenter.rb:3:in `escapeHTML' app/presenters/presenter.rb:3:in `h' app/presenters/post_presenter.rb:91:in `data_attributes' app/presenters/post_presenter.rb:24:in `preview' app/presenters/post_set_presenters/base.rb:15:in `block in post_previews_html' app/presenters/post_set_presenters/base.rb:14:in `post_previews_html' app/views/posts/partials/index/_posts.html.erb:3:in `_app_views_posts_partials_index__posts_html_erb___2716396856929289512_70158315986180' app/views/posts/index.html.erb:39:in `_app_views_posts_index_html_erb__1345252938355811220_70158296799280' app/controllers/posts_controller.rb:16:in `index' --- app/presenters/presenter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/presenters/presenter.rb b/app/presenters/presenter.rb index 19497050d..1773cc0f7 100644 --- a/app/presenters/presenter.rb +++ b/app/presenters/presenter.rb @@ -1,6 +1,6 @@ class Presenter def self.h(s) - CGI.escapeHTML(s) + CGI.escapeHTML(s.to_s) end def self.u(s) From fd24ea5876b6d16400b9e87f2a0485ff1e141ad5 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 4 Feb 2017 03:40:14 -0600 Subject: [PATCH 2/7] posts.rb: vote on behalf of correct user when moving favorites. Bug: when an approver moves the favorites of a post, each favorite is removed from the child post and added to the parent post. For gold+ users, this triggers an upvote, but these upvotes were performed by the approver rather than the favoriter. --- app/models/post.rb | 22 ++++++++++------------ app/models/post_vote.rb | 2 +- app/models/user.rb | 1 + 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index d8053308a..a45f1bfe9 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -943,7 +943,7 @@ class Post < ActiveRecord::Base def add_favorite!(user) Favorite.add(self, user) - vote!("up") if CurrentUser.is_gold? + vote!("up", user) if user.is_gold? rescue PostVote::Error end @@ -953,7 +953,7 @@ class Post < ActiveRecord::Base def remove_favorite!(user) Favorite.remove(self, user) - unvote! if CurrentUser.is_gold? + unvote!(user) if user.is_gold? rescue PostVote::Error end @@ -1063,27 +1063,25 @@ class Post < ActiveRecord::Base !PostVote.exists?(:user_id => user.id, :post_id => id) end - def vote!(score) - unless CurrentUser.is_voter? + def vote!(score, voter = CurrentUser.user) + unless voter.is_voter? raise PostVote::Error.new("You do not have permission to vote") end - unless can_be_voted_by?(CurrentUser.user) + unless can_be_voted_by?(voter) raise PostVote::Error.new("You have already voted for this post") end - PostVote.create!(:post_id => id, :score => score) + votes.create!(user: voter, score: vote) reload # PostVote.create modifies our score. Reload to get the new score. end - def unvote! - if can_be_voted_by?(CurrentUser.user) + def unvote!(voter = CurrentUser.user) + if can_be_voted_by?(voter) raise PostVote::Error.new("You have not voted for this post") else - vote = PostVote.where("post_id = ? and user_id = ?", id, CurrentUser.user.id).first - vote.destroy - - self.reload + votes.where(user: voter).destroy_all + reload end end end diff --git a/app/models/post_vote.rb b/app/models/post_vote.rb index a3d7eb143..26fd280d4 100644 --- a/app/models/post_vote.rb +++ b/app/models/post_vote.rb @@ -35,7 +35,7 @@ class PostVote < ActiveRecord::Base end def initialize_user - self.user_id = CurrentUser.user.id + self.user_id ||= CurrentUser.user.id end def update_post_on_destroy diff --git a/app/models/user.rb b/app/models/user.rb index 1e9eeb41d..c253907c5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -77,6 +77,7 @@ class User < ActiveRecord::Base #after_create :notify_sock_puppets has_many :feedback, :class_name => "UserFeedback", :dependent => :destroy has_many :posts, :foreign_key => "uploader_id" + has_many :post_votes has_many :bans, lambda {order("bans.id desc")} has_one :recent_ban, lambda {order("bans.id desc")}, :class_name => "Ban" has_one :api_key From 07707b257abc19583aa7d990de10d96de0a0d826 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 24 Mar 2017 02:48:01 -0500 Subject: [PATCH 3/7] post.rb: make non-gold supervoters upvote when fav'ing a post. --- app/models/post.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index a45f1bfe9..0f662e034 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -943,7 +943,7 @@ class Post < ActiveRecord::Base def add_favorite!(user) Favorite.add(self, user) - vote!("up", user) if user.is_gold? + vote!("up", user) if user.is_voter? rescue PostVote::Error end @@ -953,7 +953,7 @@ class Post < ActiveRecord::Base def remove_favorite!(user) Favorite.remove(self, user) - unvote!(user) if user.is_gold? + unvote!(user) if user.is_voter? rescue PostVote::Error end From 3dc854c0c8012006a3a57a2b6db230a7ea84036e Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 24 Mar 2017 14:55:37 -0500 Subject: [PATCH 4/7] post_vote.rb: determine vote magnitude from voter, not CurrentUser. Bug: when moving favorites, the parent is given an upvote for each gold+ favoriter, but the magnitude of these upvotes was based on the CurrentUser, rather than the upvoter. This meant that upvotes for supervoter favorites weren't given correctly. To fix this, `magnitude` is changed to use the voting `user` instead of CurrentUser. New problem: when setting the score, `score=` calls `magnitude`, but `user` isn't initialized yet. So we also refactor so that 1. `initialize_attributes` initializes `user` before setting `score` 2. the vote direction is given by `vote`, so it's separate from `score` 3. updating the score on the post happens in a callback instead of directly in `score=`. --- app/models/post.rb | 4 ++-- app/models/post_vote.rb | 32 ++++++++++++++++++++------------ test/unit/post_vote_test.rb | 6 +++--- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 0f662e034..2c47e7845 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1063,7 +1063,7 @@ class Post < ActiveRecord::Base !PostVote.exists?(:user_id => user.id, :post_id => id) end - def vote!(score, voter = CurrentUser.user) + def vote!(vote, voter = CurrentUser.user) unless voter.is_voter? raise PostVote::Error.new("You do not have permission to vote") end @@ -1072,7 +1072,7 @@ class Post < ActiveRecord::Base raise PostVote::Error.new("You have already voted for this post") end - votes.create!(user: voter, score: vote) + votes.create!(user: voter, vote: vote) reload # PostVote.create modifies our score. Reload to get the new score. end diff --git a/app/models/post_vote.rb b/app/models/post_vote.rb index 26fd280d4..ac503d64d 100644 --- a/app/models/post_vote.rb +++ b/app/models/post_vote.rb @@ -2,10 +2,14 @@ class PostVote < ActiveRecord::Base class Error < Exception ; end belongs_to :post - before_validation :initialize_user, :on => :create + belongs_to :user + attr_accessor :vote + attr_accessible :post, :post_id, :user, :user_id, :score, :vote + + after_initialize :initialize_attributes, if: :new_record? validates_presence_of :post_id, :user_id, :score validates_inclusion_of :score, :in => [SuperVoter::MAGNITUDE, 1, -1, -SuperVoter::MAGNITUDE] - attr_accessible :post_id, :user_id, :score + after_create :update_post_on_create after_destroy :update_post_on_destroy def self.prune! @@ -24,18 +28,22 @@ class PostVote < ActiveRecord::Base select_values_sql("select post_id from post_votes where score > 0 and user_id = ?", user_id) end - def score=(x) - if x == "up" - Post.where(:id => post_id).update_all("score = score + #{magnitude}, up_score = up_score + #{magnitude}") - write_attribute(:score, magnitude) - elsif x == "down" - Post.where(:id => post_id).update_all("score = score - #{magnitude}, down_score = down_score - #{magnitude}") - write_attribute(:score, -magnitude) + def initialize_attributes + self.user_id ||= CurrentUser.user.id + + if vote == "up" + self.score = magnitude + elsif vote == "down" + self.score = -magnitude end end - def initialize_user - self.user_id ||= CurrentUser.user.id + def update_post_on_create + if score > 0 + Post.where(:id => post_id).update_all("score = score + #{score}, up_score = up_score + #{score}") + else + Post.where(:id => post_id).update_all("score = score + #{score}, down_score = down_score + #{score}") + end end def update_post_on_destroy @@ -47,7 +55,7 @@ class PostVote < ActiveRecord::Base end def magnitude - if CurrentUser.is_super_voter? + if user.is_super_voter? SuperVoter::MAGNITUDE else 1 diff --git a/test/unit/post_vote_test.rb b/test/unit/post_vote_test.rb index 4b35fa200..27e820225 100644 --- a/test/unit/post_vote_test.rb +++ b/test/unit/post_vote_test.rb @@ -14,17 +14,17 @@ class PostVoteTest < ActiveSupport::TestCase context "Voting for a post" do should "interpret up as +1 score" do - vote = PostVote.create(:post_id => @post.id, :score => "up") + vote = PostVote.create(:post_id => @post.id, :vote => "up") assert_equal(1, vote.score) end should "interpret down as -1 score" do - vote = PostVote.create(:post_id => @post.id, :score => "down") + vote = PostVote.create(:post_id => @post.id, :vote => "down") assert_equal(-1, vote.score) end should "not accept any other scores" do - vote = PostVote.create(:post_id => @post.id, :score => "xxx") + vote = PostVote.create(:post_id => @post.id, :vote => "xxx") assert(vote.errors.any?) end end From a5ecda2f0ecdbbd0498935e005f6670015e65604 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 24 Mar 2017 00:30:27 -0500 Subject: [PATCH 5/7] post_test.rb: fix voting tests. --- test/unit/post_test.rb | 41 +++++++++-------------------------------- 1 file changed, 9 insertions(+), 32 deletions(-) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 63a5fa1c5..f7bf213bf 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -1420,45 +1420,34 @@ class PostTest < ActiveSupport::TestCase context "Removing a post from a user's favorites" do setup do @user = FactoryGirl.create(:contributor_user) - CurrentUser.user = @user - CurrentUser.ip_addr = "127.0.0.1" @post = FactoryGirl.create(:post) @post.add_favorite!(@user) @user.reload end - teardown do - CurrentUser.user = nil - CurrentUser.ip_addr = nil - end - should "decrement the user's favorite_count" do assert_difference("@user.favorite_count", -1) do @post.remove_favorite!(@user) - @user.reload end end should "decrement the post's score for gold users" do - @post.remove_favorite!(@user) - @post.reload - assert_equal(0, @post.score) + assert_difference("@post.score", -1) do + @post.remove_favorite!(@user) + end end should "not decrement the post's score for basic users" do @member = FactoryGirl.create(:user) - CurrentUser.scoped(@member, "127.0.0.1") do - @post.remove_favorite!(@member) - end - @post.reload - assert_equal(1, @post.score) + + assert_no_difference("@post.score") { @post.add_favorite!(@member) } + assert_no_difference("@post.score") { @post.remove_favorite!(@member) } end should "not decrement the user's favorite_count if the user did not favorite the post" do @post2 = FactoryGirl.create(:post) - assert_difference("@user.favorite_count", 0) do + assert_no_difference("@user.favorite_count") do @post2.remove_favorite!(@user) - @user.reload end end end @@ -1466,16 +1455,9 @@ class PostTest < ActiveSupport::TestCase context "Adding a post to a user's favorites" do setup do @user = FactoryGirl.create(:contributor_user) - CurrentUser.user = @user - CurrentUser.ip_addr = "127.0.0.1" @post = FactoryGirl.create(:post) end - teardown do - CurrentUser.user = nil - CurrentUser.ip_addr = nil - end - should "periodically clean the fav_string" do @post.update_column(:fav_string, "fav:1 fav:1 fav:1") @post.update_column(:fav_count, 3) @@ -1486,24 +1468,19 @@ class PostTest < ActiveSupport::TestCase end should "increment the user's favorite_count" do - assert_difference("CurrentUser.favorite_count", 1) do + assert_difference("@user.favorite_count", 1) do @post.add_favorite!(@user) - CurrentUser.reload end end should "increment the post's score for gold users" do @post.add_favorite!(@user) - @post.reload assert_equal(1, @post.score) end should "not increment the post's score for basic users" do @member = FactoryGirl.create(:user) - CurrentUser.scoped(@member, "127.0.0.1") do - @post.add_favorite!(@member) - end - @post.reload + @post.add_favorite!(@member) assert_equal(0, @post.score) end From 4470828a3dfb5e9ff1165a46b9f2c4e3a6f08097 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 24 Mar 2017 01:39:24 -0500 Subject: [PATCH 6/7] post_test.rb: add tests for moving favorites. --- test/unit/post_test.rb | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index f7bf213bf..151afe9a2 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -1506,6 +1506,42 @@ class PostTest < ActiveSupport::TestCase assert(!Favorite.exists?(:user_id => @user.id, :post_id => @post.id)) end end + + context "Moving favorites to a parent post" do + setup do + @parent = FactoryGirl.create(:post) + @child = FactoryGirl.create(:post, parent: @parent) + + @user1 = FactoryGirl.create(:user) + @gold1 = FactoryGirl.create(:gold_user) + @supervoter1 = FactoryGirl.create(:user, is_super_voter: true) + + @child.add_favorite!(@user1) + @child.add_favorite!(@gold1) + @child.add_favorite!(@supervoter1) + @parent.add_favorite!(@supervoter1) + + @child.give_favorites_to_parent + @child.reload + @parent.reload + end + + should "move the favorites" do + assert_equal(0, @child.fav_count) + assert_equal(0, @child.favorites.count) + assert_equal("", @child.fav_string) + assert_equal([], @child.favorites.pluck(:user_id)) + + assert_equal(3, @parent.fav_count) + assert_equal(3, @parent.favorites.count) + end + + should "create a vote for each user who can vote" do + assert(@parent.votes.where(user: @gold1).exists?) + assert(@parent.votes.where(user: @supervoter1).exists?) + assert_equal(4, @parent.score) + end + end end context "Pools:" do From 163d338874c9722f306db7ae702cfbee53cd99f0 Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 24 Mar 2017 15:26:41 -0500 Subject: [PATCH 7/7] post_vote_test.rb: add more voting tests. --- test/unit/post_vote_test.rb | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/test/unit/post_vote_test.rb b/test/unit/post_vote_test.rb index 27e820225..1280c77d9 100644 --- a/test/unit/post_vote_test.rb +++ b/test/unit/post_vote_test.rb @@ -4,8 +4,9 @@ class PostVoteTest < ActiveSupport::TestCase def setup super - user = FactoryGirl.create(:user) - CurrentUser.user = user + @supervoter = FactoryGirl.create(:user, is_super_voter: true) + @user = FactoryGirl.create(:user) + CurrentUser.user = @user CurrentUser.ip_addr = "127.0.0.1" MEMCACHE.flush_all @@ -23,9 +24,38 @@ class PostVoteTest < ActiveSupport::TestCase assert_equal(-1, vote.score) end + should "interpret up as +3 score for a supervoter" do + vote = PostVote.create(:user_id => @supervoter.id, :post_id => @post.id, :vote => "up") + assert_equal(3, vote.score) + end + should "not accept any other scores" do vote = PostVote.create(:post_id => @post.id, :vote => "xxx") assert(vote.errors.any?) end + + should "increase the score of the post" do + @post.votes.create(vote: "up") + @post.reload + + assert_equal(1, @post.score) + assert_equal(1, @post.up_score) + end + + should "decrease the score of the post when removed" do + @post.votes.create(vote: "up").destroy + @post.reload + + assert_equal(0, @post.score) + assert_equal(0, @post.up_score) + end + + should "upvote by 3 points if the voter is a supervoter" do + @post.votes.create(user: @supervoter, vote: "up") + @post.reload + + assert_equal(3, @post.score) + assert_equal(3, @post.up_score) + end end end