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.
This commit is contained in:
Justin DiRose 2020-09-17 11:02:19 -05:00 committed by GitHub
parent 5e69dbfd5b
commit c82d5a6645
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 31 additions and 26 deletions

View File

@ -1,5 +1,7 @@
# frozen_string_literal: true
source 'https://rubygems.org'
group :development do
gem 'rubocop-discourse'
end

View File

@ -1,4 +1,5 @@
GEM
remote: https://rubygems.org/
specs:
ast (2.4.0)
jaro_winkler (1.5.4)

View File

@ -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

View File

@ -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)