diff --git a/app/components/polls/questions/options_component.html.erb b/app/components/polls/questions/options_component.html.erb index b687ed249..a183b2aac 100644 --- a/app/components/polls/questions/options_component.html.erb +++ b/app/components/polls/questions/options_component.html.erb @@ -11,7 +11,7 @@ "aria-pressed": true %> <% else %> <%= button_to question_option.title, - answer_question_path(question, answer: question_option.title), + question_answers_path(question, option_id: question_option.id), remote: true, title: t("poll_questions.show.vote_answer", answer: question_option.title), class: "button secondary hollow", diff --git a/app/controllers/polls/answers_controller.rb b/app/controllers/polls/answers_controller.rb index 174cc673c..a969c5edf 100644 --- a/app/controllers/polls/answers_controller.rb +++ b/app/controllers/polls/answers_controller.rb @@ -2,7 +2,24 @@ class Polls::AnswersController < ApplicationController load_and_authorize_resource :question, class: "::Poll::Question" load_and_authorize_resource :answer, class: "::Poll::Answer", through: :question, - through_association: :answers + through_association: :answers, + only: :destroy + + def create + authorize! :answer, @question + + answer = @question.find_or_initialize_user_answer(current_user, params[:option_id]) + answer.save_and_record_voter_participation + + respond_to do |format| + format.html do + redirect_to request.referer + end + format.js do + render :show + end + end + end def destroy @answer.destroy_and_remove_voter_participation @@ -12,7 +29,7 @@ class Polls::AnswersController < ApplicationController redirect_to request.referer end format.js do - render "polls/questions/options" + render :show end end end diff --git a/app/controllers/polls/questions_controller.rb b/app/controllers/polls/questions_controller.rb deleted file mode 100644 index 0e7f9a1c9..000000000 --- a/app/controllers/polls/questions_controller.rb +++ /dev/null @@ -1,20 +0,0 @@ -class Polls::QuestionsController < ApplicationController - load_and_authorize_resource :poll - load_and_authorize_resource :question, class: "Poll::Question" - - has_orders %w[most_voted newest oldest], only: :show - - def answer - answer = @question.find_or_initialize_user_answer(current_user, params[:answer]) - answer.save_and_record_voter_participation - - respond_to do |format| - format.html do - redirect_to request.referer - end - format.js do - render :options - end - end - end -end diff --git a/app/lib/poll_option_finder.rb b/app/lib/poll_option_finder.rb new file mode 100644 index 000000000..37544926c --- /dev/null +++ b/app/lib/poll_option_finder.rb @@ -0,0 +1,31 @@ +class PollOptionFinder + attr_reader :question + + def initialize(question) + @question = question + end + + def manageable_choices + choices_map.select { |choice, ids| ids.count == 1 } + end + + def unmanageable_choices + choices_map.reject { |choice, ids| ids.count == 1 } + end + + private + + def choices_map + @choices_map ||= existing_choices.to_h do |choice| + [choice, options.where("lower(title) = lower(?)", choice).distinct.ids] + end + end + + def options + question.question_options.joins(:translations).reorder(:id) + end + + def existing_choices + question.answers.where(option_id: nil).distinct.pluck(:answer) + end +end diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index 039744fd5..b9c52fb38 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -1,12 +1,14 @@ class Poll::Answer < ApplicationRecord belongs_to :question, -> { with_hidden }, inverse_of: :answers - belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :poll_answers + belongs_to :option, class_name: "Poll::Question::Option" + belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :poll_answers delegate :poll, :poll_id, to: :question validates :question, presence: true validates :author, presence: true validates :answer, presence: true + validates :option, uniqueness: { scope: :author_id }, allow_nil: true validate :max_votes validates :answer, inclusion: { in: ->(a) { a.question.possible_answers }}, diff --git a/app/models/poll/question.rb b/app/models/poll/question.rb index 729cdbe46..c55320560 100644 --- a/app/models/poll/question.rb +++ b/app/models/poll/question.rb @@ -98,20 +98,23 @@ class Poll::Question < ApplicationRecord end end - def find_or_initialize_user_answer(user, title) - answer = answers.find_or_initialize_by(find_by_attributes(user, title)) - answer.answer = title + def find_or_initialize_user_answer(user, option_id) + option = question_options.find(option_id) + + answer = answers.find_or_initialize_by(find_by_attributes(user, option)) + answer.option = option + answer.answer = option.title answer end private - def find_by_attributes(user, title) + def find_by_attributes(user, option) case vote_type when "unique", nil { author: user } when "multiple" - { author: user, answer: title } + { author: user, answer: option.title } end end end diff --git a/app/models/poll/question/option.rb b/app/models/poll/question/option.rb index 59e38aa33..c191c5923 100644 --- a/app/models/poll/question/option.rb +++ b/app/models/poll/question/option.rb @@ -11,6 +11,7 @@ class Poll::Question::Option < ApplicationRecord accepts_nested_attributes_for :documents, allow_destroy: true belongs_to :question, class_name: "Poll::Question" + has_many :answers, class_name: "Poll::Answer", dependent: :nullify has_many :videos, class_name: "Poll::Question::Option::Video", dependent: :destroy, foreign_key: "answer_id", diff --git a/app/views/polls/questions/options.js.erb b/app/views/polls/answers/show.js.erb similarity index 100% rename from app/views/polls/questions/options.js.erb rename to app/views/polls/answers/show.js.erb diff --git a/config/routes/poll.rb b/config/routes/poll.rb index 94cf504b4..f5be6b301 100644 --- a/config/routes/poll.rb +++ b/config/routes/poll.rb @@ -4,9 +4,8 @@ resources :polls, only: [:show, :index] do get :results end - resources :questions, controller: "polls/questions", shallow: true do - post :answer, on: :member - resources :answers, controller: "polls/answers", only: :destroy, shallow: false + resources :questions, controller: "polls/questions", shallow: true, only: [] do + resources :answers, controller: "polls/answers", only: [:create, :destroy], shallow: false end end diff --git a/db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb b/db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb new file mode 100644 index 000000000..d96d9027e --- /dev/null +++ b/db/migrate/20240513135357_add_question_answer_id_to_poll_answers.rb @@ -0,0 +1,9 @@ +class AddQuestionAnswerIdToPollAnswers < ActiveRecord::Migration[7.0] + def change + change_table :poll_answers do |t| + t.references :option, index: true, foreign_key: { to_table: :poll_question_answers } + + t.index [:option_id, :author_id], unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 0dfa54bd7..d565cb35c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_05_11_141119) do +ActiveRecord::Schema[7.0].define(version: 2024_05_13_135357) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" enable_extension "plpgsql" @@ -1024,7 +1024,10 @@ ActiveRecord::Schema[7.0].define(version: 2024_05_11_141119) do t.string "answer" t.datetime "created_at", precision: nil t.datetime "updated_at", precision: nil + t.bigint "option_id" t.index ["author_id"], name: "index_poll_answers_on_author_id" + t.index ["option_id", "author_id"], name: "index_poll_answers_on_option_id_and_author_id", unique: true + t.index ["option_id"], name: "index_poll_answers_on_option_id" t.index ["question_id", "answer"], name: "index_poll_answers_on_question_id_and_answer" t.index ["question_id"], name: "index_poll_answers_on_question_id" end @@ -1798,6 +1801,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_05_11_141119) do add_foreign_key "moderators", "users" add_foreign_key "notifications", "users" add_foreign_key "organizations", "users" + add_foreign_key "poll_answers", "poll_question_answers", column: "option_id" add_foreign_key "poll_answers", "poll_questions", column: "question_id" add_foreign_key "poll_booth_assignments", "polls" add_foreign_key "poll_officer_assignments", "poll_booth_assignments", column: "booth_assignment_id" diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 9f56f3c38..64225f60b 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -8,6 +8,7 @@ namespace :consul do desc "Runs tasks needed to upgrade from 2.1.1 to 2.2.0" task "execute_release_2.2.0_tasks": [ "db:mask_ips", - "polls:remove_duplicate_voters" + "polls:remove_duplicate_voters", + "polls:populate_option_id" ] end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index b6040a127..a3ce11e4b 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -28,4 +28,83 @@ namespace :polls do end end end + + desc "Removes duplicate poll answers" + task remove_duplicate_answers: :environment do + logger = ApplicationLogger.new + duplicate_records_logger = DuplicateRecordsLogger.new + + logger.info "Removing duplicate answers in polls" + + Tenant.run_on_each do + Poll::Question.find_each do |question| + manageable_titles = PollOptionFinder.new(question).manageable_choices.keys + + question.question_options.each do |option| + titles = option.translations.where(title: manageable_titles).select(:title).distinct + + author_ids = question.answers + .where(answer: titles) + .select(:author_id) + .group(:author_id) + .having("count(*) > 1") + .pluck(:author_id) + + author_ids.each do |author_id| + poll_answers = question.answers.where(option_id: nil, answer: titles, author_id: author_id) + + poll_answers.excluding(poll_answers.first).each do |poll_answer| + poll_answer.delete + + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + log_message = "Deleted duplicate record with ID #{poll_answer.id} " \ + "from the #{Poll::Answer.table_name} table " \ + "with question_id #{question.id}, " \ + "author_id #{author_id} " \ + "and answer #{poll_answer.answer}" + tenant_info.to_s + logger.info(log_message) + duplicate_records_logger.info(log_message) + end + end + end + end + end + end + + desc "populates the poll answers option_id column" + task populate_option_id: :remove_duplicate_answers do + logger = ApplicationLogger.new + logger.info "Updating option_id column in poll_answers" + + Tenant.run_on_each do + questions = Poll::Question.select do |question| + # Change this if you've added a custom votation type + # to your Consul Democracy installation that implies + # choosing among a few given options + question.unique? || question.multiple? + end + + questions.each do |question| + option_finder = PollOptionFinder.new(question) + + option_finder.manageable_choices.each do |choice, ids| + question.answers.where(option_id: nil, answer: choice).update_all(option_id: ids.first) + end + + option_finder.unmanageable_choices.each do |choice, ids| + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + + if ids.count == 0 + logger.warn "Skipping poll_answers with the text \"#{choice}\" for the poll_question " \ + "with ID #{question.id}. This question has no poll_question_answers " \ + "containing the text \"#{choice}\"" + tenant_info.to_s + else + logger.warn "Skipping poll_answers with the text \"#{choice}\" for the poll_question " \ + "with ID #{question.id}. The text \"#{choice}\" could refer to any of these " \ + "IDs in the poll_question_answers table: #{ids.join(", ")}" + tenant_info.to_s + end + end + end + end + end end diff --git a/spec/controllers/polls/answers_controller_spec.rb b/spec/controllers/polls/answers_controller_spec.rb new file mode 100644 index 000000000..08f129c0b --- /dev/null +++ b/spec/controllers/polls/answers_controller_spec.rb @@ -0,0 +1,25 @@ +require "rails_helper" + +describe Polls::AnswersController do + describe "POST create" do + it "doesn't create duplicate records on simultaneous requests", :race_condition do + question = create(:poll_question_multiple, :abc) + sign_in(create(:user, :level_two)) + + 2.times.map do + Thread.new do + begin + post :create, params: { + question_id: question.id, + option_id: question.question_options.find_by(title: "Answer A").id, + format: :js + } + rescue ActionDispatch::IllegalStateError, ActiveRecord::RecordInvalid + end + end + end.each(&:join) + + expect(Poll::Answer.count).to eq 1 + end + end +end diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 4f61232bc..735c20cb8 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -66,13 +66,21 @@ FactoryBot.define do end trait :abc do - after(:create) do |question, evaluator| + after(:create) do |question| %w[A B C].each do |letter| create(:poll_question_option, question: question, title: "Answer #{letter}") end end end + trait :abcde do + after(:create) do |question| + %w[A B C D E].each do |letter| + create(:poll_question_option, question: question, title: "Answer #{letter}") + end + end + end + factory :poll_question_unique do after(:create) do |question| create(:votation_type_unique, questionable: question) @@ -205,6 +213,7 @@ FactoryBot.define do question factory: [:poll_question, :yes_no] author factory: [:user, :level_two] answer { question.question_options.sample.title } + option { question.question_options.find_by(title: answer) } end factory :poll_partial_result, class: "Poll::PartialResult" do diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index 2b43c969d..b3b2eb56b 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -46,4 +46,223 @@ describe "polls tasks" do end end end + + describe "polls:remove_duplicate_answers" do + before { Rake::Task["polls:remove_duplicate_answers"].reenable } + + it "removes duplicate answers" do + question = create(:poll_question_multiple, :abcde, poll: poll, max_votes: 4) + abc_question = create(:poll_question_multiple, :abc, poll: poll) + + answer_attributes = { + question_id: question.id, + author_id: user.id, + answer: "Answer A", + option_id: nil + } + abc_answer_attributes = answer_attributes.merge(question_id: abc_question.id, answer: "Answer B") + + answer = create(:poll_answer, answer_attributes) + other_answer = create(:poll_answer, answer_attributes.merge(answer: "Answer B")) + other_user_answer = create(:poll_answer, answer_attributes.merge(author_id: create(:user).id)) + abc_answer = create(:poll_answer, abc_answer_attributes) + + 2.times { insert(:poll_answer, answer_attributes) } + insert(:poll_answer, abc_answer_attributes) + + expect(Poll::Answer.count).to eq 7 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 4 + expect(Poll::Answer.all).to match_array [answer, other_answer, other_user_answer, abc_answer] + end + + it "does not remove answers with the same text and different options" do + question = create(:poll_question_multiple, :abcde, max_votes: 4) + option_a = question.question_options.find_by(title: "Answer A") + option_b = question.question_options.find_by(title: "Answer B") + + answer_attributes = { question: question, author: user, answer: "Answer A" } + create(:poll_answer, answer_attributes.merge(option: option_a)) + create(:poll_answer, answer_attributes.merge(option: option_b)) + + expect(Poll::Answer.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 2 + end + + it "removes duplicate answers in different languages" do + question = create(:poll_question_multiple, max_votes: 2) + + create(:poll_question_option, question: question, title_en: "Yes", title_de: "Ja") + create(:poll_question_option, question: question, title_en: "No", title_de: "Nein") + create(:poll_question_option, question: question, title_en: "Maybe", title_de: "Vielleicht") + + create(:poll_answer, author: user, question: question, answer: "Yes", option: nil) + create(:poll_answer, author: user, question: question, answer: "Ja", option: nil) + + expect(Poll::Answer.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 1 + end + + it "does not remove duplicate answers when many options are possible" do + question = create(:poll_question_multiple, title: "How do you pronounce it?", max_votes: 2) + + create(:poll_question_option, question: question, title_en: "A", title_es: "EI") + create(:poll_question_option, question: question, title_en: "E", title_es: "I") + create(:poll_question_option, question: question, title_en: "I", title_es: "AI") + + create(:poll_answer, question: question, author: user, answer: "I", option: nil) + create(:poll_answer, question: question, author: user, answer: "AI", option: nil) + + expect(Poll::Answer.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + expect(Poll::Answer.count).to eq 2 + end + + it "removes duplicate answers on tenants" do + create(:tenant, schema: "answers") + + Tenant.switch("answers") do + user = create(:user, :level_two) + question = create(:poll_question_multiple, :abc) + + answer_attributes = { + question_id: question.id, + author_id: user.id, + answer: "Answer A", + option_id: nil + } + create(:poll_answer, answer_attributes) + insert(:poll_answer, answer_attributes) + + expect(Poll::Answer.count).to eq 2 + end + + Rake.application.invoke_task("polls:remove_duplicate_answers") + + Tenant.switch("answers") do + expect(Poll::Answer.count).to eq 1 + end + end + end + + describe "polls:populate_option_id" do + before do + Rake::Task["polls:remove_duplicate_answers"].reenable + Rake::Task["polls:populate_option_id"].reenable + end + + it "populates the option_id column of existing answers when there's one valid answer" do + yes_no_question = create(:poll_question, :yes_no, poll: poll) + abc_question = create(:poll_question_multiple, :abc, poll: poll) + option_a = abc_question.question_options.find_by(title: "Answer A") + option_b = abc_question.question_options.find_by(title: "Answer B") + + answer = create(:poll_answer, question: yes_no_question, author: user, answer: "Yes", option: nil) + abc_answer = create(:poll_answer, question: abc_question, author: user, answer: "Answer A", option: nil) + answer_with_inconsistent_option = create(:poll_answer, question: abc_question, + author: user, + answer: "Answer A", + option: option_b) + answer_with_invalid_option = build(:poll_answer, question: abc_question, + author: user, + answer: "Non existing", + option: nil) + answer_with_invalid_option.save!(validate: false) + + Rake.application.invoke_task("polls:populate_option_id") + answer.reload + abc_answer.reload + answer_with_inconsistent_option.reload + answer_with_invalid_option.reload + + expect(answer.option_id).to eq yes_no_question.question_options.find_by(title: "Yes").id + expect(abc_answer.option_id).to eq option_a.id + expect(answer_with_inconsistent_option.option_id).to eq option_b.id + expect(answer_with_invalid_option.option_id).to be nil + end + + it "does not populate the option_id column when there are several valid options" do + question = create(:poll_question, title: "How do you pronounce it?") + + create(:poll_question_option, question: question, title_en: "A", title_es: "EI") + create(:poll_question_option, question: question, title_en: "E", title_es: "I") + create(:poll_question_option, question: question, title_en: "I", title_es: "AI") + + answer = create(:poll_answer, question: question, author: user, answer: "I", option: nil) + + Rake.application.invoke_task("polls:populate_option_id") + answer.reload + + expect(answer.option_id).to be nil + end + + it "removes duplicate answers before populating the option_id column" do + user = create(:user, :level_two) + question = create(:poll_question_multiple, :abc) + + localized_question = create(:poll_question_multiple) + create(:poll_question_option, question: localized_question, title_en: "Yes", title_de: "Ja") + create(:poll_question_option, question: localized_question, title_en: "No", title_de: "Nein") + create(:poll_question_option, question: localized_question, title_en: "Maybe", title_de: "Vielleicht") + + answer_attributes = { + question_id: question.id, + author_id: user.id, + answer: "Answer A", + option_id: nil + } + answer = create(:poll_answer, answer_attributes) + insert(:poll_answer, answer_attributes) + + localized_answer_attributes = { author: user, question: localized_question, option: nil } + localized_answer = create(:poll_answer, localized_answer_attributes.merge(answer: "Yes")) + create(:poll_answer, localized_answer_attributes.merge(answer: "Ja")) + + answer.reload + localized_answer.reload + + expect(answer.option_id).to be nil + expect(localized_answer.option_id).to be nil + + Rake.application.invoke_task("polls:populate_option_id") + answer.reload + localized_answer.reload + + expect(Poll::Answer.count).to eq 2 + expect(answer.option_id).to eq question.question_options.find_by(title: "Answer A").id + expect(localized_answer.option_id).to eq localized_question.question_options.find_by(title: "Yes").id + end + + it "populates the option_id column on tenants" do + create(:tenant, schema: "answers") + + Tenant.switch("answers") do + question = create(:poll_question_multiple, :abc) + + create(:poll_answer, question: question, answer: "Answer A", option: nil) + end + + Rake.application.invoke_task("polls:populate_option_id") + + Tenant.switch("answers") do + expect(Poll::Question.count).to eq 1 + expect(Poll::Answer.count).to eq 1 + + question = Poll::Question.first + option_a = question.question_options.find_by(title: "Answer A") + + expect(Poll::Answer.first.option_id).to eq option_a.id + end + end + end end diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index 6e76ca44d..b06ccbcd7 100644 --- a/spec/models/poll/answer_spec.rb +++ b/spec/models/poll/answer_spec.rb @@ -51,6 +51,44 @@ describe Poll::Answer do expect(answer).not_to be_valid end + it "is not valid when there are two identical answers" do + author = create(:user) + question = create(:poll_question_multiple, :abc) + option = question.question_options.first + + create(:poll_answer, author: author, question: question, option: option, answer: "Answer A") + + answer = build(:poll_answer, author: author, question: question, option: option, answer: "Answer A") + + expect(answer).not_to be_valid + expect { answer.save(validate: false) }.to raise_error ActiveRecord::RecordNotUnique + end + + it "is not valid when there are two answers with the same option and different answer" do + author = create(:user) + question = create(:poll_question_multiple, :abc) + option = question.question_options.first + + create(:poll_answer, author: author, question: question, option: option, answer: "Answer A") + + answer = build(:poll_answer, author: author, question: question, option: option, answer: "Answer B") + + expect(answer).not_to be_valid + expect { answer.save(validate: false) }.to raise_error ActiveRecord::RecordNotUnique + end + + it "is valid when there are two identical answers and the option is nil" do + author = create(:user) + question = create(:poll_question_multiple, :abc) + + create(:poll_answer, author: author, question: question, option: nil, answer: "Answer A") + + answer = build(:poll_answer, author: author, question: question, option: nil, answer: "Answer A") + + expect(answer).to be_valid + expect { answer.save }.not_to raise_error + end + it "is valid for answers included in the Poll::Question's question_options list" do question = create(:poll_question) create(:poll_question_option, title: "One", question: question) @@ -119,7 +157,10 @@ describe Poll::Answer do expect(poll.reload.voters.size).to eq(1) - updated_answer = answer.question.find_or_initialize_user_answer(answer.author, "No") + updated_answer = answer.question.find_or_initialize_user_answer( + answer.author, + answer.question.question_options.excluding(answer.option).sample.id + ) updated_answer.save_and_record_voter_participation expect(poll.reload.voters.size).to eq(1) diff --git a/spec/routing/polymorphic_routes_spec.rb b/spec/routing/polymorphic_routes_spec.rb index c39b986d5..cf2e748dd 100644 --- a/spec/routing/polymorphic_routes_spec.rb +++ b/spec/routing/polymorphic_routes_spec.rb @@ -53,12 +53,6 @@ describe "Polymorphic routes" do ) end - it "routes poll questions" do - question = create(:poll_question) - - expect(polymorphic_path(question)).to eq question_path(question) - end - it "routes topics" do community = create(:proposal).community topic = create(:topic, community: community)