From 896ebc82fde8e7b9316eef864dc48be6b5265f0c Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 10 Sep 2025 09:28:00 +0200 Subject: [PATCH 01/13] Remove unused go_back_to_new calls and unused error_create key - Remove two redundant go_back_to_new calls in build_results, since @poll.questions.find already raises RecordNotFound if a question does not exist. - Drop the fallback flash translation error_create, which is no longer used since commit 592fdffe4e3 and only remained as a default in go_back_to_new. - Move check_officer_assignment from Officing::BaseController to Officing::ResultsController, its only place of use. --- app/controllers/officing/base_controller.rb | 6 ------ app/controllers/officing/results_controller.rb | 12 ++++++++---- config/locales/en/officing.yml | 1 - config/locales/es/officing.yml | 1 - 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/controllers/officing/base_controller.rb b/app/controllers/officing/base_controller.rb index 706ef166c..0ac95364b 100644 --- a/app/controllers/officing/base_controller.rb +++ b/app/controllers/officing/base_controller.rb @@ -13,12 +13,6 @@ class Officing::BaseController < ApplicationController raise CanCan::AccessDenied unless current_user&.poll_officer? end - def check_officer_assignment - if @officer_assignment.blank? - go_back_to_new(t("officing.results.flash.error_wrong_booth")) - end - end - def load_officer_assignment @officer_assignments ||= current_user.poll_officer.officer_assignments.where(date: Time.current.to_date) end diff --git a/app/controllers/officing/results_controller.rb b/app/controllers/officing/results_controller.rb index 4ebd7fba2..21bb8e5f3 100644 --- a/app/controllers/officing/results_controller.rb +++ b/app/controllers/officing/results_controller.rb @@ -38,13 +38,11 @@ class Officing::ResultsController < Officing::BaseController params[:questions].each_pair do |question_id, results| question = @poll.questions.find(question_id) - go_back_to_new if question.blank? results.each_pair do |answer_index, count| next if count.blank? answer = question.question_options.find_by(given_order: answer_index.to_i + 1).title - go_back_to_new if question.blank? partial_result = ::Poll::PartialResult.find_or_initialize_by( booth_assignment_id: @officer_assignment.booth_assignment_id, @@ -79,10 +77,10 @@ class Officing::ResultsController < Officing::BaseController @results << recount end - def go_back_to_new(alert = nil) + def go_back_to_new(alert) params[:d] = Date.current params[:oa] = results_params[:officer_assignment_id] - flash.now[:alert] = (alert || t("officing.results.flash.error_create")) + flash.now[:alert] = alert load_officer_assignments load_partial_results render :new @@ -123,4 +121,10 @@ class Officing::ResultsController < Officing::BaseController def index_params params.permit(:booth_assignment_id, :date) end + + def check_officer_assignment + if @officer_assignment.blank? + go_back_to_new(t("officing.results.flash.error_wrong_booth")) + end + end end diff --git a/config/locales/en/officing.yml b/config/locales/en/officing.yml index 066c137ae..c72ef0b7b 100644 --- a/config/locales/en/officing.yml +++ b/config/locales/en/officing.yml @@ -41,7 +41,6 @@ en: results: flash: create: "Results saved" - error_create: "Results NOT saved. Error in data." error_wrong_booth: "Wrong booth. Results NOT saved." new: title: "%{poll} - Add results" diff --git a/config/locales/es/officing.yml b/config/locales/es/officing.yml index f668ce8a2..7887f6f19 100644 --- a/config/locales/es/officing.yml +++ b/config/locales/es/officing.yml @@ -41,7 +41,6 @@ es: results: flash: create: "Datos guardados" - error_create: "Resultados NO añadidos. Error en los datos" error_wrong_booth: "Urna incorrecta. Resultados NO guardados." new: title: "%{poll} - Añadir resultados" From bc6506da5a91f11c1ca56ecc07795bcd0724a738 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 17 Sep 2025 15:40:58 +0200 Subject: [PATCH 02/13] Unify Officing and Admin results views Unify the code from app/views/officing/results/index.html.erb with app/views/admin/poll/results/_result.html.erb. This prepares the ground to extract a component in the next commit and avoid duplication. --- app/views/officing/results/index.html.erb | 8 ++++---- config/locales/en/officing.yml | 2 -- config/locales/es/officing.yml | 2 -- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/views/officing/results/index.html.erb b/app/views/officing/results/index.html.erb index 2b2d8ef15..c6e181551 100644 --- a/app/views/officing/results/index.html.erb +++ b/app/views/officing/results/index.html.erb @@ -31,11 +31,11 @@ <% @poll.questions.each do |question| %>

<%= question.title %>

- +
- - + + @@ -43,7 +43,7 @@ <% by_answer = by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} %> - + <% end %> diff --git a/config/locales/en/officing.yml b/config/locales/en/officing.yml index c72ef0b7b..b31d11c0f 100644 --- a/config/locales/en/officing.yml +++ b/config/locales/en/officing.yml @@ -57,8 +57,6 @@ en: index: no_results: "No results" results: Results - table_answer: Answer - table_votes: Votes table_whites: "Totally blank ballots" table_nulls: "Invalid ballots" table_total: "Valid ballots" diff --git a/config/locales/es/officing.yml b/config/locales/es/officing.yml index 7887f6f19..f3cfbc89e 100644 --- a/config/locales/es/officing.yml +++ b/config/locales/es/officing.yml @@ -57,8 +57,6 @@ es: index: no_results: "No hay resultados" results: Resultados - table_answer: Respuesta - table_votes: Votos table_whites: "Papeletas totalmente en blanco" table_nulls: "Papeletas nulas" table_total: "Papeletas válidas" From 7f376c30050c827250cc75ab45e0b6cfc4280a21 Mon Sep 17 00:00:00 2001 From: taitus Date: Tue, 16 Sep 2025 10:01:19 +0200 Subject: [PATCH 03/13] Extract admin poll results to component Note that we have the same code in the officing section. Then we can use the same component. Note also that we are removing the parts of the system specs that are now covered by the component itself, and taking the chance to unify tests. In these removals and unifications we take into account that there are other specs which already cover user interaction in this section. --- .../poll/results/question_component.html.erb | 20 ++++ .../admin/poll/results/question_component.rb | 12 +++ app/views/admin/poll/results/_result.html.erb | 20 +--- app/views/officing/results/index.html.erb | 21 +---- .../poll/results/question_component_spec.rb | 71 ++++++++++++++ .../admin/poll/booth_assigments_spec.rb | 26 +---- spec/system/admin/poll/polls_spec.rb | 94 ++----------------- spec/system/officing/results_spec.rb | 49 +--------- 8 files changed, 118 insertions(+), 195 deletions(-) create mode 100644 app/components/admin/poll/results/question_component.html.erb create mode 100644 app/components/admin/poll/results/question_component.rb create mode 100644 spec/components/admin/poll/results/question_component_spec.rb diff --git a/app/components/admin/poll/results/question_component.html.erb b/app/components/admin/poll/results/question_component.html.erb new file mode 100644 index 000000000..9cf0cd4d1 --- /dev/null +++ b/app/components/admin/poll/results/question_component.html.erb @@ -0,0 +1,20 @@ +
+

<%= question.title %>

+
<%= t("officing.results.index.table_answer") %><%= t("officing.results.index.table_votes") %><%= t("admin.results.result.table_answer") %><%= t("admin.results.result.table_votes") %>
<%= option.title %><%= by_answer[option.title].present? ? by_answer[option.title].first.amount : 0 %><%= by_answer[option.title].present? ? by_answer[option.title].sum(&:amount) : 0 %>
+ + + + + + + + <% question.question_options.each_with_index do |option, i| %> + <% by_answer = by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} %> + + + + + <% end %> + +
<%= t("admin.results.result.table_answer") %><%= t("admin.results.result.table_votes") %>
<%= option.title %><%= by_answer[option.title].present? ? by_answer[option.title].sum(&:amount) : 0 %>
+ diff --git a/app/components/admin/poll/results/question_component.rb b/app/components/admin/poll/results/question_component.rb new file mode 100644 index 000000000..2bee2a328 --- /dev/null +++ b/app/components/admin/poll/results/question_component.rb @@ -0,0 +1,12 @@ +class Admin::Poll::Results::QuestionComponent < ApplicationComponent + attr_reader :question, :partial_results + + def initialize(question, partial_results) + @question = question + @partial_results = partial_results + end + + def by_question + @by_question ||= partial_results.group_by(&:question_id) + end +end diff --git a/app/views/admin/poll/results/_result.html.erb b/app/views/admin/poll/results/_result.html.erb index 0f2600f4f..05b287253 100644 --- a/app/views/admin/poll/results/_result.html.erb +++ b/app/views/admin/poll/results/_result.html.erb @@ -1,21 +1,3 @@ -<% by_question = @partial_results.group_by(&:question_id) %> <% @poll.questions.each do |question| %> -

<%= question.title %>

- - - - - - - - - <% question.question_options.each_with_index do |option, i| %> - <% by_answer = by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} %> - - - - - <% end %> - -
<%= t("admin.results.result.table_answer") %><%= t("admin.results.result.table_votes") %>
<%= option.title %><%= by_answer[option.title].present? ? by_answer[option.title].sum(&:amount) : 0 %>
+ <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> <% end %> diff --git a/app/views/officing/results/index.html.erb b/app/views/officing/results/index.html.erb index c6e181551..9b0d330c5 100644 --- a/app/views/officing/results/index.html.erb +++ b/app/views/officing/results/index.html.erb @@ -27,27 +27,8 @@ - <% by_question = @partial_results.group_by(&:question_id) %> <% @poll.questions.each do |question| %> -

<%= question.title %>

- - - - - - - - - - <% question.question_options.each_with_index do |option, i| %> - <% by_answer = by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} %> - - - - - <% end %> - -
<%= t("admin.results.result.table_answer") %><%= t("admin.results.result.table_votes") %>
<%= option.title %><%= by_answer[option.title].present? ? by_answer[option.title].sum(&:amount) : 0 %>
+ <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> <% end %> diff --git a/spec/components/admin/poll/results/question_component_spec.rb b/spec/components/admin/poll/results/question_component_spec.rb new file mode 100644 index 000000000..aaca189f5 --- /dev/null +++ b/spec/components/admin/poll/results/question_component_spec.rb @@ -0,0 +1,71 @@ +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 + + it "renders question title and headers" do + render_inline Admin::Poll::Results::QuestionComponent.new(question, poll.partial_results) + + expect(page).to have_css "h3", text: "What do you want?" + expect(page).to have_css "th", text: "Answer" + expect(page).to have_css "th", text: "Votes" + end + + it "renders one row per option" do + render_inline Admin::Poll::Results::QuestionComponent.new(question, poll.partial_results) + + expect(page).to have_css "tbody tr", count: 2 + expect(page).to have_css "td", text: "Yes" + 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) + + render_inline Admin::Poll::Results::QuestionComponent.new(question, question.partial_results) + + page.find("tr#question_#{question.id}_0_result") do |yes_result| + expect(yes_result).to have_css "td", text: "Yes" + expect(yes_result).to have_css "td", text: "3" + end + + page.find("tr#question_#{question.id}_1_result") do |no_result| + expect(no_result).to have_css "td", text: "No" + expect(no_result).to have_css "td", text: "5" + end + end + + it "shows 0 when an option has no partial results" do + render_inline Admin::Poll::Results::QuestionComponent.new(question, poll.partial_results) + + page.find("tr#question_#{question.id}_0_result") do |yes_result| + expect(yes_result).to have_css "td", text: "Yes" + expect(yes_result).to have_css "td", text: "0" + end + + page.find("tr#question_#{question.id}_1_result") do |no_result| + expect(no_result).to have_css "td", text: "No" + expect(no_result).to have_css "td", text: "0" + end + end + + it "ignores partial results from other questions" do + other_question = create(:poll_question, poll: poll) + create(:poll_question_option, question: other_question, title: "Yes") + create(:poll_question_option, question: other_question, title: "Irrelevant") + create(:poll_partial_result, question: other_question, answer: "Yes", amount: 9) + create(:poll_partial_result, question: other_question, answer: "Irrelevant", amount: 9) + + render_inline Admin::Poll::Results::QuestionComponent.new(question, poll.partial_results) + + expect(page).to have_css "td", text: "0", count: 2 + end +end diff --git a/spec/system/admin/poll/booth_assigments_spec.rb b/spec/system/admin/poll/booth_assigments_spec.rb index 0402b0d39..3548c57f6 100644 --- a/spec/system/admin/poll/booth_assigments_spec.rb +++ b/spec/system/admin/poll/booth_assigments_spec.rb @@ -228,7 +228,7 @@ describe "Admin booths assignments", :admin do expect(page).not_to have_css "#recounts_list" end - scenario "Results for a booth assignment" do + scenario "Recounts for a booth assignment" do poll = create(:poll) booth_assignment = create(:poll_booth_assignment, poll: poll) other_booth_assignment = create(:poll_booth_assignment, poll: poll) @@ -285,30 +285,6 @@ describe "Admin booths assignments", :admin do click_link "Results" - expect(page).to have_content(question_1.title) - - within("#question_#{question_1.id}_0_result") do - expect(page).to have_content("Yes") - expect(page).to have_content(11) - end - - within("#question_#{question_1.id}_1_result") do - expect(page).to have_content("No") - expect(page).to have_content(4) - end - - expect(page).to have_content(question_2.title) - - within("#question_#{question_2.id}_0_result") do - expect(page).to have_content("Today") - expect(page).to have_content(5) - end - - within("#question_#{question_2.id}_1_result") do - expect(page).to have_content("Tomorrow") - expect(page).to have_content(6) - end - within("#white_results") { expect(page).to have_content("21") } within("#null_results") { expect(page).to have_content("44") } within("#total_results") { expect(page).to have_content("66") } diff --git a/spec/system/admin/poll/polls_spec.rb b/spec/system/admin/poll/polls_spec.rb index 5559679f8..52c44137d 100644 --- a/spec/system/admin/poll/polls_spec.rb +++ b/spec/system/admin/poll/polls_spec.rb @@ -343,46 +343,6 @@ describe "Admin polls", :admin do expect(page).to have_content "There are no results" end - scenario "Show partial results" do - poll = create(:poll) - - booth_assignment_1 = create(:poll_booth_assignment, poll: poll) - booth_assignment_2 = create(:poll_booth_assignment, poll: poll) - booth_assignment_3 = create(:poll_booth_assignment, poll: poll) - - question_1 = create(:poll_question, poll: poll) - create(:poll_question_option, title: "Oui", question: question_1) - create(:poll_question_option, title: "Non", question: question_1) - - question_2 = create(:poll_question, poll: poll) - create(:poll_question_option, title: "Aujourd'hui", question: question_2) - create(:poll_question_option, title: "Demain", question: question_2) - - [booth_assignment_1, booth_assignment_2, booth_assignment_3].each do |ba| - create(:poll_partial_result, - booth_assignment: ba, - question: question_1, - answer: "Oui", - amount: 11) - - create(:poll_partial_result, - booth_assignment: ba, - question: question_2, - answer: "Demain", - amount: 5) - end - - create(:poll_recount, - booth_assignment: booth_assignment_1, - white_amount: 21, - null_amount: 44, - total_amount: 66) - - visit admin_poll_results_path(poll) - - expect(page).to have_content "Results by booth" - end - scenario "Enable stats and results for booth polls" do unvoted_poll = create(:poll) @@ -418,8 +378,9 @@ describe "Admin polls", :admin do expect(page).not_to have_content "Results by booth" end - scenario "Results by answer" do + scenario "Show results, recount and details by booth" do poll = create(:poll) + booth_assignment_1 = create(:poll_booth_assignment, poll: poll) booth_assignment_2 = create(:poll_booth_assignment, poll: poll) booth_assignment_3 = create(:poll_booth_assignment, poll: poll) @@ -427,7 +388,7 @@ describe "Admin polls", :admin do question_1 = create(:poll_question, :yes_no, poll: poll) question_2 = create(:poll_question, poll: poll) - create(:poll_question_option, title: "Today", question: question_2) + create(:poll_question_option, title: "Today", question: question_2) create(:poll_question_option, title: "Tomorrow", question: question_2) [booth_assignment_1, booth_assignment_2, booth_assignment_3].each do |ba| @@ -436,12 +397,14 @@ describe "Admin polls", :admin do question: question_1, answer: "Yes", amount: 11) + create(:poll_partial_result, booth_assignment: ba, question: question_2, answer: "Tomorrow", amount: 5) end + create(:poll_recount, booth_assignment: booth_assignment_1, white_amount: 21, @@ -449,60 +412,21 @@ describe "Admin polls", :admin do total_amount: 66) visit admin_poll_path(poll) - click_link "Results" - expect(page).to have_content(question_1.title) - question_1.question_options.each_with_index do |option, i| - within("#question_#{question_1.id}_#{i}_result") do - expect(page).to have_content(option.title) - expect(page).to have_content([33, 0][i]) - end - end - - expect(page).to have_content(question_2.title) - question_2.question_options.each_with_index do |option, i| - within("#question_#{question_2.id}_#{i}_result") do - expect(page).to have_content(option.title) - expect(page).to have_content([0, 15][i]) - end - end + expect(page).to have_content "Results by booth" within("#white_results") { expect(page).to have_content("21") } within("#null_results") { expect(page).to have_content("44") } within("#total_results") { expect(page).to have_content("66") } - end - scenario "Link to results by booth" do - poll = create(:poll) - booth_assignment1 = create(:poll_booth_assignment, poll: poll) - booth_assignment2 = create(:poll_booth_assignment, poll: poll) + expect(page).to have_link("See results", count: 3) - question = create(:poll_question, :yes_no, poll: poll) - - create(:poll_partial_result, - booth_assignment: booth_assignment1, - question: question, - answer: "Yes", - amount: 5) - - create(:poll_partial_result, - booth_assignment: booth_assignment2, - question: question, - answer: "Yes", - amount: 6) - - visit admin_poll_path(poll) - - click_link "Results" - - expect(page).to have_link("See results", count: 2) - - within("#booth_assignment_#{booth_assignment1.id}_result") do + within("#booth_assignment_#{booth_assignment_1.id}_result") do click_link "See results" end - expect(page).to have_content booth_assignment1.booth.name + expect(page).to have_content booth_assignment_1.booth.name expect(page).to have_content "Results" expect(page).to have_content "Yes" expect(page).to have_content "5" diff --git a/spec/system/officing/results_spec.rb b/spec/system/officing/results_spec.rb index 9eb49ad59..e17da5857 100644 --- a/spec/system/officing/results_spec.rb +++ b/spec/system/officing/results_spec.rb @@ -123,6 +123,9 @@ describe "Officing Results", :with_frozen_time do click_link "See results" end + expect(page).to have_content(I18n.l(Date.current.to_date, format: :long)) + expect(page).to have_content(booth_name) + within("#question_#{question_1.id}_0_result") do expect(page).to have_content("5555") expect(page).not_to have_content("7777") @@ -134,50 +137,4 @@ describe "Officing Results", :with_frozen_time do within("#question_#{question_1.id}_0_result") { expect(page).to have_content("5555") } within("#question_#{question_1.id}_1_result") { expect(page).to have_content("200") } end - - scenario "Index lists all questions and answers" do - officer_assignment = poll_officer.officer_assignments.first - booth_assignment = officer_assignment.booth_assignment - booth = booth_assignment.booth - - create( - :poll_partial_result, - officer_assignment: officer_assignment, - booth_assignment: booth_assignment, - date: poll.ends_at, - question: question_1, - amount: 33 - ) - - create( - :poll_recount, - officer_assignment: officer_assignment, - booth_assignment: booth_assignment, - date: poll.ends_at, - white_amount: 21, - null_amount: 44, - total_amount: 66 - ) - - visit officing_poll_results_path(poll, - date: I18n.l(poll.ends_at.to_date), - booth_assignment_id: officer_assignment.booth_assignment_id) - - expect(page).to have_content(I18n.l(poll.ends_at.to_date, format: :long)) - expect(page).to have_content(booth.name) - - expect(page).to have_content(question_1.title) - question_1.question_options.each_with_index do |answer, i| - within("#question_#{question_1.id}_#{i}_result") { expect(page).to have_content(answer.title) } - end - - expect(page).to have_content(question_2.title) - question_2.question_options.each_with_index do |answer, i| - within("#question_#{question_2.id}_#{i}_result") { expect(page).to have_content(answer.title) } - end - - within("#white_results") { expect(page).to have_content("21") } - within("#null_results") { expect(page).to have_content("44") } - within("#total_results") { expect(page).to have_content("66") } - end end From 7996933fc2a4aba0a409bfc7c6a8a036c9418f78 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 17 Sep 2025 10:44:28 +0200 Subject: [PATCH 04/13] Extract by_answer variable to component method --- .../admin/poll/results/question_component.html.erb | 1 - .../admin/poll/results/question_component.rb | 10 ++++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/components/admin/poll/results/question_component.html.erb b/app/components/admin/poll/results/question_component.html.erb index 9cf0cd4d1..aac83af2f 100644 --- a/app/components/admin/poll/results/question_component.html.erb +++ b/app/components/admin/poll/results/question_component.html.erb @@ -9,7 +9,6 @@ <% question.question_options.each_with_index do |option, i| %> - <% by_answer = by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} %> <%= option.title %> <%= by_answer[option.title].present? ? by_answer[option.title].sum(&:amount) : 0 %> diff --git a/app/components/admin/poll/results/question_component.rb b/app/components/admin/poll/results/question_component.rb index 2bee2a328..2b14743ce 100644 --- a/app/components/admin/poll/results/question_component.rb +++ b/app/components/admin/poll/results/question_component.rb @@ -6,7 +6,13 @@ class Admin::Poll::Results::QuestionComponent < ApplicationComponent @partial_results = partial_results end - def by_question - @by_question ||= partial_results.group_by(&:question_id) + def by_answer + @by_answer ||= by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} end + + private + + def by_question + @by_question ||= partial_results.group_by(&:question_id) + end end From 5945bfe9ed42f6d92d0d6efae5f9a143d59260db Mon Sep 17 00:00:00 2001 From: taitus Date: Tue, 16 Sep 2025 14:11:41 +0200 Subject: [PATCH 05/13] Replace poll result partial with Admin::Poll::Results::QuestionComponent --- app/views/admin/poll/booth_assignments/_results.html.erb | 5 ++++- app/views/admin/poll/results/_result.html.erb | 3 --- app/views/admin/poll/results/index.html.erb | 6 +++++- 3 files changed, 9 insertions(+), 5 deletions(-) delete mode 100644 app/views/admin/poll/results/_result.html.erb diff --git a/app/views/admin/poll/booth_assignments/_results.html.erb b/app/views/admin/poll/booth_assignments/_results.html.erb index 535485096..d5a4b1fe2 100644 --- a/app/views/admin/poll/booth_assignments/_results.html.erb +++ b/app/views/admin/poll/booth_assignments/_results.html.erb @@ -7,6 +7,9 @@ <% else %> <%= render "admin/poll/results/recount", resource: @booth_assignment %> - <%= render "admin/poll/results/result" %> + + <% @poll.questions.each do |question| %> + <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> + <% end %> <% end %> diff --git a/app/views/admin/poll/results/_result.html.erb b/app/views/admin/poll/results/_result.html.erb deleted file mode 100644 index 05b287253..000000000 --- a/app/views/admin/poll/results/_result.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<% @poll.questions.each do |question| %> - <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> -<% end %> diff --git a/app/views/admin/poll/results/index.html.erb b/app/views/admin/poll/results/index.html.erb index eaf6ad745..36d862246 100644 --- a/app/views/admin/poll/results/index.html.erb +++ b/app/views/admin/poll/results/index.html.erb @@ -13,7 +13,11 @@ <% if @partial_results.present? %> <%= render "recount", resource: @poll %> - <%= render "result" %> + + <% @poll.questions.each do |question| %> + <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> + <% end %> + <%= render "results_by_booth" %> <% end %> From 7565fc5fc2aabf47db1fd5a439741306365b8e6b Mon Sep 17 00:00:00 2001 From: taitus Date: Tue, 16 Sep 2025 14:56:45 +0200 Subject: [PATCH 06/13] Remove redundant by_question grouping in Admin::Poll::Results::QuestionComponent Stop grouping partial results by question_id inside the component. Note that group_by on an empty collection already returns an empty hash, so the previous "|| {}" is not needed. --- app/components/admin/poll/results/question_component.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/components/admin/poll/results/question_component.rb b/app/components/admin/poll/results/question_component.rb index 2b14743ce..c12d9d344 100644 --- a/app/components/admin/poll/results/question_component.rb +++ b/app/components/admin/poll/results/question_component.rb @@ -7,12 +7,6 @@ class Admin::Poll::Results::QuestionComponent < ApplicationComponent end def by_answer - @by_answer ||= by_question[question.id].present? ? by_question[question.id].group_by(&:answer) : {} + @by_answer ||= partial_results.where(question: question).group_by(&:answer) end - - private - - def by_question - @by_question ||= partial_results.group_by(&:question_id) - end end From e3e475b4df2fadb9d3c07eb35146d493d067c348 Mon Sep 17 00:00:00 2001 From: taitus Date: Tue, 16 Sep 2025 15:19:11 +0200 Subject: [PATCH 07/13] Add votes_for(option) method and simplify results template Move the summing logic from the template into the component. Introduce a votes_for(option) method that looks up grouped partial results and returns the total amount or 0. --- .../admin/poll/results/question_component.html.erb | 2 +- .../admin/poll/results/question_component.rb | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/app/components/admin/poll/results/question_component.html.erb b/app/components/admin/poll/results/question_component.html.erb index aac83af2f..8a3d6633a 100644 --- a/app/components/admin/poll/results/question_component.html.erb +++ b/app/components/admin/poll/results/question_component.html.erb @@ -11,7 +11,7 @@ <% question.question_options.each_with_index do |option, i| %> <%= option.title %> - <%= by_answer[option.title].present? ? by_answer[option.title].sum(&:amount) : 0 %> + <%= votes_for(option) %> <% end %> diff --git a/app/components/admin/poll/results/question_component.rb b/app/components/admin/poll/results/question_component.rb index c12d9d344..8836a6b91 100644 --- a/app/components/admin/poll/results/question_component.rb +++ b/app/components/admin/poll/results/question_component.rb @@ -6,7 +6,14 @@ class Admin::Poll::Results::QuestionComponent < ApplicationComponent @partial_results = partial_results end - def by_answer - @by_answer ||= partial_results.where(question: question).group_by(&:answer) + def votes_for(option) + grouped = by_answer[option.title] || [] + grouped.sum(&:amount) end + + private + + def by_answer + @by_answer ||= partial_results.where(question: question).group_by(&:answer) + end end From f2153f2b4d833b65882f77b0f0c3ba7a428335a6 Mon Sep 17 00:00:00 2001 From: taitus Date: Tue, 16 Sep 2025 10:01:59 +0200 Subject: [PATCH 08/13] Extract officing results index to component --- .../officing/results/index_component.html.erb | 41 +++++++++++++++++++ .../officing/results/index_component.rb | 10 +++++ app/views/officing/results/index.html.erb | 40 +----------------- 3 files changed, 52 insertions(+), 39 deletions(-) create mode 100644 app/components/officing/results/index_component.html.erb create mode 100644 app/components/officing/results/index_component.rb diff --git a/app/components/officing/results/index_component.html.erb b/app/components/officing/results/index_component.html.erb new file mode 100644 index 000000000..046f2319d --- /dev/null +++ b/app/components/officing/results/index_component.html.erb @@ -0,0 +1,41 @@ +<% provide :main_class, "officing-results-index" %> + +<%= back_link_to new_officing_poll_result_path(poll) %> +

<%= poll.name %> - <%= t("officing.results.index.results") %>

+ +<% if partial_results.present? %> +
+

+ <%= booth_assignment.booth.name %> - <%= l partial_results.first.date, format: :long %> +

+
+ +
+
+ + + + + + + + + + + + + + + +
<%= t("officing.results.index.table_whites") %><%= t("officing.results.index.table_nulls") %><%= t("officing.results.index.table_total") %>
<%= recounts.sum(:white_amount) %><%= recounts.sum(:null_amount) %><%= recounts.sum(:total_amount) %>
+ + <% @poll.questions.each do |question| %> + <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> + <% end %> +
+
+<% else %> +
+ <%= t("officing.results.index.no_results") %> +
+<% end %> diff --git a/app/components/officing/results/index_component.rb b/app/components/officing/results/index_component.rb new file mode 100644 index 000000000..e4e86a9fe --- /dev/null +++ b/app/components/officing/results/index_component.rb @@ -0,0 +1,10 @@ +class Officing::Results::IndexComponent < ApplicationComponent + attr_reader :poll, :partial_results, :booth_assignment, :recounts + + def initialize(poll, partial_results, booth_assignment, recounts) + @poll = poll + @partial_results = partial_results + @booth_assignment = booth_assignment + @recounts = recounts + end +end diff --git a/app/views/officing/results/index.html.erb b/app/views/officing/results/index.html.erb index 9b0d330c5..946c5611d 100644 --- a/app/views/officing/results/index.html.erb +++ b/app/views/officing/results/index.html.erb @@ -1,39 +1 @@ -<%= back_link_to new_officing_poll_result_path(@poll) %> -

<%= @poll.name %> - <%= t("officing.results.index.results") %>

- -<% if @partial_results.present? %> -
-

- <%= @booth_assignment.booth.name %> - <%= l @partial_results.first.date, format: :long %> -

-
- -
-
- - - - - - - - - - - - - - - -
<%= t("officing.results.index.table_whites") %><%= t("officing.results.index.table_nulls") %><%= t("officing.results.index.table_total") %>
<%= @recounts.sum(:white_amount) %><%= @recounts.sum(:null_amount) %><%= @recounts.sum(:total_amount) %>
- - <% @poll.questions.each do |question| %> - <%= render Admin::Poll::Results::QuestionComponent.new(question, @partial_results) %> - <% end %> -
-
-<% else %> -
- <%= t("officing.results.index.no_results") %> -
-<% end %> +<%= render Officing::Results::IndexComponent.new(@poll, @partial_results, @booth_assignment, @recounts) %> From a29eeaf2e273af660f749e0df9bd7f45a240c768 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 10 Sep 2025 12:22:08 +0200 Subject: [PATCH 09/13] 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 From e286ee69435484f8e4e8249c6ca2de07f536999f Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 10 Sep 2025 15:09:04 +0200 Subject: [PATCH 10/13] Add task to delete duplicate poll partial results Adds rake task "polls:remove_duplicate_partial_results" to delete duplicated rows in "poll_partial_results" made before the DB was strict about duplicates. Duplicates are considered only for records without "option_id", grouping by: (question_id, booth_assignment_id, date, answer). We keep the first one and delete the rest, per tenant. The controller use: Poll::PartialResult.find_or_initialize_by(booth_assignment_id, date, question_id, answer) which is not a strong protection against race conditions. Without a unique index at the DB level, duplicates could be created. This task cleans up any existing duplicates. --- lib/tasks/consul.rake | 8 ++-- lib/tasks/polls.rake | 40 +++++++++++++++++ spec/lib/tasks/polls_spec.rb | 84 ++++++++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 3 deletions(-) diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index a43bdaaf6..9ad01c340 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -3,8 +3,10 @@ namespace :consul do task execute_release_tasks: ["settings:rename_setting_keys", "settings:add_new_settings", "cache:clear", - "execute_release_2.3.0_tasks"] + "execute_release_2.4.0_tasks"] - desc "Runs tasks needed to upgrade from 2.2.2 to 2.3.0" - task "execute_release_2.3.0_tasks": [] + desc "Runs tasks needed to upgrade from 2.3.1 to 2.4.0" + task "execute_release_2.4.0_tasks": [ + "polls:remove_duplicate_partial_results" + ] end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index a3ce11e4b..95cfe5799 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -107,4 +107,44 @@ namespace :polls do end end end + + desc "Removes duplicate poll partial results" + task remove_duplicate_partial_results: :environment do + logger = ApplicationLogger.new + duplicate_records_logger = DuplicateRecordsLogger.new + + logger.info "Removing duplicate partial results in polls" + + Tenant.run_on_each do + duplicate_ids = Poll::PartialResult.where(option_id: nil) + .select(:question_id, :booth_assignment_id, :date, :answer) + .group(:question_id, :booth_assignment_id, :date, :answer) + .having("count(*) > 1") + .pluck(:question_id, :booth_assignment_id, :date, :answer) + + duplicate_ids.each do |question_id, booth_assignment_id, date, answer| + partial_results = Poll::PartialResult.where( + question_id: question_id, + booth_assignment_id: booth_assignment_id, + date: date, + answer: answer, + option_id: nil + ) + + partial_results.excluding(partial_results.first).each do |partial_result| + partial_result.delete + + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + log_message = "Deleted duplicate record with ID #{partial_result.id} " \ + "from the #{Poll::PartialResult.table_name} table " \ + "with question_id #{question_id}, " \ + "booth_assignment_id #{booth_assignment_id}, " \ + "date #{date} " \ + "and answer #{answer}" + tenant_info.to_s + logger.info(log_message) + duplicate_records_logger.info(log_message) + end + end + end + end end diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index 06f40b979..af035e50f 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -223,4 +223,88 @@ describe "polls tasks" do end end end + + describe "polls:remove_duplicate_partial_results" do + before { Rake::Task["polls:remove_duplicate_partial_results"].reenable } + + let(:booth_assignment) { create(:poll_booth_assignment) } + let(:other_booth_assignment) { create(:poll_booth_assignment) } + + it "removes duplicate partial results" do + question = create(:poll_question_multiple, :abcde, poll: poll, max_votes: 4) + + result_attributes = { + question_id: question.id, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A", + option_id: nil + } + other_result_attributes = result_attributes.merge(answer: "Answer B") + + result = create(:poll_partial_result, result_attributes) + other_result = create(:poll_partial_result, other_result_attributes) + other_booth_result = create(:poll_partial_result, + result_attributes.merge(booth_assignment_id: other_booth_assignment.id)) + + 2.times { insert(:poll_partial_result, result_attributes) } + insert(:poll_partial_result, other_result_attributes) + insert(:poll_partial_result, result_attributes.merge(booth_assignment_id: other_booth_assignment.id)) + + expect(Poll::PartialResult.count).to eq 7 + + Rake.application.invoke_task("polls:remove_duplicate_partial_results") + + expect(Poll::PartialResult.count).to eq 3 + expect(Poll::PartialResult.all).to match_array [result, other_result, other_booth_result] + end + + it "does not remove partial results 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") + + result_attributes = { + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A" + } + create(:poll_partial_result, result_attributes.merge(option: option_a)) + insert(:poll_partial_result, result_attributes.merge(option_id: option_b.id)) + + expect(Poll::PartialResult.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_partial_results") + + expect(Poll::PartialResult.count).to eq 2 + end + + it "removes duplicate partial results on tenants" do + create(:tenant, schema: "partial_results") + + Tenant.switch("partial_results") do + question = create(:poll_question_multiple, :abc) + booth_assignment = create(:poll_booth_assignment) + + result_attributes = { + question_id: question.id, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A", + option_id: nil + } + create(:poll_partial_result, result_attributes) + insert(:poll_partial_result, result_attributes) + + expect(Poll::PartialResult.count).to eq 2 + end + + Rake.application.invoke_task("polls:remove_duplicate_partial_results") + + Tenant.switch("partial_results") do + expect(Poll::PartialResult.count).to eq 1 + end + end + end end From ed2a25663b824536a6b08714185a611d46342964 Mon Sep 17 00:00:00 2001 From: taitus Date: Wed, 10 Sep 2025 16:12:50 +0200 Subject: [PATCH 11/13] Add task to add option_id to existing partial results --- lib/tasks/consul.rake | 2 +- lib/tasks/polls.rake | 37 ++++++++++ spec/lib/tasks/polls_spec.rb | 128 ++++++++++++++++++++++++++++++++++- 3 files changed, 163 insertions(+), 4 deletions(-) diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 9ad01c340..a6eb30dbf 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -7,6 +7,6 @@ namespace :consul do desc "Runs tasks needed to upgrade from 2.3.1 to 2.4.0" task "execute_release_2.4.0_tasks": [ - "polls:remove_duplicate_partial_results" + "polls:populate_partial_results_option_id" ] end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index 95cfe5799..8645243f7 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -147,4 +147,41 @@ namespace :polls do end end end + + desc "populates the poll_partial_results option_id column" + task populate_partial_results_option_id: :remove_duplicate_partial_results do + logger = ApplicationLogger.new + logger.info "Updating option_id column in poll_partial_results" + + Tenant.run_on_each do + Poll::Question.find_each do |question| + options = question.question_options.joins(:translations).reorder(:id) + existing_choices = question.partial_results.where(option_id: nil).distinct.pluck(:answer) + + choices_map = existing_choices.to_h do |choice| + [choice, options.where("lower(title) = lower(?)", choice).distinct.ids] + end + + manageable_choices, unmanageable_choices = choices_map.partition { |choice, ids| ids.count == 1 } + + manageable_choices.each do |choice, ids| + question.partial_results.where(option_id: nil, answer: choice).update_all(option_id: ids.first) + end + + unmanageable_choices.each do |choice, ids| + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + + if ids.count == 0 + logger.warn "Skipping poll_partial_results 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_partial_results 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/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index af035e50f..98ed48283 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -3,6 +3,8 @@ require "rails_helper" describe "polls tasks" do let(:poll) { create(:poll) } let(:user) { create(:user, :level_two) } + let(:booth_assignment) { create(:poll_booth_assignment) } + let(:other_booth_assignment) { create(:poll_booth_assignment) } describe "polls:remove_duplicate_answers" do before { Rake::Task["polls:remove_duplicate_answers"].reenable } @@ -227,9 +229,6 @@ describe "polls tasks" do describe "polls:remove_duplicate_partial_results" do before { Rake::Task["polls:remove_duplicate_partial_results"].reenable } - let(:booth_assignment) { create(:poll_booth_assignment) } - let(:other_booth_assignment) { create(:poll_booth_assignment) } - it "removes duplicate partial results" do question = create(:poll_question_multiple, :abcde, poll: poll, max_votes: 4) @@ -307,4 +306,127 @@ describe "polls tasks" do end end end + + describe "polls:populate_partial_results_option_id" do + before do + Rake::Task["polls:remove_duplicate_partial_results"].reenable + Rake::Task["polls:populate_partial_results_option_id"].reenable + end + + it "populates the option_id column of existing partial results when there's one valid option" 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") + + result = create(:poll_partial_result, + question: yes_no_question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Yes", + option: nil) + abc_result = create(:poll_partial_result, + question: abc_question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A", + option: nil) + insert(:poll_partial_result, + question: abc_question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A", + option_id: option_b.id) + inconsistent_result = Poll::PartialResult.find_by!(option: option_b) + invalid_result = build(:poll_partial_result, + question: abc_question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Non existing", + option: nil) + invalid_result.save!(validate: false) + + Rake.application.invoke_task("polls:populate_partial_results_option_id") + result.reload + abc_result.reload + inconsistent_result.reload + invalid_result.reload + + expect(result.option_id).to eq yes_no_question.question_options.find_by(title: "Yes").id + expect(abc_result.option_id).to eq option_a.id + expect(inconsistent_result.option_id).to eq option_b.id + expect(invalid_result.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") + + result = create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "I", + option: nil) + + Rake.application.invoke_task("polls:populate_partial_results_option_id") + result.reload + + expect(result.option_id).to be nil + end + + it "removes duplicate partial results before populating the option_id column" do + question = create(:poll_question_multiple, :abc) + + result_attributes = { + question_id: question.id, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A", + option_id: nil + } + result = create(:poll_partial_result, result_attributes) + insert(:poll_partial_result, result_attributes) + + result.reload + expect(result.option_id).to be nil + + Rake.application.invoke_task("polls:populate_partial_results_option_id") + result.reload + + expect(Poll::PartialResult.count).to eq 1 + expect(result.option_id).to eq question.question_options.find_by(title: "Answer A").id + end + + it "populates the option_id column on tenants" do + create(:tenant, schema: "partial_results") + + Tenant.switch("partial_results") do + question = create(:poll_question_multiple, :abc) + booth_assignment = create(:poll_booth_assignment) + + create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Answer A", + option: nil) + end + + Rake.application.invoke_task("polls:populate_partial_results_option_id") + + Tenant.switch("partial_results") do + expect(Poll::Question.count).to eq 1 + expect(Poll::PartialResult.count).to eq 1 + + question = Poll::Question.first + option_a = question.question_options.find_by(title: "Answer A") + + expect(Poll::PartialResult.first.option_id).to eq option_a.id + end + end + end end From c9fb47aa3d12b0c2cc265f2cddd4b94c79655380 Mon Sep 17 00:00:00 2001 From: taitus Date: Thu, 11 Sep 2025 09:31:24 +0200 Subject: [PATCH 12/13] Add PollPartialResultOptionFinder to extend PollOptionFinder Introduce a dedicated finder for partial results, reusing the logic of PollOptionFinder. This will be used in rake tasks to avoid code duplication and make the intent clearer. --- app/lib/poll_partial_result_option_finder.rb | 7 +++++++ lib/tasks/polls.rake | 13 +++---------- 2 files changed, 10 insertions(+), 10 deletions(-) create mode 100644 app/lib/poll_partial_result_option_finder.rb diff --git a/app/lib/poll_partial_result_option_finder.rb b/app/lib/poll_partial_result_option_finder.rb new file mode 100644 index 000000000..1bf64da83 --- /dev/null +++ b/app/lib/poll_partial_result_option_finder.rb @@ -0,0 +1,7 @@ +class PollPartialResultOptionFinder < PollOptionFinder + private + + def existing_choices + question.partial_results.where(option_id: nil).distinct.pluck(:answer) + end +end diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index 8645243f7..2d7d2d064 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -155,20 +155,13 @@ namespace :polls do Tenant.run_on_each do Poll::Question.find_each do |question| - options = question.question_options.joins(:translations).reorder(:id) - existing_choices = question.partial_results.where(option_id: nil).distinct.pluck(:answer) + option_finder = PollPartialResultOptionFinder.new(question) - choices_map = existing_choices.to_h do |choice| - [choice, options.where("lower(title) = lower(?)", choice).distinct.ids] - end - - manageable_choices, unmanageable_choices = choices_map.partition { |choice, ids| ids.count == 1 } - - manageable_choices.each do |choice, ids| + option_finder.manageable_choices.each do |choice, ids| question.partial_results.where(option_id: nil, answer: choice).update_all(option_id: ids.first) end - unmanageable_choices.each do |choice, ids| + option_finder.unmanageable_choices.each do |choice, ids| tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? if ids.count == 0 From 24239c98e3ae5f759ec00b257762c8c4611e0605 Mon Sep 17 00:00:00 2001 From: taitus Date: Thu, 11 Sep 2025 09:33:01 +0200 Subject: [PATCH 13/13] Delete duplicate records in different languages Also logs a message when duplicates have different amounts, keeping the first partial result and deleting the others. --- lib/tasks/polls.rake | 69 +++++++++++++++-------- spec/lib/tasks/polls_spec.rb | 103 ++++++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 25 deletions(-) diff --git a/lib/tasks/polls.rake b/lib/tasks/polls.rake index 2d7d2d064..2fb3a5adf 100644 --- a/lib/tasks/polls.rake +++ b/lib/tasks/polls.rake @@ -116,33 +116,54 @@ namespace :polls do logger.info "Removing duplicate partial results in polls" Tenant.run_on_each do - duplicate_ids = Poll::PartialResult.where(option_id: nil) - .select(:question_id, :booth_assignment_id, :date, :answer) - .group(:question_id, :booth_assignment_id, :date, :answer) - .having("count(*) > 1") - .pluck(:question_id, :booth_assignment_id, :date, :answer) + Poll::Question.find_each do |question| + manageable_titles = PollPartialResultOptionFinder.new(question).manageable_choices.keys - duplicate_ids.each do |question_id, booth_assignment_id, date, answer| - partial_results = Poll::PartialResult.where( - question_id: question_id, - booth_assignment_id: booth_assignment_id, - date: date, - answer: answer, - option_id: nil - ) + question.question_options.each do |option| + titles = option.translations.where(title: manageable_titles).select(:title).distinct - partial_results.excluding(partial_results.first).each do |partial_result| - partial_result.delete + groups = question.partial_results.where(option_id: nil, answer: titles) + .select(:booth_assignment_id, :date) + .group(:booth_assignment_id, :date) + .having("count(*) > 1") + .pluck(:booth_assignment_id, :date) - tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? - log_message = "Deleted duplicate record with ID #{partial_result.id} " \ - "from the #{Poll::PartialResult.table_name} table " \ - "with question_id #{question_id}, " \ - "booth_assignment_id #{booth_assignment_id}, " \ - "date #{date} " \ - "and answer #{answer}" + tenant_info.to_s - logger.info(log_message) - duplicate_records_logger.info(log_message) + groups.each do |booth_assignment_id, date| + partial_results = question.partial_results.where( + option_id: nil, + booth_assignment_id: booth_assignment_id, + date: date, + answer: titles + ) + + tenant_info = " on tenant #{Tenant.current_schema}" unless Tenant.default? + + amounts_by_id = partial_results.pluck(:id, :amount).to_h + if amounts_by_id.values.uniq.size > 1 + log_message = "Found duplicate partial results with different amounts " \ + "for question_id #{question.id}, " \ + "booth_assignment_id #{booth_assignment_id} " \ + "and date #{date}. " \ + "Keeping ID #{partial_results.first.id} " \ + "with amount #{partial_results.first.amount}. " \ + "Deleting partial results with these IDs and amounts: " \ + "#{amounts_by_id.except(partial_results.first.id)}" + tenant_info.to_s + logger.info(log_message) + duplicate_records_logger.info(log_message) + end + + partial_results.excluding(partial_results.first).each do |partial_result| + partial_result.delete + + log_message = "Deleted duplicate record with ID #{partial_result.id} " \ + "from the #{Poll::PartialResult.table_name} table " \ + "with question_id #{question.id}, " \ + "booth_assignment_id #{booth_assignment_id} " \ + "and date #{date}" + tenant_info.to_s + logger.info(log_message) + duplicate_records_logger.info(log_message) + end + end end end end diff --git a/spec/lib/tasks/polls_spec.rb b/spec/lib/tasks/polls_spec.rb index 98ed48283..10ad6a93b 100644 --- a/spec/lib/tasks/polls_spec.rb +++ b/spec/lib/tasks/polls_spec.rb @@ -258,6 +258,60 @@ describe "polls tasks" do expect(Poll::PartialResult.all).to match_array [result, other_result, other_booth_result] end + it "removes duplicate partial results 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_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Yes", + option: nil) + create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Ja", + option: nil) + + expect(Poll::PartialResult.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_partial_results") + + expect(Poll::PartialResult.count).to eq 1 + end + + it "does not remove duplicate partial results when many options are possible" 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") + + create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "I", + option: nil) + create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "AI", + option: nil) + + expect(Poll::PartialResult.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_partial_results") + + expect(Poll::PartialResult.count).to eq 2 + end + it "does not remove partial results 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") @@ -305,6 +359,34 @@ describe "polls tasks" do expect(Poll::PartialResult.count).to eq 1 end end + + it "removes duplicates in different languages even when amounts differ" do + question = create(:poll_question_multiple, max_votes: 2) + + create(:poll_question_option, question: question, title_en: "Yes", title_de: "Ja") + + create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Yes", + option: nil, + amount: 3) + + create(:poll_partial_result, + question: question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + answer: "Ja", + option: nil, + amount: 5) + + expect(Poll::PartialResult.count).to eq 2 + + Rake.application.invoke_task("polls:remove_duplicate_partial_results") + + expect(Poll::PartialResult.count).to eq 1 + end end describe "polls:populate_partial_results_option_id" do @@ -381,6 +463,11 @@ describe "polls tasks" do it "removes duplicate partial results before populating the option_id column" do 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") + result_attributes = { question_id: question.id, booth_assignment_id: booth_assignment.id, @@ -391,14 +478,28 @@ describe "polls tasks" do result = create(:poll_partial_result, result_attributes) insert(:poll_partial_result, result_attributes) + localized_result_attributes = { + question: localized_question, + booth_assignment_id: booth_assignment.id, + date: Date.current, + option: nil + } + localized_result = create(:poll_partial_result, localized_result_attributes.merge(answer: "Yes")) + create(:poll_partial_result, localized_result_attributes.merge(answer: "Ja")) + result.reload + localized_result.reload + expect(result.option_id).to be nil + expect(localized_result.option_id).to be nil Rake.application.invoke_task("polls:populate_partial_results_option_id") result.reload + localized_result.reload - expect(Poll::PartialResult.count).to eq 1 + expect(Poll::PartialResult.count).to eq 2 expect(result.option_id).to eq question.question_options.find_by(title: "Answer A").id + expect(localized_result.option_id).to eq localized_question.question_options.find_by(title: "Yes").id end it "populates the option_id column on tenants" do