From dd28163be7ff3a0103a0247a1c45d37269ea39be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 2 Feb 2023 20:52:06 +0100 Subject: [PATCH] Show admin heading stats for the current budget To get the heading where a user voted, we were relying on the `balloted_heading_id` field. Our guess is this was done so the total number of users is the same as the sum of users who voted on a heading. That is, if 2000 people voted just on the "All city" heading, 1000 voted just on the "North district" heading, and 500 people voted on both, instead of showing "3500 people voted in total, 2500 voted in all city, 1500 voted in north district", we show something like "3500 people voted in total, 2250 voted in all city, and 1250 voted in north district". However, this approach has some disadvantages. The first disadvantage is, the stats aren't correct. In the case above, 2500 voted on the "All city heading", so the statistics for this heading don't show reality. The second one is we weren't considering the last heading where users voted inside the budget being displayed, but the last heading where users voted, period. That means that, if all the people above voted on a later budget, the stats for the budget above would become "3500 people voted in total, 0 voted in all city, and 0 voted in north district". That also means we were including headings from previous budgets in the statistics for more recent budgets when people hadn't voted on the recent ones. So we're removing the `balloted_heading_id` since its data is lost once people vote on a new budget. And, in order to show the right stats and simplify the code, we're no longer trying to add votes just to one heading when users vote on several headings. Co-Authored-By: Julian Nicolas Herrero --- .../stats/budget_balloting_component.html.erb | 2 +- .../admin/stats/budget_balloting_component.rb | 6 ++- app/models/budget/ballot/line.rb | 5 -- ...2_remove_balloted_heading_id_from_users.rb | 5 ++ db/schema.rb | 3 +- .../stats/budget_balloting_component_spec.rb | 54 ++++++++++++++++++- spec/models/budget/ballot/line_spec.rb | 12 ----- 7 files changed, 65 insertions(+), 22 deletions(-) create mode 100644 db/migrate/20230206141152_remove_balloted_heading_id_from_users.rb diff --git a/app/components/admin/stats/budget_balloting_component.html.erb b/app/components/admin/stats/budget_balloting_component.html.erb index 4d118b732..4e13af338 100644 --- a/app/components/admin/stats/budget_balloting_component.html.erb +++ b/app/components/admin/stats/budget_balloting_component.html.erb @@ -45,7 +45,7 @@ - +
diff --git a/app/components/admin/stats/budget_balloting_component.rb b/app/components/admin/stats/budget_balloting_component.rb index de1b06a0f..4d604cca3 100644 --- a/app/components/admin/stats/budget_balloting_component.rb +++ b/app/components/admin/stats/budget_balloting_component.rb @@ -20,6 +20,10 @@ class Admin::Stats::BudgetBallotingComponent < ApplicationComponent end def user_count_by_heading - User.where.not(balloted_heading_id: nil).group(:balloted_heading_id).count.map { |k, v| [Budget::Heading.find(k).name, v] }.sort + budget.headings.map do |heading| + ballots = budget.ballots.joins(:lines).where(budget_ballot_lines: { heading_id: heading }) + + [heading.name, ballots.select(:user_id).distinct.count] + end.select { |_, count| count > 0 }.sort end end diff --git a/app/models/budget/ballot/line.rb b/app/models/budget/ballot/line.rb index 58078de98..635f33499 100644 --- a/app/models/budget/ballot/line.rb +++ b/app/models/budget/ballot/line.rb @@ -16,7 +16,6 @@ class Budget scope :by_investment, ->(investment_id) { where(investment_id: investment_id) } before_validation :set_denormalized_ids - after_save :store_user_heading def check_enough_resources ballot.lock! @@ -43,10 +42,6 @@ class Budget self.group_id ||= investment&.group_id self.budget_id ||= investment&.budget_id end - - def store_user_heading - ballot.user.update(balloted_heading_id: heading.id) unless ballot.physical == true - end end end end diff --git a/db/migrate/20230206141152_remove_balloted_heading_id_from_users.rb b/db/migrate/20230206141152_remove_balloted_heading_id_from_users.rb new file mode 100644 index 000000000..cc2d3bbe5 --- /dev/null +++ b/db/migrate/20230206141152_remove_balloted_heading_id_from_users.rb @@ -0,0 +1,5 @@ +class RemoveBallotedHeadingIdFromUsers < ActiveRecord::Migration[6.0] + def change + remove_column :users, :balloted_heading_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index be20fc451..7b3a7349d 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.define(version: 2022_12_03_140136) do +ActiveRecord::Schema.define(version: 2023_02_06_141152) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -1637,7 +1637,6 @@ ActiveRecord::Schema.define(version: 2022_12_03_140136) do t.boolean "created_from_signature", default: false t.integer "failed_email_digests_count", default: 0 t.text "former_users_data_log", default: "" - t.integer "balloted_heading_id" t.boolean "public_interests", default: false t.boolean "recommended_debates", default: true t.boolean "recommended_proposals", default: true diff --git a/spec/components/admin/stats/budget_balloting_component_spec.rb b/spec/components/admin/stats/budget_balloting_component_spec.rb index 9595559ef..d70efe979 100644 --- a/spec/components/admin/stats/budget_balloting_component_spec.rb +++ b/spec/components/admin/stats/budget_balloting_component_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" describe Admin::Stats::BudgetBallotingComponent do let(:budget) { create(:budget, :balloting) } - let(:heading) { create(:budget_heading, budget: budget) } + let(:heading) { create(:budget_heading, budget: budget, name: "Main heading") } let(:investment) { create(:budget_investment, :feasible, :selected, heading: heading) } it "shows the number of votes in investment projects" do @@ -25,4 +25,56 @@ describe Admin::Stats::BudgetBallotingComponent do expect(page).to have_css "p", exact_text: "Participants 2", normalize_ws: true end + + it "does not show participants per district from old budgets" do + old_budget = create(:budget, :balloting) + old_heading = create(:budget_heading, budget: old_budget, name: "Old Heading") + old_investment = create(:budget_investment, :feasible, :selected, heading: old_heading) + create(:user, ballot_lines: [old_investment]) + create(:user, ballot_lines: [old_investment]) + + create(:user, ballot_lines: [investment]) + + render_inline Admin::Stats::BudgetBallotingComponent.new(budget) + + expect(page).to have_css "p", exact_text: "Votes 1", normalize_ws: true + expect(page).to have_css "p", exact_text: "Participants 1", normalize_ws: true + expect(page).not_to have_content "Old Heading" + end + + it "keeps votes on old budgets when there are new votes" do + old_budget = create(:budget, :balloting) + old_heading = create(:budget_heading, budget: old_budget, name: "Old Heading") + old_investment = create(:budget_investment, :feasible, :selected, heading: old_heading) + user = create(:user) + + create(:budget_ballot, user: user, budget: old_budget, investments: [old_investment]) + create(:budget_ballot, user: user, budget: budget, investments: [investment]) + + render_inline Admin::Stats::BudgetBallotingComponent.new(old_budget) + + expect(page).to have_css "p", exact_text: "Votes 1", normalize_ws: true + expect(page).to have_css "p", exact_text: "Participants 1", normalize_ws: true + + page.find ".user-count-by-heading tbody" do |table_body| + expect(table_body).to have_css "tr", exact_text: "Old Heading 1", normalize_ws: true + end + end + + it "counts a participant in all headings where the participant voted" do + another_heading = create(:budget_heading, budget: budget, name: "Another heading") + another_investment = create(:budget_investment, :selected, heading: another_heading) + + create(:user, ballot_lines: [investment, another_investment]) + + render_inline Admin::Stats::BudgetBallotingComponent.new(budget) + + expect(page).to have_css "p", exact_text: "Votes 2", normalize_ws: true + expect(page).to have_css "p", exact_text: "Participants 1", normalize_ws: true + + page.find ".user-count-by-heading tbody" do |table_body| + expect(table_body).to have_css "tr", exact_text: "Main heading 1", normalize_ws: true + expect(table_body).to have_css "tr", exact_text: "Another heading 1", normalize_ws: true + end + end end diff --git a/spec/models/budget/ballot/line_spec.rb b/spec/models/budget/ballot/line_spec.rb index c9c8a887d..329b48e38 100644 --- a/spec/models/budget/ballot/line_spec.rb +++ b/spec/models/budget/ballot/line_spec.rb @@ -88,18 +88,6 @@ describe Budget::Ballot::Line do end end - describe "#store_user_heading" do - it "stores the heading where the user has voted" do - user = create(:user, :level_two) - investment = create(:budget_investment, :selected) - ballot = create(:budget_ballot, user: user, budget: investment.budget) - - create(:budget_ballot_line, ballot: ballot, investment: investment) - - expect(user.reload.balloted_heading_id).to eq(investment.heading.id) - end - end - describe "scopes" do describe "by_investment" do it "returns ballot lines for an investment" do
<%= t("admin.stats.budget_balloting.participants_per_district") %>