From 1d0faedfbc3a8b77b971dc70d25e30791dbb6e0b Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 19 Nov 2021 09:50:08 +0800 Subject: [PATCH] FIX: Validate number of votes allowed per poll per user. (#15001) * DEV: Remove spec that we no longer need. As far as we know, the migration has been successful for a number of years. * FIX: Validate number of votes allowed per poll per user. --- plugins/poll/config/locales/server.en.yml | 7 + plugins/poll/lib/poll.rb | 44 ++- .../post_migrate/migrate_polls_data_spec.rb | 333 ------------------ .../spec/integration/poll_endpoints_spec.rb | 31 +- plugins/poll/spec/lib/poll_spec.rb | 131 +++++++ 5 files changed, 207 insertions(+), 339 deletions(-) delete mode 100644 plugins/poll/spec/db/post_migrate/migrate_polls_data_spec.rb create mode 100644 plugins/poll/spec/lib/poll_spec.rb diff --git a/plugins/poll/config/locales/server.en.yml b/plugins/poll/config/locales/server.en.yml index 58e121d5d3d..19bedc39033 100644 --- a/plugins/poll/config/locales/server.en.yml +++ b/plugins/poll/config/locales/server.en.yml @@ -47,6 +47,13 @@ en: topic_must_be_open_to_vote: "The topic must be open to vote." poll_must_be_open_to_vote: "Poll must be open to vote." + one_vote_per_user: "Only 1 vote is allowed for this poll." + max_vote_per_user: + one: Only %{count} vote is allowed for this poll. + other: A maximum of %{count} votes is allowed for this poll. + min_vote_per_user: + one: A minimum of %{count} vote is required for this poll. + other: A minimum of %{count} votes is required for this poll. topic_must_be_open_to_toggle_status: "The topic must be open to toggle status." only_staff_or_op_can_toggle_status: "Only a staff member or the original poster can toggle a poll status." diff --git a/plugins/poll/lib/poll.rb b/plugins/poll/lib/poll.rb index fba328e5f66..fceefc090b4 100644 --- a/plugins/poll/lib/poll.rb +++ b/plugins/poll/lib/poll.rb @@ -2,7 +2,10 @@ class DiscoursePoll::Poll def self.vote(user, post_id, poll_name, options) + poll_id = nil + serialized_poll = DiscoursePoll::Poll.change_vote(user, post_id, poll_name) do |poll| + poll_id = poll.id # remove options that aren't available in the poll available_options = poll.poll_options.map { |o| o.digest }.to_set options.select! { |o| available_options.include?(o) } @@ -13,6 +16,8 @@ class DiscoursePoll::Poll obj << option.id if options.include?(option.digest) end + self.validate_votes!(poll, new_option_ids) + old_option_ids = poll.poll_options.each_with_object([]) do |option, obj| if option.poll_votes.where(user_id: user.id).exists? obj << option.id @@ -31,6 +36,24 @@ class DiscoursePoll::Poll end end + # Ensure consistency here as we do not have a unique index to limit the + # number of votes per the poll's configuration. + DB.query(<<~SQL, poll_id: poll_id, user_id: user.id, offset: serialized_poll[:type] == "multiple" ? serialized_poll[:max] : 1) + DELETE FROM poll_votes + USING ( + SELECT + poll_id, + user_id + FROM poll_votes + WHERE poll_id = :poll_id + AND user_id = :user_id + ORDER BY created_at DESC + OFFSET :offset + ) to_delete_poll_votes + WHERE poll_votes.poll_id = to_delete_poll_votes.poll_id + AND poll_votes.user_id = to_delete_poll_votes.user_id + SQL + [serialized_poll, options] end @@ -288,7 +311,26 @@ class DiscoursePoll::Poll end end - private + def self.validate_votes!(poll, options) + num_of_options = options.length + + if poll.multiple? + if num_of_options < poll.min + raise DiscoursePoll::Error.new(I18n.t( + "poll.min_vote_per_user", + count: poll.min + )) + elsif num_of_options > poll.max + raise DiscoursePoll::Error.new(I18n.t( + "poll.max_vote_per_user", + count: poll.max + )) + end + elsif num_of_options > 1 + raise DiscoursePoll::Error.new(I18n.t("poll.one_vote_per_user")) + end + end + private_class_method :validate_votes! def self.change_vote(user, post_id, poll_name) Poll.transaction do diff --git a/plugins/poll/spec/db/post_migrate/migrate_polls_data_spec.rb b/plugins/poll/spec/db/post_migrate/migrate_polls_data_spec.rb deleted file mode 100644 index a22e819dd7f..00000000000 --- a/plugins/poll/spec/db/post_migrate/migrate_polls_data_spec.rb +++ /dev/null @@ -1,333 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' -require_relative '../../../db/migrate/20180820080623_migrate_polls_data' - -RSpec.describe MigratePollsData do - let!(:user) { Fabricate(:user, id: 1) } - let!(:user2) { Fabricate(:user, id: 2) } - let!(:user3) { Fabricate(:user, id: 3) } - let!(:user4) { Fabricate(:user, id: 4) } - let!(:user5) { Fabricate(:user, id: 5) } - let(:post) { Fabricate(:post, user: user) } - - describe 'for a number poll' do - before do - post.custom_fields = { - "polls" => { - "poll" => { - "options" => [ - { "id" => "4d8a15e3cc35750f016ce15a43937620", "html" => "1", "votes" => 0 }, - { "id" => "aa2393b424f2f395abb63bf785760a3b", "html" => "4", "votes" => 0 }, - { "id" => "9ab1070dec27185440cdabb4948a5e9a", "html" => "7", "votes" => 1 }, - { "id" => "46c01f638a50d86e020f47469733b8be", "html" => "10", "votes" => 0 }, - { "id" => "b4f15431e07443c372d521e4ed131abe", "html" => "13", "votes" => 0 }, - { "id" => "4e885ead68ff4456f102843df9fbbd7f", "html" => "16", "votes" => 0 }, - { "id" => "eb8661f072794ea57baa7827cd8ffc88", "html" => "19", "votes" => 0 } - ], - "voters" => 1, - "name" => "poll", - "status" => "open", - "type" => "number", - "min" => "1", - "max" => "20", - "step" => "3" - }, - }, - "polls-votes" => { - "1" => { - "poll" => [ - "9ab1070dec27185440cdabb4948a5e9a" - ] - } - } - } - - post.save_custom_fields - end - - it "should migrate the data correctly" do - expect do - silence_stdout { MigratePollsData.new.up } - end.to \ - change { Poll.count }.by(1) & - change { PollOption.count }.by(7) & - change { PollVote.count }.by(1) - - poll = Poll.find_by(name: "poll", post: post) - - expect(poll.close_at).to eq(nil) - - expect(poll.number?).to eq(true) - expect(poll.open?).to eq(true) - expect(poll.always?).to eq(true) - expect(poll.secret?).to eq(true) - - expect(poll.min).to eq(1) - expect(poll.max).to eq(20) - expect(poll.step).to eq(3) - - expect(PollOption.all.pluck(:digest, :html)).to eq([ - ["4d8a15e3cc35750f016ce15a43937620", "1"], - ["aa2393b424f2f395abb63bf785760a3b", "4"], - ["9ab1070dec27185440cdabb4948a5e9a", "7"], - ["46c01f638a50d86e020f47469733b8be", "10"], - ["b4f15431e07443c372d521e4ed131abe", "13"], - ["4e885ead68ff4456f102843df9fbbd7f", "16"], - ["eb8661f072794ea57baa7827cd8ffc88", "19"] - ]) - - poll_vote = PollVote.first - - expect(poll_vote.poll).to eq(poll) - expect(poll_vote.poll_option.html).to eq("7") - expect(poll_vote.user).to eq(user) - end - end - - describe 'for a multiple poll' do - before do - post.custom_fields = { - "polls-votes" => { - "1" => { - "testing" => [ - "b2c3e3668a886d09e97e38b8adde7d45", - "b2c3e3668a886d09e97e38b8adde7d45", - "28df49fa9e9c09d3a1eb8cfbcdcda7790" - ] - }, - "2" => { - "testing" => [ - "b2c3e3668a886d09e97e38b8adde7d45", - "d01af008ec373e948c0ab3ad61009f35", - ] - }, - }, - "polls" => { - "poll" => { - "options" => [ - { - "id" => "b2c3e3668a886d09e97e38b8adde7d45", - "html" => "Choice 1", - "votes" => 2, - "voter_ids" => [user.id, user2.id] - }, - { - "id" => "28df49fa9e9c09d3a1eb8cfbcdcda7790", - "html" => "Choice 2", - "votes" => 1, - "voter_ids" => [user.id] - }, - { - "id" => "d01af008ec373e948c0ab3ad61009f35", - "html" => "Choice 3", - "votes" => 1, - "voter_ids" => [user2.id] - }, - ], - "voters" => 4, - "name" => "testing", - "status" => "closed", - "type" => "multiple", - "public" => "true", - "min" => 1, - "max" => 2 - } - } - } - - post.save_custom_fields - end - - it 'should migrate the data correctly' do - expect do - silence_stdout { MigratePollsData.new.up } - end.to \ - change { Poll.count }.by(1) & - change { PollOption.count }.by(3) & - change { PollVote.count }.by(4) - - poll = Poll.last - - expect(poll.post_id).to eq(post.id) - expect(poll.name).to eq("testing") - expect(poll.close_at).to eq(nil) - - expect(poll.multiple?).to eq(true) - expect(poll.closed?).to eq(true) - expect(poll.always?).to eq(true) - expect(poll.everyone?).to eq(true) - - expect(poll.min).to eq(1) - expect(poll.max).to eq(2) - expect(poll.step).to eq(nil) - - poll_options = PollOption.all - - poll_option_1 = poll_options[0] - expect(poll_option_1.poll_id).to eq(poll.id) - expect(poll_option_1.digest).to eq("b2c3e3668a886d09e97e38b8adde7d45") - expect(poll_option_1.html).to eq("Choice 1") - - poll_option_2 = poll_options[1] - expect(poll_option_2.poll_id).to eq(poll.id) - expect(poll_option_2.digest).to eq("28df49fa9e9c09d3a1eb8cfbcdcda7790") - expect(poll_option_2.html).to eq("Choice 2") - - poll_option_3 = poll_options[2] - expect(poll_option_3.poll_id).to eq(poll.id) - expect(poll_option_3.digest).to eq("d01af008ec373e948c0ab3ad61009f35") - expect(poll_option_3.html).to eq("Choice 3") - - expect(PollVote.all.pluck(:poll_id).uniq).to eq([poll.id]) - - { - user => [poll_option_1, poll_option_2], - user2 => [poll_option_1, poll_option_3] - }.each do |user, options| - options.each do |option| - expect(PollVote.exists?(poll_option_id: option.id, user_id: user.id)) - .to eq(true) - end - end - end - end - - describe 'for a regular poll' do - before do - post.custom_fields = { - "polls" => { - "testing" => { - "options" => [ - { - "id" => "e94c09aae2aa071610212a5c5042111b", - "html" => "Yes", - "votes" => 0, - "anonymous_votes" => 1, - "voter_ids" => [] - }, - { - "id" => "802c50392a68e426d4b26d81ddc5ab33", - "html" => "No", - "votes" => 0, - "anonymous_votes" => 2, - "voter_ids" => [] - } - ], - "voters" => 0, - "anonymous_voters" => 3, - "name" => "testing", - "status" => "open", - "type" => "regular" - }, - "poll" => { - "options" => [ - { - "id" => "edeee5dae4802ab24185d41039efb545", - "html" => "Yes", - "votes" => 2, - "voter_ids" => [1, 2] - }, - { - "id" => "38d8e35c8fc80590f836f22189064835", - "html" => - "No", - "votes" => 3, - "voter_ids" => [3, 4, 5] - } - ], - "voters" => 5, - "name" => "poll", - "status" => "open", - "type" => "regular", - "public" => "true", - "close" => "2018-10-08T00:00:00.000Z" - }, - }, - "polls-votes" => { - "1" => { "poll" => ["edeee5dae4802ab24185d41039efb545"] }, - "2" => { "poll" => ["edeee5dae4802ab24185d41039efb545"] }, - "3" => { "poll" => ["38d8e35c8fc80590f836f22189064835"] }, - "4" => { "poll" => ["38d8e35c8fc80590f836f22189064835"] }, - "5" => { "poll" => ["38d8e35c8fc80590f836f22189064835"] } - } - } - - post.save_custom_fields - end - - it 'should migrate the data correctly' do - expect do - silence_stdout { MigratePollsData.new.up } - end.to \ - change { Poll.count }.by(2) & - change { PollOption.count }.by(4) & - change { PollVote.count }.by(5) - - poll = Poll.find_by(name: "poll") - - expect(poll.post_id).to eq(post.id) - expect(poll.close_at).to eq_time(Time.parse("2018-10-08T00:00:00.000Z")) - - expect(poll.regular?).to eq(true) - expect(poll.open?).to eq(true) - expect(poll.always?).to eq(true) - expect(poll.everyone?).to eq(true) - - expect(poll.min).to eq(nil) - expect(poll.max).to eq(nil) - expect(poll.step).to eq(nil) - - poll_options = PollOption.where(poll_id: poll.id).to_a - expect(poll_options.size).to eq(2) - - option_1 = poll_options.first - expect(option_1.digest).to eq("edeee5dae4802ab24185d41039efb545") - expect(option_1.html).to eq("Yes") - - option_2 = poll_options.last - expect(option_2.digest).to eq("38d8e35c8fc80590f836f22189064835") - expect(option_2.html).to eq("No") - - expect(PollVote.pluck(:poll_id).uniq).to eq([poll.id]) - - [user, user2].each do |user| - expect(PollVote.exists?(poll_option_id: option_1.id, user_id: user.id)) - .to eq(true) - end - - [user3, user4, user5].each do |user| - expect(PollVote.exists?(poll_option_id: option_2.id, user_id: user.id)) - .to eq(true) - end - - poll = Poll.find_by(name: "testing") - - expect(poll.post_id).to eq(post.id) - expect(poll.close_at).to eq(nil) - expect(poll.anonymous_voters).to eq(3) - - expect(poll.regular?).to eq(true) - expect(poll.open?).to eq(true) - expect(poll.always?).to eq(true) - expect(poll.secret?).to eq(true) - - expect(poll.min).to eq(nil) - expect(poll.max).to eq(nil) - expect(poll.step).to eq(nil) - - poll_options = PollOption.where(poll: poll).to_a - expect(poll_options.size).to eq(2) - - option_1 = poll_options.first - expect(option_1.digest).to eq("e94c09aae2aa071610212a5c5042111b") - expect(option_1.html).to eq("Yes") - expect(option_1.anonymous_votes).to eq(1) - - option_2 = poll_options.last - expect(option_2.digest).to eq("802c50392a68e426d4b26d81ddc5ab33") - expect(option_2.html).to eq("No") - expect(option_2.anonymous_votes).to eq(2) - end - end -end diff --git a/plugins/poll/spec/integration/poll_endpoints_spec.rb b/plugins/poll/spec/integration/poll_endpoints_spec.rb index 61e1e8b5f5b..68eb963c9e4 100644 --- a/plugins/poll/spec/integration/poll_endpoints_spec.rb +++ b/plugins/poll/spec/integration/poll_endpoints_spec.rb @@ -4,8 +4,19 @@ require "rails_helper" describe "DiscoursePoll endpoints" do describe "fetch voters for a poll" do - let(:user) { Fabricate(:user) } - let(:post) { Fabricate(:post, raw: "[poll public=true]\n- A\n- B\n[/poll]") } + fab!(:user) { Fabricate(:user) } + fab!(:post) { Fabricate(:post, raw: "[poll public=true]\n- A\n- B\n[/poll]") } + + fab!(:post_with_multiple_poll) do + Fabricate(:post, raw: <<~SQL) + [poll type=multiple public=true min=1 max=2] + - A + - B + - C + [/poll] + SQL + end + let(:option_a) { "5c24fc1df56d764b550ceae1b9319125" } let(:option_b) { "e89dec30bbd9bf50fabf6a05b4324edf" } @@ -35,13 +46,13 @@ describe "DiscoursePoll endpoints" do it 'should return the right response for a single option' do DiscoursePoll::Poll.vote( user, - post.id, + post_with_multiple_poll.id, DiscoursePoll::DEFAULT_POLL_NAME, [option_a, option_b] ) get "/polls/voters.json", params: { - post_id: post.id, + post_id: post_with_multiple_poll.id, poll_name: DiscoursePoll::DEFAULT_POLL_NAME, option_id: option_b } @@ -125,7 +136,16 @@ describe "DiscoursePoll endpoints" do fab!(:user2) { Fabricate(:user) } fab!(:user3) { Fabricate(:user) } fab!(:user4) { Fabricate(:user) } - fab!(:post) { Fabricate(:post, raw: "[poll public=true]\n- A\n- B\n[/poll]") } + + fab!(:post) do + Fabricate(:post, raw: <<~SQL) + [poll type=multiple public=true min=1 max=2] + - A + - B + [/poll] + SQL + end + let(:option_a) { "5c24fc1df56d764b550ceae1b9319125" } let(:option_b) { "e89dec30bbd9bf50fabf6a05b4324edf" } @@ -135,6 +155,7 @@ describe "DiscoursePoll endpoints" do user_1: option_a, user_2: option_b, } + [user1, user2, user3].each_with_index do |user, index| DiscoursePoll::Poll.vote( user, diff --git a/plugins/poll/spec/lib/poll_spec.rb b/plugins/poll/spec/lib/poll_spec.rb new file mode 100644 index 00000000000..92740546c07 --- /dev/null +++ b/plugins/poll/spec/lib/poll_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe DiscoursePoll::Poll do + fab!(:user) { Fabricate(:user) } + fab!(:user_2) { Fabricate(:user) } + + fab!(:post_with_regular_poll) do + Fabricate(:post, raw: <<~RAW) + [poll] + * 1 + * 2 + [/poll] + RAW + end + + fab!(:post_with_multiple_poll) do + Fabricate(:post, raw: <<~RAW) + [poll type=multiple min=2 max=3] + * 1 + * 2 + * 3 + * 4 + * 5 + [/poll] + RAW + end + + describe '.vote' do + it 'should only allow one vote per user for a regular poll' do + poll = post_with_regular_poll.polls.first + + expect do + DiscoursePoll::Poll.vote( + user, + post_with_regular_poll.id, + "poll", + poll.poll_options.map(&:digest) + ) + end.to raise_error(DiscoursePoll::Error, I18n.t("poll.one_vote_per_user")) + end + + it 'should clean up bad votes for a regular poll' do + poll = post_with_regular_poll.polls.first + + PollVote.create!( + poll: poll, + poll_option: poll.poll_options.first, + user: user + ) + + PollVote.create!( + poll: poll, + poll_option: poll.poll_options.last, + user: user + ) + + DiscoursePoll::Poll.vote( + user, + post_with_regular_poll.id, + "poll", + [poll.poll_options.first.digest] + ) + + expect(PollVote.where(poll: poll, user: user).pluck(:poll_option_id)) + .to contain_exactly(poll.poll_options.first.id) + end + + it 'allows user to vote on multiple options correctly for a multiple poll' do + poll = post_with_multiple_poll.polls.first + poll_options = poll.poll_options + + [ + poll_options.first, + poll_options.second, + poll_options.third, + ].each do |poll_option| + PollVote.create!(poll: poll, poll_option: poll_option, user: user) + end + + DiscoursePoll::Poll.vote( + user, + post_with_multiple_poll.id, + "poll", + [poll_options.first.digest, poll_options.second.digest] + ) + + DiscoursePoll::Poll.vote( + user_2, + post_with_multiple_poll.id, + "poll", + [poll_options.third.digest, poll_options.fourth.digest] + ) + + expect(PollVote.where(poll: poll, user: user).pluck(:poll_option_id)) + .to contain_exactly(poll_options.first.id, poll_options.second.id) + + expect(PollVote.where(poll: poll, user: user_2).pluck(:poll_option_id)) + .to contain_exactly(poll_options.third.id, poll_options.fourth.id) + end + + it 'should respect the min/max votes per user for a multiple poll' do + poll = post_with_multiple_poll.polls.first + + expect do + DiscoursePoll::Poll.vote( + user, + post_with_multiple_poll.id, + "poll", + poll.poll_options.map(&:digest) + ) + end.to raise_error( + DiscoursePoll::Error, + I18n.t("poll.max_vote_per_user", count: poll.max) + ) + + expect do + DiscoursePoll::Poll.vote( + user, + post_with_multiple_poll.id, + "poll", + [poll.poll_options.first.digest] + ) + end.to raise_error( + DiscoursePoll::Error, + I18n.t("poll.min_vote_per_user", count: poll.min) + ) + end + end +end