From c82d5a6645534a160dc32220b27a6f5d23a7af1c Mon Sep 17 00:00:00 2001 From: Justin DiRose Date: Thu, 17 Sep 2020 11:02:19 -0500 Subject: [PATCH] FIX: Stop trying to merge duplicate votes (#56) Was able to reproduce a bug where votes were not properly moved when merging two topics together. We were only checking duplicate votes if either both votes were either active or archived, and not a mix of the two. As a result, there were cases where the plugin was trying to move votes that already existed on an open topic, causing the merge to partially fail with a duplicate index error. What this commit does is as follows: - Cleaned up the logic so it's more readable. Previously it was duplicative and difficult to read. - We're now checking if a user has a vote on Topic A (active or archived) and Topic B (active or archived) that we're properly deleting the origin vote and keeping the destination vote instead of trying to merge in a duplicate. (This caused the original bug described above). - Per the specs on meta, topic merges move all votes to the destination topic, and if this causes a user to go over the limit, they'll have to wait until enough votes are released. - If the destination topic is closed, the votes will be archived; if the destination topic is open, those votes will be active. This is regardless of origin vote state. Also, the Gemfile was missing a source declaration to allow installation of gems/dependencies, so I've added that. --- Gemfile | 2 ++ Gemfile.lock | 1 + plugin.rb | 19 +++++++------------ spec/voting_spec.rb | 35 +++++++++++++++++++++-------------- 4 files changed, 31 insertions(+), 26 deletions(-) diff --git a/Gemfile b/Gemfile index fc48edf..7da32ec 100644 --- a/Gemfile +++ b/Gemfile @@ -1,5 +1,7 @@ # frozen_string_literal: true +source 'https://rubygems.org' + group :development do gem 'rubocop-discourse' end diff --git a/Gemfile.lock b/Gemfile.lock index 25eeb8a..20f57a5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,4 +1,5 @@ GEM + remote: https://rubygems.org/ specs: ast (2.4.0) jaro_winkler (1.5.4) diff --git a/plugin.rb b/plugin.rb index c1dc7ef..b2fdeb8 100755 --- a/plugin.rb +++ b/plugin.rb @@ -301,20 +301,15 @@ after_initialize do if orig.who_voted.present? && orig.closed orig.who_voted.each do |user| - if user.topics_with_vote.pluck(:topic_id).include?(orig.id) - if user.topics_with_vote.pluck(:topic_id).include?(dest.id) + user_votes = user.topics_with_vote.pluck(:topic_id) + user_archived_votes = user.topics_with_archived_vote.pluck(:topic_id) + + if user_votes.include?(orig.id) || user_archived_votes.include?(orig.id) + if user_votes.include?(dest.id) || user_archived_votes.include?(dest.id) duplicated_votes += 1 - user.votes.destroy_by(topic_id: orig.id, archive: false) + user.votes.destroy_by(topic_id: orig.id) else - user.votes.find_by(topic_id: orig.id, archive: false).update!(topic_id: dest.id) - moved_votes += 1 - end - elsif user.topics_with_archived_vote.pluck(:topic_id).include?(orig.id) - if user.topics_with_archived_vote.pluck(:topic_id).include?(dest.id) - duplicated_votes += 1 - user.votes.destroy_by(topic_id: orig.id, user_id: user.id, archive: true) - else - user.votes.find_by(topic_id: orig.id, user_id: user.id, archive: true).update!(topic_id: dest.id) + user.votes.find_by(topic_id: orig.id, user_id: user.id).update!(topic_id: dest.id, archive: dest.closed) moved_votes += 1 end else diff --git a/spec/voting_spec.rb b/spec/voting_spec.rb index eb906b7..08c9b69 100644 --- a/spec/voting_spec.rb +++ b/spec/voting_spec.rb @@ -48,7 +48,7 @@ describe DiscourseVoting do DiscourseVoting::Vote.create!(user: users[2], topic: topic1) DiscourseVoting::Vote.create!(user: users[4], topic: topic0, archive: true) DiscourseVoting::Vote.create!(user: users[5], topic: topic0, archive: true) - DiscourseVoting::Vote.create!(user: users[5], topic: topic1, archive: true) + DiscourseVoting::Vote.create!(user: users[5], topic: topic1) [topic0, topic1].each { |t| t.update_vote_count } end @@ -58,15 +58,22 @@ describe DiscourseVoting do users.each { |user| user.reload } expect(users[0].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic1.id) - expect(users[0].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() + expect(users[0].topics_with_archived_vote.pluck(:topic_id)).to be_blank + expect(users[1].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic1.id) - expect(users[1].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() + expect(users[1].topics_with_archived_vote.pluck(:topic_id)).to be_blank + expect(users[2].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic1.id) - expect(users[2].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() - expect(users[3].topics_with_vote.pluck(:topic_id)).to contain_exactly() - expect(users[3].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() - expect(users[4].topics_with_vote.pluck(:topic_id)).to contain_exactly() - expect(users[4].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly(topic1.id) + expect(users[2].topics_with_archived_vote.pluck(:topic_id)).to be_blank + + expect(users[3].topics_with_vote.pluck(:topic_id)).to be_blank + expect(users[3].topics_with_archived_vote.pluck(:topic_id)).to be_blank + + expect(users[4].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic1.id) + expect(users[4].topics_with_archived_vote.pluck(:topic_id)).to be_blank + + expect(users[5].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic1.id) + expect(users[5].topics_with_archived_vote.pluck(:topic_id)).to be_blank expect(topic0.reload.vote_count).to eq(0) expect(topic1.reload.vote_count).to eq(5) @@ -81,14 +88,14 @@ describe DiscourseVoting do users.each { |user| user.reload } expect(users[0].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic0.id) - expect(users[0].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() + expect(users[0].topics_with_archived_vote.pluck(:topic_id)).to be_blank expect(users[1].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic1.id) - expect(users[1].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() + expect(users[1].topics_with_archived_vote.pluck(:topic_id)).to be_blank expect(users[2].topics_with_vote.pluck(:topic_id)).to contain_exactly(topic0.id, topic1.id) - expect(users[2].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() - expect(users[3].topics_with_vote.pluck(:topic_id)).to contain_exactly() - expect(users[3].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly() - expect(users[4].topics_with_vote.pluck(:topic_id)).to contain_exactly() + expect(users[2].topics_with_archived_vote.pluck(:topic_id)).to be_blank + expect(users[3].topics_with_vote.pluck(:topic_id)).to be_blank + expect(users[3].topics_with_archived_vote.pluck(:topic_id)).to be_blank + expect(users[4].topics_with_vote.pluck(:topic_id)).to be_blank expect(users[4].topics_with_archived_vote.pluck(:topic_id)).to contain_exactly(topic0.id) expect(topic0.reload.vote_count).to eq(4)