discourse
/
discourse-topic-voting
mirror of https://github.com/discourse/discourse-topic-voting.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: improve reconciliation of votes job 

Also cleans up all custom field usage and tests

Adds a few missing tests
This commit is contained in:
Sam 2018-09-21 11:49:41 +10:00
parent 5ed0ef3412
commit 25c5f2d5ea
7 changed files with 273 additions and 172 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
app/controllers/discourse_voting/votes_controller.rb
View File

@ -10,8 +10,9 @@ module DiscourseVoting
render json: MultiJson.dump(who_voted(topic))
end
def add
topic = Topic.find_by(id: params["topic_id"])
def vote
topic_id = params["topic_id"].to_i
topic = Topic.find_by(id: topic_id)
raise Discourse::InvalidAccess if !topic.can_vote? || topic.user_voted(current_user)
guardian.ensure_can_see!(topic)
@ -20,11 +21,10 @@ module DiscourseVoting
unless current_user.reached_voting_limit?
current_user.custom_fields[DiscourseVoting::VOTES] = current_user.votes.dup.push(params["topic_id"]).uniq
current_user.custom_fields[DiscourseVoting::VOTES] = current_user.votes.dup.push(topic_id).uniq
current_user.save!
topic.update_vote_count
voted = true
end
@ -40,12 +40,13 @@ module DiscourseVoting
render json: obj, status: voted ? 200 : 403
end
def remove
topic = Topic.find_by(id: params["topic_id"])
def unvote
topic_id = params["topic_id"].to_i
topic = Topic.find_by(id: topic_id)
guardian.ensure_can_see!(topic)
current_user.custom_fields[DiscourseVoting::VOTES] = current_user.votes.dup - [params["topic_id"].to_s]
current_user.custom_fields[DiscourseVoting::VOTES] = current_user.votes.dup - [topic_id]
current_user.save!
topic.update_vote_count

81
app/jobs/onceoff/clean_dup_null_votes.rb
View File

@ -1,81 +0,0 @@
module Jobs
class CleanDupNullVotes < Jobs::Onceoff
def execute_onceoff(args)
# archive votes to closed or archived topics
DB.exec("
UPDATE user_custom_fields ucf
SET name = '#{DiscourseVoting::VOTES_ARCHIVE}'
FROM topics t
WHERE ucf.name = '#{DiscourseVoting::VOTES}'
AND (t.closed OR t.archived)
AND t.id::text = ucf.value
")
# un-archive votes to open topics
DB.exec("
UPDATE user_custom_fields ucf
SET name = '#{DiscourseVoting::VOTES}'
FROM topics t
WHERE ucf.name = '#{DiscourseVoting::VOTES_ARCHIVE}'
AND NOT t.closed
AND NOT t.archived
AND t.id::text = ucf.value
")
# delete duplicate votes
DB.exec("
DELETE FROM user_custom_fields ucf1
USING user_custom_fields ucf2
WHERE ucf1.id < ucf2.id AND
ucf1.user_id = ucf2.user_id AND
ucf1.value = ucf2.value AND
ucf1.name = ucf2.name AND
(ucf1.name IN ('#{DiscourseVoting::VOTES}', '#{DiscourseVoting::VOTES_ARCHIVE}'))
")
# delete votes associated with no topics
DB.exec("
DELETE FROM user_custom_fields ucf
WHERE ucf.value IS NULL
AND ucf.name IN ('#{DiscourseVoting::VOTES}', '#{DiscourseVoting::VOTES_ARCHIVE}')
")
# delete duplicate vote counts for topics
DB.exec("
DELETE FROM topic_custom_fields tcf
USING topic_custom_fields tcf2
WHERE tcf.id < tcf2.id AND
tcf.name = tcf2.name AND
tcf.topic_id = tcf2.topic_id AND
tcf.value = tcf.value AND
tcf.name = '#{DiscourseVoting::VOTE_COUNT}'
")
# insert missing vote counts for topics
DB.exec("
WITH missing_ids AS (
SELECT t.id FROM topics t
JOIN user_custom_fields ucf
ON t.id::text = ucf.value
LEFT JOIN topic_custom_fields tcf
ON t.id = tcf.topic_id
WHERE ucf.name IN ('#{DiscourseVoting::VOTES}', '#{DiscourseVoting::VOTES_ARCHIVE}') AND
tcf.topic_id IS NULL OR tcf.name <> '#{DiscourseVoting::VOTE_COUNT}'
)
INSERT INTO topic_custom_fields (value, topic_id, name, created_at, updated_at)
SELECT '0', id, '#{DiscourseVoting::VOTE_COUNT}', now(), now() FROM missing_ids
")
# correct topics vote counts
DB.exec("
UPDATE topic_custom_fields tcf
SET value = (
SELECT COUNT(*) FROM user_custom_fields ucf
WHERE tcf.topic_id::text = ucf.value AND ucf.name IN ('#{DiscourseVoting::VOTES}', '#{DiscourseVoting::VOTES_ARCHIVE}')
GROUP BY ucf.value
)
WHERE tcf.name = '#{DiscourseVoting::VOTE_COUNT}'
")
end
end
end

102
app/jobs/onceoff/voting_ensure_consistency.rb Normal file
View File

@ -0,0 +1,102 @@
module Jobs
class VotingEnsureConsistency < Jobs::Onceoff
def execute_onceoff(args)
aliases = {
vote_count: DiscourseVoting::VOTE_COUNT,
votes: DiscourseVoting::VOTES,
votes_archive: DiscourseVoting::VOTES_ARCHIVE,
}
# archive votes to closed or archived or deleted topics
DB.exec(<<~SQL, aliases)
UPDATE user_custom_fields ucf
SET name = :votes_archive
FROM topics t
WHERE ucf.name = :votes
AND (t.closed OR t.archived OR t.deleted_at IS NOT NULL)
AND t.id::text = ucf.value
SQL
# un-archive votes to open topics
DB.exec(<<~SQL, aliases)
UPDATE user_custom_fields ucf
SET name = :votes
FROM topics t
WHERE ucf.name = :votes_archive
AND NOT t.closed
AND NOT t.archived
AND t.deleted_at IS NULL
AND t.id::text = ucf.value
SQL
# delete duplicate votes
DB.exec(<<~SQL, aliases)
DELETE FROM user_custom_fields ucf1
USING user_custom_fields ucf2
WHERE ucf1.id < ucf2.id AND
ucf1.user_id = ucf2.user_id AND
ucf1.value = ucf2.value AND
ucf1.name = ucf2.name AND
(ucf1.name IN (:votes, :votes_archive))
SQL
# delete votes associated with no topics
DB.exec(<<~SQL, aliases)
DELETE FROM user_custom_fields ucf
WHERE ucf.value IS NULL
AND ucf.name IN (:votes, :votes_archive)
SQL
# delete duplicate vote counts for topics
DB.exec(<<~SQL, aliases)
DELETE FROM topic_custom_fields tcf
USING topic_custom_fields tcf2
WHERE tcf.id < tcf2.id AND
tcf.name = tcf2.name AND
tcf.topic_id = tcf2.topic_id AND
tcf.value = tcf.value AND
tcf.name = :vote_count
SQL
# insert missing vote counts for topics
# ensures we have "something" for every topic with votes
DB.exec(<<~SQL, aliases)
WITH missing_ids AS (
SELECT DISTINCT t.id FROM topics t
JOIN user_custom_fields ucf ON t.id::text = ucf.value AND
ucf.name IN (:votes, :votes_archive)
LEFT JOIN topic_custom_fields tcf ON t.id = tcf.topic_id
AND tcf.name = :vote_count
WHERE tcf.topic_id IS NULL
)
INSERT INTO topic_custom_fields (value, topic_id, name, created_at, updated_at)
SELECT '0', id, :vote_count, now(), now() FROM missing_ids
SQL
# remove all superflous vote count custom fields
DB.exec(<<~SQL, aliases)
DELETE FROM topic_custom_fields
WHERE name = :vote_count
AND topic_id IN (
SELECT t1.id FROM topics t1
LEFT JOIN user_custom_fields ucf
ON ucf.value = t1.id::text AND
ucf.name IN (:votes, :votes_archive)
WHERE ucf.id IS NULL
)
SQL
# correct topics vote counts
DB.exec(<<~SQL, aliases)
UPDATE topic_custom_fields tcf
SET value = (
SELECT COUNT(*) FROM user_custom_fields ucf
WHERE tcf.topic_id::text = ucf.value AND
ucf.name IN (:votes, :votes_archive)
GROUP BY ucf.value
)
WHERE tcf.name = :vote_count
SQL
end
end
end

152
plugin.rb
View File

@ -26,7 +26,11 @@ after_initialize do
end
end
load File.expand_path('../app/jobs/onceoff/clean_dup_null_votes.rb', __FILE__)
User.register_custom_field_type(::DiscourseVoting::VOTES, [:integer])
User.register_custom_field_type(::DiscourseVoting::VOTES_ARCHIVE, [:integer])
Topic.register_custom_field_type(::DiscourseVoting::VOTE_COUNT, :integer)
load File.expand_path('../app/jobs/onceoff/voting_ensure_consistency.rb', __FILE__)
require_dependency 'basic_category_serializer'
class ::BasicCategorySerializer
@ -84,69 +88,66 @@ after_initialize do
}
class ::Category
def self.reset_voting_cache
@allowed_voting_cache["allowed"] =
begin
Set.new(
CategoryCustomField
.where(name: "enable_topic_voting", value: "true")
.pluck(:category_id)
)
end
end
@allowed_voting_cache = DistributedCache.new("allowed_voting")
def self.can_vote?(category_id)
return false unless SiteSetting.voting_enabled
unless set = @allowed_voting_cache["allowed"]
set = reset_voting_cache
def self.reset_voting_cache
@allowed_voting_cache["allowed"] =
begin
Set.new(
CategoryCustomField
.where(name: "enable_topic_voting", value: "true")
.pluck(:category_id)
)
end
set.include?(category_id)
end
@allowed_voting_cache = DistributedCache.new("allowed_voting")
def self.can_vote?(category_id)
return false unless SiteSetting.voting_enabled
unless set = @allowed_voting_cache["allowed"]
set = reset_voting_cache
end
set.include?(category_id)
end
after_save :reset_voting_cache
after_save :reset_voting_cache
protected
def reset_voting_cache
::Category.reset_voting_cache
end
protected
def reset_voting_cache
::Category.reset_voting_cache
end
end
require_dependency 'user'
class ::User
def vote_count
votes.length
end
def vote_count
votes.length
end
def alert_low_votes?
(vote_limit - vote_count) <= SiteSetting.voting_alert_votes_left
end
def alert_low_votes?
(vote_limit - vote_count) <= SiteSetting.voting_alert_votes_left
end
def votes
votes = self.custom_fields[DiscourseVoting::VOTES]
votes = [votes] unless votes.kind_of?(Array)
votes = votes.reject { |v| v.blank? }.uniq
votes
end
def votes
votes = self.custom_fields[DiscourseVoting::VOTES] || []
# "" can be in there sometimes, it gets turned into a 0
votes = votes.reject { |v| v == 0 }.uniq
votes
end
def votes_archive
archived_votes = self.custom_fields[DiscourseVoting::VOTES_ARCHIVE]
archived_votes = [archived_votes] unless archived_votes.kind_of?(Array)
archived_votes = archived_votes.reject { |v| v.blank? }.uniq
archived_votes
end
def votes_archive
archived_votes = self.custom_fields[DiscourseVoting::VOTES_ARCHIVE] || []
archived_votes = archived_votes.reject { |v| v == 0 }.uniq
archived_votes
end
def reached_voting_limit?
vote_count >= vote_limit
end
def reached_voting_limit?
vote_count >= vote_limit
end
def vote_limit
SiteSetting.send("voting_tl#{self.trust_level}_vote_limit")
end
def vote_limit
SiteSetting.send("voting_tl#{self.trust_level}_vote_limit")
end
end
@ -183,15 +184,18 @@ after_initialize do
def user_voted(user)
if user && user.custom_fields[DiscourseVoting::VOTES]
user.custom_fields[DiscourseVoting::VOTES].include? self.id.to_s
user.custom_fields[DiscourseVoting::VOTES].include? self.id
else
false
end
end
def update_vote_count
custom_fields[DiscourseVoting::VOTE_COUNT] = UserCustomField.where("value = :value AND name IN (:keys)", value: id.to_s, keys: [DiscourseVoting::VOTES, DiscourseVoting::VOTES_ARCHIVE]).count
count =
UserCustomField.where("value = :value AND name IN (:keys)",
value: id.to_s, keys: [DiscourseVoting::VOTES, DiscourseVoting::VOTES_ARCHIVE]).count
custom_fields[DiscourseVoting::VOTE_COUNT] = count
save!
end
@ -233,7 +237,7 @@ after_initialize do
def list_votes
create_list(:votes, unordered: true) do |topics|
topics.joins("left join topic_custom_fields tfv ON tfv.topic_id = topics.id AND tfv.name = '#{DiscourseVoting::VOTE_COUNT}'")
.order("coalesce(tfv.value,'0')::integer desc, topics.bumped_at desc")
.order("coalesce(tfv.value,'0')::integer desc, topics.bumped_at desc")
end
end
end
@ -246,7 +250,7 @@ after_initialize do
if topic = Topic.find_by(id: args[:topic_id])
UserCustomField.where(name: DiscourseVoting::VOTES, value: args[:topic_id]).find_each do |user_field|
user = User.find(user_field.user_id)
user.custom_fields[DiscourseVoting::VOTES] = user.votes.dup - [args[:topic_id].to_s]
user.custom_fields[DiscourseVoting::VOTES] = user.votes.dup - [args[:topic_id]]
user.custom_fields[DiscourseVoting::VOTES_ARCHIVE] = user.votes_archive.dup.push(args[:topic_id]).uniq
user.save!
end
@ -258,10 +262,10 @@ after_initialize do
class VoteReclaim < Jobs::Base
def execute(args)
if topic = Topic.find_by(id: args[:topic_id])
UserCustomField.where(name: DiscourseVoting::VOTES_ARCHIVE, value: args[:topic_id]).find_each do |user_field|
UserCustomField.where(name: DiscourseVoting::VOTES_ARCHIVE, value: topic.id).find_each do |user_field|
user = User.find(user_field.user_id)
user.custom_fields[DiscourseVoting::VOTES] = user.votes.dup.push(args[:topic_id]).uniq
user.custom_fields[DiscourseVoting::VOTES_ARCHIVE] = user.votes_archive.dup - [args[:topic_id].to_s]
user.custom_fields[DiscourseVoting::VOTES] = user.votes.dup.push(topic.id).uniq
user.custom_fields[DiscourseVoting::VOTES_ARCHIVE] = user.votes_archive.dup - [topic.id]
user.save!
end
topic.update_vote_count
@ -273,27 +277,27 @@ after_initialize do
DiscourseEvent.on(:topic_status_updated) do |topic, status, enabled|
if (status == 'closed' || status == 'autoclosed' || status == 'archived') && enabled == true
Jobs.enqueue(:vote_release, { topic_id: topic.id })
Jobs.enqueue(:vote_release, topic_id: topic.id)
end
if (status == 'closed' || status == 'autoclosed' || status == 'archived') && enabled == false
Jobs.enqueue(:vote_reclaim, { topic_id: topic.id })
Jobs.enqueue(:vote_reclaim, topic_id: topic.id)
end
end
DiscourseEvent.on(:post_edited) do |post, topic_changed|
if topic_changed &&
SiteSetting.voting_enabled &&
UserCustomField.where(
"value = :value AND name in (:keys)",
value: post.topic_id.to_s,
keys: [DiscourseVoting::VOTES, DiscourseVoting::VOTES_ARCHIVE]
).exists?
SiteSetting.voting_enabled &&
UserCustomField.where(
"value = :value AND name in (:keys)",
value: post.topic_id.to_s,
keys: [DiscourseVoting::VOTES, DiscourseVoting::VOTES_ARCHIVE]
).exists?
new_category_id = post.reload.topic.category_id
if Category.can_vote?(new_category_id)
Jobs.enqueue(:vote_reclaim, { topic_id: post.topic_id })
Jobs.enqueue(:vote_reclaim, topic_id: post.topic_id)
else
Jobs.enqueue(:vote_release, { topic_id: post.topic_id })
Jobs.enqueue(:vote_release, topic_id: post.topic_id)
end
end
end
@ -302,13 +306,13 @@ after_initialize do
if orig.who_voted.present?
orig.who_voted.each do |user|
if user.votes.include?(dest.id.to_s)
if user.votes.include?(dest.id)
# User has voted for both +orig+ and +dest+.
# Remove vote for topic +orig+.
user.custom_fields[DiscourseVoting::VOTES] = user.votes.dup - [orig.id.to_s]
user.custom_fields[DiscourseVoting::VOTES] = user.votes.dup - [orig.id]
else
# Change the vote for +orig+ in a vote for +dest+.
user.custom_fields[DiscourseVoting::VOTES] = user.votes.map { |vote| vote == orig.id.to_s ? dest.id.to_s : vote }
user.custom_fields[DiscourseVoting::VOTES] = user.votes.map { |vote| vote == orig.id ? dest.id : vote }
end
user.save!
@ -322,8 +326,8 @@ after_initialize do
require File.expand_path(File.dirname(__FILE__) + '/app/controllers/discourse_voting/votes_controller')
DiscourseVoting::Engine.routes.draw do
post 'vote' => 'votes#add'
post 'unvote' => 'votes#remove'
post 'vote' => 'votes#vote'
post 'unvote' => 'votes#unvote'
get 'who' => 'votes#who'
end
@ -331,7 +335,7 @@ after_initialize do
mount ::DiscourseVoting::Engine, at: "/voting"
# USERNAME_ROUTE_FORMAT is deprecated but we may need to support it for older installs
username_route_format = defined?(RouteFormat) ? RouteFormat.username : USERNAME_ROUTE_FORMAT
get "topics/voted-by/:username" => "list#voted_by", as: "voted_by", constraints: {username: username_route_format}
get "topics/voted-by/:username" => "list#voted_by", as: "voted_by", constraints: { username: username_route_format }
end
TopicList.preloaded_custom_fields << DiscourseVoting::VOTE_COUNT if TopicList.respond_to? :preloaded_custom_fields

48
spec/ensure_consistency_spec.rb Normal file
View File

@ -0,0 +1,48 @@
require 'rails_helper'
describe "Voting Consistency" do
it "cleans up mess" do
user = Fabricate(:user)
user2 = Fabricate(:user)
no_vote_topic = Fabricate(:topic)
no_vote_topic.custom_fields[DiscourseVoting::VOTE_COUNT] = "10"
no_vote_topic.custom_fields["random1"] = "random"
no_vote_topic.save_custom_fields
one_vote_topic = Fabricate(:topic)
one_vote_topic.custom_fields[DiscourseVoting::VOTE_COUNT] = "10"
one_vote_topic.custom_fields["random2"] = "random"
one_vote_topic.save_custom_fields
two_vote_topic = Fabricate(:topic)
two_vote_topic.custom_fields["random3"] = "random"
two_vote_topic.save_custom_fields
# one vote
UserCustomField.create!(user_id: user.id, name: DiscourseVoting::VOTES_ARCHIVE, value: one_vote_topic.id)
# two votes
UserCustomField.create!(user_id: user.id, name: DiscourseVoting::VOTES_ARCHIVE, value: two_vote_topic.id)
UserCustomField.create!(user_id: user2.id, name: DiscourseVoting::VOTES, value: two_vote_topic.id)
Jobs::VotingEnsureConsistency.new.execute_onceoff(nil)
no_vote_topic.reload
expect(no_vote_topic.custom_fields).to eq("random1" => "random")
user.reload
expect(user.custom_fields).to eq("votes" => [one_vote_topic.id, two_vote_topic.id])
user2.reload
expect(user2.custom_fields).to eq("votes" => [two_vote_topic.id])
one_vote_topic.reload
expect(one_vote_topic.custom_fields).to eq("vote_count" => 1, "random2" => "random")
two_vote_topic.reload
expect(two_vote_topic.custom_fields).to eq("vote_count" => 2, "random3" => "random")
end
end

24
spec/requests/votes_controller_spec.rb
View File

@ -14,7 +14,16 @@ describe DiscourseVoting::VotesController do
sign_in(user)
end
it "doesn't allow users to vote more than once on a topic" do
it "does not allow voting if voting is not enabled" do
SiteSetting.voting_enabled = false
post "/voting/vote.json", params: { topic_id: topic.id }
expect(response.status).to eq(403)
end
it "can correctly show deal with voting workflow" do
SiteSetting.send "voting_tl#{user.trust_level}_vote_limit=", 2
post "/voting/vote.json", params: { topic_id: topic.id }
expect(response.status).to eq(200)
@ -22,6 +31,19 @@ describe DiscourseVoting::VotesController do
expect(response.status).to eq(403)
expect(topic.reload.vote_count).to eq(1)
expect(user.reload.vote_count).to eq(1)
get "/voting/who.json", params: { topic_id: topic.id }
expect(response.status).to eq(200)
json = JSON.parse(response.body)
expect(json.length).to eq(1)
expect(json.first.keys.sort).to eq(["avatar_template", "id", "name", "username"])
expect(json.first["id"]).to eq(user.id)
post "/voting/unvote.json", params: { topic_id: topic.id }
expect(response.status).to eq(200)
expect(topic.reload.vote_count).to eq(0)
expect(user.reload.vote_count).to eq(0)
end
context "when a user has tallyed votes with no topic id" do

23
spec/voting_spec.rb
View File

@ -39,7 +39,9 @@ describe DiscourseVoting do
users[1].custom_fields[DiscourseVoting::VOTES] = users[1].votes.dup.push(topic1.id.to_s)
users[2].custom_fields[DiscourseVoting::VOTES] = users[2].votes.dup.push(topic0.id.to_s)
users[2].custom_fields[DiscourseVoting::VOTES] = users[2].votes.dup.push(topic1.id.to_s)
users.each { |u| u.save }
users.each { |u| u.save! }
[topic0, topic1].each { |t| t.update_vote_count }
# Simulating merger of +topic0+ into +topic1+.
@ -48,9 +50,9 @@ describe DiscourseVoting do
# Force user refresh.
users.each(&:reload)
expect(users[0].votes).to eq([topic1.id.to_s])
expect(users[1].votes).to eq([topic1.id.to_s])
expect(users[2].votes).to eq([topic1.id.to_s])
expect(users[0].votes).to eq([topic1.id])
expect(users[1].votes).to eq([topic1.id])
expect(users[2].votes).to eq([topic1.id])
expect(users[3].votes).to eq([])
expect(topic0.vote_count).to eq(0)
@ -60,11 +62,14 @@ describe DiscourseVoting do
context "when a user has an empty string as the votes custom field" do
before do
user0.custom_fields[DiscourseVoting::VOTES] = ""
user0.custom_fields[DiscourseVoting::VOTES_ARCHIVE] = ""
user0.save
user0.reload
end
it "returns a vote count of zero" do
expect(user0.vote_count).to eq (0)
expect(user0.votes_archive).to eq ([])
end
end
@ -86,7 +91,7 @@ describe DiscourseVoting do
let(:admin) { Fabricate(:admin) }
let(:post0) { Fabricate(:post, topic: topic0, post_number: 1) }
let(:post1) { Fabricate(:post, topic: topic1, post_number: 1) }
before do
category1.custom_fields["enable_topic_voting"] = "true"
category1.save!
@ -104,8 +109,8 @@ describe DiscourseVoting do
Jobs::VoteReclaim.new.execute(topic_id: post1.topic_id)
user.reload
expect(user.votes).to contain_exactly(post1.topic_id.to_s)
expect(user.votes_archive).to contain_exactly("456456")
expect(user.votes).to eq([post1.topic_id])
expect(user.votes_archive).to eq([456456])
end
it "enqueus a job to release votes if voting is disabled for the new category" do
@ -119,8 +124,8 @@ describe DiscourseVoting do
Jobs::VoteRelease.new.execute(topic_id: post0.topic_id)
user.reload
expect(user.votes_archive).to contain_exactly(post0.topic_id.to_s)
expect(user.votes).to contain_exactly("456456")
expect(user.votes_archive).to eq([post0.topic_id])
expect(user.votes).to eq([456456])
end
it "doesn't enqueue a job if the topic has no votes" do