FIX: Stop trying to merge duplicate votes (#56)

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.

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)
-            duplicated_votes += 1
-            user.votes.destroy_by(topic_id: orig.id, archive: false)
-          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)
+        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, user_id: user.id, archive: true)
+            user.votes.destroy_by(topic_id: orig.id)
           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)

GitHub sha: c82d5a66

1 Like

This commit appears in #56 which was approved by eviltrout. It was merged by justindirose.