From a29eeaf2e273af660f749e0df9bd7f45a240c768 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 10 Sep 2025 12:22:08 +0200 Subject: [PATCH] Add option_id to partial results and unique index Similar to what we did in PR "Avoid duplicate records in poll answers" 5539, specifically in commit 503369166, we want to stop relying on the plain text "answer" and start using "option_id" to avoid issues with counts across translations and to add consistency to the poll_partial_results table. Note that we also moved the `possible_answers` method from Poll::Question to Poll::Question::Option, since the list of valid answers really comes from the options of a question and not from the question itself. Tests were updated to validate answers against the translations of the assigned option. Additionally, we renamed lambda parameters in validations to improve clarity. --- .../admin/poll/results/question_component.rb | 9 +- .../officing/results_controller.rb | 7 +- app/models/poll/answer.rb | 4 +- app/models/poll/partial_result.rb | 6 +- app/models/poll/question.rb | 4 - app/models/poll/question/option.rb | 5 ++ ...7_add_option_id_to_poll_partial_results.rb | 9 ++ db/schema.rb | 6 +- .../poll/results/question_component_spec.rb | 15 ++-- spec/factories/polls.rb | 9 +- spec/lib/tasks/polls_spec.rb | 11 +-- spec/models/poll/answer_spec.rb | 17 ++-- spec/models/poll/partial_result_spec.rb | 86 +++++++++++++++++-- 13 files changed, 137 insertions(+), 51 deletions(-) create mode 100644 db/migrate/20250909145207_add_option_id_to_poll_partial_results.rb diff --git a/app/components/admin/poll/results/question_component.rb b/app/components/admin/poll/results/question_component.rb index 8836a6b91..a539e8d88 100644 --- a/app/components/admin/poll/results/question_component.rb +++ b/app/components/admin/poll/results/question_component.rb @@ -7,13 +7,6 @@ class Admin::Poll::Results::QuestionComponent < ApplicationComponent end def votes_for(option) - grouped = by_answer[option.title] || [] - grouped.sum(&:amount) + partial_results.where(option: option).sum(:amount) end - - private - - def by_answer - @by_answer ||= partial_results.where(question: question).group_by(&:answer) - end end diff --git a/app/controllers/officing/results_controller.rb b/app/controllers/officing/results_controller.rb index 21bb8e5f3..8aa69ae7d 100644 --- a/app/controllers/officing/results_controller.rb +++ b/app/controllers/officing/results_controller.rb @@ -39,18 +39,19 @@ class Officing::ResultsController < Officing::BaseController params[:questions].each_pair do |question_id, results| question = @poll.questions.find(question_id) - results.each_pair do |answer_index, count| + results.each_pair do |option_index, count| next if count.blank? - answer = question.question_options.find_by(given_order: answer_index.to_i + 1).title + option = question.question_options.find_by(given_order: option_index.to_i + 1) partial_result = ::Poll::PartialResult.find_or_initialize_by( booth_assignment_id: @officer_assignment.booth_assignment_id, date: Date.current, question_id: question_id, - answer: answer + option_id: option.id ) partial_result.officer_assignment_id = @officer_assignment.id + partial_result.answer = option.title partial_result.amount = count.to_i partial_result.author = current_user partial_result.origin = "booth" diff --git a/app/models/poll/answer.rb b/app/models/poll/answer.rb index d4b0150e2..7663aa42b 100644 --- a/app/models/poll/answer.rb +++ b/app/models/poll/answer.rb @@ -11,8 +11,8 @@ class Poll::Answer < ApplicationRecord validates :option, uniqueness: { scope: :author_id }, allow_nil: true validate :max_votes - validates :answer, inclusion: { in: ->(a) { a.question.possible_answers }}, - unless: ->(a) { a.question.blank? } + validates :answer, inclusion: { in: ->(poll_answer) { poll_answer.option.possible_answers }}, + if: ->(poll_answer) { poll_answer.option.present? } scope :by_author, ->(author_id) { where(author_id: author_id) } scope :by_question, ->(question_id) { where(question_id: question_id) } diff --git a/app/models/poll/partial_result.rb b/app/models/poll/partial_result.rb index eb11dd4e3..d671d6184 100644 --- a/app/models/poll/partial_result.rb +++ b/app/models/poll/partial_result.rb @@ -5,13 +5,15 @@ class Poll::PartialResult < ApplicationRecord belongs_to :author, -> { with_hidden }, class_name: "User", inverse_of: :poll_partial_results belongs_to :booth_assignment belongs_to :officer_assignment + belongs_to :option, class_name: "Poll::Question::Option" validates :question, presence: true validates :author, presence: true validates :answer, presence: true - validates :answer, inclusion: { in: ->(a) { a.question.possible_answers }}, - unless: ->(a) { a.question.blank? } + validates :answer, inclusion: { in: ->(partial_result) { partial_result.option.possible_answers }}, + if: ->(partial_result) { partial_result.option.present? } validates :origin, inclusion: { in: ->(*) { VALID_ORIGINS }} + validates :option, uniqueness: { scope: [:booth_assignment_id, :date] }, allow_nil: true scope :by_author, ->(author_id) { where(author_id: author_id) } scope :by_question, ->(question_id) { where(question_id: question_id) } diff --git a/app/models/poll/question.rb b/app/models/poll/question.rb index 93e9c4453..97d1d2c60 100644 --- a/app/models/poll/question.rb +++ b/app/models/poll/question.rb @@ -49,10 +49,6 @@ class Poll::Question < ApplicationRecord question_options.max_by(&:total_votes)&.id end - def possible_answers - question_options.joins(:translations).pluck(:title) - end - def options_with_read_more? options_with_read_more.any? end diff --git a/app/models/poll/question/option.rb b/app/models/poll/question/option.rb index 9de143c06..4b59ec04b 100644 --- a/app/models/poll/question/option.rb +++ b/app/models/poll/question/option.rb @@ -12,6 +12,7 @@ class Poll::Question::Option < ApplicationRecord belongs_to :question, class_name: "Poll::Question" has_many :answers, class_name: "Poll::Answer", dependent: :nullify + has_many :partial_results, class_name: "Poll::PartialResult", dependent: :nullify has_many :videos, class_name: "Poll::Question::Option::Video", dependent: :destroy, foreign_key: "answer_id", @@ -50,4 +51,8 @@ class Poll::Question::Option < ApplicationRecord def with_read_more? description.present? || images.any? || documents.any? || videos.any? end + + def possible_answers + translations.pluck(:title) + end end diff --git a/db/migrate/20250909145207_add_option_id_to_poll_partial_results.rb b/db/migrate/20250909145207_add_option_id_to_poll_partial_results.rb new file mode 100644 index 000000000..f07f3c2a4 --- /dev/null +++ b/db/migrate/20250909145207_add_option_id_to_poll_partial_results.rb @@ -0,0 +1,9 @@ +class AddOptionIdToPollPartialResults < ActiveRecord::Migration[7.1] + def change + change_table :poll_partial_results do |t| + t.references :option, index: true, foreign_key: { to_table: :poll_question_answers } + + t.index [:booth_assignment_id, :date, :option_id], unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 42c7e3ecd..10cc0d023 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.1].define(version: 2025_08_14_122241) do +ActiveRecord::Schema[7.1].define(version: 2025_09_09_145207) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" enable_extension "plpgsql" @@ -1084,9 +1084,12 @@ ActiveRecord::Schema[7.1].define(version: 2025_08_14_122241) do t.text "amount_log", default: "" t.text "officer_assignment_id_log", default: "" t.text "author_id_log", default: "" + t.bigint "option_id" t.index ["answer"], name: "index_poll_partial_results_on_answer" t.index ["author_id"], name: "index_poll_partial_results_on_author_id" + t.index ["booth_assignment_id", "date", "option_id"], name: "idx_on_booth_assignment_id_date_option_id_2ffcf6ea3b", unique: true t.index ["booth_assignment_id", "date"], name: "index_poll_partial_results_on_booth_assignment_id_and_date" + t.index ["option_id"], name: "index_poll_partial_results_on_option_id" t.index ["origin"], name: "index_poll_partial_results_on_origin" t.index ["question_id"], name: "index_poll_partial_results_on_question_id" end @@ -1799,6 +1802,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_08_14_122241) do add_foreign_key "poll_officer_assignments", "poll_booth_assignments", column: "booth_assignment_id" add_foreign_key "poll_partial_results", "poll_booth_assignments", column: "booth_assignment_id" add_foreign_key "poll_partial_results", "poll_officer_assignments", column: "officer_assignment_id" + add_foreign_key "poll_partial_results", "poll_question_answers", column: "option_id" add_foreign_key "poll_partial_results", "poll_questions", column: "question_id" add_foreign_key "poll_partial_results", "users", column: "author_id" add_foreign_key "poll_question_answer_videos", "poll_question_answers", column: "answer_id" diff --git a/spec/components/admin/poll/results/question_component_spec.rb b/spec/components/admin/poll/results/question_component_spec.rb index aaca189f5..dd366e759 100644 --- a/spec/components/admin/poll/results/question_component_spec.rb +++ b/spec/components/admin/poll/results/question_component_spec.rb @@ -3,11 +3,8 @@ require "rails_helper" describe Admin::Poll::Results::QuestionComponent do let(:poll) { create(:poll) } let(:question) { create(:poll_question, poll: poll, title: "What do you want?") } - - before do - create(:poll_question_option, question: question, title: "Yes") - create(:poll_question_option, question: question, title: "No") - end + let!(:option_yes) { create(:poll_question_option, question: question, title: "Yes") } + let!(:option_no) { create(:poll_question_option, question: question, title: "No") } it "renders question title and headers" do render_inline Admin::Poll::Results::QuestionComponent.new(question, poll.partial_results) @@ -25,10 +22,10 @@ describe Admin::Poll::Results::QuestionComponent do expect(page).to have_css "td", text: "No" end - it "sums votes by answer title" do - create(:poll_partial_result, question: question, answer: "Yes", amount: 2) - create(:poll_partial_result, question: question, answer: "Yes", amount: 1) - create(:poll_partial_result, question: question, answer: "No", amount: 5) + it "sums votes by option" do + create(:poll_partial_result, question: question, option: option_yes, amount: 2, date: Date.current) + create(:poll_partial_result, question: question, option: option_yes, amount: 1, date: Date.yesterday) + create(:poll_partial_result, question: question, option: option_no, amount: 5) render_inline Admin::Poll::Results::QuestionComponent.new(question, question.partial_results) diff --git a/spec/factories/polls.rb b/spec/factories/polls.rb index 100029ec7..358af5e80 100644 --- a/spec/factories/polls.rb +++ b/spec/factories/polls.rb @@ -226,7 +226,14 @@ FactoryBot.define do question factory: [:poll_question, :yes_no] author factory: :user origin { "web" } - answer { question.question_options.sample.title } + option do + if answer + question.question_options.find_by(title: answer) + else + question.question_options.sample + end + end + after(:build) { |poll_partial_result| poll_partial_result.answer ||= poll_partial_result.option&.title } end factory :poll_recount, class: "Poll::Recount" do diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index abe5f6130..06f40b979 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -42,7 +42,7 @@ describe "polls tasks" do 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)) + insert(:poll_answer, answer_attributes.merge(option_id: option_b.id)) expect(Poll::Answer.count).to eq 2 @@ -126,10 +126,11 @@ describe "polls tasks" do 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) + insert(:poll_answer, question: abc_question, + author: user, + answer: "Answer A", + option_id: option_b.id) + answer_with_inconsistent_option = Poll::Answer.find_by!(option: option_b) answer_with_invalid_option = build(:poll_answer, question: abc_question, author: user, answer: "Non existing", diff --git a/spec/models/poll/answer_spec.rb b/spec/models/poll/answer_spec.rb index aec882fc5..a70ed3a4d 100644 --- a/spec/models/poll/answer_spec.rb +++ b/spec/models/poll/answer_spec.rb @@ -89,17 +89,18 @@ describe Poll::Answer do expect { answer.save }.not_to raise_error end - it "is valid for answers included in the Poll::Question's question_options list" do + it "is valid for answers included in the list of titles for the option" do question = create(:poll_question) - create(:poll_question_option, title: "One", question: question) + option = create(:poll_question_option, title_en: "One", title_es: "Uno", question: question) + create(:poll_question_option, title: "Two", question: question) - create(:poll_question_option, title: "Three", question: question) + create(:poll_question_option, title: "Three", question: create(:poll_question, poll: create(:poll))) - expect(build(:poll_answer, question: question, answer: "One")).to be_valid - expect(build(:poll_answer, question: question, answer: "Two")).to be_valid - expect(build(:poll_answer, question: question, answer: "Three")).to be_valid - - expect(build(:poll_answer, question: question, answer: "Four")).not_to be_valid + expect(build(:poll_answer, option: option, answer: "One")).to be_valid + expect(build(:poll_answer, option: option, answer: "Uno")).to be_valid + expect(build(:poll_answer, option: option, answer: "Two")).not_to be_valid + expect(build(:poll_answer, option: option, answer: "Three")).not_to be_valid + expect(build(:poll_answer, option: option, answer: "Any")).not_to be_valid end end end diff --git a/spec/models/poll/partial_result_spec.rb b/spec/models/poll/partial_result_spec.rb index 2c05b69d2..429fb9393 100644 --- a/spec/models/poll/partial_result_spec.rb +++ b/spec/models/poll/partial_result_spec.rb @@ -2,17 +2,18 @@ require "rails_helper" describe Poll::PartialResult do describe "validations" do - it "validates that the answers are included in the Poll::Question's list" do + it "validates that the answers are included in the list of titles for the option" do question = create(:poll_question) - create(:poll_question_option, title: "One", question: question) + option = create(:poll_question_option, title_en: "One", title_es: "Uno", question: question) + create(:poll_question_option, title: "Two", question: question) - create(:poll_question_option, title: "Three", question: question) + create(:poll_question_option, title: "Three", question: create(:poll_question, poll: create(:poll))) - expect(build(:poll_partial_result, question: question, answer: "One")).to be_valid - expect(build(:poll_partial_result, question: question, answer: "Two")).to be_valid - expect(build(:poll_partial_result, question: question, answer: "Three")).to be_valid - - expect(build(:poll_partial_result, question: question, answer: "Four")).not_to be_valid + expect(build(:poll_partial_result, option: option, answer: "One")).to be_valid + expect(build(:poll_partial_result, option: option, answer: "Uno")).to be_valid + expect(build(:poll_partial_result, option: option, answer: "Two")).not_to be_valid + expect(build(:poll_partial_result, option: option, answer: "Three")).not_to be_valid + expect(build(:poll_partial_result, option: option, answer: "Any")).not_to be_valid end it "dynamically validates the valid origins" do @@ -21,6 +22,75 @@ describe Poll::PartialResult do expect(build(:poll_partial_result, origin: "custom")).to be_valid expect(build(:poll_partial_result, origin: "web")).not_to be_valid end + + describe "option_id uniqueness" do + let(:booth_assignment) { create(:poll_booth_assignment) } + + it "is not valid when there are two identical partial results" do + question = create(:poll_question_multiple, :abc) + option = question.question_options.first + + create(:poll_partial_result, + question: question, + booth_assignment: booth_assignment, + date: Date.current, + option: option, + answer: "Answer A") + + partial_result = build(:poll_partial_result, + question: question, + booth_assignment: booth_assignment, + date: Date.current, + option: option, + answer: "Answer A") + + expect(partial_result).not_to be_valid + expect { partial_result.save(validate: false) }.to raise_error ActiveRecord::RecordNotUnique + end + + it "is not valid when there are two results with the same option and different answer" do + question = create(:poll_question_multiple, :abc) + option = question.question_options.first + + create(:poll_partial_result, + question: question, + booth_assignment: booth_assignment, + date: Date.current, + option: option, + answer: "Answer A") + + partial_result = build(:poll_partial_result, + question: question, + booth_assignment: booth_assignment, + date: Date.current, + option: option, + answer: "Answer B") + + expect(partial_result).not_to be_valid + expect { partial_result.save(validate: false) }.to raise_error ActiveRecord::RecordNotUnique + end + + it "is valid when there are two identical results and the option is nil" do + question = create(:poll_question_multiple, :abc) + + create(:poll_partial_result, + question: question, + booth_assignment: booth_assignment, + date: Date.current, + option: nil, + answer: "Answer A") + + partial_result = build(:poll_partial_result, + question: question, + booth_assignment: booth_assignment, + date: Date.current, + option: nil, + answer: "Answer A") + + expect(partial_result).to be_valid + expect { partial_result.save }.not_to raise_error + end + end end describe "logging changes" do